sql: CREATEROLE now includes ability to grant non-admin roles

This matches the PostgreSQL behavior.

Release note (security update): Users who have the CREATEROLE role
option can now grant and revoke role membership in any non-admin role.

This change also removes the sql.auth.createrole_allows_grant_role_membership.enabled
cluster setting, which was added in v23.1. Now, the cluster setting is
effectively always true.

pkg/bench/rttanalysis/testdata/benchmark_expectations

@@ -91,7 +91,7 @@ exp,benchmark
 17,Revoke/revoke_all_on_2_tables
 21,Revoke/revoke_all_on_3_tables
 14,RevokeRole/revoke_1_role
-16,RevokeRole/revoke_2_roles
+18,RevokeRole/revoke_2_roles
 10,ShowGrants/grant_2_roles
 11,ShowGrants/grant_3_roles
 12,ShowGrants/grant_4_roles

pkg/settings/registry.go

@@ -164,7 +164,10 @@ var retiredSettings = map[string]struct{}{
 	// renamed.
 	"spanconfig.host_coalesce_adjacent.enabled":            {},
 	"sql.defaults.experimental_stream_replication.enabled": {},
-	"sql.log.unstructured_entries.enabled":                 {},
+
+	// removed as of 23.2.
+	"sql.log.unstructured_entries.enabled":                     {},
+	"sql.auth.createrole_allows_grant_role_membership.enabled": {},
 }
 
 // sqlDefaultSettings is the list of "grandfathered" existing sql.defaults

pkg/sql/grant_role.go

@@ -54,7 +54,7 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 	ctx, span := tracing.ChildSpan(ctx, n.StatementTag())
 	defer span.Finish()
 
-	hasAdminRole, err := p.HasAdminRole(ctx)
+	hasCreateRolePriv, err := p.HasRoleOption(ctx, roleoption.CREATEROLE)
 	if err != nil {
 		return nil, err
 	}
@@ -63,6 +63,7 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 	if err != nil {
 		return nil, err
 	}
+	grantingRoleHasAdminOptionOnAdmin := allRoles[username.AdminRoleName()]
 
 	inputRoles, err := decodeusername.FromNameList(n.Roles)
 	if err != nil {
@@ -76,19 +77,24 @@ func (p *planner) GrantRoleNode(ctx context.Context, n *tree.GrantRole) (*GrantR
 	}
 
 	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.
-		if hasAdminRole && !r.IsAdminRole() {
+		// If the current user has CREATEROLE, and the role being granted is not an
+		// admin there is no need to check if the user is allowed to grant/revoke
+		// membership in the role. However, if the role being granted is an admin,
+		// then make sure the current user also has the admin option for that role.
+		grantedRoleIsAdmin, err := p.UserHasAdminRole(ctx, r)
+		if err != nil {
+			return nil, err
+		}
+		if hasCreateRolePriv && !grantedRoleIsAdmin {
 			continue
 		}
-		if isAdmin, ok := allRoles[r]; !ok || !isAdmin {
-			if r.IsAdminRole() {
+		if hasAdminOption := allRoles[r]; !hasAdminOption && !grantingRoleHasAdminOptionOnAdmin {
+			if grantedRoleIsAdmin {
 				return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-					"%s is not a role admin for role %s", p.User(), r)
+					"%s must have admin option on role %q", p.User(), r)
 			}
 			return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-				"%s is not a superuser or role admin for role %s", p.User(), r)
+				"%s must have CREATEROLE or have admin option on role %q", p.User(), r)
 		}
 	}
 

pkg/sql/logictest/testdata/logic_test/grant_in_txn

@@ -100,7 +100,7 @@ SAVEPOINT before_invalid_grant
 # This grant should fail as testuser no longer has right to this grant
 # via role_foo.
 
-statement error testuser is not a superuser or role admin for role role_bar
+statement error testuser must have CREATEROLE or have admin option on role \"role_bar\"
 GRANT role_bar TO testuser;
 
 statement ok

pkg/sql/logictest/testdata/logic_test/grant_role

@@ -32,3 +32,130 @@ GRANT testuser TO public
 
 statement error pgcode 42704 role/user \"public\" does not exist
 REVOKE testuser FROM public
