Merge #158085

158085: sql: set the default to allow_unsafe_internals to false r=angles-n-daemons a=angles-n-daemons

The system and crdb_internal namespaces have historically been open access for anyone who wants to use them. This has caused problems in the past, as many of the objects in these namespaces were developed for internal use only, and have been discovered and misused by operators, which have created difficult to recover from failures.

To avoid this in the future, and provide better visibility into use of these objects, we'll begin gating access to them, requiring customers to manually override access to these records, and auditing visibly on their usage.

This PR is the final step in this process, changing the default of this already existing gate, and merging a few utilities for testing and extenuating overrides.

Fixes: #149595
Epic: CRDB-55276
Release note (ops change): All queries to system and crdb_internal by default will begin failing, notifying users that they must override the access gate if they wish to use those namespaces.

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

Author: craig[bot]

.github/workflows/github-actions-essential-ci.yml

@@ -122,7 +122,7 @@ jobs:
         with:
           path: examples-orms
           repository: cockroachdb/examples-orms
-          ref: 876b2d52ae2b63aa9cc1741c8d189ff0b66ab0d7
+          ref: 4422aff128585c3e0558b530558daa31972c3a40
       - name: compute metadata
         run: echo GITHUB_ACTIONS_BRANCH=${{ github.event.pull_request.number || github.ref_name}} >> "$GITHUB_ENV"
       - run: ./cockroach/build/github/get-engflow-keys.sh

pkg/acceptance/compose/flyway/docker-compose.yml

@@ -5,7 +5,7 @@ services:
     image: us-east1-docker.pkg.dev/crl-docker-sync/docker-io/library/ubuntu:xenial-20210804
     command: /cockroach/cockroach start-single-node --insecure --listen-addr cockroach
     environment:
-      - COCKROACH_ACCEPTANCE_ALLOW_UNSAFE=true
+      - COCKROACH_OVERRIDE_ALLOW_UNSAFE_INTERNALS=true
     volumes:
       - ${COCKROACH_BINARY:-../../../../cockroach-linux-2.6.32-gnu-amd64}:/cockroach/cockroach
   flyway:

pkg/acceptance/compose/gss/docker-compose-python.yml

@@ -13,7 +13,7 @@ services:
     command: /cockroach/cockroach --certs-dir=/certs start-single-node --listen-addr cockroach
     environment:
       - KRB5_KTNAME=/keytab/crdb.keytab
-      - COCKROACH_ACCEPTANCE_ALLOW_UNSAFE=true
+      - COCKROACH_OVERRIDE_ALLOW_UNSAFE_INTERNALS=true
     volumes:
       - ${CERTS_DIR:-../../.localcluster.certs}:/certs
       - keytab:/keytab

pkg/acceptance/compose/gss/docker-compose.yml

@@ -14,7 +14,7 @@ services:
     command: /cockroach/cockroach --certs-dir=/certs start-single-node --listen-addr cockroach
     environment:
       - KRB5_KTNAME=/keytab/crdb.keytab
-      - COCKROACH_ACCEPTANCE_ALLOW_UNSAFE=true
+      - COCKROACH_OVERRIDE_ALLOW_UNSAFE_INTERNALS=true
     volumes:
       - ${CERTS_DIR:-../../.localcluster.certs}:/certs
       - keytab:/keytab

pkg/cli/democluster/demo_cluster.go

@@ -964,6 +964,11 @@ func (demoCtx *Context) testServerArgsForTransientCluster(
 		args.SSLCertsDir = demoDir
 	}
 
+	// Allow access to system and crdb_internal tables in demo mode.
+	// Demo mode is for development/testing, so we want to allow access
+	// to these tables for debugging and introspection.
+	serverutils.SetUnsafeOverride(&args.Knobs)
+
 	return args
 }
 

pkg/cli/democluster/demo_cluster_test.go

@@ -140,6 +140,10 @@ func TestTestServerArgsForTransientCluster(t *testing.T) {
 			actual.StoreSpecs = nil
 			actual.Knobs.JobsTestingKnobs = nil
 
+			// Copy the SQLEvalContext from actual to expected since it's set by SetUnsafeOverride
+			// and contains function pointers that we can't predict
+			tc.expected.Knobs.SQLEvalContext = actual.Knobs.SQLEvalContext
+
 			assert.Equal(t, tc.expected, actual)
 		})
 	}

