From de5dec6b156359e02f8a8629d8fd5b66a598ffd0 Mon Sep 17 00:00:00 2001 From: vishalnayak Date: Tue, 4 Oct 2016 19:30:25 -0400 Subject: [PATCH] Refactor mysql's revoke SQL --- builtin/logical/mysql/path_roles.go | 36 +++++++------- builtin/logical/mysql/secret_creds.go | 68 +++++++++------------------ 2 files changed, 37 insertions(+), 67 deletions(-) diff --git a/builtin/logical/mysql/path_roles.go b/builtin/logical/mysql/path_roles.go index cef976e73..b6094fa3c 100644 --- a/builtin/logical/mysql/path_roles.go +++ b/builtin/logical/mysql/path_roles.go @@ -27,34 +27,34 @@ func pathRoles(b *backend) *framework.Path { return &framework.Path{ Pattern: "roles/" + framework.GenericNameRegex("name"), Fields: map[string]*framework.FieldSchema{ - "name": &framework.FieldSchema{ + "name": { Type: framework.TypeString, Description: "Name of the role.", }, - "sql": &framework.FieldSchema{ + "sql": { Type: framework.TypeString, Description: "SQL string to create a user. See help for more info.", }, - "revoke_sql": &framework.FieldSchema{ + "revoke_sql": { Type: framework.TypeString, Description: "SQL string to revoke a user. See help for more info.", }, - "username_length": &framework.FieldSchema{ + "username_length": { Type: framework.TypeInt, Description: "number of characters to truncate generated mysql usernames to (default 16)", Default: 16, }, - "rolename_length": &framework.FieldSchema{ + "rolename_length": { Type: framework.TypeInt, Description: "number of characters to truncate the rolename portion of generated mysql usernames to (default 4)", Default: 4, }, - "displayname_length": &framework.FieldSchema{ + "displayname_length": { Type: framework.TypeInt, Description: "number of characters to truncate the displayname portion of generated mysql usernames to (default 4)", Default: 4, @@ -136,11 +136,6 @@ func (b *backend) pathRoleList( 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) // Get our connection db, err := b.DB(req.Storage) @@ -149,6 +144,7 @@ func (b *backend) pathRoleCreate( } // Test the query by trying to prepare it + sql := data.Get("sql").(string) for _, query := range strutil.ParseArbitraryStringSlice(sql, ";") { query = strings.TrimSpace(query) if len(query) == 0 { @@ -169,10 +165,10 @@ 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, + RevokeSQL: data.Get("revoke_sql").(string), + UsernameLength: data.Get("username_length").(int), + DisplaynameLength: data.Get("displayname_length").(int), + RolenameLength: data.Get("rolename_length").(int), }) if err != nil { return nil, err @@ -184,11 +180,11 @@ 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"` + SQL string `json:"sql" mapstructure:"sql" structs:"sql"` + RevokeSQL string `json:"revoke_sql" mapstructure:"revoke_sql" structs:"revoke_sql"` + UsernameLength int `json:"username_length" mapstructure:"username_length" structs:"username_length"` + DisplaynameLength int `json:"displayname_length" mapstructure:"displayname_length" structs:"displayname_length"` + RolenameLength int `json:"rolename_length" mapstructure:"rolename_length" structs:"rolename_length"` } const pathRoleHelpSyn = ` diff --git a/builtin/logical/mysql/secret_creds.go b/builtin/logical/mysql/secret_creds.go index 6ec696d17..ecd90f233 100644 --- a/builtin/logical/mysql/secret_creds.go +++ b/builtin/logical/mysql/secret_creds.go @@ -11,15 +11,11 @@ import ( 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/ +// defaultRevokeSQL is a default SQL statement for revoking a user. 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. const defaultRevokeSQL = ` REVOKE ALL PRIVILEGES, GRANT OPTION FROM '{{name}}'@'%'; DROP USER '{{name}}'@'%' @@ -67,6 +63,7 @@ func (b *backend) secretCredsRenew( func (b *backend) secretCredsRevoke( req *logical.Request, d *framework.FieldData) (*logical.Response, error) { + var resp *logical.Response // Get the username from the internal data usernameRaw, ok := req.Secret.InternalData["username"] @@ -81,42 +78,30 @@ 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 + roleName := "" roleNameRaw, ok := req.Secret.InternalData["role"] - if !ok { - roleName = "" - } else { - roleName, _ = roleNameRaw.(string) + if ok { + 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 + var role *roleEntry if roleName != "" { - role, err := b.Role(req.Storage, roleName) + role, err = b.Role(req.Storage, roleName) if err != nil { return nil, err } + } - if role == nil { - nonNilResponse = true - } + // Use a default SQL statement for revocation if one cannot be fetched from the role + revokeSQL := defaultRevokeSQL - // Check for a revokeSQL string - // if one exists use that instead of the default - if role.RevokeSQL != "" && role != nil { - revokeSQL = role.RevokeSQL + if role != nil && role.RevokeSQL != "" { + revokeSQL = role.RevokeSQL + } else { + if resp == nil { + resp = &logical.Response{} } + resp.AddWarning(fmt.Sprintf("Role %q cannot be found. Using default SQL for revoking user.", roleName)) } // Start a transaction @@ -148,16 +133,5 @@ func (b *backend) secretCredsRevoke( 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 + return resp, nil }