From 25732fdd0df01a1f4be5c69b8ee8cc9b5f101215 Mon Sep 17 00:00:00 2001 From: Rafi Shamim Date: Wed, 14 Jul 2021 19:12:39 +0000 Subject: [PATCH] sql: GRANT/REVOKE treat names case insensitively Release note (bug fix): Previously the GRANT and REVOKE commands would incorrectly handle role names. CockroachDB treats role names as case insensitive, but these commands were incorrectly handling the names. Now, GRANT and REVOKE normalize the names and are case-insensitive. --- pkg/sql/grant_revoke.go | 21 +++--- pkg/sql/grant_role.go | 84 +++++++++------------- pkg/sql/logictest/testdata/logic_test/role | 22 ++++++ pkg/sql/parser/testdata/grant | 39 ++++++++++ pkg/sql/revoke_role.go | 65 ++++++++--------- 5 files changed, 134 insertions(+), 97 deletions(-) create mode 100644 pkg/sql/parser/testdata/grant diff --git a/pkg/sql/grant_revoke.go b/pkg/sql/grant_revoke.go index ad5ebfe90a15..327ac4d41a9c 100644 --- a/pkg/sql/grant_revoke.go +++ b/pkg/sql/grant_revoke.go @@ -61,13 +61,13 @@ func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) { return nil, err } - // TODO(solon): there are SQL identifiers (tree.Name) in n.Grantees, - // but we want SQL usernames. Do we normalize or not? For reference, - // REASSIGN / OWNER TO do normalize. - // Related: https://github.com/cockroachdb/cockroach/issues/54696 grantees := make([]security.SQLUsername, len(n.Grantees)) for i, grantee := range n.Grantees { - grantees[i] = security.MakeSQLUsernameFromPreNormalizedString(string(grantee)) + normalizedGrantee, err := security.MakeSQLUsernameFromUserInput(string(grantee), security.UsernameValidation) + if err != nil { + return nil, err + } + grantees[i] = normalizedGrantee } return &changePrivilegesNode{ @@ -87,7 +87,6 @@ func (p *planner) Grant(ctx context.Context, n *tree.Grant) (planNode, error) { // - Target: single database, table, or view. // TODO(marc): open questions: // - should we have root always allowed and not present in the permissions list? -// - should we make users case-insensitive? // Privileges: GRANT on database/table/view. // Notes: postgres requires the object owner. // mysql requires the "grant option" and the same privileges, and sometimes superuser. @@ -112,13 +111,13 @@ func (p *planner) Revoke(ctx context.Context, n *tree.Revoke) (planNode, error) return nil, err } - // TODO(solon): there are SQL identifiers (tree.Name) in n.Grantees, - // but we want SQL usernames. Do we normalize or not? For reference, - // REASSIGN / OWNER TO do normalize. - // Related: https://github.com/cockroachdb/cockroach/issues/54696 grantees := make([]security.SQLUsername, len(n.Grantees)) for i, grantee := range n.Grantees { - grantees[i] = security.MakeSQLUsernameFromPreNormalizedString(string(grantee)) + normalizedGrantee, err := security.MakeSQLUsernameFromUserInput(string(grantee), security.UsernameValidation) + if err != nil { + return nil, err + } + grantees[i] = normalizedGrantee } return &changePrivilegesNode{ diff --git a/pkg/sql/grant_role.go b/pkg/sql/grant_role.go index ac1fc3027d64..aa14723458b5 100644 --- a/pkg/sql/grant_role.go +++ b/pkg/sql/grant_role.go @@ -28,8 +28,8 @@ import ( // GrantRoleNode creates entries in the system.role_members table. // This is called from GRANT type GrantRoleNode struct { - roles tree.NameList - members tree.NameList + roles []security.SQLUsername + members []security.SQLUsername adminOption bool run grantRoleRun @@ -59,13 +59,25 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR if err != nil { return nil, err } - for i := range n.Roles { - // TODO(solon): there are SQL identifiers (tree.Name) in - // n.Roles, but we want SQL usernames. Do we normalize or not? For - // reference, REASSIGN / OWNER TO do normalize. Related: - // https://github.com/cockroachdb/cockroach/issues/54696 - r := security.MakeSQLUsernameFromPreNormalizedString(string(n.Roles[i])) + inputRoles := make([]security.SQLUsername, len(n.Roles)) + inputMembers := make([]security.SQLUsername, len(n.Members)) + for i, role := range n.Roles { + normalizedRole, err := security.MakeSQLUsernameFromUserInput(string(role), security.UsernameValidation) + if err != nil { + return nil, err + } + inputRoles[i] = normalizedRole + } + for i, member := range n.Members { + normalizedMember, err := security.MakeSQLUsernameFromUserInput(string(member), security.UsernameValidation) + if err != nil { + return nil, err + } + inputMembers[i] = normalizedMember + } + + for _, r := range inputRoles { // If the user is an admin, don't check if the user is allowed to add/drop // roles in the role. However, if the role being modified is the admin role, then // make sure the user is an admin with the admin option. @@ -75,10 +87,10 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR if isAdmin, ok := allRoles[r]; !ok || !isAdmin { if r.IsAdminRole() { return nil, pgerror.Newf(pgcode.InsufficientPrivilege, - "%s is not a role admin for role %s", p.User(), n.Roles[i]) + "%s is not a role admin for role %s", p.User(), r) } return nil, pgerror.Newf(pgcode.InsufficientPrivilege, - "%s is not a superuser or role admin for role %s", p.User(), n.Roles[i]) + "%s is not a superuser or role admin for role %s", p.User(), r) } } @@ -93,37 +105,24 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR // NOTE: membership manipulation involving the "public" pseudo-role fails with // "role public does not exist". This matches postgres behavior. - for i := range n.Roles { - // TODO(solon): there are SQL identifiers (tree.Name) in - // n.Roles, but we want SQL usernames. Do we normalize or not? For - // reference, REASSIGN / OWNER TO do normalize. Related: - // https://github.com/cockroachdb/cockroach/issues/54696 - r := security.MakeSQLUsernameFromPreNormalizedString(string(n.Roles[i])) - + for _, r := range inputRoles { if _, ok := roles[r]; !ok { maybeOption := strings.ToUpper(r.Normalized()) for name := range roleoption.ByName { if maybeOption == name { return nil, errors.WithHintf( pgerror.Newf(pgcode.UndefinedObject, - "role/user %s does not exist", n.Roles[i]), + "role/user %s does not exist", r), "%s is a role option, try using ALTER ROLE to change a role's options.", maybeOption) } } - return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", n.Roles[i]) + return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r) } } - for i := range n.Members { - // TODO(solon): there are SQL identifiers (tree.Name) in - // n.Members but we want SQL usernames. Do we normalize or not? For - // reference, REASSIGN / OWNER TO do normalize. Related: - // https://github.com/cockroachdb/cockroach/issues/54696 - m := security.MakeSQLUsernameFromPreNormalizedString(string(n.Members[i])) - + for _, m := range inputMembers { if _, ok := roles[m]; !ok { - return nil, pgerror.Newf(pgcode.UndefinedObject, - "role/user %s does not exist", n.Members[i]) + return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", m) } } @@ -132,13 +131,7 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR // For each grant.Role, we lookup all the roles it is a member of. // After adding a given edge (grant.Member ∈ grant.Role), we add the edge to the list as well. allRoleMemberships := make(map[security.SQLUsername]map[security.SQLUsername]bool) - for _, rawR := range n.Roles { - // TODO(solon): there are SQL identifiers (tree.Name) in - // n.Roles but we want SQL usernames. Do we normalize or not? For - // reference, REASSIGN / OWNER TO do normalize. Related: - // https://github.com/cockroachdb/cockroach/issues/54696 - r := security.MakeSQLUsernameFromPreNormalizedString(string(rawR)) - + for _, r := range inputRoles { allRoles, err := p.MemberOfWithAdminOption(ctx, r) if err != nil { return nil, err @@ -148,24 +141,17 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR // Since we perform no queries here, check all role/member pairs for cycles. // Only if there are no errors do we proceed to write them. - for _, rawR := range n.Roles { - // TODO(solon): there are SQL identifiers (tree.Name) in - // n.Roles but we want SQL usernames. Do we normalize or not? For - // reference, REASSIGN / OWNER TO do normalize. Related: - // https://github.com/cockroachdb/cockroach/issues/54696 - r := security.MakeSQLUsernameFromPreNormalizedString(string(rawR)) - for _, rawM := range n.Members { - // TODO(solon): ditto above, names in n.Members. - m := security.MakeSQLUsernameFromPreNormalizedString(string(rawM)) + for _, r := range inputRoles { + for _, m := range inputMembers { if r == m { // self-cycle. - return nil, pgerror.Newf(pgcode.InvalidGrantOperation, "%s cannot be a member of itself", rawM) + return nil, pgerror.Newf(pgcode.InvalidGrantOperation, "%s cannot be a member of itself", m) } // Check if grant.Role ∈ ... ∈ grant.Member if memberOf, ok := allRoleMemberships[r]; ok { if _, ok = memberOf[m]; ok { return nil, pgerror.Newf(pgcode.InvalidGrantOperation, - "making %s a member of %s would create a cycle", rawM, rawR) + "making %s a member of %s would create a cycle", m, r) } } // Add the new membership. We don't care about the actual bool value. @@ -177,8 +163,8 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR } return &GrantRoleNode{ - roles: n.Roles, - members: n.Members, + roles: inputRoles, + members: inputMembers, adminOption: n.AdminOption, }, nil } @@ -205,7 +191,7 @@ func (n *GrantRoleNode) startExec(params runParams) error { params.p.txn, sessiondata.InternalExecutorOverride{User: security.RootUserName()}, memberStmt, - r, m, n.adminOption, + r.Normalized(), m.Normalized(), n.adminOption, ) if err != nil { return err diff --git a/pkg/sql/logictest/testdata/logic_test/role b/pkg/sql/logictest/testdata/logic_test/role index a239f99b3f2d..df48ca5327e8 100644 --- a/pkg/sql/logictest/testdata/logic_test/role +++ b/pkg/sql/logictest/testdata/logic_test/role @@ -590,6 +590,28 @@ SELECT * FROM system.role_members role member isAdmin admin root true +# Verify that GRANT/REVOKE are not sensitive to the case of role names. + +statement ok +GRANT roLea,rOleB TO tEstUSER WITH ADMIN OPTION + +query TTB colnames +SELECT * FROM system.role_members +---- +role member isAdmin +admin root true +rolea testuser true +roleb testuser true + +statement ok +REVOKE roleA, roleB FROM TestUser + +query TTB colnames +SELECT * FROM system.role_members +---- +role member isAdmin +admin root true + # Test privilege checks. statement ok diff --git a/pkg/sql/parser/testdata/grant b/pkg/sql/parser/testdata/grant new file mode 100644 index 000000000000..c376d5c15de2 --- /dev/null +++ b/pkg/sql/parser/testdata/grant @@ -0,0 +1,39 @@ +parse +GRANT roLea,rOleB TO rOLeC WITH ADMIN OPTION +---- +GRANT rolea, roleb TO rolec WITH ADMIN OPTION -- normalized! +GRANT rolea, roleb TO rolec WITH ADMIN OPTION -- fully parenthesized +GRANT rolea, roleb TO rolec WITH ADMIN OPTION -- literals removed +GRANT _, _ TO _ WITH ADMIN OPTION -- identifiers removed + +parse +GRANT roLea TO roLeB WITH ADMIN OPTION +---- +GRANT rolea TO roleb WITH ADMIN OPTION -- normalized! +GRANT rolea TO roleb WITH ADMIN OPTION -- fully parenthesized +GRANT rolea TO roleb WITH ADMIN OPTION -- literals removed +GRANT _ TO _ WITH ADMIN OPTION -- identifiers removed + +parse +GRANT ALL ON TABLE the_table TO rolEC +---- +GRANT ALL ON TABLE the_table TO rolec -- normalized! +GRANT ALL ON TABLE (the_table) TO rolec -- fully parenthesized +GRANT ALL ON TABLE the_table TO rolec -- literals removed +GRANT ALL ON TABLE _ TO _ -- identifiers removed + +parse +REVOKE roleA, roleB FROM RoleC +---- +REVOKE rolea, roleb FROM rolec -- normalized! +REVOKE rolea, roleb FROM rolec -- fully parenthesized +REVOKE rolea, roleb FROM rolec -- literals removed +REVOKE _, _ FROM _ -- identifiers removed + +parse +REVOKE roleA FROM roleB +---- +REVOKE rolea FROM roleb -- normalized! +REVOKE rolea FROM roleb -- fully parenthesized +REVOKE rolea FROM roleb -- literals removed +REVOKE _ FROM _ -- identifiers removed diff --git a/pkg/sql/revoke_role.go b/pkg/sql/revoke_role.go index 998dab33d884..a67f76299f0f 100644 --- a/pkg/sql/revoke_role.go +++ b/pkg/sql/revoke_role.go @@ -25,8 +25,8 @@ import ( // RevokeRoleNode removes entries from the system.role_members table. // This is called from REVOKE type RevokeRoleNode struct { - roles tree.NameList - members tree.NameList + roles []security.SQLUsername + members []security.SQLUsername adminOption bool run revokeRoleRun @@ -56,13 +56,25 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo if err != nil { return nil, err } - for i := range n.Roles { - // TODO(solon): there are SQL identifiers (tree.Name) in n.Roles, - // but we want SQL usernames. Do we normalize or not? For reference, - // REASSIGN / OWNER TO do normalize. - // Related: https://github.com/cockroachdb/cockroach/issues/54696 - r := security.MakeSQLUsernameFromPreNormalizedString(string(n.Roles[i])) + inputRoles := make([]security.SQLUsername, len(n.Roles)) + inputMembers := make([]security.SQLUsername, len(n.Members)) + for i, role := range n.Roles { + normalizedRole, err := security.MakeSQLUsernameFromUserInput(string(role), security.UsernameValidation) + if err != nil { + return nil, err + } + inputRoles[i] = normalizedRole + } + for i, member := range n.Members { + normalizedMember, err := security.MakeSQLUsernameFromUserInput(string(member), security.UsernameValidation) + if err != nil { + return nil, err + } + inputMembers[i] = normalizedMember + } + + for _, r := range inputRoles { // If the user is an admin, don't check if the user is allowed to add/drop // roles in the role. However, if the role being modified is the admin role, then // make sure the user is an admin with the admin option. @@ -87,33 +99,21 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo return nil, err } - for i := range n.Roles { - // TODO(solon): there are SQL identifiers (tree.Name) in n.Roles, - // but we want SQL usernames. Do we normalize or not? For reference, - // REASSIGN / OWNER TO do normalize. - // Related: https://github.com/cockroachdb/cockroach/issues/54696 - r := security.MakeSQLUsernameFromPreNormalizedString(string(n.Roles[i])) - + for _, r := range inputRoles { if _, ok := roles[r]; !ok { - return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", n.Roles[i]) + return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", r) } } - for i := range n.Members { - // TODO(solon): there are SQL identifiers (tree.Name) in n.Roles, - // but we want SQL usernames. Do we normalize or not? For reference, - // REASSIGN / OWNER TO do normalize. - // Related: https://github.com/cockroachdb/cockroach/issues/54696 - m := security.MakeSQLUsernameFromPreNormalizedString(string(n.Members[i])) - + for _, m := range inputMembers { if _, ok := roles[m]; !ok { - return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", n.Members[i]) + return nil, pgerror.Newf(pgcode.UndefinedObject, "role/user %s does not exist", m) } } return &RevokeRoleNode{ - roles: n.Roles, - members: n.Members, + roles: inputRoles, + members: inputMembers, adminOption: n.AdminOption, }, nil } @@ -131,17 +131,8 @@ func (n *RevokeRoleNode) startExec(params runParams) error { } var rowsAffected int - for i := range n.roles { - // TODO(solon): there are SQL identifiers (tree.Name) in - // n.Roles, but we want SQL usernames. Do we normalize or not? For - // reference, REASSIGN / OWNER TO do normalize. Related: - // https://github.com/cockroachdb/cockroach/issues/54696 - r := security.MakeSQLUsernameFromPreNormalizedString(string(n.roles[i])) - - for j := range n.members { - // TODO(solon): ditto above, names in n.members. - m := security.MakeSQLUsernameFromPreNormalizedString(string(n.members[j])) - + for _, r := range n.roles { + for _, m := range n.members { if r.IsAdminRole() && m.IsRootUser() { // We use CodeObjectInUseError which is what happens if you tried to delete the current user in pg. return pgerror.Newf(pgcode.ObjectInUse,