+
+# CREATEROLE should allow a user to GRANT/REVOKE on a role even if they do not
+# have the admin option for that role.
+subtest grant_with_createrole
+
+statement ok
+CREATE USER grantor WITH CREATEROLE;
+CREATE ROLE transitiveadmin;
+GRANT admin TO transitiveadmin
+
+statement ok
+SET ROLE grantor
+
+statement ok
+CREATE ROLE parent1;
+CREATE ROLE child1;
+GRANT parent1 TO child1
+
+# Verify that CREATEROLE is not sufficient to give admin to other users.
+statement error grantor must have admin option on role \"admin\"
+GRANT admin TO child2
+
+# It also shouldn't allow anyone to get admin transitively.
+statement error grantor must have admin option on role \"transitiveadmin\"
+GRANT transitiveadmin TO child2
+
+statement ok
+RESET ROLE
+
+query TTB colnames
+SHOW GRANTS ON ROLE parent1
+----
+role_name  member  is_admin
+parent1    child1  false
+
+statement ok
+SET ROLE grantor;
+REVOKE parent1 FROM child1;
+RESET ROLE
+
+# Without CREATEROLE, the admin option is required to grant a role.
+subtest grant_with_admin_option
+
+statement ok
+CREATE ROLE parent2;
+CREATE ROLE child2;
+GRANT parent2 TO grantor WITH ADMIN OPTION;
+ALTER USER grantor WITH NOCREATEROLE
+
+statement ok
+SET ROLE grantor
+
+statement ok
+GRANT parent2 TO child2
+
+statement ok
+RESET ROLE
+
+query TTB colnames,rowsort
+SHOW GRANTS ON ROLE parent2
+----
+role_name  member   is_admin
+parent2    child2   false
+parent2    grantor  true
+
+statement ok
+SET ROLE grantor;
+REVOKE parent2 FROM child2;
+RESET ROLE
+
+statement ok
+GRANT admin TO grantor;
+SET ROLE grantor
+
+# Verify that testuser can only grant an admin role if it has the admin option
+# on that role.
+statement error grantor must have admin option on role \"transitiveadmin\"
+GRANT transitiveadmin TO child2
+
+statement ok
+RESET ROLE;
+GRANT transitiveadmin TO grantor;
+SET ROLE grantor
+
+statement error grantor must have admin option on role \"transitiveadmin\"
+GRANT transitiveadmin TO child2
+
+statement ok
+RESET ROLE;
+GRANT transitiveadmin TO grantor WITH ADMIN OPTION;
+SET ROLE grantor
+
+# Now that grantor has the admin option on transitiveadmin, it can grant the role.
+statement ok
+GRANT transitiveadmin TO child2
+
+statement ok
+RESET ROLE;
+REVOKE transitiveadmin FROM grantor;
+REVOKE transitiveadmin FROM child2;
+GRANT admin TO grantor WITH ADMIN OPTION
+
+# If grantor has the admin option on admin, it also can grant transitiveadmin.
+statement ok
+GRANT transitiveadmin TO child2
+
+statement ok
+RESET ROLE;
+REVOKE admin FROM grantor;
+REVOKE transitiveadmin FROM child2
+
+# Without CREATEROLE or the admin option, then an error should occur during
+# granting.
+subtest grant_no_privilege
+
+statement ok
+CREATE ROLE parent3;
+CREATE ROLE child3
+
+statement ok
+SET ROLE grantor
+
+statement error grantor must have CREATEROLE or have admin option on role \"parent3\"
+GRANT parent3 TO child3
+
+statement ok
+RESET ROLE

pkg/sql/logictest/testdata/logic_test/role

@@ -149,7 +149,7 @@ GRANT unknownrole TO testuser
 # Test role "grant" and WITH ADMIN option.
 user testuser
 
-statement error pq: testuser is not a superuser or role admin for role testrole
+statement error testuser must have CREATEROLE or have admin option on role "testrole"
 GRANT testrole TO testuser2
 
 user root
@@ -173,7 +173,7 @@ testrole   testuser  false
 
 user testuser
 
-statement error pq: testuser is not a superuser or role admin for role testrole
+statement error testuser must have CREATEROLE or have admin option on role "testrole"
 GRANT testrole TO testuser2
 
 user root
@@ -428,13 +428,13 @@ rolec
 roled
 testuser
 
-statement error pq: testuser is not a superuser or role admin for role roled
+statement error testuser must have CREATEROLE or have admin option on role "roled"
 GRANT roled TO rolee
 
-statement error pq: testuser is not a superuser or role admin for role rolec
+statement error testuser must have CREATEROLE or have admin option on role "rolec"
 GRANT rolec TO rolee
 
-statement error pq: testuser is not a superuser or role admin for role roleb
+statement error testuser must have CREATEROLE or have admin option on role "roleb"
 GRANT roleb TO rolee
 
 statement ok
@@ -617,7 +617,7 @@ testuser
 statement ok
 REVOKE ADMIN OPTION FOR rolea FROM testuser
 
-statement error pq: testuser is not a superuser or role admin for role rolea
+statement error testuser must have CREATEROLE or have admin option on role "rolea"
 REVOKE ADMIN OPTION FOR rolea FROM root
 
 statement ok
