Skip to content

Commit b32b3c2

Browse files
sql: prevent delegate behavior from being blocked by the unsafesql gate
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 prevents that gate from being checked within delegate query behavior, thus allowing users to issue delegates from commands like `SHOW DATABASES`. Fixes: #151406 Epic: CRDB-24527 Release note (ops change): Delegate queries (like SHOW etc) are excluded from unsafesql checks to accessing the system or crdb_internal tables. 
1 parent 835eda7 commit b32b3c2

File tree

8 files changed

+94
-19
lines changed

8 files changed

+94
-19
lines changed

pkg/sql/authorization.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -952,6 +952,10 @@ func (p *planner) HasViewActivityOrViewActivityRedactedRole(
952952
// objectIsUnsafe checks if the privilege object is considered unsafe for external usage.
953953
// Unsafe objects are any system tables, and crdb_internal tables which are not listed as externally supported.
954954
func (p *planner) objectIsUnsafe(ctx context.Context, privilegeObject privilege.Object) bool {
955+
if p.skipUnsafeInternalsCheck {
956+
return false
957+
}
958+
955959
d, ok := privilegeObject.(catalog.TableDescriptor)
956960
if !ok {
957961
return true

pkg/sql/authorization_test.go

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ package sql_test
77

88
import (
99
"context"
10+
gosql "database/sql"
1011
"fmt"
1112
"strings"
1213
"sync"
@@ -321,6 +322,14 @@ func TestCheckUnsafeInternalsAccess(t *testing.T) {
321322
})
322323
defer s.Stopper().Stop(ctx)
323324

325+
// helper func for setting a safe connection.
326+
safeConn := func(t *testing.T) *gosql.DB {
327+
conn := s.SQLConn(t)
328+
_, err := conn.Exec("SET allow_unsafe_internals = false")
329+
require.NoError(t, err)
330+
return conn
331+
}
332+
324333
t.Run("accessing the system database", func(t *testing.T) {
325334
q := "SELECT * FROM system.namespace"
326335

@@ -378,47 +387,66 @@ func TestCheckUnsafeInternalsAccess(t *testing.T) {
378387

379388
t.Run("accessing the crdb_internal schema", func(t *testing.T) {
380389
t.Run("supported table allowed", func(t *testing.T) {
381-
conn := s.SQLConn(t)
382-
_, err := conn.Exec("SET allow_unsafe_internals = false")
383-
require.NoError(t, err)
390+
conn := safeConn(t)
384391

385392
// Supported crdb_internal tables should be allowed even when allow_unsafe_internals = false
386-
_, err = conn.Query("SELECT * FROM crdb_internal.zones")
393+
_, err := conn.Query("SELECT * FROM crdb_internal.zones")
387394
require.NoError(t, err, "supported crdb_internal table (zones) should be accessible when allow_unsafe_internals = false")
388395
})
389396

390397
t.Run("unsupported table denied", func(t *testing.T) {
391-
conn := s.SQLConn(t)
392-
_, err := conn.Exec("SET allow_unsafe_internals = false")
393-
require.NoError(t, err)
398+
conn := safeConn(t)
394399

395400
// Unsupported crdb_internal tables should be denied when allow_unsafe_internals = false
396-
_, err = conn.Query("SELECT * FROM crdb_internal.gossip_alerts")
401+
_, err := conn.Query("SELECT * FROM crdb_internal.gossip_alerts")
397402
checkUnsafeErr(t, err)
398403
})
399404
})
400405

401-
// The functionality for this lies in the pkg/sql/optbuilder/scalar.go
402-
// file, but it is tested here as that package does not setup a test
403-
// server.
406+
// The functionality for this lies in the optbuilder package file,
407+
// but it is tested here as that package does not setup a test server.
404408
t.Run("accessing crdb_internal builtins", func(t *testing.T) {
405409
t.Run("non crdb_internal builtin allowed", func(t *testing.T) {
406-
conn := s.SQLConn(t)
407-
_, err := conn.Exec("SET allow_unsafe_internals = false")
408-
require.NoError(t, err)
410+
conn := safeConn(t)
409411

410412
// Non crdb_internal tables should be allowed.
411-
_, err = conn.Query("SELECT * FROM generate_series(1,5)")
413+
_, err := conn.Query("SELECT * FROM generate_series(1,5)")
412414
require.NoError(t, err)
413415
})
414416

415417
t.Run("crdb_internal builtin not allowed", func(t *testing.T) {
416-
conn := s.SQLConn(t)
417-
_, err := conn.Exec("SET allow_unsafe_internals = false")
418-
require.NoError(t, err)
418+
conn := safeConn(t)
419419

420420
// Unsupported crdb_internal builtins should be denied.
421-
_, err = conn.Query("SELECT * FROM crdb_internal.tenant_span_stats()")
421+
_, err := conn.Query("SELECT * FROM crdb_internal.tenant_span_stats()")
422+
checkUnsafeErr(t, err)
423+
})
424+
})
425+
426+
// The functionality for this check also lives in the optbuilder package
427+
// but is tested here.
428+
t.Run("skips delegation", func(t *testing.T) {
429+
t.Run("delegation is allowed", func(t *testing.T) {
430+
conn := safeConn(t)
431+
432+
// tests delegation to builtins
433+
_, err := conn.Exec("show grants")
434+
require.NoError(t, err)
435+
436+
// tests delegation to crdb_internal tables
437+
_, err = conn.Exec("show databases")
438+
require.NoError(t, err)
439+
})
440+
441+
t.Run("underlying tables which delegates rely on are not", func(t *testing.T) {
442+
conn := safeConn(t)
443+
444+
// tests delegation to builtins
445+
_, err := conn.Exec("SELECT * FROM crdb_internal.privilege_name('DELETE')")
446+
checkUnsafeErr(t, err)
447+
448+
// tests delegation to crdb_internal tables
449+
_, err = conn.Exec("SELECT * FROM crdb_internal.databases")
422450
checkUnsafeErr(t, err)
423451
})
424452
})

pkg/sql/opt/cat/catalog.go

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,4 +286,7 @@ type Catalog interface {
286286

287287
// IsOwner returns true if user is the owner of the object o
288288
IsOwner(ctx context.Context, o Object, user username.SQLUsername) (bool, error)
289+
290+
// DisableUnsafeInternalsCheck disables the check for unsafe internals in the catalog.
291+
DisableUnsafeInternalCheck() func()
289292
}

pkg/sql/opt/optbuilder/builder.go

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,10 @@ type Builder struct {
202202
// built, and can be safely reused across different call-sites within the same
203203
// memo.
204204
builtTriggerFuncs map[cat.StableID][]cachedTriggerFunc
205+
206+
// skipUnsafeInternalsCheck is used to skip the check that the
207+
// planner is not used for unsafe internal statements.
208+
skipUnsafeInternalsCheck bool
205209
}
206210

207211
// New creates a new Builder structure initialized with the given
@@ -499,6 +503,8 @@ func (b *Builder) buildStmt(
499503
// register all those dependencies with the metadata (for cache
500504
// invalidation). We don't care about caching plans for these statements.
501505
b.DisableMemoReuse = true
506+
// It's considered acceptable when we delegate to unsafe internals.
507+
defer b.disableUnsafeInternalCheck()()
502508
return b.buildStmt(newStmt, desiredTypes, inScope)
503509
}
504510

@@ -584,6 +590,18 @@ func (b *Builder) maybeTrackUserDefinedTypeDepsForViews(texpr tree.TypedExpr) {
584590
}
585591
}
586592

593+
// disableUnsafeInternalCheck is used to disable the check that the
594+
// prevents external users from accessing unsafe internals.
595+
func (b *Builder) disableUnsafeInternalCheck() func() {
596+
b.skipUnsafeInternalsCheck = true
597+
cleanup := b.catalog.DisableUnsafeInternalCheck()
598+
599+
return func() {
600+
b.skipUnsafeInternalsCheck = false
601+
cleanup()
602+
}
603+
}
604+
587605
// optTrackingTypeResolver is a wrapper around a TypeReferenceResolver that
588606
// remembers all of the resolved types in the provided Metadata.
589607
type optTrackingTypeResolver struct {

pkg/sql/opt/optbuilder/scalar.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -890,6 +890,10 @@ func (b *Builder) constructUnary(
890890
func (b *Builder) isUnsafeBuiltin(
891891
overload *tree.Overload, def *tree.ResolvedFunctionDefinition,
892892
) bool {
893+
if b.skipUnsafeInternalsCheck {
894+
return false
895+
896+
}
893897
if overload.Type != tree.BuiltinRoutine {
894898
return false
895899
}

pkg/sql/opt/testutils/testcat/test_catalog.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -551,6 +551,11 @@ func (tc *Catalog) ExecuteMultipleDDL(sql string) error {
551551
return nil
552552
}
553553

554+
// DisableUnsafeInternalCheck is part of the cat.Catalog interface.
555+
func (tc *Catalog) DisableUnsafeInternalCheck() func() {
556+
return func() {}
557+
}
558+
554559
// ExecuteDDL parses the given DDL SQL statement and creates objects in the test
555560
// catalog. This is used to test without spinning up a cluster.
556561
func (tc *Catalog) ExecuteDDL(sql string) (string, error) {

pkg/sql/opt_catalog.go

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -742,6 +742,15 @@ func (oc *optCatalog) codec() keys.SQLCodec {
742742
return oc.planner.ExecCfg().Codec
743743
}
744744

745+
// DisableUnsafeInternalCheck sets the planners skipUnsafeInternalsCheck
746+
// to true, and returns a function which reverses it to false.
747+
func (oc *optCatalog) DisableUnsafeInternalCheck() func() {
748+
oc.planner.skipUnsafeInternalsCheck = true
749+
return func() {
750+
oc.planner.skipUnsafeInternalsCheck = false
751+
}
752+
}
753+
745754
// optView is a wrapper around catalog.TableDescriptor that implements
746755
// the cat.Object, cat.DataSource, and cat.View interfaces.
747756
type optView struct {

pkg/sql/planner.go

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -305,6 +305,10 @@ type planner struct {
305305
// statement. It's similar to autoRetryCounter / txnState.mu.autoRetryCounter
306306
// but for statement retries.
307307
autoRetryStmtCounter int
308+
309+
// skipUnsafeInternalsCheck is used to skip the check that the
310+
// planner is not used for unsafe internal statements.
311+
skipUnsafeInternalsCheck bool
308312
}
309313

310314
// hasFlowForPausablePortal returns true if the planner is for re-executing a

0 commit comments

Comments
 (0)