Merge #151362

151362: sql: gate system database behind session variable r=angles-n-daemons a=angles-n-daemons

It today isn't quite clear which internal objects are safe for external usage. For example, selecting from crdb_internal.zones is considered safe, while deleting from system.jobs is not.

To address this, we're adding a gate protecting some of these internals, as well as auditing around their usage so that we can better see how the internal state of the database may have been modified.

The start of this effort is this PR, which adds an gate on the privilege check for system descriptors. This gate is simple now, it checks whether the caller is internal or if the override session variable is set to access "unsafe internals".

It should be noted that allowance is the default to start, so that extensive testing can be done on this feature before its rolled out more widely.

Fixes: #149594 
Fixes: #151333
Epic: CRDB-24527

Release note (sql change): Adds a new session variable `allow_unsafe_internals` and corresponding cluster setting (`sql.defaults.allow_unsafe_internals`) which controls access to the `system` database.

Co-authored-by: Brian Dillmann <brian.dillmann@cockroachlabs.com>

Author: craig[bot]

pkg/BUILD.bazel

@@ -662,6 +662,7 @@ ALL_TESTS = [
     "//pkg/sql/ttl/ttljob:ttljob_test",
     "//pkg/sql/types:types_disallowed_imports_test",
     "//pkg/sql/types:types_test",
+    "//pkg/sql/unsafesql:unsafesql_test",
     "//pkg/sql/vecindex/cspann/memstore:memstore_test",
     "//pkg/sql/vecindex/cspann/quantize:quantize_test",
     "//pkg/sql/vecindex/cspann/utils:utils_test",
@@ -2397,6 +2398,8 @@ GO_TARGETS = [
     "//pkg/sql/ttl/ttlschedule:ttlschedule",
     "//pkg/sql/types:types",
     "//pkg/sql/types:types_test",
+    "//pkg/sql/unsafesql:unsafesql",
+    "//pkg/sql/unsafesql:unsafesql_test",
     "//pkg/sql/vecindex/cspann/commontest:commontest",
     "//pkg/sql/vecindex/cspann/memstore:memstore",
     "//pkg/sql/vecindex/cspann/memstore:memstore_test",

pkg/sql/BUILD.bazel

@@ -546,6 +546,7 @@ go_library(
         "//pkg/sql/tablemetadatacache/util",
         "//pkg/sql/ttl/ttlbase",
         "//pkg/sql/types",
+        "//pkg/sql/unsafesql",
         "//pkg/sql/vecindex",
         "//pkg/sql/vecindex/vecpb",
         "//pkg/sql/vecindex/vecstore",

pkg/sql/authorization.go

@@ -32,6 +32,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"github.com/cockroachdb/cockroach/pkg/sql/syntheticprivilege"
+	"github.com/cockroachdb/cockroach/pkg/sql/unsafesql"
 	"github.com/cockroachdb/errors"
 )
 
@@ -121,6 +122,16 @@ func (p *planner) HasPrivilege(
 		return false, errors.AssertionFailedf("cannot use CheckPrivilege without a txn")
 	}
 
+	// Check for system table access restrictions before any admin bypasses
+	if d, ok := privilegeObject.(catalog.TableDescriptor); ok {
+		// Check for system table access restrictions before any admin bypasses
+		if catalog.IsSystemDescriptor(d) {
+			if err := unsafesql.CheckInternalsAccess(p.SessionData()); err != nil {
+				return false, err
+			}
+		}
+	}
+
 	// root, admin and node user should always have privileges, except NOSQLLOGIN.
 	// This allows us to short-circuit privilege checks for
 	// virtual object such that we don't have to query the system.privileges

pkg/sql/authorization_test.go

@@ -8,6 +8,7 @@ package sql_test
 import (
 	"context"
 	"fmt"
+	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -16,13 +17,17 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descs"
+	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
+	"github.com/cockroachdb/cockroach/pkg/sql/sqlerrors"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/skip"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/ctxgroup"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/errors"
+	"github.com/lib/pq"
 	"github.com/stretchr/testify/require"
 )
 
@@ -289,3 +294,84 @@ func TestEnsureUserOnlyBelongsToRoles(t *testing.T) {
 		require.Empty(t, getRoles(testUser))
 	})
 }
