fix broken node API x509 validation

The gRPC server TLS configuration was misconfigured to only request a
client certificate, but not perform validation. The implication is that
a certificate chained to any root with an expected SPIFFE ID could
impersonate a node SVID and obtain workload SVIDs.

Signed-off-by: Andrew Harding <azdagron@gmail.com>

pkg/agent/manager/manager_test.go

@@ -679,9 +679,6 @@ func TestFetchJWTSVID(t *testing.T) {
 	}
 	defer l.Close()
 
-	ca, cakey := createCA(t, trustDomain)
-	baseSVID, baseSVIDKey := createSVID(t, ca, cakey, "spiffe://"+trustDomain+"/agent", 1*time.Hour)
-
 	fetchResp := &node.FetchJWTSVIDResponse{}
 
 	apiHandler := newMockNodeAPIHandler(&mockNodeAPIHandlerConfig{
@@ -693,6 +690,9 @@ func TestFetchJWTSVID(t *testing.T) {
 		},
 		svidTTL: 200,
 	})
+
+	baseSVID, baseSVIDKey := apiHandler.newSVID("spiffe://"+trustDomain+"/spire/agent/join_token/abcd", 1*time.Hour)
+
 	apiHandler.start()
 	defer apiHandler.stop()
 
@@ -1097,7 +1097,7 @@ func (h *mockNodeAPIHandler) getGRPCServerConfig(hello *tls.ClientHelloInfo) (*t
 	roots.AddCert(h.ca())
 
 	c := &tls.Config{
-		ClientAuth:   tls.RequestClientCert,
+		ClientAuth:   tls.VerifyClientCertIfGiven,
 		Certificates: certs,
 		ClientCAs:    roots,
 	}
@@ -1116,11 +1116,18 @@ func (h *mockNodeAPIHandler) getCertFromCtx(ctx context.Context) (certificate *x
 		return nil, errors.New("It was not posible to extract AuthInfo from request")
 	}
 
-	if len(tlsInfo.State.PeerCertificates) == 0 {
-		return nil, errors.New("PeerCertificates was empty")
+	if len(tlsInfo.State.VerifiedChains) == 0 {
+		return nil, errors.New("VerifiedChains was empty")
+	}
+
+	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 tlsInfo.State.PeerCertificates[0], nil
+	return chain[0], nil
 }
 
 func createTempDir(t *testing.T) string {

pkg/server/endpoints/endpoints.go

@@ -200,7 +200,7 @@ func (e *endpoints) getGRPCServerConfig(ctx context.Context) func(*tls.ClientHel
 			// an SVID. In order to include the bootstrap endpoint
 			// in the same server as the rest of the Node API,
 			// request but don't require a client certificate
-			ClientAuth: tls.RequestClientCert,
+			ClientAuth: tls.VerifyClientCertIfGiven,
 
 			Certificates: certs,
 			ClientCAs:    roots,

pkg/server/endpoints/endpoints_test.go

@@ -11,7 +11,7 @@ import (
 	"testing"
 	"time"
 
-	"github.com/imkira/go-observer"
+	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"
@@ -152,7 +152,7 @@ func (s *EndpointsTestSuite) TestGetGRPCServerConfig() {
 	tlsConfig, err := s.e.getGRPCServerConfig(ctx)(nil)
 	require.NoError(s.T(), err)
 
-	s.Assert().Equal(tls.RequestClientCert, tlsConfig.ClientAuth)
+	s.Assert().Equal(tls.VerifyClientCertIfGiven, tlsConfig.ClientAuth)
 	s.Assert().Equal(certs, tlsConfig.Certificates)
 	s.Assert().Equal(pool, tlsConfig.ClientCAs)
 }

pkg/server/endpoints/node/handler.go

@@ -570,11 +570,17 @@ func (h *Handler) getCertFromCtx(ctx context.Context) (certificate *x509.Certifi
 		return nil, errors.New("It was not posible to extract AuthInfo from request")
 	}
 
-	if len(tlsInfo.State.PeerCertificates) == 0 {
-		return nil, errors.New("PeerCertificates was empty")
+	if len(tlsInfo.State.VerifiedChains) == 0 {
+		return nil, errors.New("VerifiedChains was empty")
+	}
+	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 tlsInfo.State.PeerCertificates[0], nil
+	return chain[0], nil
 }
 
 func (h *Handler) signCSRs(ctx context.Context,

pkg/server/endpoints/node/handler_test.go

@@ -27,11 +27,11 @@ import (
 	"github.com/spiffe/spire/test/fakes/fakeserverca"
 	"github.com/spiffe/spire/test/fakes/fakeservercatalog"
 	"github.com/spiffe/spire/test/fakes/fakeupstreamca"
-	"github.com/spiffe/spire/test/mock/proto/api/node"
-	"github.com/spiffe/spire/test/mock/proto/server/datastore"
-	"github.com/spiffe/spire/test/mock/proto/server/nodeattestor"
-	"github.com/spiffe/spire/test/mock/proto/server/noderesolver"
-	"github.com/spiffe/spire/test/mock/server/ca"
+	mock_node "github.com/spiffe/spire/test/mock/proto/api/node"
+	mock_datastore "github.com/spiffe/spire/test/mock/proto/server/datastore"
+	mock_nodeattestor "github.com/spiffe/spire/test/mock/proto/server/nodeattestor"
+	mock_noderesolver "github.com/spiffe/spire/test/mock/proto/server/noderesolver"
+	mock_ca "github.com/spiffe/spire/test/mock/server/ca"
 	"github.com/spiffe/spire/test/util"
 	"github.com/stretchr/testify/require"
 	"github.com/stretchr/testify/suite"
@@ -732,7 +732,7 @@ func getFakePeer() *peer.Peer {
 	parsedCert := loadCertFromPEM("base_cert.pem")
 
 	state := tls.ConnectionState{
-		PeerCertificates: []*x509.Certificate{parsedCert},
+		VerifiedChains: [][]*x509.Certificate{{parsedCert}},
 	}
 
 	addr, _ := net.ResolveTCPAddr("tcp", "127.0.0.1:12345")