From 24c6ce0be04c3a5317173f8239f66ea6842681c7 Mon Sep 17 00:00:00 2001 From: Hans Hasselberg Date: Thu, 9 Sep 2021 21:48:54 +0200 Subject: [PATCH] tls: consider presented intermediates during server connection tls handshake. (#10964) * use intermediates when verifying * extract connection state * remove useless import * add changelog entry * golint * better error * wording * collect errors * use SAN.DNSName instead of CommonName * Add test for unknown intermediate * improve changelog entry --- .changelog/10964.txt | 3 +++ agent/consul/rpc_test.go | 40 ++++++++++++++++++++++++++++++++++++---- tlsutil/config.go | 36 ++++++++++++++++++++---------------- tlsutil/generate.go | 5 +++++ 4 files changed, 64 insertions(+), 20 deletions(-) create mode 100644 .changelog/10964.txt diff --git a/.changelog/10964.txt b/.changelog/10964.txt new file mode 100644 index 000000000..cc0886d00 --- /dev/null +++ b/.changelog/10964.txt @@ -0,0 +1,3 @@ +```release-note:bug +tls: consider presented intermediates during server connection tls handshake. +``` diff --git a/agent/consul/rpc_test.go b/agent/consul/rpc_test.go index 14ca5fd8e..6565d1ce7 100644 --- a/agent/consul/rpc_test.go +++ b/agent/consul/rpc_test.go @@ -1072,13 +1072,22 @@ func (r isReadRequest) HasTimedOut(since time.Time, rpcHoldTimeout, maxQueryTime } func TestRPC_AuthorizeRaftRPC(t *testing.T) { - caPEM, pk, err := tlsutil.GenerateCA(tlsutil.CAOpts{Days: 5, Domain: "consul"}) + caPEM, caPK, err := tlsutil.GenerateCA(tlsutil.CAOpts{Days: 5, Domain: "consul"}) + require.NoError(t, err) + + caSigner, err := tlsutil.ParseSigner(caPK) require.NoError(t, err) dir := testutil.TempDir(t, "certs") err = ioutil.WriteFile(filepath.Join(dir, "ca.pem"), []byte(caPEM), 0600) require.NoError(t, err) + intermediatePEM, intermediatePK, err := tlsutil.GenerateCert(tlsutil.CertOpts{IsCA: true, CA: caPEM, Signer: caSigner, Days: 5}) + require.NoError(t, err) + + err = ioutil.WriteFile(filepath.Join(dir, "intermediate.pem"), []byte(intermediatePEM), 0600) + require.NoError(t, err) + newCert := func(t *testing.T, caPEM, pk, node, name string) { t.Helper() @@ -1101,7 +1110,7 @@ func TestRPC_AuthorizeRaftRPC(t *testing.T) { require.NoError(t, err) } - newCert(t, caPEM, pk, "srv1", "server.dc1.consul") + newCert(t, caPEM, caPK, "srv1", "server.dc1.consul") _, connectCApk, err := connect.GeneratePrivateKey() require.NoError(t, err) @@ -1113,7 +1122,7 @@ func TestRPC_AuthorizeRaftRPC(t *testing.T) { c.TLSConfig.KeyFile = filepath.Join(dir, "srv1-server.dc1.consul.key") c.TLSConfig.VerifyIncoming = true c.TLSConfig.VerifyServerHostname = true - // Enable Auto-Encrypt so that Conenct CA roots are added to the + // Enable Auto-Encrypt so that Connect CA roots are added to the // tlsutil.Configurator. c.AutoEncryptAllowTLS = true c.CAConfig = &structs.CAConfiguration{ @@ -1159,11 +1168,29 @@ func TestRPC_AuthorizeRaftRPC(t *testing.T) { setupAgentTLSCert := func(name string) func(t *testing.T) string { return func(t *testing.T) string { - newCert(t, caPEM, pk, "node1", name) + newCert(t, caPEM, caPK, "node1", name) return filepath.Join(dir, "node1-"+name) } } + setupAgentTLSCertWithIntermediate := func(name string) func(t *testing.T) string { + return func(t *testing.T) string { + newCert(t, intermediatePEM, intermediatePK, "node1", name) + certPrefix := filepath.Join(dir, "node1-"+name) + f, err := os.OpenFile(certPrefix+".pem", os.O_APPEND|os.O_CREATE|os.O_WRONLY, 0644) + if err != nil { + t.Fatal(err) + } + if _, err := f.Write([]byte(intermediatePEM)); err != nil { + t.Fatal(err) + } + if err := f.Close(); err != nil { + t.Fatal(err) + } + return certPrefix + } + } + setupConnectCACert := func(name string) func(t *testing.T) string { return func(t *testing.T) string { _, caRoot := srv.caManager.getCAProvider() @@ -1221,6 +1248,11 @@ func TestRPC_AuthorizeRaftRPC(t *testing.T) { setupCert: setupAgentTLSCert("server.dc1.consul"), conn: useTLSByte, }, + { + name: "TLS byte with server cert in same DC and with unknown intermediate", + setupCert: setupAgentTLSCertWithIntermediate("server.dc1.consul"), + conn: useTLSByte, + }, { name: "TLS byte with ConnectCA leaf cert", setupCert: setupConnectCACert("server.dc1.consul"), diff --git a/tlsutil/config.go b/tlsutil/config.go index 5c0466134..a3116c12a 100644 --- a/tlsutil/config.go +++ b/tlsutil/config.go @@ -15,6 +15,7 @@ import ( "time" "github.com/hashicorp/go-hclog" + "github.com/hashicorp/go-multierror" "github.com/hashicorp/consul/logging" ) @@ -842,15 +843,11 @@ func (c *Configurator) wrapTLSClient(dc string, conn net.Conn) (net.Conn, error) Intermediates: x509.NewCertPool(), } - certs := tlsConn.ConnectionState().PeerCertificates - for i, cert := range certs { - if i == 0 { - continue - } + cs := tlsConn.ConnectionState() + for _, cert := range cs.PeerCertificates[1:] { opts.Intermediates.AddCert(cert) } - - _, err = certs[0].Verify(opts) + _, err = cs.PeerCertificates[0].Verify(opts) if err != nil { tlsConn.Close() return nil, err @@ -915,22 +912,29 @@ func (c *Configurator) AuthorizeServerConn(dc string, conn *tls.Conn) error { c.lock.RUnlock() expected := c.ServerSNI(dc, "") - for _, chain := range conn.ConnectionState().VerifiedChains { + cs := conn.ConnectionState() + var errs error + for _, chain := range cs.VerifiedChains { if len(chain) == 0 { continue } - clientCert := chain[0] - _, err := clientCert.Verify(x509.VerifyOptions{ - DNSName: expected, - Roots: caPool, - KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, - }) + opts := x509.VerifyOptions{ + DNSName: expected, + Intermediates: x509.NewCertPool(), + Roots: caPool, + KeyUsages: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth}, + } + for _, cert := range cs.PeerCertificates[1:] { + opts.Intermediates.AddCert(cert) + } + _, err := cs.PeerCertificates[0].Verify(opts) if err == nil { return nil } - c.logger.Debug("AuthorizeServerConn failed certificate validation", "error", err) + multierror.Append(errs, err) } - return fmt.Errorf("a TLS certificate with a CommonName of %v is required for this operation", expected) + return fmt.Errorf("AuthorizeServerConn failed certificate validation for certificate with a SAN.DNSName of %v: %w", expected, errs) + } // ParseCiphers parse ciphersuites from the comma-separated string into diff --git a/tlsutil/generate.go b/tlsutil/generate.go index 61cf40e14..6dd32132d 100644 --- a/tlsutil/generate.go +++ b/tlsutil/generate.go @@ -54,6 +54,7 @@ type CertOpts struct { DNSNames []string IPAddresses []net.IP ExtKeyUsage []x509.ExtKeyUsage + IsCA bool } // GenerateCA generates a new CA for agent TLS (not to be confused with Connect TLS) @@ -177,6 +178,10 @@ func GenerateCert(opts CertOpts) (string, string, error) { DNSNames: opts.DNSNames, IPAddresses: opts.IPAddresses, } + if opts.IsCA { + template.IsCA = true + template.KeyUsage = x509.KeyUsageCertSign | x509.KeyUsageCRLSign | x509.KeyUsageDigitalSignature + } bs, err := x509.CreateCertificate(rand.Reader, &template, parent, signee.Public(), opts.Signer) if err != nil {