Skip to content

Commit

Permalink
Merge pull request #67901 from cockroachdb/blathers/backport-release-…
Browse files Browse the repository at this point in the history 
…21.1-67625

release-21.1: sql: GRANT/REVOKE treat names case insensitively
  • Loading branch information
@rafiss
rafiss authored Aug 4, 2021
2 parents 391ec3d + 25732fd commit 1fac61a
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 97 deletions.
21 changes: 10 additions & 11 deletions pkg/sql/grant_revoke.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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.
Expand All @@ -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{
Expand Down
84 changes: 35 additions & 49 deletions pkg/sql/grant_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ import (
// GrantRoleNode creates entries in the system.role_members table.
// This is called from GRANT <ROLE>
type GrantRoleNode struct {
roles tree.NameList
members tree.NameList
roles []security.SQLUsername
members []security.SQLUsername
adminOption bool

run grantRoleRun
Expand Down Expand Up @@ -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.
Expand All @@ -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)
}
}

Expand All @@ -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)
}
}

Expand All @@ -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
Expand All @@ -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.
Expand All @@ -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
}
Expand All @@ -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
Expand Down
22 changes: 22 additions & 0 deletions pkg/sql/logictest/testdata/logic_test/role
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
39 changes: 39 additions & 0 deletions pkg/sql/parser/testdata/grant
View file
Original file line number Diff line number Diff line change
@@ -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
65 changes: 28 additions & 37 deletions pkg/sql/revoke_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ import (
// RevokeRoleNode removes entries from the system.role_members table.
// This is called from REVOKE <ROLE>
type RevokeRoleNode struct {
roles tree.NameList
members tree.NameList
roles []security.SQLUsername
members []security.SQLUsername
adminOption bool

run revokeRoleRun
Expand Down Expand Up @@ -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.
Expand All @@ -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
}
Expand All @@ -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,
Expand Down

0 comments on commit 1fac61a

Please sign in to comment.