+
+func checkUnsafeErr(t *testing.T, err error) {
+	var pqErr *pq.Error
+
+	if errors.Is(err, sqlerrors.ErrUnsafeTableAccess) {
+		return
+	}
+
+	if errors.As(err, &pqErr) &&
+		pqErr.Code == "42501" &&
+		strings.Contains(pqErr.Message, "Access to crdb_internal and system is restricted") {
+		return
+	}
+
+	t.Fatal("expected unsafe access error, got", err)
+}
+
+func TestCheckUnsafeInternalsAccess(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	s := serverutils.StartServerOnly(t, base.TestServerArgs{
+		DefaultTestTenant: base.TestControlsTenantsExplicitly,
+	})
+	defer s.Stopper().Stop(ctx)
+
+	q := "SELECT * FROM system.namespace"
+
+	t.Run("as an external querier", func(t *testing.T) {
+		for _, test := range []struct {
+			AllowUnsafeInternals bool
+			Passes               bool
+		}{
+			{AllowUnsafeInternals: false, Passes: false},
+			{AllowUnsafeInternals: true, Passes: true},
+		} {
+			t.Run(fmt.Sprintf("%t", test), func(t *testing.T) {
+				conn := s.SQLConn(t)
+				_, err := conn.Exec("SET allow_unsafe_internals = $1", test.AllowUnsafeInternals)
+				require.NoError(t, err)
+
+				_, err = conn.Query(q)
+				if test.Passes {
+					require.NoError(t, err)
+				} else {
+					checkUnsafeErr(t, err)
+				}
+			})
+		}
+	})
+
+	t.Run("as an internal querier", func(t *testing.T) {
+		for _, test := range []struct {
+			AllowUnsafeInternals bool
+			Passes               bool
+		}{
+			{AllowUnsafeInternals: false, Passes: true},
+			{AllowUnsafeInternals: true, Passes: true},
+		} {
+			t.Run(fmt.Sprintf("%t", test), func(t *testing.T) {
+				idb := s.InternalDB().(isql.DB)
+				err := idb.Txn(ctx, func(ctx context.Context, txn isql.Txn) error {
+					txn.SessionData().LocalOnlySessionData.AllowUnsafeInternals = test.AllowUnsafeInternals
+
+					_, err := txn.QueryBuffered(ctx, "internal-query", txn.KV(), q)
+					return err
+				})
+
+				require.NoError(t, err)
+
+				if test.Passes {
+					require.NoError(t, err)
+				} else {
+					checkUnsafeErr(t, err)
+				}
+			})
+		}
+	})
+
+}

pkg/sql/exec_util.go

@@ -4383,6 +4383,10 @@ func (m *sessionDataMutator) SetUseProcTxnControlExtendedProtocolFix(val bool) {
 	m.data.UseProcTxnControlExtendedProtocolFix = val
 }
 
+func (m *sessionDataMutator) SetAllowUnsafeInternals(val bool) {
+	m.data.AllowUnsafeInternals = val
+}
+
 // Utility functions related to scrubbing sensitive information on SQL Stats.
 
 // quantizeCounts ensures that the Count field in the

pkg/sql/logictest/testdata/logic_test/information_schema

@@ -3936,6 +3936,7 @@ variable                                                         value
 allow_create_trigger_function_with_argv_references               off
 allow_ordinal_column_references                                  off
 allow_role_memberships_to_change_during_transaction              off
+allow_unsafe_internals                                           on
 alter_primary_region_super_region_override                       off
 always_distribute_full_scans                                     off
 application_name                                                 ·

pkg/sql/logictest/testdata/logic_test/pg_catalog

