Merge #71134

71134: security: remove the node TLS client cert exemption r=catj-cockroach,andy-kimball a=knz

Fixes #71102.

Release note (security update): It is not possible any more to use a
node TLS certificate to establish a SQL connection with another
username than `node`. This facility had existed as an "escape hatch"
so that an operator could use the node cert to perform operations on
behalf of another SQL user. However, this facility is not necessary:
an operator with access to a node cert can log in as `node` directly
and create new credentials for another user anyway. By removing
this facility, we tighten the guarantee that the principal in the TLS
client cert always matches the SQL identity.

Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/acceptance/cluster/certs.go

@@ -39,7 +39,7 @@ func GenerateCerts(ctx context.Context) func() {
 	// Root user.
 	maybePanic(security.CreateClientPair(
 		certsDir, filepath.Join(certsDir, security.EmbeddedCAKey),
-		1024, 48*time.Hour, false, security.RootUserName(), true /* generate pk8 key */))
+		2048, 48*time.Hour, false, security.RootUserName(), true /* generate pk8 key */))
 
 	// Test user.
 	maybePanic(security.CreateClientPair(

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

@@ -20,13 +20,10 @@ services:
     build: ./python
     depends_on:
       - cockroach
-    command: /start.sh
     environment:
       - PGHOST=cockroach
       - PGPORT=26257
       - PGSSLMODE=require
-      - PGSSLCERT=/certs/node.crt
-      - PGSSLKEY=/certs/node.key
     volumes:
       - ./kdc/krb5.conf:/etc/krb5.conf
       - ./python/start.sh:/start.sh

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

@@ -23,8 +23,6 @@ services:
     environment:
       - PGHOST=cockroach
       - PGPORT=26257
-      - PGSSLCERT=/certs/node.crt
-      - PGSSLKEY=/certs/node.key
     volumes:
       - ./kdc/krb5.conf:/etc/krb5.conf
       - ./psql/gss_test.go:/test/gss_test.go

pkg/acceptance/compose/gss/psql/gss_test.go

@@ -35,7 +35,7 @@ func init() {
 }
 
 func TestGSS(t *testing.T) {
-	connector, err := pq.NewConnector("user=root sslmode=require")
+	connector, err := pq.NewConnector("user=root password=rootpw sslmode=require")
 	if err != nil {
 		t.Fatal(err)
 	}

pkg/acceptance/compose/gss/psql/start.sh

@@ -2,6 +2,20 @@
 
 set -e
 
+echo "Available certs:"
+ls -l /certs
+
+echo "Environment:"
+env
+
+echo "Creating a k5s token..."
 echo psql | kinit [email protected]
 
+echo "Preparing SQL user ahead of test"
+env \
+    PGSSLKEY=/certs/client.root.key \
+    PGSSLCERT=/certs/client.root.crt \
+    psql -c "ALTER USER root WITH PASSWORD rootpw"
+
+echo "Running test"
 ./gss.test

pkg/acceptance/compose/gss/python/Dockerfile

@@ -11,3 +11,5 @@ WORKDIR /code
 COPY requirements.txt /code/
 RUN pip install -r requirements.txt
 COPY . /code/
+
+ENTRYPOINT ["/start.sh"]

pkg/acceptance/compose/gss/python/start.sh

@@ -2,10 +2,29 @@
 
 set -e
 
-psql -c "SET CLUSTER SETTING server.host_based_authentication.configuration = 'host all all all gss include_realm=0'"
-psql -c "CREATE USER tester"
+echo "Available certs:"
+ls -l /certs
+
+echo "Environment:"
+env
 
+echo "Creating a k5s token..."
 echo psql | kinit [email protected]
 
+export PGSSLKEY=/certs/client.root.key
+export PGSSLCERT=/certs/client.root.crt
+export PGUSER=root
+
+echo "Creating test user"
+psql -c "CREATE USER tester"
+echo "Configuring the HBA rule prior to running the test..."
+psql -c "SET CLUSTER SETTING server.host_based_authentication.configuration = 'host all all all gss include_realm=0'"
+
+echo "Testing the django connection..."
+
+unset PGSSLKEY
+unset PGSSLCERT
+export PGUSER=tester
+
 # Exit with error unless we find the expected error message.
 python manage.py inspectdb 2>&1 | grep 'use of GSS authentication requires an enterprise license'

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -39,8 +39,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
-	"github.com/jackc/pgproto3/v2"
-	"github.com/jackc/pgx/v4"
+	pgproto3 "github.com/jackc/pgproto3/v2"
+	pgx "github.com/jackc/pgx/v4"
 	"github.com/stretchr/testify/require"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
@@ -364,15 +364,22 @@ func TestProxyModifyRequestParams(t *testing.T) {
 	te := newTester()
 	defer te.Close()
 
-	sql, _, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
+	sql, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
 	sql.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)
 	defer sql.Stopper().Stop(ctx)
 
+	// Create some user with password authn.
+	_, err := sqlDB.Exec("CREATE USER testuser WITH PASSWORD foo123")
+	require.NoError(t, err)
+
 	outgoingTLSConfig, err := sql.RPCContext().GetClientTLSConfig()
 	require.NoError(t, err)
 	proxyOutgoingTLSConfig := outgoingTLSConfig.Clone()
 	proxyOutgoingTLSConfig.InsecureSkipVerify = true
 
+	// We wish the proxy to work even without providing a valid TLS client cert to the SQL server.
+	proxyOutgoingTLSConfig.Certificates = nil
+
 	originalBackendDial := BackendDial
 	defer testutils.TestingHook(&BackendDial, func(
 		msg *pgproto3.StartupMessage, outgoingAddress string, tlsConfig *tls.Config,
@@ -389,14 +396,14 @@ func TestProxyModifyRequestParams(t *testing.T) {
 		// NB: This test will fail unless the user used between the proxy
 		// and the backend is changed to a user that actually exists.
 		delete(params, "authToken")
-		params["user"] = "root"
+		params["user"] = "testuser"
 
 		return originalBackendDial(msg, sql.ServingSQLAddr(), proxyOutgoingTLSConfig)
 	})()
 
 	s, proxyAddr := newSecureProxyServer(ctx, t, sql.Stopper(), &ProxyOptions{})
 
-	u := fmt.Sprintf("postgres://bogususer@%s/?sslmode=require&authToken=abc123&options=--cluster=dim-dog-28&sslmode=require", proxyAddr)
+	u := fmt.Sprintf("postgres://bogususer:foo123@%s/?sslmode=require&authToken=abc123&options=--cluster=dim-dog-28&sslmode=require", proxyAddr)
 	te.TestConnect(ctx, t, u, func(conn *pgx.Conn) {
 		require.Equal(t, int64(1), s.metrics.CurConnCount.Value())
 		require.NoError(t, runTestQuery(ctx, conn))
@@ -516,15 +523,22 @@ func TestDenylistUpdate(t *testing.T) {
 	denyList, err := ioutil.TempFile("", "*_denylist.yml")
 	require.NoError(t, err)
 
-	sql, _, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
+	sql, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
 	sql.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)
 	defer sql.Stopper().Stop(ctx)
 
+	// Create some user with password authn.
+	_, err = sqlDB.Exec("CREATE USER testuser WITH PASSWORD foo123")
+	require.NoError(t, err)
+
 	outgoingTLSConfig, err := sql.RPCContext().GetClientTLSConfig()
 	require.NoError(t, err)
 	proxyOutgoingTLSConfig := outgoingTLSConfig.Clone()
 	proxyOutgoingTLSConfig.InsecureSkipVerify = true
 
+	// We wish the proxy to work even without providing a valid TLS client cert to the SQL server.
+	proxyOutgoingTLSConfig.Certificates = nil
+
 	originalBackendDial := BackendDial
 	defer testutils.TestingHook(&BackendDial, func(
 		msg *pgproto3.StartupMessage, outgoingAddress string, tlsConfig *tls.Config,
@@ -554,7 +568,7 @@ func TestDenylistUpdate(t *testing.T) {
 	})
 	defer func() { _ = os.Remove(denyList.Name()) }()
 
-	url := fmt.Sprintf("postgres://root:admin@%s/defaultdb_29?sslmode=require&options=--cluster=dim-dog-28&sslmode=require", addr)
+	url := fmt.Sprintf("postgres://testuser:foo123@%s/defaultdb_29?sslmode=require&options=--cluster=dim-dog-28&sslmode=require", addr)
 	te.TestConnect(ctx, t, url, func(conn *pgx.Conn) {
 		require.Eventuallyf(
 			t,

pkg/security/auth.go

@@ -134,10 +134,8 @@ func UserAuthCertHook(insecureMode bool, tlsState *tls.ConnectionState) (UserAut
 				errors.Errorf("using tenant client certificate as user certificate is not allowed")
 		}
 
-		// The client certificate user must match the requested user,
-		// except if the certificate user is NodeUser, which is allowed to
-		// act on behalf of all other users.
-		if !Contains(certUsers, requestedUser.Normalized()) && !Contains(certUsers, NodeUser) {
+		// The client certificate user must match the requested user.
+		if !Contains(certUsers, requestedUser.Normalized()) {
 			return nil, errors.Errorf("requested user is %s, but certificate is for %s", requestedUser, certUsers)
 		}
 

pkg/security/auth_test.go

@@ -195,6 +195,8 @@ func TestAuthenticationHook(t *testing.T) {
 		{false, "foo", security.NodeUserName(), "", true, false, false},
 		// Secure mode, node user.
 		{false, security.NodeUser, security.NodeUserName(), "", true, true, true},
+		// Secure mode, node cert and unrelated user.
+		{false, security.NodeUser, fooUser, "", true, false, false},
 		// Secure mode, root user.
 		{false, security.RootUser, security.NodeUserName(), "", true, false, false},
 		// Secure mode, tenant cert, foo user.