TMP

Signed-off-by: Ciprian Hacman <ciprian@hakman.dev>

Author: hakman

upup/pkg/fi/cloudup/azure/attest.go

@@ -34,18 +34,35 @@ import (
 
 	"github.com/smallstep/pkcs7"
 	expirationcache "k8s.io/client-go/tools/cache"
+	"k8s.io/klog/v2"
 )
 
 const (
 	// attestedDocumentTimeFormat is the timestamp format used by the Azure IMDS attested document.
 	attestedDocumentTimeFormat = "01/02/06 15:04:05 -0700"
 
+	// attestedDocumentMaxAge bounds how old an IMDS attested document may be,
+	// independent of the service-provided expiration. This narrows the replay
+	// window when the nonce is derived from deterministic request content.
+	attestedDocumentMaxAge = 5 * time.Minute
+
+	// attestedDocumentMaxClockSkew allows a modest amount of node/controller
+	// clock skew on all verifier-vs-local-time checks before rejecting the document.
+	attestedDocumentMaxClockSkew = 2 * time.Minute
+
 	// attestedDocumentNonceLength is the length of the hex-encoded SHA256 prefix
 	// used as the Azure IMDS attested document nonce. IMDS enforces a 32-character
 	// maximum for the nonce parameter; 32 hex chars is 128 bits of entropy, well
 	// above the cryptographic nonce floor.
 	attestedDocumentNonceLength = 32
 
+	// azureMetadataDNSName and azureMetadataSubdomainSuffix restrict the PKCS7
+	// signer certificate to AzureCloud (public) metadata endpoints.
+	// TODO: When sovereign cloud environments are supported, add their domains:
+	//   - metadata.azure.us         (AzureUSGovernment)
+	//   - metadata.azure.cn         (AzureChinaCloud)
+	//   - metadata.microsoftazure.de (AzureGermanCloud)
+	// https://learn.microsoft.com/en-us/azure/virtual-machines/instance-metadata-service#signature-validation-guidance
 	azureMetadataDNSName         = "metadata.azure.com"
 	azureMetadataSubdomainSuffix = ".metadata.azure.com"
 
@@ -132,8 +149,9 @@ type attestedTimeStamp struct {
 }
 
 // verifyAttestedDocument verifies a PKCS7 attested document using the system
-// root certificate pool, first trying the embedded PKCS7 chain and only
-// falling back to Microsoft PKI when intermediates are missing.
+// root certificate pool, validating cheap signed-content checks before chain
+// building and consulting Microsoft PKI only if the embedded PKCS7 chain does
+// not already provide the required issuer certificate.
 func verifyAttestedDocument(signature string, body []byte) (*attestedData, error) {
 	rootCertPool, err := systemCertPool()
 	if err != nil {
@@ -143,10 +161,14 @@ func verifyAttestedDocument(signature string, body []byte) (*attestedData, error
 	return verifyAttestedDocumentWithRootAndFetcher(signature, body, rootCertPool, intermediateCertPoolForSigner)
 }
 
+// intermediateCertPoolForSigner returns intermediates for the signer's issuer
+// using the package-level TTL caches, fetching from Microsoft PKI on miss.
+func intermediateCertPoolForSigner(signer *x509.Certificate) (*x509.CertPool, error) {
+	return intermediateCertPoolWithCaches(signer, fetchIntermediateCerts, intermediateCertPositiveCache, intermediateCertNegativeCache)
+}
+
 // verifyAttestedDocumentWithRootAndFetcher verifies a PKCS7 attested document
-// using the supplied root pool and intermediate fetcher. It prefers the
-// embedded PKCS7 certificates and only consults the fetcher when the embedded
-// chain does not already verify.
+// using the supplied root pool and intermediate fetcher.
 func verifyAttestedDocumentWithRootAndFetcher(signature string, body []byte, rootCertPool *x509.CertPool, fetchIntermediates func(*x509.Certificate) (*x509.CertPool, error)) (*attestedData, error) {
 	if rootCertPool == nil {
 		return nil, fmt.Errorf("root certificate pool is required")
@@ -160,41 +182,36 @@ func verifyAttestedDocumentWithRootAndFetcher(signature string, body []byte, roo
 		return nil, err
 	}
 
-	embeddedOnlyIntermediates := x509.NewCertPool()
-	if err := verifySignerCertChain(signer, p7.Certificates, rootCertPool, embeddedOnlyIntermediates); err != nil {
-		// Signer SAN was validated above; only now is it safe to hit the network.
-		intermediateCerts, fetchErr := fetchIntermediates(signer)
-		if fetchErr != nil {
-			return nil, fmt.Errorf("verifying PKCS7 certificate chain: embedded=%w; fetch=%w", err, fetchErr)
-		}
-		if err := verifySignerCertChain(signer, p7.Certificates, rootCertPool, intermediateCerts); err != nil {
-			return nil, fmt.Errorf("verifying PKCS7 certificate chain: %w", err)
-		}
+	// The PKCS7 signature is already integrity-checked above, so it is safe to
+	// parse the signed content now for rejection-only checks like nonce and time
+	// bounds before paying for chain building or network fetches.
+	data, err := parseAndValidateAttestedDocumentContent(p7.Content, body)
+	if err != nil {
+		return nil, err
 	}
 
-	return parseAndValidateAttestedDocumentContent(p7.Content, body)
-}
-
-// verifyAttestedDocumentWithCertPools verifies a base64-encoded PKCS7 attested
-// document using caller-supplied root and intermediate certificate pools.
-func verifyAttestedDocumentWithCertPools(signature string, body []byte, rootCertPool *x509.CertPool, intermediateCerts *x509.CertPool) (*attestedData, error) {
-	if rootCertPool == nil {
-		return nil, fmt.Errorf("root certificate pool is required")
-	}
-	if intermediateCerts == nil {
-		return nil, fmt.Errorf("intermediate certificate pool is required")
+	if err := verifySignerCertChain(signer, p7.Certificates, rootCertPool, x509.NewCertPool()); err == nil {
+		klog.V(2).Infof("PKCS7 certificate chain verified with embedded certificates for signer issuer %q", signer.Issuer)
+		return data, nil
+	} else {
+		for _, cert := range p7.Certificates {
+			if validateFetchedIntermediateForSigner(signer, cert) == nil {
+				return nil, fmt.Errorf("verifying PKCS7 certificate chain: %w", err)
+			}
+		}
 	}
 
-	p7, signer, err := parseAndValidatePKCS7Signer(signature)
+	klog.V(2).Infof("Resolving intermediate certificates for signer issuer %q", signer.Issuer)
+	intermediateCerts, err := fetchIntermediates(signer)
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("fetching intermediate certificates: %w", err)
 	}
-
 	if err := verifySignerCertChain(signer, p7.Certificates, rootCertPool, intermediateCerts); err != nil {
 		return nil, fmt.Errorf("verifying PKCS7 certificate chain: %w", err)
 	}
+	klog.V(2).Infof("PKCS7 certificate chain verified after resolving intermediate certificates for signer issuer %q", signer.Issuer)
 
-	return parseAndValidateAttestedDocumentContent(p7.Content, body)
+	return data, nil
 }
 
 // parseAndValidatePKCS7Signer decodes and parses a base64-encoded PKCS7
@@ -211,24 +228,29 @@ func parseAndValidatePKCS7Signer(signature string) (*pkcs7.PKCS7, *x509.Certific
 	if err != nil {
 		return nil, nil, fmt.Errorf("decoding PKCS7 signature: %w", err)
 	}
+	klog.V(4).Infof("Decoded PKCS7 signature (%d bytes)", len(sigBytes))
 
 	p7, err := pkcs7.Parse(sigBytes)
 	if err != nil {
 		return nil, nil, fmt.Errorf("parsing PKCS7 signature: %w", err)
 	}
+	klog.V(8).Infof("Parsed PKCS7 structure with %d embedded certificate(s)", len(p7.Certificates))
 
 	// Verify the PKCS7 signature against the embedded leaf certificate.
 	if err := p7.Verify(); err != nil {
 		return nil, nil, fmt.Errorf("verifying PKCS7 signature: %w", err)
 	}
+	klog.V(4).Infof("PKCS7 self-signature verified")
 
 	signer := p7.GetOnlySigner()
 	if signer == nil {
 		return nil, nil, fmt.Errorf("getting PKCS7 signer certificate")
 	}
+	klog.V(8).Infof("PKCS7 signer certificate: subject=%q issuer=%q SANs=%v", signer.Subject, signer.Issuer, signer.DNSNames)
 	if err := validateAzureMetadataSignerSAN(signer); err != nil {
 		return nil, nil, fmt.Errorf("validating PKCS7 signer SAN: %w", err)
 	}
+	klog.V(4).Infof("PKCS7 signer SAN validated as Azure metadata endpoint")
 
 	return p7, signer, nil
 }
@@ -241,28 +263,58 @@ func nonceForBody(body []byte) string {
 }
 
 // parseAndValidateAttestedDocumentContent unmarshals the signed attestation
-// payload and validates its nonce and expiration.
+// payload and validates its nonce and freshness timestamps.
 func parseAndValidateAttestedDocumentContent(content []byte, body []byte) (*attestedData, error) {
-	// Extract and validate the signed attested data.
+	// Parse the signed attested data.
 	var data attestedData
 	if err := json.Unmarshal(content, &data); err != nil {
 		return nil, fmt.Errorf("unmarshalling attested data: %w", err)
 	}
+	klog.V(4).Infof("Attested document content: vmId=%q subscriptionId=%q createdOn=%q expiresOn=%q", data.VMId, data.SubscriptionId, data.TimeStamp.CreatedOn, data.TimeStamp.ExpiresOn)
+
+	if data.VMId == "" {
+		return nil, fmt.Errorf("attested document vmId is required")
+	}
+	if data.SubscriptionId == "" {
+		return nil, fmt.Errorf("attested document subscriptionId is required")
+	}
 
 	// Verify the nonce matches the request body hash (replay protection).
-	if data.Nonce != nonceForBody(body) {
-		return nil, fmt.Errorf("attested document nonce mismatch")
+	expectedNonce := nonceForBody(body)
+	if data.Nonce != expectedNonce {
+		return nil, fmt.Errorf("attested document nonce mismatch: got=%q expected=%q", data.Nonce, expectedNonce)
 	}
 
-	// Verify the attested document has not expired.
+	now := time.Now().UTC()
+	if data.TimeStamp.CreatedOn == "" {
+		return nil, fmt.Errorf("attested document createdOn is required")
+	}
+	createdOn, err := time.Parse(attestedDocumentTimeFormat, data.TimeStamp.CreatedOn)
+	if err != nil {
+		return nil, fmt.Errorf("parsing attested document creation: %w", err)
+	}
+	if createdOn.After(now.Add(attestedDocumentMaxClockSkew)) {
+		return nil, fmt.Errorf("attested document createdOn %s is too far in the future", data.TimeStamp.CreatedOn)
+	}
+	oldestAllowedCreatedOn := now.Add(-(attestedDocumentMaxAge + attestedDocumentMaxClockSkew))
+	if createdOn.Before(oldestAllowedCreatedOn) {
+		return nil, fmt.Errorf("attested document createdOn %s is older than allowed freshness window of %s plus %s clock skew", data.TimeStamp.CreatedOn, attestedDocumentMaxAge, attestedDocumentMaxClockSkew)
+	}
+	klog.V(4).Infof("Attested document createdOn is fresh (createdOn=%s now=%s)", createdOn.Format(time.RFC3339), now.Format(time.RFC3339))
+
+	// Verify the attested document has not expired and has a coherent lifetime.
 	if data.TimeStamp.ExpiresOn != "" {
 		expiresOn, err := time.Parse(attestedDocumentTimeFormat, data.TimeStamp.ExpiresOn)
 		if err != nil {
 			return nil, fmt.Errorf("parsing attested document expiration: %w", err)
 		}
-		if time.Now().After(expiresOn) {
+		if expiresOn.Before(createdOn) {
+			return nil, fmt.Errorf("attested document expiresOn %s is before createdOn %s", data.TimeStamp.ExpiresOn, data.TimeStamp.CreatedOn)
+		}
+		if expiresOn.Before(now.Add(-attestedDocumentMaxClockSkew)) {
 			return nil, fmt.Errorf("attested document expired at %s", data.TimeStamp.ExpiresOn)
 		}
+		klog.V(4).Infof("Attested document not expired (expiresOn=%s)", expiresOn.Format(time.RFC3339))
 	}
 
 	return &data, nil
@@ -287,12 +339,6 @@ func systemCertPool() (*x509.CertPool, error) {
 	return pool, nil
 }
 
-// intermediateCertPoolForSigner returns intermediates for the signer's issuer
-// using the package-level TTL caches, fetching from Microsoft PKI on miss.
-func intermediateCertPoolForSigner(signer *x509.Certificate) (*x509.CertPool, error) {
-	return intermediateCertPoolWithCaches(signer, fetchIntermediateCerts, intermediateCertPositiveCache, intermediateCertNegativeCache)
-}
-
 // intermediateCertPoolWithCaches performs a cached lookup against the supplied
 // positive and negative TTL caches, invoking fetch on a miss. Tests inject
 // their own stores and fetchers.
@@ -319,12 +365,14 @@ func intermediateCertPoolWithCaches(signer *x509.Certificate, fetch func(*x509.C
 	// Positive cache wins over negative: a successful later fetch overwrites
 	// any stale negative entry, which expires on its own shorter TTL.
 	if obj, ok, _ := positive.GetByKey(keyStr); ok {
+		klog.V(4).Infof("Intermediate certificate cache hit (positive) for signer issuer %q", signer.Issuer)
 		return obj.(*intermediateCertCacheEntry).pool, nil
 	}
 	if _, ok, _ := negative.GetByKey(keyStr); ok {
-		return nil, fmt.Errorf("intermediate certificate fetch recently failed for signer issuer (cached)")
+		return nil, fmt.Errorf("intermediate certificate fetch recently failed for signer issuer %q (cached)", signer.Issuer)
 	}
 
+	klog.V(2).Infof("Intermediate certificate cache miss for signer issuer %q; fetching from Microsoft PKI", signer.Issuer)
 	pool, fetchErr := fetch(signer)
 	entry.pool = pool
 	if fetchErr != nil {
@@ -373,19 +421,23 @@ func fetchIntermediateCertsFromBaseURL(client *http.Client, baseURL string, sign
 	pool := x509.NewCertPool()
 	matched := 0
 	for _, url := range urls {
+		klog.V(2).Infof("Fetching intermediate certificate from %s", url)
 		cert, err := fetchCertificate(client, url)
 		if err != nil {
 			return nil, err
 		}
 		if err := validateFetchedIntermediateForSigner(signer, cert); err != nil {
+			klog.V(2).Infof("Fetched intermediate certificate from %s did not match signer issuer: %v", url, err)
 			continue
 		}
+		klog.V(2).Infof("Fetched intermediate certificate from %s matched signer issuer", url)
 		pool.AddCert(cert)
 		matched++
 	}
 	if matched == 0 {
-		return nil, fmt.Errorf("no fetched intermediate certificates matched signer issuer")
+		return nil, fmt.Errorf("no fetched intermediate certificates matched signer issuer %q", signer.Issuer)
 	}
+	klog.V(2).Infof("Fetched %d intermediate certificate(s) matching signer issuer %q from %d candidate URL(s)", matched, signer.Issuer, len(urls))
 
 	return pool, nil
 }