[v17] fix: sanitize URL when fetching azure attested data intermediate cert

lib/auth/azure_certs.go

@@ -23,12 +23,12 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"net/http"
+	"net/url"
 	"strings"
 
 	"github.com/gravitational/trace"
 
 	"github.com/gravitational/teleport"
-	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/utils"
 )
 
@@ -48,45 +48,88 @@ func isAllowedDomain(cn string, domains []string) bool {
 	return false
 }
 
-// getAzureIssuerCert fetches a x509 certificate's issuing certificate.
-func getAzureIssuerCert(ctx context.Context, cert *x509.Certificate) (*x509.Certificate, error) {
-	if len(cert.IssuingCertificateURL) == 0 {
-		return nil, nil
-	}
+func validateAzureCertIssuerURL(issuerURLString string) (string, error) {
+	// All active issuing certs are listed here
+	// https://www.microsoft.com/pkiops/docs/repository.htm
+	//
+	// The cert path always looks like the following, although this does not
+	// appear to be guaranteed by Microsoft.
+	// url: http://www.microsoft.com/pkiops/certs/<cert-name>.crt
+	//
+	// In v18.5+ certs are never fetched over the network and this will cease
+	// to be an issue.
+	const (
+		allowedHost       = "www.microsoft.com"
+		allowedPathPrefix = "/pkiops/certs/"
+		allowedPathSuffix = ".crt"
+	)
 
-	httpClient, err := defaults.HTTPClient()
+	issuerURL, err := url.Parse(issuerURLString)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return "", trace.AccessDenied("url failed to parse")
+	}
+
+	switch issuerURL.Scheme {
+	case "http", "https":
+	default:
+		return "", trace.AccessDenied("invalid url scheme %q", issuerURL.Scheme)
+	}
+
+	if issuerURL.Host != allowedHost {
+		return "", trace.AccessDenied("invalid host %q", issuerURL.Host)
+	}
+
+	if !strings.HasPrefix(issuerURL.Path, allowedPathPrefix) ||
+		!strings.HasSuffix(issuerURL.Path, allowedPathSuffix) {
+		return "", trace.AccessDenied("invalid path, must match %s<name>%s",
+			allowedPathPrefix, allowedPathSuffix)
+	}
+
+	// Construct a new URL with only the scheme, host, and path to strip any
+	// possible extra fields like query params or fragments.
+	sanitizedURL := url.URL{
+		Scheme: issuerURL.Scheme,
+		Host:   allowedHost,
+		Path:   issuerURL.Path,
+	}
+	return sanitizedURL.String(), nil
+}
+
+// getAzureIssuerCert fetches a x509 certificate's issuing certificate.
+func getAzureIssuerCert(ctx context.Context, cert *x509.Certificate, httpClient utils.HTTPDoClient) (*x509.Certificate, error) {
+	if len(cert.IssuingCertificateURL) == 0 {
+		return nil, trace.BadParameter("certificate has no issuing certificate URL")
 	}
 
 	// Azure sends only one issuing cert.
 	issuerURL := cert.IssuingCertificateURL[0]
-	commonName := cert.Subject.CommonName
-	if !isAllowedDomain(commonName, allowedAzureCommonNames) {
-		return nil, trace.AccessDenied(
-			"certificate common name does not match allow-list (%s)",
-			commonName,
-		)
+	sanitizedIssuerURL, err := validateAzureCertIssuerURL(issuerURL)
+	if err != nil {
+		return nil, trace.Wrap(err, "validating issuing certificate URL")
 	}
-	req, err := http.NewRequestWithContext(ctx, http.MethodGet, issuerURL, nil)
+
+	req, err := http.NewRequestWithContext(ctx, http.MethodGet, sanitizedIssuerURL, nil /*body*/)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
+
 	resp, err := httpClient.Do(req)
 	if err != nil {
-		return nil, trace.Wrap(err)
+		return nil, trace.Wrap(err, "fetching issuing certificate")
 	}
 	defer resp.Body.Close()
+
+	if resp.StatusCode != http.StatusOK {
+		return nil, trace.AccessDenied("failed to fetch issuing cert, got HTTP status code %d", resp.StatusCode)
+	}
+
 	body, err := utils.ReadAtMost(resp.Body, teleport.MaxHTTPResponseSize)
 	if err != nil {
-		return nil, trace.Wrap(err)
-	}
-	if resp.StatusCode != http.StatusOK {
-		return nil, trace.ReadError(resp.StatusCode, body)
+		return nil, trace.Wrap(err, "reading HTTP response body")
 	}
 
 	issuerCert, err := x509.ParseCertificate(body)
-	return issuerCert, trace.Wrap(err)
+	return issuerCert, trace.Wrap(err, "parsing issuing certificate")
 }
 
 func getAzureRootCerts() ([]*x509.Certificate, error) {

lib/auth/bot_test.go

@@ -22,11 +22,13 @@ import (
 	"bytes"
 	"context"
 	"crypto"
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/base64"
 	"encoding/json"
-	"encoding/pem"
 	"net/http"
 	"strings"
 	"testing"
@@ -66,7 +68,6 @@ import (
 	"github.com/gravitational/teleport/lib/cloud/azure"
 	libevents "github.com/gravitational/teleport/lib/events"
 	"github.com/gravitational/teleport/lib/events/eventstest"
-	"github.com/gravitational/teleport/lib/fixtures"
 	"github.com/gravitational/teleport/lib/kube/token"
 	"github.com/gravitational/teleport/lib/reversetunnelclient"
 	"github.com/gravitational/teleport/lib/tbot/identity"
@@ -774,12 +775,16 @@ func TestRegisterBot_RemoteAddr(t *testing.T) {
 			subID: vmClient,
 		})
 
-		tlsConfig, err := fixtures.LocalTLSConfig()
-		require.NoError(t, err)
+		caChain := newFakeAzureCAChain(t)
+		// Fake the HTTP client used to fetch the issuer CA.
+		httpClient := newFakeAzureIssuerHTTPClient(caChain.intermediateCertDER)
 
-		block, _ := pem.Decode(fixtures.LocalhostKey)
-		pkey, err := x509.ParsePKCS1PrivateKey(block.Bytes)
+		instanceKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
 		require.NoError(t, err)
+		instanceCert := caChain.issueLeafCert(t,
+			instanceKey.Public(),
+			"instance.metadata.azure.com",
+			"http://www.microsoft.com/pkiops/certs/testcert.crt")
 
 		certs, err := a.RegisterUsingAzureMethodWithOpts(context.Background(), func(challenge string) (*proto.RegisterUsingAzureMethodRequest, error) {
 			ad := auth.AttestedData{
@@ -791,7 +796,7 @@ func TestRegisterBot_RemoteAddr(t *testing.T) {
 			require.NoError(t, err)
 			s, err := pkcs7.NewSignedData(adBytes)
 			require.NoError(t, err)
-			require.NoError(t, s.AddSigner(tlsConfig.Certificate, pkey, pkcs7.SignerInfoConfig{}))
+			require.NoError(t, s.AddSigner(instanceCert, instanceKey, pkcs7.SignerInfoConfig{}))
 			signature, err := s.Finish()
 			require.NoError(t, err)
 			signedAD := auth.SignedAttestedData{
@@ -814,7 +819,12 @@ func TestRegisterBot_RemoteAddr(t *testing.T) {
 				AccessToken:  accessToken,
 			}
 			return req, nil
-		}, auth.WithAzureCerts([]*x509.Certificate{tlsConfig.Certificate}), auth.WithAzureVerifyFunc(mockVerifyToken(nil)), auth.WithAzureVMClientGetter(getVMClient))
+		},
+			auth.WithAzureCerts([]*x509.Certificate{caChain.rootCert}),
+			auth.WithAzureVerifyFunc(mockVerifyToken(nil)),
+			auth.WithAzureVMClientGetter(getVMClient),
+			auth.WithAzureIssuerHTTPClient(httpClient),
+		)
 		require.NoError(t, err)
 		checkCertLoginIP(t, certs.TLS, remoteAddr)
 	})

lib/auth/export_test.go

@@ -451,6 +451,12 @@ func WithAuthVersion(v *semver.Version) iamRegisterOption {
 	return withAuthVersion(v)
 }
 
+func WithAzureIssuerHTTPClient(httpClient utils.HTTPDoClient) AzureRegisterOption {
+	return func(cfg *AzureRegisterConfig) {
+		cfg.issuerHTTPClient = httpClient
+	}
+}
+
 func (s *TLSServer) GRPCServer() *GRPCServer {
 	return s.grpcServer
 }

lib/auth/join_azure.go

@@ -45,6 +45,7 @@ import (
 	workloadidentityv1pb "github.com/gravitational/teleport/api/gen/proto/go/teleport/workloadidentity/v1"
 	"github.com/gravitational/teleport/api/types"
 	"github.com/gravitational/teleport/lib/cloud/azure"
+	"github.com/gravitational/teleport/lib/defaults"
 	"github.com/gravitational/teleport/lib/utils"
 )
 
@@ -118,6 +119,7 @@ type azureRegisterConfig struct {
 	certificateAuthorities []*x509.Certificate
 	verify                 azureVerifyTokenFunc
 	getVMClient            vmClientGetter
+	issuerHTTPClient       utils.HTTPDoClient
 }
 
 func azureVerifyFuncFromOIDCVerifier(cfg *oidc.Config) azureVerifyTokenFunc {
@@ -185,6 +187,13 @@ func (cfg *azureRegisterConfig) CheckAndSetDefaults(ctx context.Context) error {
 			return client, trace.Wrap(err)
 		}
 	}
+	if cfg.issuerHTTPClient == nil {
+		httpClient, err := defaults.HTTPClient()
+		if err != nil {
+			return trace.Wrap(err)
+		}
+		cfg.issuerHTTPClient = httpClient
+	}
 	return nil
 }
 
@@ -193,7 +202,7 @@ type azureRegisterOption func(cfg *azureRegisterConfig)
 // parseAndVeryAttestedData verifies that an attested data document was signed
 // by Azure. If verification is successful, it returns the ID of the VM that
 // produced the document.
-func parseAndVerifyAttestedData(ctx context.Context, adBytes []byte, challenge string, certs []*x509.Certificate) (subscriptionID, vmID string, err error) {
+func parseAndVerifyAttestedData(ctx context.Context, cfg *azureRegisterConfig, adBytes []byte, challenge string) (subscriptionID, vmID string, err error) {
 	var signedAD signedAttestedData
 	if err := utils.FastUnmarshal(adBytes, &signedAD); err != nil {
 		return "", "", trace.Wrap(err)
@@ -223,22 +232,30 @@ func parseAndVerifyAttestedData(ctx context.Context, adBytes []byte, challenge s
 	if len(p7.Certificates) == 0 {
 		return "", "", trace.AccessDenied("no certificates for signature")
 	}
-	fixAzureSigningAlgorithm(p7)
+	signingCert := p7.Certificates[0]
 
-	// Azure only sends the leaf cert, so we have to fetch the intermediate.
-	intermediate, err := getAzureIssuerCert(ctx, p7.Certificates[0])
-	if err != nil {
-		return "", "", trace.Wrap(err)
+	if !isAllowedDomain(signingCert.Subject.CommonName, allowedAzureCommonNames) {
+		return "", "", trace.AccessDenied(
+			"certificate common name does not match allow-list (%s)",
+			signingCert.Subject.CommonName,
+		)
 	}
-	if intermediate != nil {
+
+	// Azure only sends the leaf cert, so we have to fetch the intermediate.
+	if len(signingCert.IssuingCertificateURL) != 0 {
+		intermediate, err := getAzureIssuerCert(ctx, signingCert, cfg.issuerHTTPClient)
+		if err != nil {
+			return "", "", trace.Wrap(err, "getting intermediate issuing certificate for attested data")
+		}
 		p7.Certificates = append(p7.Certificates, intermediate)
 	}
 
 	pool := x509.NewCertPool()
-	for _, cert := range certs {
+	for _, cert := range cfg.certificateAuthorities {
 		pool.AddCert(cert)
 	}
 
+	fixAzureSigningAlgorithm(p7)
 	if err := p7.VerifyWithChain(pool); err != nil {
 		return "", "", trace.Wrap(err)
 	}
@@ -421,7 +438,7 @@ func (a *Server) checkAzureRequest(
 		return nil, trace.BadParameter("azure join method only supports ProvisionTokenV2, '%T' was provided", provisionToken)
 	}
 
-	subID, vmID, err := parseAndVerifyAttestedData(ctx, req.AttestedData, challenge, cfg.certificateAuthorities)
+	subID, vmID, err := parseAndVerifyAttestedData(ctx, cfg, req.AttestedData, challenge)
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}