@@ -678,7 +678,7 @@ CREATE DATABASE db2
 statement error user testuser does not have DROP privilege on database db1
 DROP DATABASE db1
 
-statement error testuser is not a role admin for role admin
+statement error testuser must have admin option on role "admin"
 GRANT admin TO testuser
 
 user root
@@ -762,13 +762,13 @@ statement ok
 SELECT * FROM db2.foo
 
 # We may be in the 'newgroup', but we don't have the admin option.
-statement error testuser is not a superuser or role admin for role newgroup
+statement error testuser must have CREATEROLE or have admin option on role "newgroup"
 GRANT newgroup TO testuser2
 
-statement error testuser is not a superuser or role admin for role newgroup
+statement error testuser must have CREATEROLE or have admin option on role "newgroup"
 REVOKE newgroup FROM testuser
 
-statement error testuser is not a superuser or role admin for role newgroup
+statement error testuser must have CREATEROLE or have admin option on role "newgroup"
 GRANT newgroup TO testuser WITH ADMIN OPTION
 
 # Regression for #31784
@@ -781,10 +781,10 @@ GRANT admin TO testuser
 
 user testuser
 
-statement error pq: testuser is not a role admin for role admin
+statement error testuser must have admin option on role "admin"
 GRANT admin TO user1
 
-statement error pq: testuser is not a role admin for role admin
+statement error testuser must have admin option on role "admin"
 REVOKE admin FROM user1
 
 user root
@@ -990,14 +990,14 @@ CREATE ROLE IF NOT EXISTS roleg
 statement ok
 CREATE ROLE IF NOT EXISTS roleg
 
-# Need Admin option to GRANT role, CREATEROLE should not give GRANT role privilege for other roles
 statement ok
 CREATE USER testuser3
 
-statement error pq: testuser is not a role admin for role admin
+statement error testuser must have admin option on role "admin"
 GRANT admin to testuser3
 
-statement error pq: testuser is not a superuser or role admin for role roleg
+# CREATEROLE should give GRANT role privilege for other roles.
+statement ok
 GRANT roleg to testuser3
 
 user root
@@ -1117,11 +1117,12 @@ CREATE ROLE thisshouldntwork LOGIN LOGIN
 statement ok
 DROP ROLE parentrole
 
-query TTB colnames
+query TTB colnames,rowsort
 SHOW GRANTS ON ROLE
 ----
-role_name  member  is_admin
-admin      root    true
+role_name  member     is_admin
+admin      root       true
+roleg      testuser3  false
 
 query TTB colnames
 SHOW GRANTS ON ROLE admin

pkg/sql/revoke_role.go

@@ -17,6 +17,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/decodeusername"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
+	"github.com/cockroachdb/cockroach/pkg/sql/roleoption"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
@@ -43,7 +44,7 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo
 	ctx, span := tracing.ChildSpan(ctx, n.StatementTag())
 	defer span.Finish()
 
-	hasAdminRole, err := p.HasAdminRole(ctx)
+	hasCreateRolePriv, err := p.HasRoleOption(ctx, roleoption.CREATEROLE)
 	if err != nil {
 		return nil, err
 	}
@@ -52,6 +53,7 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo
 	if err != nil {
 		return nil, err
 	}
+	revokingRoleHasAdminOptionOnAdmin := allRoles[username.AdminRoleName()]
 
 	inputRoles, err := decodeusername.FromNameList(n.Roles)
 	if err != nil {
@@ -65,19 +67,24 @@ func (p *planner) RevokeRoleNode(ctx context.Context, n *tree.RevokeRole) (*Revo
 	}
 
 	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.
-		if hasAdminRole && !r.IsAdminRole() {
+		// If the current user has CREATEROLE, and the role being revoked is not an
+		// admin there is no need to check if the user is allowed to grant/revoke
+		// membership in the role. However, if the role being revoked is an admin,
+		// then make sure the current user also has the admin option for that role.
+		revokedRoleIsAdmin, err := p.UserHasAdminRole(ctx, r)
+		if err != nil {
+			return nil, err
+		}
+		if hasCreateRolePriv && !revokedRoleIsAdmin {
 			continue
 		}
-		if isAdmin, ok := allRoles[r]; !ok || !isAdmin {
-			if r.IsAdminRole() {
+		if hasAdminOption := allRoles[r]; !hasAdminOption && !revokingRoleHasAdminOptionOnAdmin {
+			if revokedRoleIsAdmin {
 				return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-					"%s is not a role admin for role %s", p.User(), r)
+					"%s must have admin option on role %q", p.User(), r)
 			}
 			return nil, pgerror.Newf(pgcode.InsufficientPrivilege,
-				"%s is not a superuser or role admin for role %s", p.User(), r)
+				"%s must have CREATEROLE or have admin option on role %q", p.User(), r)
 		}
 	}