From 24df1392b34a731c21ba2b72f04360d3a705e642 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Mon, 4 Oct 2021 13:22:30 -0400
Subject: [PATCH] rpc: include error for AuthorizeServerConn failures

The errs were not being captured because the value of append was not being assigned.
---
 tlsutil/config.go      |   8 ++-
 tlsutil/config_test.go | 120 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+), 2 deletions(-)

diff --git a/tlsutil/config.go b/tlsutil/config.go
index 6abe92329..55c8aa225 100644
--- a/tlsutil/config.go
+++ b/tlsutil/config.go
@@ -893,6 +893,10 @@ func (c *Configurator) wrapALPNTLSClient(dc, nodeName, alpnProto string, conn ne
 	return tlsConn, nil
 }
 
+type TLSConn interface {
+	ConnectionState() tls.ConnectionState
+}
+
 // AuthorizeServerConn is used to validate that the connection is being established
 // by a Consul server in the same datacenter.
 //
@@ -902,7 +906,7 @@ func (c *Configurator) wrapALPNTLSClient(dc, nodeName, alpnProto string, conn ne
 //
 // Note this check is only performed if VerifyServerHostname is enabled, otherwise
 // it does no authorization.
-func (c *Configurator) AuthorizeServerConn(dc string, conn *tls.Conn) error {
+func (c *Configurator) AuthorizeServerConn(dc string, conn TLSConn) error {
 	if !c.VerifyServerHostname() {
 		return nil
 	}
@@ -931,7 +935,7 @@ func (c *Configurator) AuthorizeServerConn(dc string, conn *tls.Conn) error {
 		if err == nil {
 			return nil
 		}
-		multierror.Append(errs, err)
+		errs = multierror.Append(errs, err)
 	}
 	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 f1cb58bca..2452933a6 100644
--- a/tlsutil/config_test.go
+++ b/tlsutil/config_test.go
@@ -7,6 +7,7 @@ import (
 	"io"
 	"io/ioutil"
 	"net"
+	"path/filepath"
 	"reflect"
 	"strings"
 	"testing"
@@ -1270,6 +1271,125 @@ func TestConfigurator_AutoEncryptCert(t *testing.T) {
 	require.Equal(t, int64(4679716209), c.AutoEncryptCert().NotAfter.Unix())
 }
 
+func TestConfigurator_AuthorizeServerConn_Error(t *testing.T) {
+	caPEM, caPK, err := GenerateCA(CAOpts{Days: 5, Domain: "consul"})
+	require.NoError(t, err)
+
+	dir := testutil.TempDir(t, "ca")
+	caPath := filepath.Join(dir, "ca.pem")
+	err = ioutil.WriteFile(caPath, []byte(caPEM), 0600)
+	require.NoError(t, err)
+
+	cfg := Config{
+		VerifyServerHostname: true,
+		Domain:               "consul",
+		CAFile:               caPath,
+	}
+	c, err := NewConfigurator(cfg, hclog.New(nil))
+	require.NoError(t, err)
+
+	t.Run("wrong DNSName", func(t *testing.T) {
+		signer, err := ParseSigner(caPK)
+		require.NoError(t, err)
+
+		pem, _, err := GenerateCert(CertOpts{
+			Signer:      signer,
+			CA:          caPEM,
+			Name:        "server.dc1.consul",
+			Days:        5,
+			DNSNames:    []string{"this-name-is-wrong", "localhost"},
+			ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
+		})
+		require.NoError(t, err)
+
+		s := fakeTLSConn{
+			state: tls.ConnectionState{
+				VerifiedChains:   [][]*x509.Certificate{certChain(t, pem, caPEM)},
+				PeerCertificates: certChain(t, pem, caPEM),
+			},
+		}
+		err = c.AuthorizeServerConn("dc1", s)
+		testutil.RequireErrorContains(t, err, "is valid for this-name-is-wrong, localhost, not server.dc1.consul")
+	})
+
+	t.Run("wrong CA", func(t *testing.T) {
+		caPEM, caPK, err := GenerateCA(CAOpts{Days: 5, Domain: "consul"})
+		require.NoError(t, err)
+
+		dir := testutil.TempDir(t, "other")
+		caPath := filepath.Join(dir, "ca.pem")
+		err = ioutil.WriteFile(caPath, []byte(caPEM), 0600)
+		require.NoError(t, err)
+
+		signer, err := ParseSigner(caPK)
+		require.NoError(t, err)
+
+		pem, _, err := GenerateCert(CertOpts{
+			Signer:      signer,
+			CA:          caPEM,
+			Name:        "server.dc1.consul",
+			Days:        5,
+			DNSNames:    []string{"server.dc1.consul", "localhost"},
+			ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageClientAuth},
+		})
+		require.NoError(t, err)
+
+		s := fakeTLSConn{
+			state: tls.ConnectionState{
+				VerifiedChains:   [][]*x509.Certificate{certChain(t, pem, caPEM)},
+				PeerCertificates: certChain(t, pem, caPEM),
+			},
+		}
+		err = c.AuthorizeServerConn("dc1", s)
+		testutil.RequireErrorContains(t, err, "signed by unknown authority")
+	})
+
+	t.Run("missing ext key usage", func(t *testing.T) {
+		signer, err := ParseSigner(caPK)
+		require.NoError(t, err)
+
+		pem, _, err := GenerateCert(CertOpts{
+			Signer:      signer,
+			CA:          caPEM,
+			Name:        "server.dc1.consul",
+			Days:        5,
+			DNSNames:    []string{"server.dc1.consul", "localhost"},
+			ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageEmailProtection},
+		})
+		require.NoError(t, err)
+
+		s := fakeTLSConn{
+			state: tls.ConnectionState{
+				VerifiedChains:   [][]*x509.Certificate{certChain(t, pem, caPEM)},
+				PeerCertificates: certChain(t, pem, caPEM),
+			},
+		}
+		err = c.AuthorizeServerConn("dc1", s)
+		testutil.RequireErrorContains(t, err, "certificate specifies an incompatible key usage")
+	})
+}
+
+type fakeTLSConn struct {
+	state tls.ConnectionState
+}
+
+func (f fakeTLSConn) ConnectionState() tls.ConnectionState {
+	return f.state
+}
+
+func certChain(t *testing.T, certs ...string) []*x509.Certificate {
+	t.Helper()
+
+	result := make([]*x509.Certificate, 0, len(certs))
+
+	for i, c := range certs {
+		cert, err := parseCert(c)
+		require.NoError(t, err, "cert %d", i)
+		result = append(result, cert)
+	}
+	return result
+}
+
 func TestConfig_tlsVersions(t *testing.T) {
 	require.Equal(t, []string{"tls10", "tls11", "tls12", "tls13"}, tlsVersions())
 	expected := "tls10, tls11, tls12, tls13"