pkg/cli/interactive_tests/common.tcl

@@ -9,7 +9,7 @@ system "mkdir -p logs"
 set histfile "cockroach_sql_history"
 
 set ::env(COCKROACH_SKIP_ENABLING_DIAGNOSTIC_REPORTING) "true"
-set ::env(COCKROACH_ACCEPTANCE_ALLOW_UNSAFE) "true"
+set ::env(COCKROACH_OVERRIDE_ALLOW_UNSAFE_INTERNALS) "true"
 set ::env(COCKROACH_CONNECT_TIMEOUT) 15
 set ::env(COCKROACH_SQL_CLI_HISTORY) $histfile
 # Set client commands as insecure. The server uses --insecure.

pkg/server/server_controller_new_server.go

@@ -22,6 +22,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/storage/fs"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util"
 	"github.com/cockroachdb/cockroach/pkg/util/admission"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -84,6 +85,16 @@ func (s *topLevelServer) newTenantServer(
 	// Apply the TestTenantArgs, if any.
 	baseCfg.TestingKnobs = testArgs.Knobs
 
+	// If we're in a test environment (the parent server has testing knobs), apply unsafe override
+	// for dynamically started tenants to allow access to crdb_internal and system tables.
+	// This is needed because tests might start tenants via SQL (ALTER TENANT ... START SERVICE SHARED)
+	// without explicitly setting up test args.
+	if s.cfg.TestingKnobs != (base.TestingKnobs{}) && baseCfg.TestingKnobs == (base.TestingKnobs{}) {
+		// Only set unsafe override if no testing knobs were provided through testArgs.
+		// This ensures we don't override explicitly set test knobs.
+		serverutils.SetUnsafeOverride(&baseCfg.TestingKnobs)
+	}
+
 	tenantServer, err := newTenantServerInternal(ctx, baseCfg, sqlCfg, tenantStopper, tenantNameContainer, s.db.AdmissionPacerFactory)
 	if err != nil {
 		return nil, err

pkg/server/tenant_delayed_id_set_test.go

@@ -73,6 +73,9 @@ func TestStartTenantWithDelayedID(t *testing.T) {
 		}, nil
 	}
 
+	// Allow unsafe introspection.
+	serverutils.SetUnsafeOverride(&baseCfg.TestingKnobs)
+
 	go func() {
 		sw, err := NewSeparateProcessTenantServer(
 			ctx,

pkg/server/testserver.go

@@ -700,6 +700,7 @@ func (ts *testServer) setupTenantTestingKnobs(tenantKnobs *base.TestingKnobs) {
 		}
 		tenantKnobs.Server.(*TestingKnobs).StubTimeNow = ts.params.Knobs.Server.(*TestingKnobs).StubTimeNow
 	}
+	serverutils.SetUnsafeOverride(tenantKnobs)
 	if ts.params.Knobs.UpgradeManager != nil {
 		tenantKnobs.UpgradeManager.(*upgradebase.TestingKnobs).SkipSomeUpgradeSteps = ts.params.Knobs.UpgradeManager.(*upgradebase.TestingKnobs).SkipSomeUpgradeSteps
 	}
@@ -1394,6 +1395,10 @@ func (ts *testServer) StartSharedProcessTenant(
 		_, err := ie.ExecEx(ctx, opName, nil /* txn */, sessiondata.NodeUserSessionDataOverride, stmt, qargs...)
 		return err
 	}
+
+	// Allow access to unsafe internals for the tenant server in test environments.
+	serverutils.SetUnsafeOverride(&args.Knobs)
+
 	// Save the args for use if the server needs to be created.
 	func() {
 		ts.topLevelServer.serverController.mu.Lock()
@@ -1778,6 +1783,9 @@ func (ts *testServer) StartTenant(
 		stopper.SetTracer(tr)
 	}
 
+	// Allow access to unsafe internals on this tenant.
+	serverutils.SetUnsafeOverride(&params.TestingKnobs)
+
 	baseCfg := makeTestBaseConfig(st, stopper.Tracer())
 	baseCfg.TestingKnobs = params.TestingKnobs
 	baseCfg.Insecure = params.ForceInsecure