Increase test coverage for AK certificate properties

acme/challenge.go

@@ -718,41 +718,40 @@ var (
 // in accordance with the required properties. The requirements come from:
 // https://www.w3.org/TR/webauthn-2/#sctn-tpm-cert-requirements.
 //
-// 	- Version MUST be set to 3.
-// 	- Subject field MUST be set to empty.
-//	- The Subject Alternative Name extension MUST be set as defined
-//	  in [TPMv2-EK-Profile] section 3.2.9.
-//	- The Extended Key Usage extension MUST contain the OID 2.23.133.8.3
-//	  ("joint-iso-itu-t(2) internationalorganizations(23) 133 tcg-kp(8) tcg-kp-AIKCertificate(3)").
-//	- The Basic Constraints extension MUST have the CA component set to false.
-//	- An Authority Information Access (AIA) extension with entry id-ad-ocsp
-// 	  and a CRL Distribution Point extension [RFC5280] are both OPTIONAL as
-//	  the status of many attestation certificates is available through metadata
-// 	  services. See, for example, the FIDO Metadata Service.
-
+//   - Version MUST be set to 3.
+//   - Subject field MUST be set to empty.
+//   - The Subject Alternative Name extension MUST be set as defined
+//     in [TPMv2-EK-Profile] section 3.2.9.
+//   - The Extended Key Usage extension MUST contain the OID 2.23.133.8.3
+//     ("joint-iso-itu-t(2) internationalorganizations(23) 133 tcg-kp(8) tcg-kp-AIKCertificate(3)").
+//   - The Basic Constraints extension MUST have the CA component set to false.
+//   - An Authority Information Access (AIA) extension with entry id-ad-ocsp
+//     and a CRL Distribution Point extension [RFC5280] are both OPTIONAL as
+//     the status of many attestation certificates is available through metadata
+//     services. See, for example, the FIDO Metadata Service.
 func validateAKCertificate(c *x509.Certificate) error {
 	if c.Version != 3 {
 		return fmt.Errorf("AK certificate has invalid version %d; only version 3 is allowed", c.Version)
 	}
 	if c.Subject.String() != "" {
-		return fmt.Errorf("AK certificate subject must be empty; got %s", c.Subject)
+		return fmt.Errorf("AK certificate subject must be empty; got %q", c.Subject)
 	}
-	if err := validateAKCertificateSubjectAlternativeNames(c); err != nil {
-		return err
+	if c.IsCA {
+		return errors.New("AK certificate must not be a CA")
 	}
 	if err := validateAKCertificateExtendedKeyUsage(c); err != nil {
 		return err
 	}
-	if c.IsCA {
-		return errors.New("AK certificate must not be a CA")
+	if err := validateAKCertificateSubjectAlternativeNames(c); err != nil {
+		return err
 	}
 
 	return nil
 }
 
+// validateAKCertificateSubjectAlternativeNames checks if the AK certificate
+// has TPM hardware details set.
 func validateAKCertificateSubjectAlternativeNames(c *x509.Certificate) error {
-	return nil // TODO(hs): remove this early return when we require AK certificates to set these
-
 	sans, err := x509util.ParseSubjectAlternativeNames(c)
 	if err != nil {
 		return fmt.Errorf("failed parsing AK certificate Subject Alternative Names: %w", err)

acme/challenge_test.go

@@ -31,15 +31,14 @@ import (
 	"time"
 
 	"github.com/fxamacker/cbor/v2"
+	"github.com/smallstep/certificates/authority/config"
+	"github.com/smallstep/certificates/authority/provisioner"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-
 	"go.step.sm/crypto/jose"
 	"go.step.sm/crypto/keyutil"
 	"go.step.sm/crypto/minica"
-
-	"github.com/smallstep/certificates/authority/config"
-	"github.com/smallstep/certificates/authority/provisioner"
+	"go.step.sm/crypto/x509util"
 )
 
 type mockClient struct {
@@ -4008,3 +4007,291 @@ func Test_deviceAttest01Validate(t *testing.T) {
 		})
 	}
 }
+
+var (
+	oidTPMManufacturer = asn1.ObjectIdentifier{2, 23, 133, 2, 1}
+	oidTPMModel        = asn1.ObjectIdentifier{2, 23, 133, 2, 2}
+	oidTPMVersion      = asn1.ObjectIdentifier{2, 23, 133, 2, 3}
+)
+
+func generateValidAKCertificate(t *testing.T) *x509.Certificate {
+	t.Helper()
+	signer, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+	template := &x509.Certificate{
+		PublicKey:          signer.Public(),
+		Version:            3,
+		IsCA:               false,
+		UnknownExtKeyUsage: []asn1.ObjectIdentifier{oidTCGKpAIKCertificate},
+	}
+	asn1Value := []byte(fmt.Sprintf(`{"extraNames":[{"type": %q, "value": %q},{"type": %q, "value": %q},{"type": %q, "value": %q}]}`, oidTPMManufacturer, "1414747215", oidTPMModel, "SLB 9670 TPM2.0", oidTPMVersion, "7.55"))
+	sans := []x509util.SubjectAlternativeName{
+		{Type: x509util.DirectoryNameType,
+			ASN1Value: asn1Value},
+	}
+	ext, err := createSubjectAltNameExtension(nil, nil, nil, nil, sans, true)
+	require.NoError(t, err)
+	ext.Set(template)
+	ca, err := minica.New()
+	require.NoError(t, err)
+	cert, err := ca.Sign(template)
+	require.NoError(t, err)
+
+	return cert
+}
+
+func Test_validateAKCertificate(t *testing.T) {
+	cert := generateValidAKCertificate(t)
+	tests := []struct {
+		name   string
+		c      *x509.Certificate
+		expErr error
+	}{
+		{
+			name:   "ok",
+			c:      cert,
+			expErr: nil,
+		},
+		{
+			name: "fail/version",
+			c: &x509.Certificate{
+				Version: 1,
+			},
+			expErr: errors.New("AK certificate has invalid version 1; only version 3 is allowed"),
+		},
+		{
+			name: "fail/subject",
+			c: &x509.Certificate{
+				Version: 3,
+				Subject: pkix.Name{CommonName: "fail!"},
+			},
+			expErr: errors.New(`AK certificate subject must be empty; got "CN=fail!"`),
+		},
+		{
+			name: "fail/isCA",
+			c: &x509.Certificate{
+				Version: 3,
+				IsCA:    true,
+			},
+			expErr: errors.New("AK certificate must not be a CA"),
+		},
+		{
+			name: "fail/extendedKeyUsage",
+			c: &x509.Certificate{
+				Version: 3,
+			},
+			expErr: errors.New("AK certificate is missing Extended Key Usage extension"),
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateAKCertificate(tt.c)
+			if tt.expErr != nil {
+				if assert.Error(t, err) {
+					assert.EqualError(t, err, tt.expErr.Error())
+				}
+				return
+			}
+
+			assert.NoError(t, err)
+		})
+	}
+}
+
+func Test_validateAKCertificateSubjectAlternativeNames(t *testing.T) {
+	ok := generateValidAKCertificate(t)
+	t.Helper()
+	signer, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	getBase := func() *x509.Certificate {
+		return &x509.Certificate{
+			PublicKey:          signer.Public(),
+			Version:            3,
+			IsCA:               false,
+			UnknownExtKeyUsage: []asn1.ObjectIdentifier{oidTCGKpAIKCertificate},
+		}
+	}
+
+	ca, err := minica.New()
+	require.NoError(t, err)
+	missingManufacturerASN1 := []byte(fmt.Sprintf(`{"extraNames":[{"type": %q, "value": %q},{"type": %q, "value": %q}]}`, oidTPMModel, "SLB 9670 TPM2.0", oidTPMVersion, "7.55"))
+	sans := []x509util.SubjectAlternativeName{
+		{Type: x509util.DirectoryNameType,
+			ASN1Value: missingManufacturerASN1},
+	}
+	ext, err := createSubjectAltNameExtension(nil, nil, nil, nil, sans, true)
+	require.NoError(t, err)
+	missingManufacturer := getBase()
+	ext.Set(missingManufacturer)
+
+	missingManufacturer, err = ca.Sign(missingManufacturer)
+	require.NoError(t, err)
+
+	missingModelASN1 := []byte(fmt.Sprintf(`{"extraNames":[{"type": %q, "value": %q},{"type": %q, "value": %q}]}`, oidTPMManufacturer, "1414747215", oidTPMVersion, "7.55"))
+	sans = []x509util.SubjectAlternativeName{
+		{Type: x509util.DirectoryNameType,
+			ASN1Value: missingModelASN1},
+	}
+	ext, err = createSubjectAltNameExtension(nil, nil, nil, nil, sans, true)
+	require.NoError(t, err)
+	missingModel := getBase()
+	ext.Set(missingModel)
+
+	missingModel, err = ca.Sign(missingModel)
+	require.NoError(t, err)
+
+	missingFirmwareVersionASN1 := []byte(fmt.Sprintf(`{"extraNames":[{"type": %q, "value": %q},{"type": %q, "value": %q}]}`, oidTPMManufacturer, "1414747215", oidTPMModel, "SLB 9670 TPM2.0"))
+	sans = []x509util.SubjectAlternativeName{
+		{Type: x509util.DirectoryNameType,
+			ASN1Value: missingFirmwareVersionASN1},
+	}
+	ext, err = createSubjectAltNameExtension(nil, nil, nil, nil, sans, true)
+	require.NoError(t, err)
+	missingFirmwareVersion := getBase()
+	ext.Set(missingFirmwareVersion)
+
+	missingFirmwareVersion, err = ca.Sign(missingFirmwareVersion)
+	require.NoError(t, err)
+
+	tests := []struct {
+		name   string
+		c      *x509.Certificate
+		expErr error
+	}{
+		{"ok", ok, nil},
+		{"fail/missing-manufacturer", missingManufacturer, errors.New("missing TPM manufacturer")},
+		{"fail/missing-model", missingModel, errors.New("missing TPM model")},
+		{"fail/missing-firmware-version", missingFirmwareVersion, errors.New("missing TPM version")},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateAKCertificateSubjectAlternativeNames(tt.c)
+			if tt.expErr != nil {
+				if assert.Error(t, err) {
+					assert.EqualError(t, err, tt.expErr.Error())
+				}
+				return
+			}
+
+			assert.NoError(t, err)
+		})
+	}
+}
+
+func Test_validateAKCertificateExtendedKeyUsage(t *testing.T) {
+	ok := generateValidAKCertificate(t)
+	missingEKU := &x509.Certificate{}
+	t.Helper()
+	signer, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+	template := &x509.Certificate{
+		PublicKey:   signer.Public(),
+		ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
+	}
+	ca, err := minica.New()
+	require.NoError(t, err)
+	wrongEKU, err := ca.Sign(template)
+	require.NoError(t, err)
+	tests := []struct {
+		name   string
+		c      *x509.Certificate
+		expErr error
+	}{
+		{"ok", ok, nil},
+		{"fail/wrong-eku", wrongEKU, errors.New("AK certificate is missing Extended Key Usage value tcg-kp-AIKCertificate (2.23.133.8.3)")},
+		{"fail/missing-eku", missingEKU, errors.New("AK certificate is missing Extended Key Usage extension")},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateAKCertificateExtendedKeyUsage(tt.c)
+			if tt.expErr != nil {
+				if assert.Error(t, err) {
+					assert.EqualError(t, err, tt.expErr.Error())
+				}
+				return
+			}
+
+			assert.NoError(t, err)
+		})
+	}
+}
+
+// createSubjectAltNameExtension will construct an Extension containing all
+// SubjectAlternativeNames held in a Certificate. It implements more types than
+// the golang x509 library, so it is used whenever OtherName or RegisteredID
+// type SANs are present in the certificate.
+//
+// See also https://datatracker.ietf.org/doc/html/rfc5280.html#section-4.2.1.6
+//
+// TODO(hs): this was copied from go.step.sm/crypto/x509util to make it easier
+// to create the SAN extension for testing purposes. Should it be exposed instead?
+func createSubjectAltNameExtension(dnsNames, emailAddresses x509util.MultiString, ipAddresses x509util.MultiIP, uris x509util.MultiURL, sans []x509util.SubjectAlternativeName, subjectIsEmpty bool) (x509util.Extension, error) {
+	var zero x509util.Extension
+
+	var rawValues []asn1.RawValue
+	for _, dnsName := range dnsNames {
+		rawValue, err := x509util.SubjectAlternativeName{
+			Type: x509util.DNSType, Value: dnsName,
+		}.RawValue()
+		if err != nil {
+			return zero, err
+		}
+
+		rawValues = append(rawValues, rawValue)
+	}
+
+	for _, emailAddress := range emailAddresses {
+		rawValue, err := x509util.SubjectAlternativeName{
+			Type: x509util.EmailType, Value: emailAddress,
+		}.RawValue()
+		if err != nil {
+			return zero, err
+		}
+
+		rawValues = append(rawValues, rawValue)
+	}
+
+	for _, ip := range ipAddresses {
+		rawValue, err := x509util.SubjectAlternativeName{
+			Type: x509util.IPType, Value: ip.String(),
+		}.RawValue()
+		if err != nil {
+			return zero, err
+		}
+
+		rawValues = append(rawValues, rawValue)
+	}
+
+	for _, uri := range uris {
+		rawValue, err := x509util.SubjectAlternativeName{
+			Type: x509util.URIType, Value: uri.String(),
+		}.RawValue()
+		if err != nil {
+			return zero, err
+		}
+
+		rawValues = append(rawValues, rawValue)
+	}
+
+	for _, san := range sans {
+		rawValue, err := san.RawValue()
+		if err != nil {
+			return zero, err
+		}
+
+		rawValues = append(rawValues, rawValue)
+	}
+
+	// Now marshal the rawValues into the ASN1 sequence, and create an Extension object to hold the extension
+	rawBytes, err := asn1.Marshal(rawValues)
+	if err != nil {
+		return zero, fmt.Errorf("error marshaling SubjectAlternativeName extension to ASN1: %w", err)
+	}
+
+	return x509util.Extension{
+		ID:       x509util.ObjectIdentifier(oidSubjectAlternativeName),
+		Critical: subjectIsEmpty,
+		Value:    rawBytes,
+	}, nil
+}

