Skip to content

Commit 8d77202

Browse files
unsafesql: report break glass usage of unsafe internals to the sql_exec log
The epic below outlines a set of work to prevent users from unauthorized access to what we deem unsafe internals. These unsafe internals lie mostly in the crdb_internal schema and the system database. This PR adds a log to SENSITIVE_ACCECSS to audit each time the operator breaks glass and reaches into the unsafe internals, as well as a log when a user is denied access to those records. There is refactoring as part of this PR, specifically the transformation of a statement to a redactable string is pulled out of the sql package and into the tree package, so that it could be leveraged by the optbuilder package (which cannot depend on sql, as sql depends on it). Fixes: 151488 Epic: CRDB-24527 Release note (ops change): A log will be emitted now to the SENSITIVE_ACCESS channel both when users override the allow_unsafe_internals as well as when they are denied access to the same unsafe records.
1 parent a78d085 commit 8d77202

File tree

18 files changed

+293
-86
lines changed

18 files changed

+293
-86
lines changed

docs/generated/eventlog.md

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -853,6 +853,56 @@ a table marked as audited.
853853
| `BulkJobId` | The job id for bulk job (IMPORT/BACKUP/RESTORE). | no |
854854
| `StmtPosInTxn` | The statement's index in the transaction, starting at 1. | no |
855855

