sql: add wire DROP PROVISIONED ROLES for dispatch - #166980
Conversation
|
😎 Merged successfully - details. |
|
Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks. 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
78248e2 to
e193759
Compare
souravcrl
changed the title
souravcrl
changed the title
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if n.options != nil && n.options.LastLoginBefore != nil { | ||
| tsExpr := tree.AsStringWithFlags( | ||
| n.options.LastLoginBefore, tree.FmtParsable, | ||
| ) | ||
| whereExprs = append(whereExprs, fmt.Sprintf( | ||
| "u.estimated_last_login_time < (%s)::TIMESTAMPTZ", tsExpr, | ||
| )) |
There was a problem hiding this comment.
Potential privilege escalation via SQL injection. The LastLoginBefore expression is serialized back to SQL text via tree.AsStringWithFlags and interpolated directly into the internal query, which is then executed with NodeUserSessionDataOverride (node/root privileges). Since the grammar accepts a_expr here, a user with only CREATEROLE can embed arbitrary subqueries (e.g., (SELECT secret FROM system.some_table)) that will execute with elevated privileges.
Suggested fix: Evaluate the user-supplied expression in the user's own session context first (to respect their privilege level), then pass the resulting scalar value as a parameterized argument ($N) to the internal query. The unused argIdx/args scaffolding at lines 178-179 suggests this was the original intent.
|
|
||
| var limitClause string | ||
| if n.limit != nil && n.limit.Count != nil { | ||
| limitClause = fmt.Sprintf("\nLIMIT %s", tree.AsString(n.limit.Count)) |
There was a problem hiding this comment.
Same privilege escalation issue as LastLoginBefore above. Limit.Count is serialized via tree.AsString and interpolated into the internal query executed with node-user privileges. Since LIMIT a_expr also accepts arbitrary subqueries, this is another injection vector. Use a parameterized query ($N) with a pre-evaluated scalar value instead.
AI Review: Potential Issue(s) DetectedInline comments have been added to the relevant lines in Summary: The If helpful: add |
github-actions
Bot
added
the
o-AI-Review-Potential-Issue-Detected
e193759 to
804cb64
Compare
| func (n *DropView) String() string { return AsString(n) } | ||
| func (n *DropProvisionedRoles) String() string { return AsString(n) } | ||
| func (n *DropRole) String() string { return AsString(n) } | ||
| func (n *DropProvisionedRoles) String() string { return AsString(n) } |
There was a problem hiding this comment.
Bug: DropProvisionedRoles.String() is declared twice (lines 2735 and 2737), which causes a Go compilation error. This duplicate on line 2737 should be removed — the declaration at line 2735 is the correct alphabetical placement.
| func (n *DropProvisionedRoles) String() string { return AsString(n) } |
AI Review: Potential Issue(s) DetectedDuplicate method declaration of If helpful: add |
| if n.options != nil && n.options.LastLoginBefore != nil { | ||
| tsExpr := tree.AsStringWithFlags( | ||
| n.options.LastLoginBefore, tree.FmtParsable, | ||
| ) | ||
| whereExprs = append(whereExprs, fmt.Sprintf( | ||
| "u.estimated_last_login_time < (%s)::TIMESTAMPTZ", tsExpr, | ||
| )) |
There was a problem hiding this comment.
Potential privilege escalation via expression injection. The grammar accepts a_expr for LastLoginBefore (and Source), which includes subqueries. The expression is serialized via tree.AsStringWithFlags and spliced directly into a SQL string executed with sessiondata.NodeUserSessionDataOverride (line 67). A user with only CREATEROLE could embed a subquery like (SELECT now() FROM system.privileges LIMIT 1) that executes with node-user (admin-level) privileges, leaking data from system tables the user cannot normally access.
Unlike SHOW USERS which uses the delegation pattern (re-planning under the current user's context), this code explicitly elevates to node-user privileges.
Suggested fix: Evaluate the LastLoginBefore expression in the user's session context first (type-check and evaluate it to a DTimestampTZ scalar), then pass the resulting value as a parameterized query argument ($N). Alternatively, restrict the grammar to accept only SCONST / timestamp literals rather than a_expr.
| // userHasDependencies checks whether the given user owns any objects, | ||
| // has grants, default privileges, scheduled jobs, or system | ||
| // privileges that would prevent dropping. | ||
| func (n *DropProvisionedRolesNode) userHasDependencies( | ||
| params runParams, normalizedUsername username.SQLUsername, allDescs nstree.Catalog, | ||
| ) (bool, error) { | ||
| // Check ownership across all descriptors. | ||
| for _, desc := range allDescs.OrderedDescriptors() { | ||
| if !descriptorIsVisible(desc, true /* allowAdding */, false /* includeDropped */) { | ||
| continue | ||
| } | ||
| if desc.GetPrivileges().Owner() == normalizedUsername { | ||
| return true, nil | ||
| } | ||
| for _, u := range desc.GetPrivileges().Users { | ||
| if u.User() == normalizedUsername { | ||
| return true, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check scheduled jobs. | ||
| row, err := params.p.InternalSQLTxn().QueryRowEx( | ||
| params.ctx, | ||
| "check-user-schedules", | ||
| params.p.txn, | ||
| sessiondata.NodeUserSessionDataOverride, | ||
| "SELECT count(*) FROM system.scheduled_jobs WHERE owner=$1", | ||
| normalizedUsername, | ||
| ) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if row != nil && int64(tree.MustBeDInt(row[0])) > 0 { | ||
| return true, nil | ||
| } | ||
|
|
||
| // Check system privileges. | ||
| row, err = params.p.InternalSQLTxn().QueryRowEx( | ||
| params.ctx, | ||
| "check-user-system-privileges", | ||
| params.p.txn, | ||
| sessiondata.NodeUserSessionDataOverride, | ||
| "SELECT count(*) FROM system.privileges WHERE username=$1", | ||
| normalizedUsername.Normalized(), | ||
| ) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if row != nil && int64(tree.MustBeDInt(row[0])) > 0 { | ||
| return true, nil | ||
| } | ||
|
|
||
| return false, nil | ||
| } |
There was a problem hiding this comment.
Missing default privileges and RLS policy dependency checks. Compared to drop_role.go, userHasDependencies omits two checks:
-
Default privileges:
drop_role.gocallsaccumulateDependentDefaultPrivileges()for both databases (line 159) and schemas (line 259). This function omits that check entirely, so a role with default privilege entries will pass the dependency check and be dropped, leaving orphaned default privilege entries in the system catalog. -
RLS policies:
drop_role.gochecksGetPolicies()(lines 206-219) for roles referenced in row-level security policies. This omission could result in dropping a provisioned role that is referenced in an RLS policy, leaving dangling role references that could affect security policy enforcement.
Suggested fix: Add checks for default privileges (via accumulateDependentDefaultPrivileges or a simplified presence-detection variant) and RLS policy references (similar to the loop in drop_role.go:208-219).
There was a problem hiding this comment.
These issues look correct. I wonder if it would be safer to use the dropRole implementation and have an option to just prevent the cascade behavior inside that.
There was a problem hiding this comment.
Good catch — addressed in ed31891. Added both missing checks to userHasDependencies:
-
Default privileges: Now type-asserts descriptors to
DatabaseDescriptorandSchemaDescriptorto accessGetDefaultPrivilegeDescriptor(), then walksForEachDefaultPrivilegeForRolechecking if the provisioned role appears as a creator (explicit role) or grantee in any default privilege entry. -
RLS policies: Now type-asserts to
TableDescriptorand checksGetPolicies()for any policy that references the provisioned role in itsRoleNameslist.
Also switched the SOURCE and LAST LOGIN BEFORE filter queries from string interpolation (lexbase.EscapeSQLString) to parameterized queries ($1, $2) per your other feedback.
AI Review: Potential Issue(s) DetectedInline comments have been added to the relevant lines in
If helpful: add |
fqazi
left a comment
fqazi
left a comment
There was a problem hiding this comment.
@fqazi reviewed 16 files and all commit messages, and made 3 comments.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on mw5h, sanchit-CRL, and souravcrl).
pkg/sql/drop_provisioned_roles.go line 204 at r7 (raw file):
) whereExprs = append(whereExprs, fmt.Sprintf( "u.estimated_last_login_time < (%s)::TIMESTAMPTZ", tsExpr,
Would it be safer to just a parameterized query instead? i.e. $2?
pkg/sql/drop_provisioned_roles.go line 193 at r7 (raw file):
) provisionFilter += fmt.Sprintf( "\n\t\tAND src.value = %s", lexbase.EscapeSQLString(sourceStr),
Would it be safer to just a parameterized query instead? i.e. $1?
| // userHasDependencies checks whether the given user owns any objects, | ||
| // has grants, default privileges, scheduled jobs, or system | ||
| // privileges that would prevent dropping. | ||
| func (n *DropProvisionedRolesNode) userHasDependencies( | ||
| params runParams, normalizedUsername username.SQLUsername, allDescs nstree.Catalog, | ||
| ) (bool, error) { | ||
| // Check ownership across all descriptors. | ||
| for _, desc := range allDescs.OrderedDescriptors() { | ||
| if !descriptorIsVisible(desc, true /* allowAdding */, false /* includeDropped */) { | ||
| continue | ||
| } | ||
| if desc.GetPrivileges().Owner() == normalizedUsername { | ||
| return true, nil | ||
| } | ||
| for _, u := range desc.GetPrivileges().Users { | ||
| if u.User() == normalizedUsername { | ||
| return true, nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Check scheduled jobs. | ||
| row, err := params.p.InternalSQLTxn().QueryRowEx( | ||
| params.ctx, | ||
| "check-user-schedules", | ||
| params.p.txn, | ||
| sessiondata.NodeUserSessionDataOverride, | ||
| "SELECT count(*) FROM system.scheduled_jobs WHERE owner=$1", | ||
| normalizedUsername, | ||
| ) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if row != nil && int64(tree.MustBeDInt(row[0])) > 0 { | ||
| return true, nil | ||
| } | ||
|
|
||
| // Check system privileges. | ||
| row, err = params.p.InternalSQLTxn().QueryRowEx( | ||
| params.ctx, | ||
| "check-user-system-privileges", | ||
| params.p.txn, | ||
| sessiondata.NodeUserSessionDataOverride, | ||
| "SELECT count(*) FROM system.privileges WHERE username=$1", | ||
| normalizedUsername.Normalized(), | ||
| ) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if row != nil && int64(tree.MustBeDInt(row[0])) > 0 { | ||
| return true, nil | ||
| } | ||
|
|
||
| return false, nil | ||
| } |
There was a problem hiding this comment.
These issues look correct. I wonder if it would be safer to use the dropRole implementation and have an option to just prevent the cascade behavior inside that.
|
|
||
| var limitClause string | ||
| if n.limit != nil && n.limit.Count != nil { | ||
| limitClause = fmt.Sprintf("\nLIMIT %s", tree.AsString(n.limit.Count)) |
There was a problem hiding this comment.
Potential privilege escalation via LIMIT clause injection.
The LIMIT expression is serialized via tree.AsString(n.limit.Count) and spliced directly into the internal query string. Since the grammar's opt_limit_clause accepts a_expr (which includes subqueries), and this statement is an opaque plan node with no expression type-checking, a user with only CREATEROLE can embed arbitrary read subqueries (e.g., DROP PROVISIONED ROLES LIMIT (SELECT count(*) FROM system.privileges)) that will be evaluated with NodeUserSessionDataOverride (node/root privileges) at line 68.
This is inconsistent with Source and LastLoginBefore which correctly use parameterized queries ($1, $2). The fix should parameterize the LIMIT value similarly:
| limitClause = fmt.Sprintf("\nLIMIT %s", tree.AsString(n.limit.Count)) | |
| limitClause = fmt.Sprintf("\nLIMIT $%d", argIdx) | |
| args = append(args, tree.AsString(n.limit.Count)) | |
| argIdx++ |
Alternatively, type-check and evaluate the LIMIT expression to a concrete integer during planning (in DropProvisionedRoles) to reject subqueries entirely.
AI Review: Potential Issue(s) DetectedAn inline comment has been added to The LIMIT expression is interpolated directly into an internal query string that executes with node-level privileges ( If helpful: add |
|
Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link) |
ed31891 to
fb60a61
Compare
| whereExprs = append(whereExprs, fmt.Sprintf( | ||
| "u.estimated_last_login_time < ($%d)::TIMESTAMPTZ", argIdx, | ||
| )) |
There was a problem hiding this comment.
Bug: estimated_last_login_time is TIMESTAMPTZ NULL and remains NULL for provisioned users who have never logged in. The comparison NULL < timestamp evaluates to NULL (not TRUE), so these users are silently excluded from the result set — defeating the cleanup purpose of the statement.
| whereExprs = append(whereExprs, fmt.Sprintf( | |
| "u.estimated_last_login_time < ($%d)::TIMESTAMPTZ", argIdx, | |
| )) | |
| whereExprs = append(whereExprs, fmt.Sprintf( | |
| "(u.estimated_last_login_time IS NULL OR u.estimated_last_login_time < ($%d)::TIMESTAMPTZ)", argIdx, | |
| )) |
Extend the SQL parser grammar to support the DROP PROVISIONED ROLES statement with optional WITH clauses and LIMIT: DROP PROVISIONED ROLES [WITH <options>] [LIMIT <n>] Options (comma-separated): SOURCE = <string> LAST LOGIN BEFORE <expr> Add `PROVISIONED` as an unreserved keyword. Define dedicated grammar rules (`opt_with_drop_provisioned_roles_options`, `drop_provisioned_roles_options_list`, `drop_provisioned_roles_option`) and wire the new `drop_provisioned_roles_stmt` production into `drop_stmt`. Add parse roundtrip tests covering all option combinations, including error tests for duplicate option detection. Fix a pre-existing format string bug in `combineExpr` and `combineStringOrPlaceholderOptList` where `%` was used instead of `%s`, producing garbled error messages for duplicate options. Epic CRDB-52460 fixes: CRDB-52797 Release note: None Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
6f4bef1 to
50e981f
Compare
Release note: None Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
f8b0758 to
37e973a
Compare
| whereExprs = append(whereExprs, fmt.Sprintf( | ||
| "(u.estimated_last_login_time IS NULL OR u.estimated_last_login_time < ($%d)::TIMESTAMPTZ)", argIdx, |
There was a problem hiding this comment.
Bug: The IS NULL OR clause causes users who have never logged in (NULL estimated_last_login_time) to match the LAST LOGIN BEFORE filter and be dropped. A newly provisioned user who hasn't logged in yet would be immediately deleted. This is inconsistent with SHOW USERS (pkg/sql/delegate/show_roles.go:66-71), which explicitly excludes NULL login times — meaning an admin cannot use SHOW USERS to accurately preview which users will be affected by this destructive operation.
Suggested fix — remove the IS NULL OR to align with SHOW USERS semantics:
| whereExprs = append(whereExprs, fmt.Sprintf( | |
| "(u.estimated_last_login_time IS NULL OR u.estimated_last_login_time < ($%d)::TIMESTAMPTZ)", argIdx, | |
| "u.estimated_last_login_time < ($%d)::TIMESTAMPTZ", argIdx, |
There was a problem hiding this comment.
Not a valid suggestion as users can only be provisioned when they login and estimated last login timestamp is populated for all newly provisioned users.
AI Review: Potential Issue DetectedData Loss Bug: NULL In This is semantically inconsistent with An inline comment with a suggested fix has been added to the relevant line. If helpful: add |
37e973a to
a0ccf87
Compare
mw5h
left a comment
mw5h
left a comment
There was a problem hiding this comment.
Looks like you may need to --rewrite your testdata, but otherwise
@mw5h reviewed 7 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status:
cockroach-teamcity
added
the
X-perf-gain
rafiss
left a comment
rafiss
left a comment
There was a problem hiding this comment.
I left a few comments inline. We should also make sure this is tested well before merging this.
- There is no integration/logic test for the actual statement. The unit tests only assert substrings of buildFilterQuery's output. Nothing creates provisioned users, runs DROP PROVISIONED ROLES, and verifies the right users are dropped while non-provisioned users remain. I recommend a logic test at
pkg/sql/logictest/testdata/logic_test/drop_provisioned_rolesmodeled after drop_user. - No test of the skip-on-dependency contract for any of the seven dependency branches (ownership, grants, default privs explicit role, default privs per-object, RLS policies, scheduled jobs, system privileges). All seven are unreachable from current tests.
- No test of admin-skip behavior for non-admin callers. This is where the bulk path diverges from DropRole's "error" behavior, so it's worth testing.
- No test of per-user audit log emission, session revocation, or CREATEROLE-only caller path. (there are other audit logging tests we can use as an example)
@rafiss made 11 comments.
Reviewable status:
pkg/sql/drop_provisioned_roles.go line 64 at r23 (raw file):
func (n *DropProvisionedRolesNode) startExec(params runParams) error { sqltelemetry.IncIAMDropCounter(sqltelemetry.User)
should we use sqltelemetry.Role here?
pkg/sql/drop_provisioned_roles.go line 78 at r23 (raw file):
} rows, err := params.p.InternalSQLTxn().QueryBufferedEx(
With no LIMIT and many provisioned users, this could become a really big transaction. Dependency check is also O(users × descriptors). Could we make the LIMIT clause required and enforce a max value?
pkg/sql/drop_provisioned_roles.go line 108 at r23 (raw file):
// Skip reserved roles. if normalizedUsername.IsAdminRole() ||
please use IsReserved() || IsRootUser() || IsAdminRole() for this check to make sure all reserved roles are covered.
pkg/sql/drop_provisioned_roles.go line 213 at r23 (raw file):
"(u.estimated_last_login_time IS NULL OR u.estimated_last_login_time < ($%d)::TIMESTAMPTZ)", argIdx, )) args = append(args, tree.AsStringWithFlags(n.options.LastLoginBefore, tree.FmtBareStrings))
tree.AsStringWithFlags(..., FmtBareStrings) is a pretty-printer; it does not evaluate the tree.Expr. LAST LOGIN BEFORE (now() - '7d'::interval) passes the literal text now() -'7d'::interval as $N, then attempts ($N)::TIMESTAMPTZ, which fails at runtime. Same issue for SOURCE = ('a' || 'b'). Either evaluate the expression in Go via EvalContext and pass a concrete value, or restrict the grammar to string literals.
for example, here's a logic test to show what fails
# Reproduce: DROP PROVISIONED ROLES stringifies SOURCE / LAST LOGIN BEFORE
# expressions instead of evaluating them.
#
# The grammar accepts a_expr for both SOURCE = <expr> and
# LAST LOGIN BEFORE <expr>, but buildFilterQuery passes the textual form
# of the expression as a parameter — so non-literal expressions either
# silently mismatch or get coerced through ::TIMESTAMPTZ in surprising
# ways.
statement ok
CREATE ROLE alice PROVISIONSRC 'ldap:foo'
statement ok
CREATE ROLE bob PROVISIONSRC 'ldap:bar'
# Sanity: pretend alice and bob have a recorded last-login of one year
# ago, so the LAST LOGIN BEFORE filter has something to compare against.
statement ok
UPSERT INTO system.users (username, estimated_last_login_time) VALUES
('alice', '2025-01-01 00:00:00+00'),
('bob', '2025-01-01 00:00:00+00')
# Confirm both users exist before we start.
query T rowsort
SELECT username FROM system.users WHERE username IN ('alice', 'bob')
----
alice
bob
# ----------------------------------------------------------------------
# Sanity: the literal-string form works as advertised.
# ----------------------------------------------------------------------
statement ok
DROP PROVISIONED ROLES WITH SOURCE = 'ldap:foo'
# alice was dropped because her PROVISIONSRC matched the literal.
query T rowsort
SELECT username FROM system.users WHERE username IN ('alice', 'bob')
----
bob
# ----------------------------------------------------------------------
# BUG #1: SOURCE = <non-literal expression>
#
# A user types SOURCE = ('ldap:' || 'foo'). PostgreSQL semantics would
# evaluate the expression to the string 'ldap:foo' and match alice.
# CRDB stringifies the expression text itself ("'ldap:' || 'foo'") and
# uses it as the parameter, so no row matches and alice survives.
# ----------------------------------------------------------------------
statement ok
CREATE ROLE alice PROVISIONSRC 'ldap:foo'
statement ok
DROP PROVISIONED ROLES WITH SOURCE = ('ldap:' || 'foo')
# Buggy: alice should be gone, but she is still here.
# When the bug is fixed, alice should not appear below.
query T rowsort
SELECT username FROM system.users WHERE username IN ('alice', 'bob')
----
alice
bob
# ----------------------------------------------------------------------
# BUG #2: LAST LOGIN BEFORE <non-literal expression>
#
# A user types LAST LOGIN BEFORE (now() - '1 day'::interval). The
# implementation passes the literal expression text as a string and
# casts it via ($N)::TIMESTAMPTZ. The string "now() - '1 day'::interval"
# is not a valid TIMESTAMPTZ literal, so the cast fails at execution
# time with a parse error — the user gets a confusing error for what
# looks like a perfectly reasonable filter expression.
# ----------------------------------------------------------------------
# When the bug is fixed, this should not cause an error.
statement error parse
DROP PROVISIONED ROLES WITH LAST LOGIN BEFORE (now() - '1 day'::interval)
# Cleanup so the test is rerunnable.
statement ok
DROP ROLE IF EXISTS alice, bob
Please add these logic tests as part of the PR.
pkg/sql/drop_provisioned_roles.go line 224 at r23 (raw file):
// Extract it as int64 so the internal executor receives the // correct type for the LIMIT placeholder. numVal := n.limit.Count.(*tree.NumVal)
use a numVal, ok := assignment here so the code does not panic on non-numerical input
pkg/sql/drop_provisioned_roles.go line 228 at r23 (raw file):
if numErr != nil { return "", nil, pgerror.Wrapf(numErr, pgcode.InvalidParameterValue, "LIMIT must be a non-negative integer")
nothing here is checking that the limit is non-negative.
also, why "non-negative"? should we check for "positive" instead?
pkg/sql/drop_provisioned_roles.go line 232 at r23 (raw file):
limitClause = fmt.Sprintf(" LIMIT $%d", argIdx) args = append(args, limitInt) argIdx++
why do we use argIdx++?
pkg/sql/drop_provisioned_roles.go line 240 at r23 (raw file):
) _ = argIdx
why is this _ = argIdx here?
pkg/sql/drop_provisioned_roles.go line 274 at r23 (raw file):
if defaultPrivs != nil { hasDep := false _ = defaultPrivs.ForEachDefaultPrivilegeForRole(
we should not ignore the error returned by ForEachDefaultPrivilegeForRole.
pkg/sql/drop_provisioned_roles_test.go line 28 at r23 (raw file):
contains []string excludes []string expectedArgs []interface{}
instead of interface{} can we use any?
Register DropProvisionedRoles in opaque.go so the SQL executor
dispatches to the plan node. Add the missing String() method on the
AST node to satisfy the tree.Statement interface.
Epic CRDB-52460
fixes None
Release note (sql change):
The DROP PROVISIONED ROLES statement is now
fully wired into the SQL execution pipeline. It bulk-drops
provisioned (auto-created) users matching filter criteria, skipping
users that own objects or have other dependencies. The existing
DROP ROLE / DROP USER statements are not affected by this change.
Examples:
DROP PROVISIONED ROLES;
DROP PROVISIONED ROLES WITH SOURCE = 'ldap:ldap.example.com';
DROP PROVISIONED ROLES WITH SOURCE = 'oidc:okta.corp.com',
LAST ACCESS TIME OLDER THAN '2025-01-01' LIMIT 100;
a0ccf87 to
dc65753
Compare
souravcrl
left a comment
souravcrl
left a comment
There was a problem hiding this comment.
I have fixed the issues pointed out here. e2e changes I am not taking them up here as there is a separate PR for this #167003 and this PR is already very long. Its late for me and would appreciate if you could take a look for this PR only and approve this PR as this has been in review for a long time.
@souravcrl made 11 comments.
Reviewable status:
pkg/sql/drop_provisioned_roles.go line 64 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
should we use sqltelemetry.Role here?
fixed
pkg/sql/drop_provisioned_roles.go line 78 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
With no LIMIT and many provisioned users, this could become a really big transaction. Dependency check is also
O(users × descriptors). Could we make the LIMIT clause required and enforce a max value?
updated
pkg/sql/drop_provisioned_roles.go line 108 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
please use
IsReserved() || IsRootUser() || IsAdminRole()for this check to make sure all reserved roles are covered.
done
pkg/sql/drop_provisioned_roles.go line 213 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
tree.AsStringWithFlags(..., FmtBareStrings)is a pretty-printer; it does not evaluate the tree.Expr.LAST LOGIN BEFORE (now() - '7d'::interval)passes the literal textnow() -'7d'::intervalas $N, then attempts($N)::TIMESTAMPTZ, which fails at runtime. Same issue forSOURCE = ('a' || 'b'). Either evaluate the expression in Go via EvalContext and pass a concrete value, or restrict the grammar to string literals.for example, here's a logic test to show what fails
# Reproduce: DROP PROVISIONED ROLES stringifies SOURCE / LAST LOGIN BEFORE # expressions instead of evaluating them. # # The grammar accepts a_expr for both SOURCE = <expr> and # LAST LOGIN BEFORE <expr>, but buildFilterQuery passes the textual form # of the expression as a parameter — so non-literal expressions either # silently mismatch or get coerced through ::TIMESTAMPTZ in surprising # ways. statement ok CREATE ROLE alice PROVISIONSRC 'ldap:foo' statement ok CREATE ROLE bob PROVISIONSRC 'ldap:bar' # Sanity: pretend alice and bob have a recorded last-login of one year # ago, so the LAST LOGIN BEFORE filter has something to compare against. statement ok UPSERT INTO system.users (username, estimated_last_login_time) VALUES ('alice', '2025-01-01 00:00:00+00'), ('bob', '2025-01-01 00:00:00+00') # Confirm both users exist before we start. query T rowsort SELECT username FROM system.users WHERE username IN ('alice', 'bob') ---- alice bob # ---------------------------------------------------------------------- # Sanity: the literal-string form works as advertised. # ---------------------------------------------------------------------- statement ok DROP PROVISIONED ROLES WITH SOURCE = 'ldap:foo' # alice was dropped because her PROVISIONSRC matched the literal. query T rowsort SELECT username FROM system.users WHERE username IN ('alice', 'bob') ---- bob # ---------------------------------------------------------------------- # BUG #1: SOURCE = <non-literal expression> # # A user types SOURCE = ('ldap:' || 'foo'). PostgreSQL semantics would # evaluate the expression to the string 'ldap:foo' and match alice. # CRDB stringifies the expression text itself ("'ldap:' || 'foo'") and # uses it as the parameter, so no row matches and alice survives. # ---------------------------------------------------------------------- statement ok CREATE ROLE alice PROVISIONSRC 'ldap:foo' statement ok DROP PROVISIONED ROLES WITH SOURCE = ('ldap:' || 'foo') # Buggy: alice should be gone, but she is still here. # When the bug is fixed, alice should not appear below. query T rowsort SELECT username FROM system.users WHERE username IN ('alice', 'bob') ---- alice bob # ---------------------------------------------------------------------- # BUG #2: LAST LOGIN BEFORE <non-literal expression> # # A user types LAST LOGIN BEFORE (now() - '1 day'::interval). The # implementation passes the literal expression text as a string and # casts it via ($N)::TIMESTAMPTZ. The string "now() - '1 day'::interval" # is not a valid TIMESTAMPTZ literal, so the cast fails at execution # time with a parse error — the user gets a confusing error for what # looks like a perfectly reasonable filter expression. # ---------------------------------------------------------------------- # When the bug is fixed, this should not cause an error. statement error parse DROP PROVISIONED ROLES WITH LAST LOGIN BEFORE (now() - '1 day'::interval) # Cleanup so the test is rerunnable. statement ok DROP ROLE IF EXISTS alice, bobPlease add these logic tests as part of the PR.
Good catch! I have now updated this to use (type-check + evaluate) which is the standard CockroachDB pattern and the most correct approach. It follows what ALTER ROLE ... SET and SET CLUSTER SETTING do.
pkg/sql/drop_provisioned_roles.go line 224 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
use a
numVal, ok :=assignment here so the code does not panic on non-numerical input
done
pkg/sql/drop_provisioned_roles.go line 228 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nothing here is checking that the limit is non-negative.
also, why "non-negative"? should we check for "positive" instead?
changed to LIMIT must be an integer
pkg/sql/drop_provisioned_roles.go line 232 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
why do we use
argIdx++?
removed
pkg/sql/drop_provisioned_roles.go line 240 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
why is this
_ = argIdxhere?
removed
pkg/sql/drop_provisioned_roles.go line 274 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
we should not ignore the error returned by
ForEachDefaultPrivilegeForRole.
Nice catch! Made the change to propagate it.
pkg/sql/drop_provisioned_roles_test.go line 28 at r23 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
instead of
interface{}can we useany?
replaced it here and also update buildProvisionedRolesQuery and its callers in the main file for consistency.
|
Detected infrastructure failure (matched: self-hosted runner lost communication with the server). Automatically rerunning failed jobs. (run link) |
|
Tftr! /trunk merge |
|
Will be taking up additional tests as part of the mentioned work item. Thanks again @rafiss |
Add comprehensive logic tests for the DROP PROVISIONED ROLES statement covering end-to-end behavior, LIMIT validation, admin-skip behavior for non-admin CREATEROLE callers, skip-on-dependency contract for all seven dependency branches, and expression evaluation. These tests address the review feedback from @rafiss on cockroachdb#166980 (cockroachdb#166980 (review)) which called out the lack of integration/logic tests, no coverage of skip-on-dependency branches, no test of admin-skip behavior for non-admin callers, and no test of the CREATEROLE-only caller path. The execution layer enforces a mandatory LIMIT clause to prevent accidentally dropping an unbounded number of provisioned roles in a single transaction. The LIMIT must be a constant integer between 1 and 1024. This safety guard was added during implementation but lacked test coverage until now. Tests cover: - LIMIT is mandatory (error without it) - LIMIT validation (0, negative, >1024, subquery expression) - Provisioned users are dropped, non-provisioned remain untouched - root/admin users are never dropped - SOURCE filter drops only matching source - LIMIT caps the number of dropped users - LAST LOGIN BEFORE time-based filtering - Combined filters with LIMIT - Non-CREATEROLE user is rejected - Non-admin CREATEROLE caller: admin provisioned users are silently skipped while non-admin provisioned users are dropped (contrast with DROP ROLE which errors on admin users) - All seven dependency skip branches: 1. Grants on objects 2. Ownership of objects 3. System privileges (e.g. VIEWCLUSTERMETADATA) 4. Default privileges (explicit role) 5. Default privileges (per-object grantee) 6. Row-level security policies 7. Scheduled jobs ownership - Empty match returns no error - Multiple sources filtered correctly - Role memberships cleaned up on drop - Parse roundtrip - Expression evaluation for SOURCE (concatenation) and LAST LOGIN BEFORE (now() - interval arithmetic) — verifies that non-literal expressions are properly type-checked and evaluated at execution time rather than being stringified Also re-applies the NodeUserSessionDataOverride fix for the find query. The find query originally used NodeUserSessionDataOverride but was changed to params.p.User() during review to use lower privileges. However, the AI reviewer on cockroachdb#166980 correctly identified that a user with only CREATEROLE privilege cannot directly read system.users, system.scheduled_jobs, or system.privileges, so the dependency check and find queries would get permission errors at runtime. The fix to NodeUserSessionDataOverride was applied but got overwritten during a force-push from a separate worktree. Authorization is already checked at plan time via CheckGlobalPrivilegeOrRoleOption, and the query is hardcoded with only parameterized filter values. Fixes: cockroachdb#170030 Fixes: cockroachdb#170031 Fixes: cockroachdb#170032 Fixes: cockroachdb#170048 Epic: CRDB-54682 Release note: None Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
5 participants
Register DropProvisionedRoles in opaque.go so the SQL executor
dispatches to the plan node. Add the missing String() method on the
AST node to satisfy the tree.Statement interface.
Epic CRDB-52460
fixes None
Release note (sql change):
The DROP PROVISIONED ROLES statement is now
fully wired into the SQL execution pipeline. It bulk-drops
provisioned (auto-created) users matching filter criteria, skipping
users that own objects or have other dependencies. The existing
DROP ROLE / DROP USER statements are not affected by this change.
Examples: