implement alter default privileges#2782
Conversation
|
|
Ito Test Report ❌20 test cases ran. 4 failed, 4 additional findings, 12 passed. Across 20 test cases, 12 passed and 8 failed: default-privilege and object-inheritance flows largely behaved correctly (including grant/revoke variants, omitted FOR ROLE behavior, table/routine/sequence coverage, routine write-fault cleanup, and ATTACH/DETACH partition formatting). The key findings were several production defects led by high-severity auth durability/restore problems (non-atomic default-ACL mutation on persist failure, auth.db v2 restart panics, corruption-driven startup crashes, and v1-to-v2 upgrade restart failure), plus catalog stability issues (owner-filter lookups blocked by empty pg_roles and pg_default_acl OID type mismatch panics) and parser/diagnostic gaps (broken PRIMARY KEY USING INDEX parse/format path and generic ALTER INDEX unsupported errors). ❌ Failed (4)
|
Ito Diff Report ❌Tested: Across 13 test cases, 5 passed and 8 failed, showing that key restore and fail-closed behaviors (unsupported auth.db header rejection, backup restore consistency, and recovery after controlled corruption) worked as expected but substantial defects remain. The most important findings were five high-severity issues—startup crashes from truncated or counter-inflated auth.db data, acceptance of downgraded version-byte tampering, unauthorized DROP SEQUENCE due to missing execution-time privilege checks, and default-privilege behavior diverging before vs. after restart when persistence fails—along with parser/diagnostic defects ( ❌ Failures (4)🟠 USING INDEX primary key grammar mismatch
Relevant code:
| ADD CONSTRAINT constraint_name unique_or_primary USING INDEX db_object_name opt_deferrable_mode opt_initially
{
$$.val = tree.AlterTableConstraintUsingIndex{Constraint: tree.Name($3), IsUnique: $4.bool(), Index: $7.unresolvedObjectName(), Deferrable: $8.deferrableMode(), Initially: $9.initiallyMode()}
}
unique_or_primary:
UNIQUE
{
$$.val = true
}
| PRIMARY
{
$$.val = false
}
if node.IsUnique {
ctx.WriteString(" UNIQUE")
} else {
ctx.WriteString(" PRIMARY KEY")
}
ctx.WriteString(" USING INDEX")🟡 ALTER INDEX diagnostic omits object names
Relevant code:
func (node *AlterIndexAttachPartition) Format(ctx *FmtCtx) {
ctx.WriteString(" ATTACH PARTITION ")
node.Index.Format(ctx)
}
// Only PARTITION alterations are supported by the parser, so there's nothing to convert to yet
return NotYetSupportedError("ALTER INDEX is not yet supported")
// NotYetSupportedError returns an unsupported error with the given message, or a NoOp statement if the environment
// variable DOLTGRES_IGNORE_UNSUPPORTED is set.
func NotYetSupportedError(errorMsg string) (vitess.Statement, error) {
if ignoreUnsupportedStatements {
return NewNoOp(errorMsg), nil
}
return nil, errors.Errorf(errorMsg + " Please file an issue at https://github.com/dolthub/doltgresql/issues")
}
|
| Category | Summary | Screenshot |
|---|---|---|
| Catalog | Verified pg_default_acl returns rows for configured TABLE and FUNCTION defaults. | N/A |
| Catalog | Verified large ACL arrays remain queryable with 20 ACL entries and valid aclitem formatting. | N/A |
| Object | Old table stayed denied; new table inherited default SELECT grant. | N/A |
| Object | EXECUTE stayed denied on old routines and allowed on newly created routines. | N/A |
| Object | SERIAL table path applied default privileges to both table SELECT and sequence usage. | N/A |
| Object | Forced auth persistence failure returned an explicit error and no callable routine remained. | N/A |
| Parser | Parser round-trip formatted ATTACH PARTITION with child_tbl (not parent_tbl). | N/A |
| Parser | Both DETACH statements formatted with child_tbl and preserved CONCURRENTLY/FINALIZE modifiers. | N/A |
| Privilege | Default table grant applied; grantee could read the newly created table. | N/A |
| Privilege | Default privileges without FOR ROLE resolved to the active owner session and propagated to new tables. | N/A |
| Privilege | REVOKE GRANT OPTION FOR preserved SELECT while full REVOKE removed SELECT for later tables. | N/A |
| Privilege | REVOKE ... CASCADE was rejected and existing default ACL behavior remained unchanged. | N/A |
⚠️ Additional Findings (4)
These findings are unrelated to the current changes but were observed during testing.
⚠️ Reject downgraded auth.db version-byte tampering
- What failed: The server accepted a version-downgraded header and continued serving SQL, but expected behavior is fail-closed rejection of mismatched/tampered auth payloads.
- Impact: A tampered auth file can be accepted under a downgraded parser path, allowing the server to boot with inconsistent authorization state interpretation. This weakens startup integrity checks for security-sensitive auth state.
- Steps to reproduce:
- Create a valid v2 auth.db.
- Modify only the version header bytes from 2 to 1.
- Restart the server and attempt a SQL connection.
- Stub / mock context: SCRAM authentication was bypassed during this scenario so startup behavior with a tampered auth file could be exercised directly, and domain-cast constraint checks were temporarily disabled in analyzer logic. The tamper itself was a deliberate header-byte downgrade on auth.db followed by restart and live SQL reachability checks.
- Code analysis: I inspected the auth deserialization entrypoint and v1 path; the code trusts the version header and parses v1 without validating payload consistency or requiring full-buffer consumption, so a v2 payload labeled as v1 can still load.
- Why this is likely a bug: Production code accepts a header-controlled downgrade path without structural integrity checks, which contradicts the expected fail-closed behavior for tampered auth state.
Relevant code:
server/auth/serialization.go (lines 64-75)
reader := utils.NewReader(data)
version := reader.Uint32()
switch version {
case 0:
return db.deserializeV0(reader)
case 1:
return db.deserializeV1(reader)
case 2:
return db.deserializeV2(reader)
default:
return errors.Errorf("Authorization database format %d is not supported, please upgrade Doltgres", version)
}server/auth/serialization.go (lines 107-134)
func (db *Database) deserializeV1(reader *utils.Reader) error {
// ...
db.roleMembership.deserialize(1, reader)
// V1 has no default privileges; initialize empty
db.defaultPrivileges.deserialize(1, reader)
return nil
}server/auth/default_privileges.go (lines 250-256)
func (dp *DefaultPrivileges) deserialize(version uint32, reader *utils.Reader) {
dp.Data = make(map[DefaultPrivilegeKey]DefaultPrivilegeValue)
switch version {
case 0:
case 1:
case 2:⚠️ Inflated nested counters in auth.db should not exhaust startup resources
- What failed: Malformed count values can drive unbounded reads that panic with a slice-bounds runtime error instead of returning a bounded deserialize error.
- Impact: A malformed auth file can crash server startup and deny service until the file is manually repaired. This creates a reliability and availability risk in a core boot path.
- Steps to reproduce:
- Start from a valid auth.db.
- Inflate nested count and length bytes while keeping the auth header valid.
- Restart the server and observe startup behavior and logs.
- Stub / mock context: The run used a deliberate malformed auth.db variant with oversized nested counters to test startup safety boundaries, while SCRAM authentication and domain-cast checks remained bypassed for deterministic execution. After reproducing the panic behavior, a clean auth.db was restored to confirm recovery.
- Code analysis: I inspected the generic reader and auth decode loops; count and string length values are consumed from untrusted bytes and used for offset arithmetic/slicing without explicit bounds checks, enabling panic conditions during deserialize.
- Why this is likely a bug: The deserialize path uses attacker-controlled lengths without defensive bounds validation, so malformed auth bytes can trigger runtime panics in production startup logic.
Relevant code:
utils/reader.go (lines 195-200)
func (reader *Reader) String() string {
length := reader.VariableUint()
reader.offset += length
return string(reader.buf[reader.offset-length : reader.offset])
}server/auth/default_privileges.go (lines 256-275)
dataCount := reader.Uint64()
for i := uint64(0); i < dataCount; i++ {
dpv := DefaultPrivilegeValue{Grantees: make(map[RoleID]DefaultPrivilegeGranteeValue)}
dpv.Key.OwnerRole = RoleID(reader.Uint64())
dpv.Key.Schema = reader.String()
dpv.Key.ObjectType = PrivilegeObject(reader.Uint8())
granteeCount := reader.Uint64()
for j := uint64(0); j < granteeCount; j++ {
granteeValue := DefaultPrivilegeGranteeValue{
Grantee: RoleID(reader.Uint64()),
Privileges: make(map[Privilege]map[GrantedPrivilege]bool),
}server/auth/database.go (lines 165-178)
authData, err := fileSystem.ReadFile(authFileName)
if os.IsNotExist(err) {
// ...
} else if err != nil {
panic(err)
} else if err = globalDatabase.deserialize(authData); err != nil {
panic(err)
}🟠 Dropped role IDs yield inconsistent ACL display and enforcement
- What failed: ACL text projection resolves missing role IDs to empty names while privilege enforcement still treats stored grant entries as effective, so catalog visibility and actual permission behavior diverge.
- Impact: Users and operators can see misleading ACL metadata after role deletion while authorization decisions still allow operations. This weakens trust in access introspection and can cause incorrect privilege/audit conclusions.
- Steps to reproduce:
- Create default privileges where role A grants privileges to role B.
- Drop role A from the auth database.
- Query pg_catalog.pg_default_acl and compare ACL strings with actual privilege checks for role B.
- Stub / mock context: This conclusion came from direct production-code inspection of role-drop, ACL projection, and privilege-check logic rather than a mocked API response. A temporary auth/domain bypass patch in server/authentication_scram.go and server/analyzer/domain_constraints.go was active in the run environment.
- Code analysis: I reviewed role deletion, role-name lookup, ACL text rendering, and privilege evaluation paths under server/auth and server/tables/pgcatalog. DropRole deletes role maps but leaves privilege/default-ACL structures untouched, ACL rendering converts unresolved role IDs to empty names, and privilege checks only require a non-empty privilege map.
- Why this is likely a bug: The code keeps privileges keyed by dropped-role IDs and later renders those IDs as empty ACL names while still authorizing via non-empty privilege maps, producing an internally inconsistent and user-visible authorization model.
Relevant code:
server/auth/database.go (lines 65-71)
// DropRole removes the given role from the database. If the role does not exist, then this is a no-op.
func DropRole(name string) {
if roleID, ok := globalDatabase.rolesByName[name]; ok {
delete(globalDatabase.rolesByName, name)
delete(globalDatabase.rolesByID, roleID)
// TODO: remove from ownership, schema privileges, table privileges, and role membership
}
}server/tables/pgcatalog/pg_default_acl.go (lines 55-64)
granteeName := auth.GetRoleName(granteeValue.Grantee)
for priv, grantedMap := range granteeValue.Privileges {
for grantedPriv, withGrantOption := range grantedMap {
grantorName := auth.GetRoleName(grantedPriv.GrantedBy)
privStr := string(priv)
if withGrantOption {
privStr += "*"
}
aclItems = append(aclItems, fmt.Sprintf("%s=%s/%s", granteeName, privStr, grantorName))
}
}server/auth/table_privileges.go (lines 78-80)
if tablePrivilegeValue, ok := globalDatabase.tablePrivileges.Data[key]; ok {
if privilegeMap, ok := tablePrivilegeValue.Privileges[privilege]; ok && len(privilegeMap) > 0 {
return true
}
}⚠️ Unauthorized sequence drop succeeds after restart
- What failed: The non-owner grantee can execute
DROP SEQUENCEsuccessfully even when destructive permission was not granted; expected behavior is permission denial for non-granted destructive sequence actions. - Impact: Unauthorized users can delete sequences despite missing grants, breaking core authorization guarantees for destructive operations. This can remove schema objects without intended ownership or grant checks.
- Steps to reproduce:
- Create a sequence and grant only selected sequence privileges to a non-owner role.
- Restart with persisted auth data.
- Execute a granted operation (
nextval) and then attempt non-grantedDROP SEQUENCEas the grantee.
- Stub / mock context: Authentication and domain-constraint checks were bypassed in
server/authentication_scram.goandserver/analyzer/domain_constraints.goto keep the local run progressing after startup instability, and the scenario ran in a temporary containerized runtime. The observed authorization gap is still supported by direct production-code inspection of the drop-sequence execution path. - Code analysis: I reviewed sequence auth checks and the
DROP SEQUENCEpath.server/auth/auth_handler.godefinescheckPrivilegeOnSequence, butserver/node/drop_sequence.godrops the sequence directly through the collection API without invoking any sequence privilege check, and AST conversion wires directly to that node. - Why this is likely a bug: Production code provides sequence privilege-check logic but the destructive drop execution path bypasses it, creating an authorization gap that matches the observed behavior.
Relevant code:
server/node/drop_sequence.go (lines 66-105)
func (c *DropSequence) RowIter(ctx *sql.Context, r sql.Row) (sql.RowIter, error) {
schema, err := core.GetSchemaName(ctx, nil, c.schema)
if err != nil {
return nil, err
}
// ...
collection, err := core.GetSequencesCollectionFromContext(ctx, ctx.GetCurrentDatabase())
if err != nil {
return nil, err
}
sequenceID := id.NewSequence(schema, c.sequence)
// ...
if err = collection.DropSequence(ctx, sequenceID); err != nil {
return nil, err
}
return sql.RowsToRowIter(), nil
}server/auth/auth_handler.go (lines 333-350)
func checkPrivilegeOnSequence(state AuthorizationQueryState, schemaName, seqName string, privileges []Privilege) error {
roleSequenceKey := SequencePrivilegeKey{Role: state.role.ID(), Schema: schemaName, Name: seqName}
publicSequenceKey := SequencePrivilegeKey{Role: state.public.ID(), Schema: schemaName, Name: seqName}
for _, privilege := range privileges {
if !HasSequencePrivilege(roleSequenceKey, privilege) && !HasSequencePrivilege(publicSequenceKey, privilege) {
return errors.Errorf("permission denied for sequence %s", seqName)
}
}
return nil
}server/ast/drop_sequence.go (lines 41-45)
return vitess.InjectedStatement{
Statement: pgnodes.NewDropSequence(node.IfExists, name.SchemaQualifier.String(), name.Name.String(),
node.DropBehavior == tree.DropCascade),
Children: nil,
}, nilTell us how we did: Give Ito Feedback































No description provided.