Merge pull request #1914 from jpweber/mysql-revoke

Mysql revoke with non-wildcard hosts

builtin/logical/mysql/backend_test.go

@@ -126,11 +126,39 @@ func TestBackend_basic(t *testing.T) {
 		"connection_url": connURL,
 	}
 
+	// for wildcard based mysql user
 	logicaltest.Test(t, logicaltest.TestCase{
 		Backend: b,
 		Steps: []logicaltest.TestStep{
 			testAccStepConfig(t, connData, false),
-			testAccStepRole(t),
+			testAccStepRole(t, true),
+			testAccStepReadCreds(t, "web"),
+		},
+	})
+}
+
+func TestBackend_basicHostRevoke(t *testing.T) {
+	config := logical.TestBackendConfig()
+	config.StorageView = &logical.InmemStorage{}
+	b, err := Factory(config)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	cid, connURL := prepareTestContainer(t, config.StorageView, b)
+	if cid != "" {
+		defer cleanupTestContainer(t, cid)
+	}
+	connData := map[string]interface{}{
+		"connection_url": connURL,
+	}
+
+	// for host based mysql user
+	logicaltest.Test(t, logicaltest.TestCase{
+		Backend: b,
+		Steps: []logicaltest.TestStep{
+			testAccStepConfig(t, connData, false),
+			testAccStepRole(t, false),
 			testAccStepReadCreds(t, "web"),
 		},
 	})
@@ -156,10 +184,14 @@ func TestBackend_roleCrud(t *testing.T) {
 		Backend: b,
 		Steps: []logicaltest.TestStep{
 			testAccStepConfig(t, connData, false),
-			testAccStepRole(t),
+			// test SQL with wildcard based user
-			testAccStepReadRole(t, "web", testRole),
+			testAccStepRole(t, true),
+			testAccStepReadRole(t, "web", testRoleWildCard),
+			testAccStepDeleteRole(t, "web"),
+			// test SQL with host  based user
+			testAccStepRole(t, false),
+			testAccStepReadRole(t, "web", testRoleHost),
 			testAccStepDeleteRole(t, "web"),
-			testAccStepReadRole(t, "web", ""),
 		},
 	})
 }
@@ -209,7 +241,7 @@ func testAccStepConfig(t *testing.T, d map[string]interface{}, expectError bool)
 					return err
 				}
 				if len(e.Error) == 0 {
-					return fmt.Errorf("expected error, but write succeeded.")
+					return fmt.Errorf("expected error, but write succeeded")
 				}
 				return nil
 			} else if resp != nil && resp.IsError() {
@@ -220,14 +252,26 @@ func testAccStepConfig(t *testing.T, d map[string]interface{}, expectError bool)
 	}
 }
 
-func testAccStepRole(t *testing.T) logicaltest.TestStep {
+func testAccStepRole(t *testing.T, wildCard bool) logicaltest.TestStep {
+
+	pathData := make(map[string]interface{})
+	if wildCard == true {
+		pathData = map[string]interface{}{
+			"sql": testRoleWildCard,
+		}
+	} else {
+		pathData = map[string]interface{}{
+			"sql":        testRoleHost,
+			"revoke_sql": testRevokeSQL,
+		}
+	}
+
 	return logicaltest.TestStep{
 		Operation: logical.UpdateOperation,
 		Path:      "roles/web",
-		Data: map[string]interface{}{
+		Data:      pathData,
-			"sql": testRole,
-		},
 	}
+
 }
 
 func testAccStepDeleteRole(t *testing.T, n string) logicaltest.TestStep {
@@ -310,7 +354,15 @@ func testAccStepReadLease(t *testing.T) logicaltest.TestStep {
 	}
 }
 
-const testRole = `
+const testRoleWildCard = `
 CREATE USER '{{name}}'@'%' IDENTIFIED BY '{{password}}';
 GRANT SELECT ON *.* TO '{{name}}'@'%';
 `
+const testRoleHost = `
+CREATE USER '{{name}}'@'10.1.1.2' IDENTIFIED BY '{{password}}';
+GRANT SELECT ON *.* TO '{{name}}'@'10.1.1.2';
+`
+const testRevokeSQL = `
+REVOKE ALL PRIVILEGES, GRANT OPTION FROM '{{name}}'@'10.1.1.2'; 
+DROP USER '{{name}}'@'10.1.1.2';
+`

builtin/logical/mysql/path_role_create.go

@@ -127,6 +127,7 @@ func (b *backend) pathRoleCreateRead(
 		"password": password,
 	}, map[string]interface{}{
 		"username": username,
+		"role":     name,
 	})
 	resp.Secret.TTL = lease.Lease
 	return resp, nil

builtin/logical/mysql/path_roles.go

@@ -37,6 +37,11 @@ func pathRoles(b *backend) *framework.Path {
 				Description: "SQL string to create a user. See help for more info.",
 			},
 
+			"revoke_sql": &framework.FieldSchema{
+				Type:        framework.TypeString,
+				Description: "SQL string to revoke a user. See help for more info.",
+			},
+
 			"username_length": &framework.FieldSchema{
 				Type:        framework.TypeInt,
 				Description: "number of characters to truncate generated mysql usernames to (default 16)",
@@ -113,6 +118,7 @@ func (b *backend) pathRoleRead(
 	return &logical.Response{
 		Data: map[string]interface{}{
 			"sql":        role.SQL,
+			"revoke_sql": role.RevokeSQL,
 		},
 	}, nil
 }
@@ -131,6 +137,7 @@ func (b *backend) pathRoleCreate(
 	req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
 	name := data.Get("name").(string)
 	sql := data.Get("sql").(string)
+	revoke_sql := data.Get("revoke_sql").(string)
 	username_length := data.Get("username_length").(int)
 	rolename_length := data.Get("rolename_length").(int)
 	displayname_length := data.Get("displayname_length").(int)
@@ -162,6 +169,7 @@ func (b *backend) pathRoleCreate(
 	// Store it
 	entry, err := logical.StorageEntryJSON("role/"+name, &roleEntry{
 		SQL:               sql,
+		RevokeSQL:         revoke_sql,
 		UsernameLength:    username_length,
 		DisplaynameLength: displayname_length,
 		RolenameLength:    rolename_length,
@@ -177,6 +185,7 @@ func (b *backend) pathRoleCreate(
 
 type roleEntry struct {
 	SQL               string `json:"sql"`
+	RevokeSQL         string `json:"revoke_sql"`
 	UsernameLength    int    `json:"username_length"`
 	DisplaynameLength int    `json:"displayname_length"`
 	RolenameLength    int    `json:"rolename_length"`

builtin/logical/mysql/secret_creds.go

@@ -2,13 +2,29 @@ package mysql
 
 import (
 	"fmt"
+	"strings"
 
+	"github.com/hashicorp/vault/helper/strutil"
 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
 )
 
 const SecretCredsType = "creds"
 
+// Default Revoke and Drop user SQL statment
+// Revoking permissions for the user is done before the
+// drop, because MySQL explicitly documents that open user connections
+// will not be closed. By revoking all grants, at least we ensure
+// that the open connection is useless.
+// Dropping the user will only affect the next connection
+// This is not a prepared statement because not all commands are supported
+// 1295: This command is not supported in the prepared statement protocol yet
+// Reference https://mariadb.com/kb/en/mariadb/prepare-statement/
+const defaultRevokeSQL = `
+REVOKE ALL PRIVILEGES, GRANT OPTION FROM '{{name}}'@'%'; 
+DROP USER '{{name}}'@'%'
+`
+
 func secretCreds(b *backend) *framework.Secret {
 	return &framework.Secret{
 		Type: SecretCredsType,
@@ -22,6 +38,11 @@ func secretCreds(b *backend) *framework.Secret {
 				Type:        framework.TypeString,
 				Description: "Password",
 			},
+
+			"role": &framework.FieldSchema{
+				Type:        framework.TypeString,
+				Description: "Role",
+			},
 		},
 
 		Renew:  b.secretCredsRenew,
@@ -46,6 +67,7 @@ func (b *backend) secretCredsRenew(
 
 func (b *backend) secretCredsRevoke(
 	req *logical.Request, d *framework.FieldData) (*logical.Response, error) {
+
 	// Get the username from the internal data
 	usernameRaw, ok := req.Secret.InternalData["username"]
 	if !ok {
@@ -59,6 +81,44 @@ func (b *backend) secretCredsRevoke(
 		return nil, err
 	}
 
+	// Get the role name
+	// we may not always have role data in the secret InternalData
+	// so don't exit if the roleNameRaw fails. Instead it is set
+	// as an empty string.
+	var roleName string
+	roleNameRaw, ok := req.Secret.InternalData["role"]
+	if !ok {
+		roleName = ""
+	} else {
+		roleName, _ = roleNameRaw.(string)
+	}
+	// init default revoke sql string.
+	// this will replaced by a user provided one if one exists
+	// otherwise this is what will be used when lease is revoked
+	revokeSQL := defaultRevokeSQL
+
+	// init bool to track if we should responding with warning about nil role
+	nonNilResponse := false
+
+	// if we were successful in finding a role name
+	// create role entry from that name
+	if roleName != "" {
+		role, err := b.Role(req.Storage, roleName)
+		if err != nil {
+			return nil, err
+		}
+
+		if role == nil {
+			nonNilResponse = true
+		}
+
+		// Check for a revokeSQL string
+		// if one exists use that instead of the default
+		if role.RevokeSQL != "" && role != nil {
+			revokeSQL = role.RevokeSQL
+		}
+	}
+
 	// Start a transaction
 	tx, err := db.Begin()
 	if err != nil {
@@ -66,25 +126,38 @@ func (b *backend) secretCredsRevoke(
 	}
 	defer tx.Rollback()
 
-	// Revoke all permissions for the user. This is done before the
+	for _, query := range strutil.ParseArbitraryStringSlice(revokeSQL, ";") {
-	// drop, because MySQL explicitly documents that open user connections
+		query = strings.TrimSpace(query)
-	// will not be closed. By revoking all grants, at least we ensure
+		if len(query) == 0 {
-	// that the open connection is useless.
+			continue
-	_, err = tx.Exec("REVOKE ALL PRIVILEGES, GRANT OPTION FROM '" + username + "'@'%'")
+		}
+
+		// This is not a prepared statement because not all commands are supported
+		// 1295: This command is not supported in the prepared statement protocol yet
+		// Reference https://mariadb.com/kb/en/mariadb/prepare-statement/
+		query = strings.Replace(query, "{{name}}", username, -1)
+		_, err = tx.Exec(query)
 		if err != nil {
 			return nil, err
 		}
 
-	// Drop this user. This only affects the next connection, which is
-	// why we do the revoke initially.
-	_, err = tx.Exec("DROP USER '" + username + "'@'%'")
-	if err != nil {
-		return nil, err
 	}
 
 	// Commit the transaction
 	if err := tx.Commit(); err != nil {
 		return nil, err
 	}
+
+	// Let the user know that since we had a nil role we used the default SQL revocation statment
+	if nonNilResponse == true {
+		// unable to get role continuing with default sql statements for revoking users
+		var resp *logical.Response
+		resp = &logical.Response{}
+		resp.AddWarning("Role " + roleName + "cannot be found. Using default SQL for revoking user")
+
+		// return non-nil response and nil error
+		return resp, nil
+	}
+
 	return nil, nil
 }