Merge #155742

155742: server: fix SetAcceptSQLWithoutTLS for shared-process deployments r=yuzefovich a=yuzefovich

We expose `SetAcceptSQLWithoutTLS` method on the test servers which allows us to tweak whether SQL clients can authenticate without TLS on a secure cluster. Previously, this method was incorrect for shared-process deployments, and this is now fixed. The notable distinction between this mode and single-tenant and external-process modes is that even though the shared-process tenant has its own copy of `base.Config` (where `AcceptSQLWithoutTLS` is stored), it does _not_ have its own pre-serve handler. In other words, before the authentication is performed, the connection is handled by the _system_ tenant, so for things to work we need to propagate the update of the boolean to the system tenant.

Fixes: #112961.
Epic: CRDB-48945
Release note: None

Co-authored-by: Yahor Yuzefovich <yahor@cockroachlabs.com>

Author: craig[bot]

pkg/server/server_controller_new_server.go

@@ -238,6 +238,10 @@ func makeSharedProcessTenantServerConfig(
 	baseCfg.Config.Insecure = kvServerCfg.Config.Insecure
 	baseCfg.Config.User = kvServerCfg.Config.User
 	baseCfg.Config.DisableTLSForHTTP = kvServerCfg.Config.DisableTLSForHTTP
+	// Note that since the shared-process tenant doesn't have its own pre-serve
+	// handler, only AcceptSQLWithoutTLS of the _system_ tenant matters, so this
+	// particular inherited value is meaningless, but we choose to keep it for
+	// consistency with the system tenant.
 	baseCfg.Config.AcceptSQLWithoutTLS = kvServerCfg.Config.AcceptSQLWithoutTLS
 	baseCfg.Config.RPCHeartbeatInterval = kvServerCfg.Config.RPCHeartbeatInterval
 	baseCfg.Config.RPCHeartbeatTimeout = kvServerCfg.Config.RPCHeartbeatTimeout

pkg/server/testserver.go

@@ -1553,6 +1553,11 @@ func (t *testTenant) SetReady(ready bool) {
 // SetAcceptSQLWithoutTLS is part of the serverutils.ApplicationLayerInterface.
 func (t *testTenant) SetAcceptSQLWithoutTLS(accept bool) {
 	t.Cfg.AcceptSQLWithoutTLS = accept
+	// If we're running in a shared-process mode, the pre-serve handler has its
+	// own copy of base.Config (that is shared with the system tenant), so we
+	// must propagate the updated value there too. (For other deployments this
+	// call is redundant with the update above but otherwise harmless.)
+	t.pgPreServer.TestingSetAcceptSQLWithoutTLS(accept)
 }
 
 // PrivilegeChecker is part of the serverutils.ApplicationLayerInterface.

pkg/sql/pgwire/auth_test.go

@@ -872,12 +872,6 @@ func TestSSLSessionVar(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Set the system layer to accept SQL without TLS in the event of a shared
-	// process virtual cluster.
-	srv.SystemLayer().SetAcceptSQLWithoutTLS(true)
-
-	// TODO(herko): What effect should this have on a shared process virtual
-	// cluster? See: https://github.com/cockroachdb/cockroach/issues/112961
 	s.SetAcceptSQLWithoutTLS(true)
 
 	pgURLWithCerts, cleanupFuncCerts := s.PGUrl(t,

pkg/sql/pgwire/pre_serve.go

@@ -172,6 +172,12 @@ func (s *PreServeConnHandler) AnnotateCtxForIncomingConn(
 	return logtags.AddTag(ctx, tag, conn.RemoteAddr().String())
 }
 
+// TestingSetAcceptSQLWithoutTLS updates the config to tweak whether SQL clients
+// can authenticate without TLS on a secure cluster, in tests.
+func (s *PreServeConnHandler) TestingSetAcceptSQLWithoutTLS(accept bool) {
+	s.cfg.AcceptSQLWithoutTLS = accept
+}
+
 // tenantIndependentMetrics is the set of metrics for the
 // pre-serve part of connection handling, before the connection
 // is routed to a specific tenant.