@@ -2946,6 +2946,7 @@ name                                                             setting
 allow_create_trigger_function_with_argv_references               off                 NULL      NULL        NULL        string
 allow_ordinal_column_references                                  off                 NULL      NULL        NULL        string
 allow_role_memberships_to_change_during_transaction              off                 NULL      NULL        NULL        string
+allow_unsafe_internals                                           on                  NULL      NULL        NULL        string
 alter_primary_region_super_region_override                       off                 NULL      NULL        NULL        string
 always_distribute_full_scans                                     off                 NULL      NULL        NULL        string
 application_name                                                 ·                   NULL      NULL        NULL        string
@@ -3187,6 +3188,7 @@ name                                                             setting
 allow_create_trigger_function_with_argv_references               off                 NULL  user     NULL      off                 off
 allow_ordinal_column_references                                  off                 NULL  user     NULL      off                 off
 allow_role_memberships_to_change_during_transaction              off                 NULL  user     NULL      off                 off
+allow_unsafe_internals                                           on                  NULL  user     NULL      on                  on
 alter_primary_region_super_region_override                       off                 NULL  user     NULL      off                 off
 always_distribute_full_scans                                     off                 NULL  user     NULL      off                 off
 application_name                                                 ·                   NULL  user     NULL      ·                   ·
@@ -3412,6 +3414,7 @@ name                                                             source  min_val
 allow_create_trigger_function_with_argv_references               NULL    NULL     NULL     NULL        NULL
 allow_ordinal_column_references                                  NULL    NULL     NULL     NULL        NULL
 allow_role_memberships_to_change_during_transaction              NULL    NULL     NULL     NULL        NULL
+allow_unsafe_internals                                           NULL    NULL     NULL     NULL        NULL
 alter_primary_region_super_region_override                       NULL    NULL     NULL     NULL        NULL
 always_distribute_full_scans                                     NULL    NULL     NULL     NULL        NULL
 application_name                                                 NULL    NULL     NULL     NULL        NULL

pkg/sql/logictest/testdata/logic_test/show_source

@@ -40,6 +40,7 @@ variable                                                         value
 allow_create_trigger_function_with_argv_references               off
 allow_ordinal_column_references                                  off
 allow_role_memberships_to_change_during_transaction              off
+allow_unsafe_internals                                           on
 alter_primary_region_super_region_override                       off
 always_distribute_full_scans                                     off
 application_name                                                 ·

pkg/sql/sessiondatapb/local_only_session_data.proto

@@ -729,6 +729,10 @@ message LocalOnlySessionData {
   // mutations), regardless of the table being multi-region.
   bool parallelize_multi_key_lookup_joins_only_on_mr_mutations = 182 [(gogoproto.customname) = "ParallelizeMultiKeyLookupJoinsOnlyOnMRMutations"];
 
+  // AllowUnsafeInternals allows the caller to access the crdb_internal
+  // or system databases within queries.
+  bool allow_unsafe_internals = 183;
+
   ///////////////////////////////////////////////////////////////////////////
   // WARNING: consider whether a session parameter you're adding needs to  //
   // be propagated to the remote nodes. If so, that parameter should live  //

pkg/sql/sqlerrors/errors.go

@@ -661,6 +661,7 @@ var (
 	ErrNoType            = pgerror.New(pgcode.InvalidName, "no type specified")
 	ErrNoFunction        = pgerror.New(pgcode.InvalidName, "no function specified")
 	ErrNoMatch           = pgerror.New(pgcode.UndefinedObject, "no object matched")
+	ErrUnsafeTableAccess = errors.WithHint(pgerror.New(pgcode.InsufficientPrivilege, "Access to crdb_internal and system is restricted."), "These interfaces are unsupported in production. To proceed, set the session variable allow_unsafe_internals = true (not recommended), or contact Cockroach Labs for a supported alternative.")
 )
 
 var ErrNoZoneConfigApplies = errors.New("no zone config applies")