Detect misconfiguration of bundle_endpoint_profile (spiffe#3395)

* Detect misconfiguration of bundle_endpoint_profile

Signed-off-by: Ryuma Yoshida <[email protected]>
Co-authored-by: Andrew Harding <[email protected]>

Author: 2 people

pkg/server/bundle/client/client.go

@@ -3,6 +3,7 @@ package client
 import (
 	"context"
 	"crypto/x509"
+	"errors"
 	"fmt"
 	"io"
 	"net/http"
@@ -35,6 +36,10 @@ type ClientConfig struct { //revive:disable-line:exported name stutter is intent
 	// using SPIFFE authentication. If unset, it is assumed that the endpoint
 	// is authenticated via Web PKI.
 	SPIFFEAuth *SPIFFEAuthConfig
+
+	// mutateTransportHook is a hook to influence the transport used during
+	// tests.
+	mutateTransportHook func(*http.Transport)
 }
 
 // Client is used to fetch a bundle and metadata from a bundle endpoint
@@ -48,7 +53,7 @@ type client struct {
 }
 
 func NewClient(config ClientConfig) (Client, error) {
-	httpClient := &http.Client{}
+	transport := newTransport()
 	if config.SPIFFEAuth != nil {
 		endpointID := config.SPIFFEAuth.EndpointSpiffeID
 		if endpointID.IsZero() {
@@ -59,19 +64,26 @@ func NewClient(config ClientConfig) (Client, error) {
 
 		authorizer := tlsconfig.AuthorizeID(endpointID)
 
-		httpClient.Transport = &http.Transport{
-			TLSClientConfig: tlsconfig.TLSClientConfig(bundle, authorizer),
-		}
+		transport.TLSClientConfig = tlsconfig.TLSClientConfig(bundle, authorizer)
+	}
+	if config.mutateTransportHook != nil {
+		config.mutateTransportHook(transport)
 	}
 	return &client{
 		c:      config,
-		client: httpClient,
+		client: &http.Client{Transport: transport},
 	}, nil
 }
 
 func (c *client) FetchBundle(ctx context.Context) (*bundleutil.Bundle, error) {
 	resp, err := c.client.Get(c.c.EndpointURL)
 	if err != nil {
+		var hostnameError x509.HostnameError
+		if errors.As(err, &hostnameError) && c.c.SPIFFEAuth == nil && len(hostnameError.Certificate.URIs) > 0 {
+			if id, idErr := spiffeid.FromString(hostnameError.Certificate.URIs[0].String()); idErr == nil {
+				return nil, errs.New("failed to authenticate bundle endpoint using web authentication but the server certificate contains SPIFFE ID %q: maybe use https_spiffe instead of https_web: %v", id, err)
+			}
+		}
 		return nil, errs.New("failed to fetch bundle: %v", err)
 	}
 	defer resp.Body.Close()
@@ -93,3 +105,7 @@ func tryRead(r io.Reader) string {
 	n, _ := r.Read(b)
 	return string(b[:n])
 }
+
+func newTransport() *http.Transport {
+	return http.DefaultTransport.(*http.Transport).Clone()
+}

pkg/server/bundle/client/client_test.go

@@ -7,7 +7,6 @@ import (
 	"crypto/x509"
 	"fmt"
 	"math/big"
-	"net"
 	"net/http"
 	"net/http/httptest"
 	"net/url"
@@ -33,6 +32,8 @@ func TestClient(t *testing.T) {
 		body           string
 		newClientErr   string
 		fetchBundleErr string
+		useWebAuth     bool
+		mutateConfig   func(*ClientConfig)
 	}{
 		{
 			name:   "success",
@@ -73,6 +74,18 @@ func TestClient(t *testing.T) {
 			expectedID:     serverID,
 			fetchBundleErr: "failed to decode bundle",
 		},
+		{
+			name:           "hostname validation fails",
+			status:         http.StatusOK,
+			body:           "NOT JSON",
+			serverID:       serverID,
+			expectedID:     serverID,
+			fetchBundleErr: "failed to authenticate bundle endpoint using web authentication but the server certificate contains SPIFFE ID \"spiffe://domain.test/spiffe-bundle-endpoint-server\": maybe use https_spiffe instead of https_web:",
+			useWebAuth:     true,
+			mutateConfig: func(c *ClientConfig) {
+				c.SPIFFEAuth = nil
+			},
+		},
 	}
 
 	for _, testCase := range testCases {
@@ -96,14 +109,30 @@ func TestClient(t *testing.T) {
 			server.StartTLS()
 			defer server.Close()
 
-			client, err := NewClient(ClientConfig{
+			var mutateTransportHook func(*http.Transport)
+			if testCase.useWebAuth {
+				mutateTransportHook = func(transport *http.Transport) {
+					rootCAs := x509.NewCertPool()
+					rootCAs.AddCert(serverCert)
+					transport.TLSClientConfig = &tls.Config{RootCAs: rootCAs, MinVersion: tls.VersionTLS12}
+				}
+			}
+
+			config := ClientConfig{
 				TrustDomain: trustDomain,
 				EndpointURL: server.URL,
 				SPIFFEAuth: &SPIFFEAuthConfig{
 					EndpointSpiffeID: testCase.expectedID,
 					RootCAs:          []*x509.Certificate{serverCert},
 				},
-			})
+				mutateTransportHook: mutateTransportHook,
+			}
+
+			if testCase.mutateConfig != nil {
+				testCase.mutateConfig(&config)
+			}
+
+			client, err := NewClient(config)
 			if testCase.newClientErr != "" {
 				require.Error(t, err)
 				require.Contains(t, err.Error(), testCase.newClientErr)
@@ -128,8 +157,6 @@ func TestClient(t *testing.T) {
 func createServerCertificate(t *testing.T, serverID spiffeid.ID) (*x509.Certificate, crypto.Signer) {
 	return spiretest.SelfSignCertificate(t, &x509.Certificate{
 		SerialNumber: big.NewInt(0),
-		DNSNames:     []string{"localhost"},
-		IPAddresses:  []net.IP{net.IPv4(127, 0, 0, 1)},
 		NotAfter:     time.Now().Add(time.Hour),
 		URIs:         []*url.URL{serverID.URL()},
 	})