856+
### `unsafe_internals_accessed`
857+
858+
UnsafeInternalsAccess is recorded when a query accesses unsafe internals
859+
using the allow_unsafe_internals override.
860+
861+
862+
| Field | Description | Sensitive |
863+
|--|--|--|
864+
| `Query` | The query that triggered the unsafe internals access. | partially |
865+
866+
867+
#### Common fields
868+
869+
| Field | Description | Sensitive |
870+
|--|--|--|
871+
| `Timestamp` | The timestamp of the event. Expressed as nanoseconds since the Unix epoch. | no |
872+
| `EventType` | The type of the event. | no |
873+
| `Statement` | A normalized copy of the SQL statement that triggered the event. The statement string contains a mix of sensitive and non-sensitive details (it is redactable). | partially |
874+
| `Tag` | The statement tag. This is separate from the statement string, since the statement string can contain sensitive information. The tag is guaranteed not to. | no |
875+
| `User` | The user account that triggered the event. The special usernames `root` and `node` are not considered sensitive. | depends |
876+
| `DescriptorID` | The primary object descriptor affected by the operation. Set to zero for operations that don't affect descriptors. | no |
877+
| `ApplicationName` | The application name for the session where the event was emitted. This is included in the event to ease filtering of logging output by application. | no |
878+
| `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
879+
| `TxnReadTimestamp` | The current read timestamp of the transaction that triggered the event, if in a transaction. | no |
880+
881+
### `unsafe_internals_denied`
882+
883+
An event of type `unsafe_internals_denied` is recorded when a query attempts to access unsafe internals
884+
but lacks the appropriate session variables.
885+
886+
887+
| Field | Description | Sensitive |
888+
|--|--|--|
889+
| `Query` | The query that triggered the unsafe internals access. | partially |
890+
891+
892+
#### Common fields
893+
894+
| Field | Description | Sensitive |
895+
|--|--|--|
896+
| `Timestamp` | The timestamp of the event. Expressed as nanoseconds since the Unix epoch. | no |
897+
| `EventType` | The type of the event. | no |
898+
| `Statement` | A normalized copy of the SQL statement that triggered the event. The statement string contains a mix of sensitive and non-sensitive details (it is redactable). | partially |
899+
| `Tag` | The statement tag. This is separate from the statement string, since the statement string can contain sensitive information. The tag is guaranteed not to. | no |
900+
| `User` | The user account that triggered the event. The special usernames `root` and `node` are not considered sensitive. | depends |
901+
| `DescriptorID` | The primary object descriptor affected by the operation. Set to zero for operations that don't affect descriptors. | no |
902+
| `ApplicationName` | The application name for the session where the event was emitted. This is included in the event to ease filtering of logging output by application. | no |
903+
| `PlaceholderValues` | The mapping of SQL placeholders to their values, for prepared statements. | yes |
904+
| `TxnReadTimestamp` | The current read timestamp of the transaction that triggered the event, if in a transaction. | no |
905+
856906
## SQL Execution Log
857907

858908
Events in this category report executed queries.

pkg/cli/interactive_tests/test_audit_log.tcl

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,14 +9,9 @@ eexpect root@
99

1010
set logfile logs/db/logs/cockroach-sql-audit.log
1111

12-
start_test "Check that the audit log is not created by default"
13-
system "if test -e $logfile; then false; fi"
14-
end_test
15-
1612
start_test "Check that statements do not get logged to the audit log directly"
1713
send "CREATE DATABASE t; USE t; CREATE TABLE helloworld(abc INT) WITH (schema_locked=false); INSERT INTO helloworld VALUES (123);\r"
1814
eexpect root@
19-
system "if test -e $logfile; then false; fi"
2015
end_test
2116

2217
start_test "Check that statements start being logged synchronously if auditing is enabled"

pkg/sql/authorization.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ func (p *planner) HasPrivilege(
125125
// Do a safety check on the object, if it is considered unsafe
126126
// does the caller have the appropriate session data to access it?
127127
if p.objectIsUnsafe(ctx, privilegeObject) {
128-
if err := unsafesql.CheckInternalsAccess(p.SessionData()); err != nil {
128+
if err := unsafesql.CheckInternalsAccess(ctx, p.SessionData(), p.stmt.AST, p.extendedEvalCtx.Annotations, &p.ExecCfg().Settings.SV); err != nil {
129129
return false, err
130130
}
131131
}

pkg/sql/copy_from.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1248,6 +1248,12 @@ func (c *copyMachine) insertRowsInternal(ctx context.Context, finalBatch bool) (
12481248
Returning: tree.AbsentReturningClause,
12491249
}
12501250

1251+
// Initialize annotations for the statement. This is required for proper
1252+
// error handling and logging during plan optimization. Since this is a
1253+
// synthetic INSERT statement, we provide an empty annotations array.
1254+
c.p.semaCtx.Annotations = tree.MakeAnnotations(0)
1255+
c.p.extendedEvalCtx.Annotations = &c.p.semaCtx.Annotations
1256+
12511257
// TODO(cucaroach): We shouldn't need to do this for every batch.
12521258
if err := c.p.makeOptimizerPlan(ctx); err != nil {
12531259
return err

pkg/sql/exec_util.go

Lines changed: 1 addition & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -4455,38 +4455,11 @@ func scrubStmtStatKey(vt VirtualTabler, key string, ns eval.ClientNoticeSender)
44554455
return f.CloseAndGetString(), true
44564456
}
44574457

4458-
var redactNamesInSQLStatementLog = settings.RegisterBoolSetting(
4459-
settings.ApplicationLevel,
4460-
"sql.log.redact_names.enabled",
4461-
"if set, schema object identifers are redacted in SQL statements that appear in event logs",
4462-
false,
4463-
settings.WithPublic,
4464-
)
4465-
44664458
// FormatAstAsRedactableString implements scbuild.AstFormatter
44674459
func (p *planner) FormatAstAsRedactableString(
44684460
statement tree.Statement, annotations *tree.Annotations,
44694461
) redact.RedactableString {
4470-
fs := tree.FmtSimple | tree.FmtAlwaysQualifyTableNames | tree.FmtMarkRedactionNode
4471-
if !redactNamesInSQLStatementLog.Get(&p.extendedEvalCtx.Settings.SV) {
4472-
fs = fs | tree.FmtOmitNameRedaction
4473-
}
4474-
return formatStmtKeyAsRedactableString(statement, annotations, fs)
4475-
}
4476-
4477-
// formatStmtKeyAsRedactableString given an AST node this function will fully
4478-
// qualify names using annotations to format it out into a redactable string.
4479-
// Object names are not redacted, but constants and datums are.
4480-
func formatStmtKeyAsRedactableString(
4481-
rootAST tree.Statement, ann *tree.Annotations, fs tree.FmtFlags,
4482-
) redact.RedactableString {
4483-
f := tree.NewFmtCtx(
4484-
fs,
4485-
tree.FmtAnnotations(ann),
4486-
)
4487-
f.FormatNode(rootAST)
4488-
formattedRedactableStatementString := f.CloseAndGetString()
4489-
return redact.RedactableString(formattedRedactableStatementString)
4462+
return tree.FormatAstAsRedactableString(statement, annotations, &p.extendedEvalCtx.Settings.SV)
44904463
}
44914464

44924465
// FailedHashedValue is used as a default return value for when HashForReporting

pkg/sql/opt/optbuilder/builder.go

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,7 @@ func (b *Builder) buildStmt(
504504
// invalidation). We don't care about caching plans for these statements.
505505
b.DisableMemoReuse = true
506506
// It's considered acceptable when we delegate to unsafe internals.
507-
defer b.disableUnsafeInternalCheck()()
507+
defer b.DisableUnsafeInternalCheck()()
508508
return b.buildStmt(newStmt, desiredTypes, inScope)
509509
}
510510

@@ -590,15 +590,20 @@ func (b *Builder) maybeTrackUserDefinedTypeDepsForViews(texpr tree.TypedExpr) {
590590
}
591591
}
592592

593-
// disableUnsafeInternalCheck is used to disable the check that the
593+
// DisableUnsafeInternalCheck is used to disable the check that the
594594
// prevents external users from accessing unsafe internals.
595-
func (b *Builder) disableUnsafeInternalCheck() func() {
595+
func (b *Builder) DisableUnsafeInternalCheck() func() {
596596
b.skipUnsafeInternalsCheck = true
597-
cleanup := b.catalog.DisableUnsafeInternalCheck()
597+
var cleanup func()
598+
if b.catalog != nil {
599+
cleanup = b.catalog.DisableUnsafeInternalCheck()
600+
}
598601

599602
return func() {
600603
b.skipUnsafeInternalsCheck = false
601-
cleanup()
604+
if cleanup != nil {
605+
cleanup()
606+
}
602607
}
603608
}
604609

pkg/sql/opt/optbuilder/scalar.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -550,7 +550,7 @@ func (b *Builder) buildFunction(
550550
return b.buildUDF(f, def, inScope, outScope, outCol, colRefs)
551551
}
552552
if b.isUnsafeBuiltin(overload, def) {
553-
if err := unsafesql.CheckInternalsAccess(b.evalCtx.SessionData()); err != nil {
553+
if err := unsafesql.CheckInternalsAccess(b.ctx, b.evalCtx.SessionData(), b.stmt, b.evalCtx.Annotations, &b.evalCtx.Settings.SV); err != nil {
554554
panic(err)
555555
}
556556
}

pkg/sql/opt/testutils/opttester/opt_tester.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2316,6 +2316,7 @@ func (ot *OptTester) buildExpr(factory *norm.Factory) error {
23162316
ot.semaCtx.Placeholders.Init(stmt.NumPlaceholders, nil /* typeHints */)
23172317
ot.semaCtx.Annotations = tree.MakeAnnotations(stmt.NumAnnotations)
23182318
ot.semaCtx.TypeResolver = ot.catalog
2319+
ot.evalCtx.Annotations = &ot.semaCtx.Annotations
23192320
b := optbuilder.New(ot.ctx, &ot.semaCtx, &ot.evalCtx, ot.catalog, factory, stmt.AST)
23202321
return b.Build()
23212322
}

pkg/sql/sem/eval/eval_test.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ func optBuildScalar(evalCtx *eval.Context, e tree.Expr) (tree.TypedExpr, error)
9595
o.Init(ctx, evalCtx, nil /* catalog */)
9696
semaCtx := tree.MakeSemaContext(nil /* resolver */)
9797
b := optbuilder.NewScalar(ctx, &semaCtx, evalCtx, o.Factory())
98+
defer b.DisableUnsafeInternalCheck()()
9899
scalar, err := b.Build(e)
99100
if err != nil {
100101
return nil, err

pkg/sql/sem/tree/BUILD.bazel

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,7 @@ go_library(
8686
"prepare.go",
8787
"pretty.go",
8888
"reassign_owned_by.go",
89+
"redact_ast.go",
8990
"regexp_cache.go",
9091
"region.go",
9192
"rename.go",
@@ -140,6 +141,7 @@ go_library(
140141
"//pkg/geo",
141142
"//pkg/geo/geopb",
142143
"//pkg/kv/kvserver/concurrency/isolation",
144+
"//pkg/settings",
143145
"//pkg/sql/lex",
144146
"//pkg/sql/lexbase",
145147
"//pkg/sql/oidext",

0 commit comments

Comments
 (0)