From b0d88e5005384126b3faff0d980e992ad850c09f Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Tue, 5 Oct 2021 16:25:28 +0200 Subject: [PATCH] security: remove the node TLS client cert exemption This patch also fixes the TestComposeGSSPython test. 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. --- pkg/acceptance/cluster/certs.go | 2 +- .../compose/gss/docker-compose-python.yml | 3 -- pkg/acceptance/compose/gss/docker-compose.yml | 2 -- pkg/acceptance/compose/gss/psql/gss_test.go | 2 +- pkg/acceptance/compose/gss/psql/start.sh | 14 ++++++++++ pkg/acceptance/compose/gss/python/Dockerfile | 2 ++ pkg/acceptance/compose/gss/python/start.sh | 23 +++++++++++++-- pkg/ccl/sqlproxyccl/proxy_handler_test.go | 28 ++++++++++++++----- pkg/security/auth.go | 6 ++-- pkg/security/auth_test.go | 2 ++ 10 files changed, 64 insertions(+), 20 deletions(-) diff --git a/pkg/acceptance/cluster/certs.go b/pkg/acceptance/cluster/certs.go index a9891c96e53b..0e149eaae852 100644 --- a/pkg/acceptance/cluster/certs.go +++ b/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( diff --git a/pkg/acceptance/compose/gss/docker-compose-python.yml b/pkg/acceptance/compose/gss/docker-compose-python.yml index d07d26200131..8ab1b00278ef 100644 --- a/pkg/acceptance/compose/gss/docker-compose-python.yml +++ b/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 diff --git a/pkg/acceptance/compose/gss/docker-compose.yml b/pkg/acceptance/compose/gss/docker-compose.yml index ca306ab03fef..1e3d8dd653ef 100644 --- a/pkg/acceptance/compose/gss/docker-compose.yml +++ b/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 diff --git a/pkg/acceptance/compose/gss/psql/gss_test.go b/pkg/acceptance/compose/gss/psql/gss_test.go index 25dc6f2bd278..601020501766 100644 --- a/pkg/acceptance/compose/gss/psql/gss_test.go +++ b/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) } diff --git a/pkg/acceptance/compose/gss/psql/start.sh b/pkg/acceptance/compose/gss/psql/start.sh index c143406bf29c..5daaa77ced80 100755 --- a/pkg/acceptance/compose/gss/psql/start.sh +++ b/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 tester@MY.EX +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 diff --git a/pkg/acceptance/compose/gss/python/Dockerfile b/pkg/acceptance/compose/gss/python/Dockerfile index 097736f02324..adfd3121dcaa 100644 --- a/pkg/acceptance/compose/gss/python/Dockerfile +++ b/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"] diff --git a/pkg/acceptance/compose/gss/python/start.sh b/pkg/acceptance/compose/gss/python/start.sh index 8463488c7ee3..e0e53227e6e9 100755 --- a/pkg/acceptance/compose/gss/python/start.sh +++ b/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 tester@MY.EX +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' diff --git a/pkg/ccl/sqlproxyccl/proxy_handler_test.go b/pkg/ccl/sqlproxyccl/proxy_handler_test.go index 5471017f52ab..3c30c61df4da 100644 --- a/pkg/ccl/sqlproxyccl/proxy_handler_test.go +++ b/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, diff --git a/pkg/security/auth.go b/pkg/security/auth.go index 04f3cd5aa59c..f5dd5de42953 100644 --- a/pkg/security/auth.go +++ b/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) } diff --git a/pkg/security/auth_test.go b/pkg/security/auth_test.go index 6e1e237606df..94c5cadf3b16 100644 --- a/pkg/security/auth_test.go +++ b/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.