From d9aa2c110fdba84e03082d28f512bf07013e957b Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 6 Apr 2023 14:35:48 +0200 Subject: [PATCH] Increase test coverage for AK certificate properties --- acme/challenge.go | 37 ++-- acme/challenge_test.go | 295 +++++++++++++++++++++++++++- acme/challenge_tpmsimulator_test.go | 144 +++++--------- 3 files changed, 353 insertions(+), 123 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index 85456f7d..a1d4067f 100644 --- a/acme/challenge.go +++ b/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) diff --git a/acme/challenge_test.go b/acme/challenge_test.go index ccd8f6b8..ff93bea3 100644 --- a/acme/challenge_test.go +++ b/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 +} diff --git a/acme/challenge_tpmsimulator_test.go b/acme/challenge_tpmsimulator_test.go index 086b1a33..dbd63226 100644 --- a/acme/challenge_tpmsimulator_test.go +++ b/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 -}