acme/challenge_tpmsimulator_test.go

@@ -8,7 +8,6 @@ import (
 	"crypto"
 	"crypto/sha256"
 	"crypto/x509"
-	"crypto/x509/pkix"
 	"encoding/asn1"
 	"encoding/base64"
 	"encoding/json"
@@ -97,25 +96,22 @@ func mustAttestTPM(t *testing.T, keyAuthorization string, permanentIdentifiers [
 		IsCA:               false,
 		UnknownExtKeyUsage: []asn1.ObjectIdentifier{oidTCGKpAIKCertificate},
 	}
-	if len(permanentIdentifiers) == 0 {
-		template.URIs = []*url.URL{
-			{Scheme: "urn", Opaque: "ek:sha256:" + base64.StdEncoding.EncodeToString(keyID)},
-		}
-	} else {
-		san := x509util.SubjectAlternativeName{
+	sans := []x509util.SubjectAlternativeName{}
+	uris := []*url.URL{{Scheme: "urn", Opaque: "ek:sha256:" + base64.StdEncoding.EncodeToString(keyID)}}
+	for _, pi := range permanentIdentifiers {
+		sans = append(sans, x509util.SubjectAlternativeName{
 			Type:  x509util.PermanentIdentifierType,
-			Value: permanentIdentifiers[0], // TODO(hs): multiple?
-		}
-		ext, err := createSubjectAltNameExtension(nil, nil, nil, nil, []x509util.SubjectAlternativeName{san}, true)
-		require.NoError(t, err)
-		template.ExtraExtensions = append(template.ExtraExtensions,
-			pkix.Extension{
-				Id:       asn1.ObjectIdentifier(ext.ID),
-				Critical: ext.Critical,
-				Value:    ext.Value,
-			},
-		)
+			Value: pi,
+		})
 	}
+	asn1Value := []byte(fmt.Sprintf(`{"extraNames":[{"type": %q, "value": %q},{"type": %q, "value": %q},{"type": %q, "value": %q}]}`, oidTPMManufacturer, "1414747215", oidTPMModel, "SLB 9670 TPM2.0", oidTPMVersion, "7.55"))
+	sans = append(sans, x509util.SubjectAlternativeName{
+		Type:      x509util.DirectoryNameType,
+		ASN1Value: asn1Value,
+	})
+	ext, err := createSubjectAltNameExtension(nil, nil, nil, uris, sans, true)
+	require.NoError(t, err)
+	ext.Set(template)
 	akCert, err := aca.Sign(template)
 	require.NoError(t, err)
 	require.NotNil(t, akCert)
@@ -461,14 +457,30 @@ func Test_doTPMAttestationFormat(t *testing.T) {
 		PublicKey:          akp.Public,
 		IsCA:               false,
 		UnknownExtKeyUsage: []asn1.ObjectIdentifier{oidTCGKpAIKCertificate},
-		URIs: []*url.URL{
-			{Scheme: "urn", Opaque: "ek:sha256:" + base64.StdEncoding.EncodeToString(keyID)},
-		},
 	}
+	sans := []x509util.SubjectAlternativeName{}
+	uris := []*url.URL{{Scheme: "urn", Opaque: "ek:sha256:" + base64.StdEncoding.EncodeToString(keyID)}}
+	asn1Value := []byte(fmt.Sprintf(`{"extraNames":[{"type": %q, "value": %q},{"type": %q, "value": %q},{"type": %q, "value": %q}]}`, oidTPMManufacturer, "1414747215", oidTPMModel, "SLB 9670 TPM2.0", oidTPMVersion, "7.55"))
+	sans = append(sans, x509util.SubjectAlternativeName{
+		Type:      x509util.DirectoryNameType,
+		ASN1Value: asn1Value,
+	})
+	ext, err := createSubjectAltNameExtension(nil, nil, nil, uris, sans, true)
+	require.NoError(t, err)
+	ext.Set(template)
 	akCert, err := aca.Sign(template)
 	require.NoError(t, err)
 	require.NotNil(t, akCert)
 
+	invalidTemplate := &x509.Certificate{
+		PublicKey:          akp.Public,
+		IsCA:               false,
+		UnknownExtKeyUsage: []asn1.ObjectIdentifier{oidTCGKpAIKCertificate},
+	}
+	invalidAKCert, err := aca.Sign(invalidTemplate)
+	require.NoError(t, err)
+	require.NotNil(t, invalidAKCert)
+
 	// generate a JWK and the key authorization value
 	jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0)
 	require.NoError(t, err)
@@ -655,6 +667,17 @@ func Test_doTPMAttestationFormat(t *testing.T) {
 				"pubArea":  params.Public,
 			},
 		}}, nil, newBadAttestationStatementError("x5c is not valid: x509: certificate signed by unknown authority")},
