pgwire: accept non-TLS client conns safely in secure mode

This change makes it possible for a DBA / system administrator to
reconfigure individual nodes *in a secure cluster* to accept SQL
client sessions over TCP without mandating a TLS
handshake. Authentication remains mandatory as per the HBA rules.

Motivation: we have at least two high-profile customers who keep their
nodes and client apps in a private secure network (with network-level
encryption / privacy) and who experience client-side TLS as
unnecessary and expensive friction.

Why this does not impair security:

- authentication remains mandatory (as per the HBA rules).
- the feature is opt-in: the operator must set a command-line flag
  (`--accept-sql-without-tls`), which is not enabled by default.
- there is an interlock: the user must both set up the flag
  and set log-in passwords for their SQL users (by default,
  users get created without a password and thus cannot log
  in without client certs).
- for now, node-node connections still require TLS.

For contex, the default HBA configuration is the following:

```
host  all root all cert-password # fixed rule
host  all all  all cert-password # built-in CockroachDB default
local all all      password      # built-in CockroachDB default
```

The directive `host` covers both TLS and non-TLS incoming TCP
connections (`local` is for the unix socket). The method
`cert-password` means "client cert or password": without a cert, the
password is mandatory.

As previously, the user can further secure the configuration by
restricting non-TLS connections to just a subnetwork, for example:

```
hostnossl  all all 10.0.0.0/8 password # accept non-TLS auth on the 10/8 network.
hostnossl  all all all        reject   # refuse non-TLS conns from other nets.
hostssl    all all all        cert-password # accept certs or passwords over TLS
local      all all            password
```

Note that this change is limited to the server side: CockroachDB's own
`cockroach` CLI commands do not yet know how to connect to a
CockroachDB server without TLS; such connections are only supported
from `psql` or SQL client drivers in apps.

Release justification: fixes for high-priority or high-severity bugs in existing functionality

Release note (security update): A new experimental flag
`--accept-sql-without-tls` has been introduced for `cockroach start`
and `start-single-node`: when specified, a secure node will also
accept secure SQL connections without TLS. When this flag is enabled:

- Node-to-node connections still use TLS: the server must still be
  started with `--certs-dir` and valid TLS cert configuration for nodes.
- Client authentication (spoof protection) and authorization (access control
  and privilege escalation prevention) is performed by CockroachDB
  as usual, subject to the HBA configuration (for authn) and SQL
  privileges (for authz).
- Transport-level security (integrity and confidentiality) for client
  connections must then be provided by the operator outside of
  CockroachDB -- for example, by using a private network or VPN
  dedicated to CockroachDB and its client app(s).
- The flag only applies to the SQL interface. TLS is still required
  for the HTTP endpoint (unless `--unencrypted-localhost-http` is
  passed) and for the RPC endpoint.

To introduce this feature into an existing cluster, proceed as
follows:
1. ensure the cluster ugprade is finalized.
2. set up the HBA configuration to reject `hostnossl` connections
   for any network other than the one that has been secured.
3. add the command-line flag and restart the nodes.

Note that even when the flag is supplied, clients can still negotiate
TLS and present a valid TLS certificate to identify themselves (at
least under the default HBA configuration).

Finally, this flag is experimental and its ergonomics will likely
change in a later version.

docs/generated/settings/settings.html

@@ -78,6 +78,6 @@
 <tr><td><code>trace.debug.enable</code></td><td>boolean</td><td><code>false</code></td><td>if set, traces for recent requests can be seen in the /debug page</td></tr>
 <tr><td><code>trace.lightstep.token</code></td><td>string</td><td><code></code></td><td>if set, traces go to Lightstep using this token</td></tr>
 <tr><td><code>trace.zipkin.collector</code></td><td>string</td><td><code></code></td><td>if set, traces go to the given Zipkin instance (example: '127.0.0.1:9411'); ignored if trace.lightstep.token is set</td></tr>
-<tr><td><code>version</code></td><td>custom validation</td><td><code>20.1-20</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
+<tr><td><code>version</code></td><td>custom validation</td><td><code>20.1-21</code></td><td>set the active cluster version in the format '<major>.<minor>'</td></tr>
 </tbody>
 </table>

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)
+	}
+}

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

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

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: `

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

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).
 })

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
 // <hba config>
 //       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)

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)

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
 	}
 

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
+