diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index dd4a79583a65..840ba2495415 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -78,6 +78,6 @@ trace.debug.enablebooleanfalseif set, traces for recent requests can be seen in the /debug page trace.lightstep.tokenstringif set, traces go to Lightstep using this token trace.zipkin.collectorstringif set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'); ignored if trace.lightstep.token is set -versioncustom validation20.1-20set the active cluster version in the format '.' +versioncustom validation20.1-21set the active cluster version in the format '.' diff --git a/pkg/acceptance/cli_test.go b/pkg/acceptance/cli_test.go index a693b4459f2b..79baf58a11a7 100644 --- a/pkg/acceptance/cli_test.go +++ b/pkg/acceptance/cli_test.go @@ -94,6 +94,8 @@ func TestDockerCLI(t *testing.T) { // TestDockerUnixSocket verifies that CockroachDB initializes a unix // socket useable by 'psql', even when the server runs insecurely. +// TODO(knz): Replace this with a roachtest when roachtest/roachprod +// know how to start secure clusters. func TestDockerUnixSocket(t *testing.T) { s := log.Scope(t) defer s.Close(t) @@ -113,3 +115,27 @@ func TestDockerUnixSocket(t *testing.T) { t.Error(err) } } + +// TestSQLWithoutTLS verifies that CockroachDB can accept clients +// without a TLS handshake in secure mode. +// TODO(knz): Replace this with a roachtest when roachtest/roachprod +// know how to start secure clusters. +func TestSQLWithoutTLS(t *testing.T) { + s := log.Scope(t) + defer s.Close(t) + + containerConfig := defaultContainerConfig() + containerConfig.Cmd = []string{"stat", cluster.CockroachBinaryInContainer} + ctx := context.Background() + + if err := testDockerOneShot(ctx, t, "cli_test", containerConfig); err != nil { + skip.IgnoreLintf(t, `TODO(dt): No binary in one-shot container, see #6086: %s`, err) + } + + containerConfig.Env = []string{fmt.Sprintf("PGUSER=%s", security.RootUser)} + containerConfig.Cmd = append(cmdBase, + "/mnt/data/psql/test-psql-notls.sh "+cluster.CockroachBinaryInContainer) + if err := testDockerOneShot(ctx, t, "notls_secure_test", containerConfig); err != nil { + t.Error(err) + } +} diff --git a/pkg/acceptance/testdata/psql/test-psql-notls.sh b/pkg/acceptance/testdata/psql/test-psql-notls.sh new file mode 100755 index 000000000000..0be261ef3b1c --- /dev/null +++ b/pkg/acceptance/testdata/psql/test-psql-notls.sh @@ -0,0 +1,38 @@ +#!/usr/bin/env bash + +CERTS_DIR=${CERTS_DIR:-/certs} +crdb=$1 +trap "set -x; killall cockroach cockroachshort || true" EXIT HUP + +set -euo pipefail + +# Disable automatic network access by psql. +unset PGHOST +unset PGPORT +# Use root access. +export PGUSER=root + +set +x +echo "Testing non-TLS TCP connection via secure server." +set -x + +# Start a server in secure mode and allow non-TLS SQL clients. +"$crdb" start-single-node --background \ + --certs-dir="$CERTS_DIR" --socket-dir=/tmp \ + --accept-sql-without-tls \ + --listen-addr=:12345 + +# Wait for server ready; also create a user that can log in. +"$crdb" sql --certs-dir="$CERTS_DIR" -e "create user foo with password 'pass'" -p 12345 + +# verify that psql can connect to the server without TLS but auth +# fails if they present the wrong password. +(env PGPASSWORD=wrongpass psql 'postgres://foo@localhost:12345?sslmode=disable' -c "select 1" 2>&1 || true) | grep "password authentication failed" + +# now verify that psql can connect to the server without TLS with +# the proper password. +env PGPASSWORD=pass psql 'postgres://foo@localhost:12345?sslmode=disable' -c "select 1" | grep "1 row" + +set +x +# Done. +"$crdb" quit --certs-dir="$CERTS_DIR" -p 12345 diff --git a/pkg/base/config.go b/pkg/base/config.go index c8f3fad9440a..d9225efe5a69 100644 --- a/pkg/base/config.go +++ b/pkg/base/config.go @@ -155,6 +155,13 @@ type Config struct { // See: https://github.com/cockroachdb/cockroach/issues/53404 Insecure bool + // AllowSecureSQLWithoutTLS, when set, makes it possible for SQL + // clients to authenticate without TLS on a secure cluster. + // + // Authentication is, as usual, subject to the HBA configuration: in + // the default case, password authentication is still mandatory. + AllowSecureSQLWithoutTLS bool + // SSLCAKey is used to sign new certs. SSLCAKey string // SSLCertsDir is the path to the certificate/key directory. @@ -262,6 +269,7 @@ func (cfg *Config) InitDefaults() { cfg.ClusterName = "" cfg.DisableClusterNameVerification = false cfg.ClockDevicePath = "" + cfg.AllowSecureSQLWithoutTLS = false } // HTTPRequestScheme returns "http" or "https" based on the value of diff --git a/pkg/cli/cliflags/flags.go b/pkg/cli/cliflags/flags.go index 6841eda26504..29eee247a43f 100644 --- a/pkg/cli/cliflags/flags.go +++ b/pkg/cli/cliflags/flags.go @@ -537,6 +537,16 @@ is then ignored. This flag is intended for use to facilitate local testing without requiring certificate setups in web browsers.`, } + AllowSecureSQLWithoutTLS = FlagInfo{ + Name: "accept-sql-without-tls", + Description: ` +When specified, this node will accept SQL client connections that do not wish +to negotiate a TLS handshake. Authentication is still otherwise required +as per the HBA configuration and all other security mechanisms continue to +apply. This flag is experimental. +`, + } + LocalityAdvertiseAddr = FlagInfo{ Name: "locality-advertise-addr", Description: ` diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index 154fa80a6420..cc6b2211695f 100644 --- a/pkg/cli/flags.go +++ b/pkg/cli/flags.go @@ -349,6 +349,10 @@ func init() { stringFlag(f, &serverSocketDir, cliflags.SocketDir) boolFlag(f, &startCtx.unencryptedLocalhostHTTP, cliflags.UnencryptedLocalhostHTTP) + // The following flag is planned to become non-experimental in 21.1. + boolFlag(f, &serverCfg.AllowSecureSQLWithoutTLS, cliflags.AllowSecureSQLWithoutTLS) + _ = f.MarkHidden(cliflags.AllowSecureSQLWithoutTLS.Name) + // Backward-compatibility flags. // These are deprecated but until we have qualitatively new diff --git a/pkg/clusterversion/cockroach_versions.go b/pkg/clusterversion/cockroach_versions.go index 28c1270dcee1..7adc3978f610 100644 --- a/pkg/clusterversion/cockroach_versions.go +++ b/pkg/clusterversion/cockroach_versions.go @@ -80,6 +80,7 @@ const ( VersionLeasedDatabaseDescriptors VersionUpdateScheduledJobsSchema VersionCreateLoginPrivilege + VersionHBAForNonTLS // Add new versions here (step one of two). ) @@ -599,6 +600,12 @@ var versionsSingleton = keyedVersions([]keyedVersion{ Key: VersionCreateLoginPrivilege, Version: roachpb.Version{Major: 20, Minor: 1, Unstable: 20}, }, + { + // VersionHBAForNonTLS is when the 'hostssl' and 'hostnossl' HBA + // configs are introduced. + Key: VersionHBAForNonTLS, + Version: roachpb.Version{Major: 20, Minor: 1, Unstable: 21}, + }, // Add new versions here (step two of two). }) diff --git a/pkg/clusterversion/versionkey_string.go b/pkg/clusterversion/versionkey_string.go index 4c6d63276590..cf42832881b6 100644 --- a/pkg/clusterversion/versionkey_string.go +++ b/pkg/clusterversion/versionkey_string.go @@ -56,11 +56,12 @@ func _() { _ = x[VersionLeasedDatabaseDescriptors-45] _ = x[VersionUpdateScheduledJobsSchema-46] _ = x[VersionCreateLoginPrivilege-47] + _ = x[VersionHBAForNonTLS-48] } -const _VersionKey_name = "Version19_1VersionStart19_2VersionLearnerReplicasVersionTopLevelForeignKeysVersionAtomicChangeReplicasTriggerVersionAtomicChangeReplicasVersionTableDescModificationTimeFromMVCCVersionPartitionedBackupVersion19_2VersionStart20_1VersionContainsEstimatesCounterVersionChangeReplicasDemotionVersionSecondaryIndexColumnFamiliesVersionNamespaceTableWithSchemasVersionProtectedTimestampsVersionPrimaryKeyChangesVersionAuthLocalAndTrustRejectMethodsVersionPrimaryKeyColumnsOutOfFamilyZeroVersionRootPasswordVersionNoExplicitForeignKeyIndexIDsVersionHashShardedIndexesVersionCreateRolePrivilegeVersionStatementDiagnosticsSystemTablesVersionSchemaChangeJobVersionSavepointsVersionTimeTZTypeVersionTimePrecisionVersion20_1VersionStart20_2VersionGeospatialTypeVersionEnumsVersionRangefeedLeasesVersionAlterColumnTypeGeneralVersionAlterSystemJobsAddCreatedByColumnsVersionAddScheduledJobsTableVersionUserDefinedSchemasVersionNoOriginFKIndexesVersionClientRangeInfosOnBatchResponseVersionNodeMembershipStatusVersionRangeStatsRespHasDescVersionMinPasswordLengthVersionAbortSpanBytesVersionAlterSystemJobsAddSqllivenessColumnsAddNewSystemSqllivenessTableVersionMaterializedViewsVersionBox2DTypeVersionLeasedDatabaseDescriptorsVersionUpdateScheduledJobsSchemaVersionCreateLoginPrivilege" +const _VersionKey_name = "Version19_1VersionStart19_2VersionLearnerReplicasVersionTopLevelForeignKeysVersionAtomicChangeReplicasTriggerVersionAtomicChangeReplicasVersionTableDescModificationTimeFromMVCCVersionPartitionedBackupVersion19_2VersionStart20_1VersionContainsEstimatesCounterVersionChangeReplicasDemotionVersionSecondaryIndexColumnFamiliesVersionNamespaceTableWithSchemasVersionProtectedTimestampsVersionPrimaryKeyChangesVersionAuthLocalAndTrustRejectMethodsVersionPrimaryKeyColumnsOutOfFamilyZeroVersionRootPasswordVersionNoExplicitForeignKeyIndexIDsVersionHashShardedIndexesVersionCreateRolePrivilegeVersionStatementDiagnosticsSystemTablesVersionSchemaChangeJobVersionSavepointsVersionTimeTZTypeVersionTimePrecisionVersion20_1VersionStart20_2VersionGeospatialTypeVersionEnumsVersionRangefeedLeasesVersionAlterColumnTypeGeneralVersionAlterSystemJobsAddCreatedByColumnsVersionAddScheduledJobsTableVersionUserDefinedSchemasVersionNoOriginFKIndexesVersionClientRangeInfosOnBatchResponseVersionNodeMembershipStatusVersionRangeStatsRespHasDescVersionMinPasswordLengthVersionAbortSpanBytesVersionAlterSystemJobsAddSqllivenessColumnsAddNewSystemSqllivenessTableVersionMaterializedViewsVersionBox2DTypeVersionLeasedDatabaseDescriptorsVersionUpdateScheduledJobsSchemaVersionCreateLoginPrivilegeVersionHBAForNonTLS" -var _VersionKey_index = [...]uint16{0, 11, 27, 49, 75, 109, 136, 176, 200, 211, 227, 258, 287, 322, 354, 380, 404, 441, 480, 499, 534, 559, 585, 624, 646, 663, 680, 700, 711, 727, 748, 760, 782, 811, 852, 880, 905, 929, 967, 994, 1022, 1046, 1067, 1138, 1162, 1178, 1210, 1242, 1269} +var _VersionKey_index = [...]uint16{0, 11, 27, 49, 75, 109, 136, 176, 200, 211, 227, 258, 287, 322, 354, 380, 404, 441, 480, 499, 534, 559, 585, 624, 646, 663, 680, 700, 711, 727, 748, 760, 782, 811, 852, 880, 905, 929, 967, 994, 1022, 1046, 1067, 1138, 1162, 1178, 1210, 1242, 1269, 1288} func (i VersionKey) String() string { if i < 0 || i >= VersionKey(len(_VersionKey_index)-1) { diff --git a/pkg/sql/pgwire/auth_test.go b/pkg/sql/pgwire/auth_test.go index b5394c8924ce..e0ce937c14cb 100644 --- a/pkg/sql/pgwire/auth_test.go +++ b/pkg/sql/pgwire/auth_test.go @@ -53,6 +53,9 @@ import ( // security mode. (The default is `config secure insecure` i.e. // the test file is applicable to both.) // +// allow_non_tls +// Enable TCP connections without TLS in secure mode. +// // set_hba // // Load the provided HBA configuration via the cluster setting @@ -156,7 +159,8 @@ func hbaRunTest(t *testing.T, insecure bool) { // Enable conn/auth logging. // We can't use the cluster settings to do this, because // cluster settings propagate asynchronously. - s.(*server.TestServer).PGServer().TestingEnableConnAuthLogging() + testServer := s.(*server.TestServer) + testServer.PGServer().TestingEnableConnAuthLogging() pgServer := s.(*server.TestServer).PGServer() @@ -189,6 +193,9 @@ func hbaRunTest(t *testing.T, insecure bool) { skip.IgnoreLint(t, "Test file not applicable at this security level.") } + case "allow_non_tls": + testServer.Cfg.AllowSecureSQLWithoutTLS = true + case "set_hba": _, err := conn.ExecContext(context.Background(), `SET CLUSTER SETTING server.host_based_authentication.configuration = $1`, td.Input) diff --git a/pkg/sql/pgwire/hba_conf.go b/pkg/sql/pgwire/hba_conf.go index e9c2e2afc19b..270ae5399802 100644 --- a/pkg/sql/pgwire/hba_conf.go +++ b/pkg/sql/pgwire/hba_conf.go @@ -150,6 +150,15 @@ func checkHBASyntaxBeforeUpdatingSetting(values *settings.Values, s string) erro clusterversion.VersionByKey(clusterversion.VersionAuthLocalAndTrustRejectMethods), ) } + case hba.ConnHostSSL, hba.ConnHostNoSSL: + if vh != nil && + !vh.IsActive(context.TODO(), clusterversion.VersionHBAForNonTLS) { + return pgerror.Newf(pgcode.ObjectNotInPrerequisiteState, + `authentication rule type 'hostssl'/'hostnossl' requires all nodes to be upgraded to %s`, + clusterversion.VersionByKey(clusterversion.VersionHBAForNonTLS), + ) + } + default: return unimplemented.Newf("hba-type-"+entry.ConnType.String(), "unsupported connection type: %s", entry.ConnType) diff --git a/pkg/sql/pgwire/server.go b/pkg/sql/pgwire/server.go index 4539bf7fe57c..265a40452bf5 100644 --- a/pkg/sql/pgwire/server.go +++ b/pkg/sql/pgwire/server.go @@ -691,17 +691,18 @@ func (s *Server) maybeUpgradeToSecureConn( if version != versionSSL { // The client did not require a SSL connection. - if !s.cfg.Insecure && connType != hba.ConnLocal { - // Currently non-SSL connections are not allowed in secure - // mode. Ideally, we want to allow this and subject it to HBA - // rules ('hostssl' vs 'hostnossl'). - // - // TODO(knz): revisit this when needed. - clientErr = pgerror.New(pgcode.ProtocolViolation, ErrSSLRequired) + // Insecure mode: nothing to say, nothing to do. + // TODO(knz): Remove this condition - see + // https://github.com/cockroachdb/cockroach/issues/53404 + if s.cfg.Insecure { return } - // Non-SSL in non-secure mode, all is well: no-op. + // Secure mode: disallow if TCP and the user did not opt into SQL + // conns. + if !s.cfg.AllowSecureSQLWithoutTLS && connType != hba.ConnLocal { + clientErr = pgerror.New(pgcode.ProtocolViolation, ErrSSLRequired) + } return } diff --git a/pkg/sql/pgwire/testdata/auth/secure_non_tls b/pkg/sql/pgwire/testdata/auth/secure_non_tls new file mode 100644 index 000000000000..fcfd21e3ef97 --- /dev/null +++ b/pkg/sql/pgwire/testdata/auth/secure_non_tls @@ -0,0 +1,66 @@ +config secure +---- + +# By default, neither root nor testuser can connect without TLS. + +connect user=root sslmode=disable +---- +ERROR: node is running secure mode, SSL connection required (SQLSTATE 08P01) + +connect user=testuser sslmode=disable +---- +ERROR: node is running secure mode, SSL connection required (SQLSTATE 08P01) + +# Enable non-TLS secure connections. +allow_non_tls +---- + +# Since root and testuser do not have a password, they stil +# cannot log in. + +connect user=root sslmode=disable +---- +ERROR: password authentication failed for user root + +connect user=testuser sslmode=disable +---- +ERROR: password authentication failed for user testuser + +# set the password for testuser. +sql +ALTER USER testuser WITH PASSWORD 'abc' +---- +ok + +# Now testuser can log in. + +connect password=abc user=testuser sslmode=disable +---- +ok defaultdb + +# Now disable all non-TLS conns via HBA. +set_hba +hostnossl all all all reject +host all all all cert-password +local all all password +---- +# Active authentication configuration on this node: +# Original configuration: +# host all root all cert-password # CockroachDB mandatory rule +# hostnossl all all all reject +# host all all all cert-password +# local all all password +# +# Interpreted configuration: +# TYPE DATABASE USER ADDRESS METHOD OPTIONS +host all root all cert-password +hostnossl all all all reject +host all all all cert-password +local all all password + +# Now testuser cannot log in any more (rejected by HBA). + +connect password=abc user=testuser sslmode=disable +---- +ERROR: authentication rejected by configuration +