From 6e9dd995eb436a3681fe47dbda074c3ea2a464a9 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Fri, 8 Oct 2021 12:08:56 -0400 Subject: [PATCH] tlsutil: only AuthorizerServerConn when VerifyIncomingRPC is true See github.com/hashicorp/consul/issues/11207 When VerifyIncomingRPC is false the TLS conn will not have the required certificates. --- .changelog/11255.txt | 3 +++ tlsutil/config.go | 9 ++++--- tlsutil/config_test.go | 36 +++++++++++++++++++++++++- tlsutil/generate.go | 8 +++--- website/content/docs/agent/options.mdx | 21 ++++++++------- 5 files changed, 59 insertions(+), 18 deletions(-) create mode 100644 .changelog/11255.txt diff --git a/.changelog/11255.txt b/.changelog/11255.txt new file mode 100644 index 000000000..20723d30e --- /dev/null +++ b/.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 +``` diff --git a/tlsutil/config.go b/tlsutil/config.go index 55c8aa225..4d0bd0525 100644 --- a/tlsutil/config.go +++ b/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) } diff --git a/tlsutil/config_test.go b/tlsutil/config_test.go index 2452933a6..b2f27d6d5 100644 --- a/tlsutil/config_test.go +++ b/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 { diff --git a/tlsutil/generate.go b/tlsutil/generate.go index 6dd32132d..04628ce4f 100644 --- a/tlsutil/generate.go +++ b/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 diff --git a/website/content/docs/agent/options.mdx b/website/content/docs/agent/options.mdx index 305598721..9703605a0 100644 --- a/website/content/docs/agent/options.mdx +++ b/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..` - 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..`. + 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