regression test and error message cleanup

Signed-off-by: Andrew Harding <[email protected]>

pkg/agent/manager/manager_test.go

@@ -1106,25 +1106,23 @@ func (h *mockNodeAPIHandler) getGRPCServerConfig(hello *tls.ClientHelloInfo) (*t
 }
 
 func (h *mockNodeAPIHandler) getCertFromCtx(ctx context.Context) (certificate *x509.Certificate, err error) {
-
 	ctxPeer, ok := peer.FromContext(ctx)
 	if !ok {
-		return nil, errors.New("It was not posible to extract peer from request")
+		return nil, errors.New("no peer information")
 	}
 	tlsInfo, ok := ctxPeer.AuthInfo.(credentials.TLSInfo)
 	if !ok {
-		return nil, errors.New("It was not posible to extract AuthInfo from request")
+		return nil, errors.New("no TLS auth info for peer")
 	}
 
 	if len(tlsInfo.State.VerifiedChains) == 0 {
-		return nil, errors.New("VerifiedChains was empty")
+		return nil, errors.New("no verified client certificate presented by peer")
 	}
-
 	chain := tlsInfo.State.VerifiedChains[0]
 	if len(chain) == 0 {
 		// this shouldn't be possible with the tls package, but we should be
 		// defensive.
-		return nil, errors.New("VerifiedChain was empty")
+		return nil, errors.New("verified client chain is missing certificates")
 	}
 
 	return chain[0], nil

pkg/server/endpoints/endpoints_test.go

@@ -8,13 +8,16 @@ import (
 	"errors"
 	"net"
 	"net/url"
+	"strings"
+	"sync"
 	"testing"
 	"time"
 
 	observer "github.com/imkira/go-observer"
 	"github.com/sirupsen/logrus/hooks/test"
 	"github.com/spiffe/spire/pkg/common/bundleutil"
 	"github.com/spiffe/spire/pkg/server/svid"
+	"github.com/spiffe/spire/proto/common"
 	"github.com/spiffe/spire/proto/server/datastore"
 	"github.com/spiffe/spire/test/fakes/fakedatastore"
 	"github.com/spiffe/spire/test/fakes/fakeservercatalog"
@@ -221,3 +224,107 @@ func (s *EndpointsTestSuite) configureBundle() ([]tls.Certificate, *x509.CertPoo
 		},
 	}, caPool
 }
+
+func (s *EndpointsTestSuite) TestClientCertificateVerification() {
+	caTmpl, err := util.NewCATemplate("example.org")
+	s.Require().NoError(err)
+	caCert, caKey, err := util.SelfSign(caTmpl)
+	s.Require().NoError(err)
+
+	serverTmpl, err := util.NewSVIDTemplate("spiffe://example.org/server")
+	s.Require().NoError(err)
+	serverTmpl.DNSNames = []string{"just-for-validation"}
+	serverCert, serverKey, err := util.Sign(serverTmpl, caCert, caKey)
+	s.Require().NoError(err)
+
+	clientTmpl, err := util.NewSVIDTemplate("spiffe://example.org/agent")
+	s.Require().NoError(err)
+	clientCert, clientKey, err := util.Sign(clientTmpl, caCert, caKey)
+	s.Require().NoError(err)
+
+	otherCaTmpl, err := util.NewCATemplate("example.org")
+	s.Require().NoError(err)
+	otherCaCert, otherCaKey, err := util.SelfSign(otherCaTmpl)
+	s.Require().NoError(err)
+
+	otherClientTmpl, err := util.NewSVIDTemplate("spiffe://example.org/agent")
+	s.Require().NoError(err)
+	otherClientCert, otherClientKey, err := util.Sign(otherClientTmpl, otherCaCert, otherCaKey)
+	s.Require().NoError(err)
+
+	rootCAs := x509.NewCertPool()
+	rootCAs.AddCert(caCert)
+
+	// set the trust bundle and plumb a CA certificate
+	_, err = s.ds.CreateBundle(context.Background(), &datastore.CreateBundleRequest{
+		Bundle: &common.Bundle{
+			TrustDomainId: "spiffe://example.org",
+			RootCas: []*common.Certificate{
+				{DerBytes: caCert.Raw},
+			},
+		},
+	})
+	s.Require().NoError(err)
+	s.svidState.Update(svid.State{
+		SVID: []*x509.Certificate{serverCert},
+		Key:  serverKey,
+	})
+
+	var wg sync.WaitGroup
+	defer wg.Wait()
+
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+
+	wg.Add(1)
+	go func() {
+		defer wg.Done()
+		s.e.ListenAndServe(ctx)
+	}()
+
+	// This helper function attempts a TLS connection to the gRPC server. It
+	// uses the supplied client certificate, if any. It gives up the 2 seconds
+	// for the server to start listening, which is generous. Any non-dial
+	// related errors (i.e. TLS handshake failures) are returned.
+	try := func(cert *tls.Certificate) error {
+		tlsConfig := &tls.Config{
+			RootCAs: rootCAs,
+			// this override is just so we don't have to set up spiffe peer
+			// validation of the server by the client, which is outside the
+			// scope of this test.
+			ServerName: "just-for-validation",
+		}
+		if cert != nil {
+			tlsConfig.Certificates = append(tlsConfig.Certificates, *cert)
+		}
+		for i := 0; i < 20; i++ {
+			conn, err := tls.Dial("tcp", "127.0.0.1:8000", tlsConfig)
+			if err != nil {
+				if strings.HasPrefix(err.Error(), "dial") {
+					time.Sleep(time.Millisecond * 100)
+					continue
+				}
+				return err
+			}
+			conn.Close()
+			return nil
+		}
+		s.FailNow("unable to connect to server within 2 seconds")
+		return errors.New("unreachable")
+	}
+
+	err = try(nil)
+	s.Require().NoError(err, "client should be allowed if no cert presented")
+
+	err = try(&tls.Certificate{
+		Certificate: [][]byte{clientCert.Raw},
+		PrivateKey:  clientKey,
+	})
+	s.Require().NoError(err, "client should be allowed if proper cert presented")
+
+	err = try(&tls.Certificate{
+		Certificate: [][]byte{otherClientCert.Raw},
+		PrivateKey:  otherClientKey,
+	})
+	s.Require().Error(err, "client should NOT be allowed if cert presented is not trusted")
+}

pkg/server/endpoints/node/handler.go

@@ -560,24 +560,23 @@ func (h *Handler) getAttestResponse(ctx context.Context,
 }
 
 func (h *Handler) getCertFromCtx(ctx context.Context) (certificate *x509.Certificate, err error) {
-
 	ctxPeer, ok := peer.FromContext(ctx)
 	if !ok {
-		return nil, errors.New("It was not posible to extract peer from request")
+		return nil, errors.New("no peer information")
 	}
 	tlsInfo, ok := ctxPeer.AuthInfo.(credentials.TLSInfo)
 	if !ok {
-		return nil, errors.New("It was not posible to extract AuthInfo from request")
+		return nil, errors.New("no TLS auth info for peer")
 	}
 
 	if len(tlsInfo.State.VerifiedChains) == 0 {
-		return nil, errors.New("VerifiedChains was empty")
+		return nil, errors.New("no verified client certificate presented by peer")
 	}
 	chain := tlsInfo.State.VerifiedChains[0]
 	if len(chain) == 0 {
 		// this shouldn't be possible with the tls package, but we should be
 		// defensive.
-		return nil, errors.New("VerifiedChain was empty")
+		return nil, errors.New("verified client chain is missing certificates")
 	}
 
 	return chain[0], nil

test/util/cert_generation.go

@@ -9,9 +9,8 @@ import (
 	"crypto/x509/pkix"
 	"math/big"
 	mrand "math/rand"
+	"net/url"
 	"time"
-
-	"github.com/spiffe/go-spiffe/uri"
 )
 
 // NewSVIDTemplate returns a default SVID template with the specified SPIFFE ID. Must
@@ -136,18 +135,11 @@ func defaultCATemplate() *x509.Certificate {
 // Create an x509 extension with the URI SAN of the given SPIFFE ID, and set it onto
 // the referenced certificate
 func addSpiffeExtension(spiffeID string, cert *x509.Certificate) error {
-	uriSANs, err := uri.MarshalUriSANs([]string{spiffeID})
+	u, err := url.Parse(spiffeID)
 	if err != nil {
 		return err
 	}
-
-	ext := []pkix.Extension{{
-		Id:       uri.OidExtensionSubjectAltName,
-		Value:    uriSANs,
-		Critical: true,
-	}}
-
-	cert.ExtraExtensions = ext
+	cert.URIs = append(cert.URIs, u)
 	return nil
 }