+		{"fail validateAKCertificate", args{ctx, mustAttestationProvisioner(t, acaRoot), &Challenge{Token: "token"}, jwk, &attestationObject{
+			Format: "tpm",
+			AttStatement: map[string]interface{}{
+				"ver":      "2.0",
+				"x5c":      []interface{}{invalidAKCert.Raw, aca.Intermediate.Raw},
+				"alg":      int64(-257), // RS256
+				"sig":      params.CreateSignature,
+				"certInfo": params.CreateAttestation,
+				"pubArea":  params.Public,
+			},
+		}}, nil, newBadAttestationStatementError("AK certificate is not valid: missing TPM manufacturer")},
 		{"fail pubArea not present", args{ctx, mustAttestationProvisioner(t, acaRoot), &Challenge{Token: "token"}, jwk, &attestationObject{
 			Format: "tpm",
 			AttStatement: map[string]interface{}{
@@ -834,82 +857,3 @@ func Test_doTPMAttestationFormat(t *testing.T) {
 		})
 	}
 }
-
-// createSubjectAltNameExtension will construct an Extension containing all
-// SubjectAlternativeNames held in a Certificate. It implements more types than
-// the golang x509 library, so it is used whenever OtherName or RegisteredID
-// type SANs are present in the certificate.
-//
-// See also https://datatracker.ietf.org/doc/html/rfc5280.html#section-4.2.1.6
-//
-// TODO(hs): this was copied from go.step.sm/crypto/x509util to make it easier
-// to create the SAN extension for testing purposes. Should it be exposed instead?
-func createSubjectAltNameExtension(dnsNames, emailAddresses x509util.MultiString, ipAddresses x509util.MultiIP, uris x509util.MultiURL, sans []x509util.SubjectAlternativeName, subjectIsEmpty bool) (x509util.Extension, error) {
-	var zero x509util.Extension
-
-	var rawValues []asn1.RawValue
-	for _, dnsName := range dnsNames {
-		rawValue, err := x509util.SubjectAlternativeName{
-			Type: x509util.DNSType, Value: dnsName,
-		}.RawValue()
-		if err != nil {
-			return zero, err
-		}
-
-		rawValues = append(rawValues, rawValue)
-	}
-
-	for _, emailAddress := range emailAddresses {
-		rawValue, err := x509util.SubjectAlternativeName{
-			Type: x509util.EmailType, Value: emailAddress,
-		}.RawValue()
-		if err != nil {
-			return zero, err
-		}
-
-		rawValues = append(rawValues, rawValue)
-	}
-
-	for _, ip := range ipAddresses {
-		rawValue, err := x509util.SubjectAlternativeName{
-			Type: x509util.IPType, Value: ip.String(),
-		}.RawValue()
-		if err != nil {
-			return zero, err
-		}
-
-		rawValues = append(rawValues, rawValue)
-	}
-
-	for _, uri := range uris {
-		rawValue, err := x509util.SubjectAlternativeName{
-			Type: x509util.URIType, Value: uri.String(),
-		}.RawValue()
-		if err != nil {
-			return zero, err
-		}
-
-		rawValues = append(rawValues, rawValue)
-	}
-
-	for _, san := range sans {
-		rawValue, err := san.RawValue()
-		if err != nil {
-			return zero, err
-		}
-
-		rawValues = append(rawValues, rawValue)
-	}
-
-	// Now marshal the rawValues into the ASN1 sequence, and create an Extension object to hold the extension
-	rawBytes, err := asn1.Marshal(rawValues)
-	if err != nil {
-		return zero, fmt.Errorf("error marshaling SubjectAlternativeName extension to ASN1: %w", err)
-	}
-
-	return x509util.Extension{
-		ID:       x509util.ObjectIdentifier(oidSubjectAlternativeName),
-		Critical: subjectIsEmpty,
-		Value:    rawBytes,
-	}, nil
-}