Merge pull request #11255 from hashicorp/dnephin/fix-auth-verify-incoming

tlsutil: only AuthorizerServerConn when VerifyIncomingRPC is true

.changelog/11255.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+rpc: only attempt to authorize the DNSName in the client cert when verify_incoming_rpc=true
+```

tlsutil/config.go

@@ -904,10 +904,10 @@ type TLSConn interface {
 // presented is signed by the Agent TLS CA, and has a DNSName that matches the
 // local ServerSNI name.
 //
-// Note this check is only performed if VerifyServerHostname is enabled, otherwise
-// it does no authorization.
+// Note this check is only performed if VerifyServerHostname and VerifyIncomingRPC
+// are both enabled, otherwise it does no authorization.
 func (c *Configurator) AuthorizeServerConn(dc string, conn TLSConn) error {
-	if !c.VerifyServerHostname() {
+	if !c.VerifyIncomingRPC() || !c.VerifyServerHostname() {
 		return nil
 	}
 
@@ -937,6 +937,9 @@ func (c *Configurator) AuthorizeServerConn(dc string, conn TLSConn) error {
 		}
 		errs = multierror.Append(errs, err)
 	}
+	if errs == nil {
+		errs = fmt.Errorf("no verified chains")
+	}
 	return fmt.Errorf("AuthorizeServerConn failed certificate validation for certificate with a SAN.DNSName of %v: %w", expected, errs)
 
 }

tlsutil/config_test.go

@@ -1271,7 +1271,7 @@ func TestConfigurator_AutoEncryptCert(t *testing.T) {
 	require.Equal(t, int64(4679716209), c.AutoEncryptCert().NotAfter.Unix())
 }
 
-func TestConfigurator_AuthorizeServerConn_Error(t *testing.T) {
+func TestConfigurator_AuthorizeServerConn(t *testing.T) {
 	caPEM, caPK, err := GenerateCA(CAOpts{Days: 5, Domain: "consul"})
 	require.NoError(t, err)
 
@@ -1280,10 +1280,28 @@ func TestConfigurator_AuthorizeServerConn_Error(t *testing.T) {
 	err = ioutil.WriteFile(caPath, []byte(caPEM), 0600)
 	require.NoError(t, err)
 
+	// Cert and key are not used, but required to get past validateConfig
+	signer, err := ParseSigner(caPK)
+	require.NoError(t, err)
+	pub, pk, err := GenerateCert(CertOpts{
+		Signer: signer,
+		CA:     caPEM,
+	})
+	require.NoError(t, err)
+	certFile := filepath.Join("cert.pem")
+	err = ioutil.WriteFile(certFile, []byte(pub), 0600)
+	require.NoError(t, err)
+	keyFile := filepath.Join("cert.key")
+	err = ioutil.WriteFile(keyFile, []byte(pk), 0600)
+	require.NoError(t, err)
+
 	cfg := Config{
 		VerifyServerHostname: true,
+		VerifyIncomingRPC:    true,
 		Domain:               "consul",
 		CAFile:               caPath,
+		CertFile:             certFile,
+		KeyFile:              keyFile,
 	}
 	c, err := NewConfigurator(cfg, hclog.New(nil))
 	require.NoError(t, err)
@@ -1367,6 +1385,22 @@ func TestConfigurator_AuthorizeServerConn_Error(t *testing.T) {
 		err = c.AuthorizeServerConn("dc1", s)
 		testutil.RequireErrorContains(t, err, "certificate specifies an incompatible key usage")
 	})
+
+	t.Run("disabled by verify_incoming_rpc", func(t *testing.T) {
+		cfg := Config{
+			VerifyServerHostname: true,
+			VerifyIncomingRPC:    false,
+			Domain:               "consul",
+			CAFile:               caPath,
+		}
+		c, err := NewConfigurator(cfg, hclog.New(nil))
+		require.NoError(t, err)
+
+		s := fakeTLSConn{}
+		err = c.AuthorizeServerConn("dc1", s)
+		require.NoError(t, err)
+	})
+
 }
 
 type fakeTLSConn struct {

tlsutil/generate.go

@@ -143,17 +143,17 @@ func GenerateCA(opts CAOpts) (string, string, error) {
 func GenerateCert(opts CertOpts) (string, string, error) {
 	parent, err := parseCert(opts.CA)
 	if err != nil {
-		return "", "", err
+		return "", "", fmt.Errorf("failed to parse CA: %w", err)
 	}
 
 	signee, pk, err := GeneratePrivateKey()
 	if err != nil {
-		return "", "", err
+		return "", "", fmt.Errorf("failed to generate private key: %w", err)
 	}
 
 	id, err := keyID(signee.Public())
 	if err != nil {
-		return "", "", err
+		return "", "", fmt.Errorf("failed to get keyID from public key: %w", err)
 	}
 
 	sn := opts.Serial
@@ -185,7 +185,7 @@ func GenerateCert(opts CertOpts) (string, string, error) {
 
 	bs, err := x509.CreateCertificate(rand.Reader, &template, parent, signee.Public(), opts.Signer)
 	if err != nil {
-		return "", "", err
+		return "", "", fmt.Errorf("failed to create certificate: %w", err)
 	}
 
 	var buf bytes.Buffer

website/content/docs/agent/options.mdx

@@ -2276,12 +2276,15 @@ signed by the CA can be used to gain full access to Consul.
   endpoint was created by the CA that the consul client was setup with. If the
   UI is served, the same checks are performed.
 
-- `verify_incoming_rpc` - If set to true, Consul
-  requires that all incoming RPC connections make use of TLS and that the client
+- `verify_incoming_rpc` - When set to true, Consul
+  requires that all incoming RPC connections use TLS and that the client
   provides a certificate signed by a Certificate Authority from the [`ca_file`](#ca_file)
   or [`ca_path`](#ca_path). By default, this is false, and Consul will not enforce
   the use of TLS or verify a client's authenticity.
 
+  ~> **Security Note:** `verify_incoming_rpc` _must_ be set to true to prevent anyone
+  with access to the RPC port from gaining full access to the Consul cluster.
+
 - `verify_incoming_https` - If set to true,
   Consul requires that all incoming HTTPS connections make use of TLS and that the
   client provides a certificate signed by a Certificate Authority from the [`ca_file`](#ca_file)
@@ -2303,14 +2306,12 @@ signed by the CA can be used to gain full access to Consul.
   server unencrypted is to also enable `verify_incoming` which requires client
   certificates too.
 
-- `verify_server_hostname` - If set to true,
-  Consul verifies for all outgoing TLS connections that the TLS certificate
-  presented by the servers matches `server.<datacenter>.<domain>`
-  hostname. By default, this is false, and Consul does not verify the hostname
-  of the certificate, only that it is signed by a trusted CA. This setting is
-  _critical_ to prevent a compromised client from being restarted as a server
-  and having all cluster state _including all ACL tokens and Connect CA root keys_
-  replicated to it. This is new in 0.5.1.
+- `verify_server_hostname` - When set to true, Consul verifies the TLS certificate
+  presented by the servers match the hostname `server.<datacenter>.<domain>`.
+  By default this is false, and Consul does not verify the hostname
+  of the certificate, only that it is signed by a trusted CA. This setting _must_ be enabled
+  to prevent a compromised client from gaining full read and write access to all
+  cluster data _including all ACL tokens and Connect CA root keys_. This is new in 0.5.1.
 
   ~> **Security Note:** From versions 0.5.1 to 1.4.0, due to a bug, setting
   this flag alone _does not_ imply `verify_outgoing` and leaves client to server