From 246cf78dd392644247278d140dd93c047a8c9d63 Mon Sep 17 00:00:00 2001 From: Matthew Miller Date: Thu, 5 Dec 2024 00:14:09 -0800 Subject: [PATCH 1/8] Refine certificate chain verification --- .../src/helpers/validateCertificatePath.ts | 170 ++++++++++-------- 1 file changed, 99 insertions(+), 71 deletions(-) diff --git a/packages/server/src/helpers/validateCertificatePath.ts b/packages/server/src/helpers/validateCertificatePath.ts index db93627a..793d9d23 100644 --- a/packages/server/src/helpers/validateCertificatePath.ts +++ b/packages/server/src/helpers/validateCertificatePath.ts @@ -1,34 +1,34 @@ import { AsnSerializer } from '@peculiar/asn1-schema'; +import type { Certificate } from '@peculiar/asn1-x509'; import { isCertRevoked } from './isCertRevoked.ts'; import { verifySignature } from './verifySignature.ts'; import { mapX509SignatureAlgToCOSEAlg } from './mapX509SignatureAlgToCOSEAlg.ts'; -import { getCertificateInfo } from './getCertificateInfo.ts'; +import { type CertificateInfo, getCertificateInfo } from './getCertificateInfo.ts'; import { convertPEMToBytes } from './convertPEMToBytes.ts'; /** * Traverse an array of PEM certificates and ensure they form a proper chain - * @param certificates Typically the result of `x5c.map(convertASN1toPEM)` - * @param rootCertificates Possible root certificates to complete the path + * @param x5cPEMCerts Typically the result of `x5c.map(convertASN1toPEM)` + * @param trustAnchors Certificates that an attestation statement x5c may chain back to */ export async function validateCertificatePath( - certificates: string[], - rootCertificates: string[] = [], + x5cPEMCerts: string[], + trustAnchors: string[] = [], ): Promise { - if (rootCertificates.length === 0) { - // We have no root certs with which to create a full path, so skip path validation - // TODO: Is this going to be acceptable default behavior?? + if (trustAnchors.length === 0) { + // We have no trust anchors to chain back to, so skip path validation return true; } let invalidSubjectAndIssuerError = false; let certificateNotYetValidOrExpiredErrorMessage = undefined; - for (const rootCert of rootCertificates) { + for (const anchor of trustAnchors) { try { - const certsWithRoot = certificates.concat([rootCert]); - await _validatePath(certsWithRoot); + const certsWithTrustAnchor = x5cPEMCerts.concat([anchor]); + await _validatePath(certsWithTrustAnchor); // If we successfully validated a path then there's no need to continue. Reset any existing - // errors that were thrown by earlier root certificates + // errors that were thrown by earlier trust anchors invalidSubjectAndIssuerError = false; certificateNotYetValidOrExpiredErrorMessage = undefined; break; @@ -43,7 +43,7 @@ export async function validateCertificatePath( } } - // We tried multiple root certs and none of them worked + // We tried multiple trust anchors and none of them worked if (invalidSubjectAndIssuerError) { throw new InvalidSubjectAndIssuer(); } else if (certificateNotYetValidOrExpiredErrorMessage) { @@ -55,84 +55,104 @@ export async function validateCertificatePath( return true; } -async function _validatePath(certificates: string[]): Promise { - if (new Set(certificates).size !== certificates.length) { +/** + * @param x5cCerts X.509 `x5c` certs in PEM string format + * @param anchorCert X.509 trust anchor cert in PEM string format + */ +async function _validatePath(x5cCertsWithTrustAnchor: string[]): Promise { + if (new Set(x5cCertsWithTrustAnchor).size !== x5cCertsWithTrustAnchor.length) { throw new Error('Invalid certificate path: found duplicate certificates'); } - // From leaf to root, make sure each cert is issued by the next certificate in the chain - for (let i = 0; i < certificates.length; i += 1) { - const subjectPem = certificates[i]; - - const isLeafCert = i === 0; - const isRootCert = i + 1 >= certificates.length; + // Make sure no certs are revoked, and all are within their time validity window + for (const certificate of x5cCertsWithTrustAnchor) { + const certInfo = getCertificateInfo(convertPEMToBytes(certificate)); + await assertCertNotRevoked(certInfo.parsedCertificate); + assertCertIsWithinValidTimeWindow(certInfo, certificate); + } - let issuerPem = ''; - if (isRootCert) { - issuerPem = subjectPem; - } else { - issuerPem = certificates[i + 1]; - } + // Make sure each x5c cert is issued by the next certificate in the chain + for (let i = 0; i < (x5cCertsWithTrustAnchor.length - 1); i += 1) { + const subjectPem = x5cCertsWithTrustAnchor[i]; + const issuerPem = x5cCertsWithTrustAnchor[i + 1]; const subjectInfo = getCertificateInfo(convertPEMToBytes(subjectPem)); const issuerInfo = getCertificateInfo(convertPEMToBytes(issuerPem)); - const x509Subject = subjectInfo.parsedCertificate; - - // Check for certificate revocation - const subjectCertRevoked = await isCertRevoked(x509Subject); - - if (subjectCertRevoked) { - throw new Error(`Found revoked certificate in certificate path`); - } - - // Check that intermediate certificate is within its valid time window - const { notBefore, notAfter } = issuerInfo; - - const now = new Date(Date.now()); - if (notBefore > now || notAfter < now) { - if (isLeafCert) { - throw new CertificateNotYetValidOrExpired( - `Leaf certificate is not yet valid or expired: ${issuerPem}`, - ); - } else if (isRootCert) { - throw new CertificateNotYetValidOrExpired( - `Root certificate is not yet valid or expired: ${issuerPem}`, - ); - } else { - throw new CertificateNotYetValidOrExpired( - `Intermediate certificate is not yet valid or expired: ${issuerPem}`, - ); - } - } - + // Make sure subject issuer is issuer subject if (subjectInfo.issuer.combined !== issuerInfo.subject.combined) { throw new InvalidSubjectAndIssuer(); } - // Verify the subject certificate's signature with the issuer cert's public key - const data = AsnSerializer.serialize(x509Subject.tbsCertificate); - const signature = x509Subject.signatureValue; - const signatureAlgorithm = mapX509SignatureAlgToCOSEAlg( - x509Subject.signatureAlgorithm.algorithm, - ); - const issuerCertBytes = convertPEMToBytes(issuerPem); + const issuerCertIsRootCert = issuerInfo.issuer.combined === issuerInfo.subject.combined; - const verified = await verifySignature({ - data: new Uint8Array(data), - signature: new Uint8Array(signature), - x509Certificate: issuerCertBytes, - hashAlgorithm: signatureAlgorithm, - }); + await assertSubjectIsSignedByIssuer(subjectInfo.parsedCertificate, issuerPem); - if (!verified) { - throw new Error('Invalid certificate path: invalid signature'); + // Perform one final check if the issuer cert is also a root certificate + if (issuerCertIsRootCert) { + await assertSubjectIsSignedByIssuer(issuerInfo.parsedCertificate, issuerPem); } } return true; } +/** + * Check if the certificate is revoked or not. If it is, raise an error + */ +async function assertCertNotRevoked(certificate: Certificate): Promise { + // Check for certificate revocation + const subjectCertRevoked = await isCertRevoked(certificate); + + if (subjectCertRevoked) { + throw new Error(`Found revoked certificate in certificate path`); + } +} + +/** + * Require the cert to be within its notBefore and notAfter time window + * + * @param certInfo Parsed cert information + * @param certPEM PEM-formatted certificate, for error reporting + */ +function assertCertIsWithinValidTimeWindow(certInfo: CertificateInfo, certPEM: string): void { + const { notBefore, notAfter } = certInfo; + + const now = new Date(Date.now()); + if (notBefore > now || notAfter < now) { + throw new CertificateNotYetValidOrExpired( + `Certificate is not yet valid or expired: ${certPEM}`, + ); + } +} + +/** + * Ensure that the subject cert has been signed by the next cert in the chain + */ +async function assertSubjectIsSignedByIssuer( + subjectCert: Certificate, + issuerPEM: string, +): Promise { + // Verify the subject certificate's signature with the issuer cert's public key + const data = AsnSerializer.serialize(subjectCert.tbsCertificate); + const signature = subjectCert.signatureValue; + const signatureAlgorithm = mapX509SignatureAlgToCOSEAlg( + subjectCert.signatureAlgorithm.algorithm, + ); + const issuerCertBytes = convertPEMToBytes(issuerPEM); + + const verified = await verifySignature({ + data: new Uint8Array(data), + signature: new Uint8Array(signature), + x509Certificate: issuerCertBytes, + hashAlgorithm: signatureAlgorithm, + }); + + if (!verified) { + throw new InvalidSubjectSignatureForIssuer(); + } +} + // Custom errors to help pass on certain errors class InvalidSubjectAndIssuer extends Error { constructor() { @@ -142,6 +162,14 @@ class InvalidSubjectAndIssuer extends Error { } } +class InvalidSubjectSignatureForIssuer extends Error { + constructor() { + const message = 'Subject signature was invalid for issuer'; + super(message); + this.name = 'InvalidSubjectSignatureForIssuer'; + } +} + class CertificateNotYetValidOrExpired extends Error { constructor(message: string) { super(message); From d4cb62c955e8a7e915586fca8acabb555dd1df0d Mon Sep 17 00:00:00 2001 From: Matthew Miller Date: Thu, 5 Dec 2024 00:14:26 -0800 Subject: [PATCH 2/8] Add test case for HID attestation --- .../verifyAttestationWithMetadata.test.ts | 62 ++++++++++++++++++- 1 file changed, 61 insertions(+), 1 deletion(-) diff --git a/packages/server/src/metadata/verifyAttestationWithMetadata.test.ts b/packages/server/src/metadata/verifyAttestationWithMetadata.test.ts index a12a1270..9cbf441b 100644 --- a/packages/server/src/metadata/verifyAttestationWithMetadata.test.ts +++ b/packages/server/src/metadata/verifyAttestationWithMetadata.test.ts @@ -1,10 +1,15 @@ import { assertEquals } from '@std/assert'; +import { FakeTime } from '@std/testing/time'; import { verifyAttestationWithMetadata } from './verifyAttestationWithMetadata.ts'; -import { MetadataStatement } from '../metadata/mdsTypes.ts'; +import type { MetadataStatement } from '../metadata/mdsTypes.ts'; import { isoBase64URL } from '../helpers/iso/index.ts'; Deno.test('should verify attestation with metadata (android-safetynet)', async () => { + // notBefore: 2022-01-25T10:00:34.000Z, + // notAfter: 2022-04-25T10:00:33.000Z + const fakedNow = new FakeTime(new Date('2022-02-01T00:00:00.000Z')); + const metadataStatementJSONSafetyNet: MetadataStatement = { legalHeader: 'https://fidoalliance.org/metadata/metadata-statement-legal-header/', aaguid: 'b93fd961-f2e6-462f-b122-82002247de78', @@ -56,6 +61,8 @@ Deno.test('should verify attestation with metadata (android-safetynet)', async ( }); assertEquals(verified, true); + + fakedNow.restore(); }); Deno.test('should verify attestation with rsa_emsa_pkcs1_sha256_raw authenticator algorithm in metadata', async () => { @@ -242,3 +249,56 @@ Deno.test('should verify idmelon attestation with updated root certificate', asy assertEquals(verified, true); }); + +Deno.test('should verify when trust anchor is an intermediate certificate', async () => { + /** + * See https://github.com/MasterKale/SimpleWebAuthn/issues/647 for more context, basically + * HID sells authenticators that are "unique" for their use of intermediate certificates as trust + * anchors. This has been a problem for many other WebAuthn libraries, and now it's my turn to + * deal with an errant assumption from early in this project's life. + * + * The metadata statement below is a slimmed down version of it as pulled from MDS on Dec 2024 + */ + const metadataStatement: MetadataStatement = { + legalHeader: + 'Submission of this statement and retrieval and use of this statement indicates acceptance of the appropriate agreement located at https://fidoalliance.org/metadata/metadata-legal-terms/.', + aaguid: '692db549-7ae5-44d5-a1e5-dd20a493b723', + description: 'HID Crescendo Key', + authenticatorVersion: 10, + protocolFamily: 'fido2', + schema: 3, + upv: [{ major: 1, minor: 0 }], + authenticationAlgorithms: ['secp256r1_ecdsa_sha256_raw'], + publicKeyAlgAndEncodings: ['cose'], + attestationTypes: ['basic_full'], + userVerificationDetails: [ + [{ userVerificationMethod: 'passcode_external' }], + [{ userVerificationMethod: 'none' }], + [{ userVerificationMethod: 'presence_internal' }], + [ + { userVerificationMethod: 'passcode_external' }, + { userVerificationMethod: 'presence_internal' }, + ], + ], + keyProtection: ['hardware', 'secure_element'], + matcherProtection: ['on_chip'], + tcDisplay: [], + attestationRootCertificates: [ + 'MIIDCDCCAq+gAwIBAgIQQAFqUNTHZ8kBN8u/bCk+xDAKBggqhkjOPQQDAjBrMQswCQYDVQQGEwJVUzETMBEGA1UEChMKSElEIEdsb2JhbDEiMCAGA1UECxMZQXV0aGVudGljYXRvciBBdHRlc3RhdGlvbjEjMCEGA1UEAxMaRklETyBBdHRlc3RhdGlvbiBSb290IENBIDEwHhcNMTkwNDI0MTkzMTIzWhcNNDQwNDI3MTkzMTIzWjBmMQswCQYDVQQGEwJVUzETMBEGA1UEChMKSElEIEdsb2JhbDEiMCAGA1UECxMZQXV0aGVudGljYXRvciBBdHRlc3RhdGlvbjEeMBwGA1UEAxMVRklETyBBdHRlc3RhdGlvbiBDQSAyMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAE4nK9ctzk6GEGFNQBcrnBBmWU+dCnuHQAARrB2Eyc8MbsljkSFhZtfz/Rw6SuVIDk5VakDzrKBAOJ9v0Rvg/406OCATgwggE0MBIGA1UdEwEB/wQIMAYBAf8CAQAwDgYDVR0PAQH/BAQDAgGGMIGEBggrBgEFBQcBAQR4MHYwLgYIKwYBBQUHMAGGImh0dHA6Ly9oaWQuZmlkby5vY3NwLmlkZW50cnVzdC5jb20wRAYIKwYBBQUHMAKGOGh0dHA6Ly92YWxpZGF0aW9uLmlkZW50cnVzdC5jb20vcm9vdHMvSElERklET1Jvb3RjYTEucDdjMB8GA1UdIwQYMBaAFB2m3iwWSYHvWTHbJiHAyKDp+CSjMEcGA1UdHwRAMD4wPKA6oDiGNmh0dHA6Ly92YWxpZGF0aW9uLmlkZW50cnVzdC5jb20vY3JsL0hJREZJRE9Sb290Y2ExLmNybDAdBgNVHQ4EFgQUDLCbuLslcclrOZIz57Fu0imSMQ8wCgYIKoZIzj0EAwIDRwAwRAIgDCW5IrbjEI/y35lPjx9a+/sF4lPSoZdBHgFgTWC+8VICIEqs2SPzUHgHVh65Ajl1oIUmhh0C2lyR/Zdk7O3u1TIK', + ], + }; + + const x5c = [ + 'MIIDLjCCAtSgAwIBAgIQQAFs2JXwQcL5Eh4rnp2ASjAKBggqhkjOPQQDAjBmMQswCQYDVQQGEwJVUzETMBEGA1UEChMKSElEIEdsb2JhbDEiMCAGA1UECxMZQXV0aGVudGljYXRvciBBdHRlc3RhdGlvbjEeMBwGA1UEAxMVRklETyBBdHRlc3RhdGlvbiBDQSAyMB4XDTE5MDgyODE0MTY0MFoXDTM5MDgyMzE0MTY0MFowaTELMAkGA1UEBhMCVVMxHzAdBgNVBAoTFkhJRCBHbG9iYWwgQ29ycG9yYXRpb24xIjAgBgNVBAsTGUF1dGhlbnRpY2F0b3IgQXR0ZXN0YXRpb24xFTATBgNVBAMTDENyZXNjZW5kb0tleTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABAGouI654w6qbGonSTStO2cESYTo8Ezr8OJiPkMl02d6K6i44wXCKV2i+w+bpR6vgYQZ/cKQxMS4uGytqPRNPIejggFfMIIBWzAOBgNVHQ8BAf8EBAMCB4AwgYAGCCsGAQUFBwEBBHQwcjAuBggrBgEFBQcwAYYiaHR0cDovL2hpZC5maWRvLm9jc3AuaWRlbnRydXN0LmNvbTBABggrBgEFBQcwAoY0aHR0cDovL3ZhbGlkYXRpb24uaWRlbnRydXN0LmNvbS9jZXJ0cy9oaWRmaWRvY2EyLnA3YzAfBgNVHSMEGDAWgBQMsJu4uyVxyWs5kjPnsW7SKZIxDzAJBgNVHRMEAjAAMEMGA1UdHwQ8MDowOKA2oDSGMmh0dHA6Ly92YWxpZGF0aW9uLmlkZW50cnVzdC5jb20vY3JsL2hpZGZpZG9jYTIuY3JsMBMGCysGAQQBguUcAgEBBAQDAgQwMB0GA1UdDgQWBBR9h/lCWeTiMUhRS1tj31hBXaOurzAhBgsrBgEEAYLlHAEBBAQSBBBpLbVJeuVE1aHl3SCkk7cjMAoGCCqGSM49BAMCA0gAMEUCIQDpDa1ZbAfCTlBMiDUuB5XH8hnhZUF1JCuCmc+ShI4ZTwIga/ApAudL5R8HxOOHgk8AA/JpgCkMmYDQLVq0QF6oxrU=', + ]; + const credentialPublicKey = + 'pQECAyYgASFYIL80Thvv0K7ftQDCmrwLFELMZxm7s1I1VmPRMYMMkleHIlggtk7VFv4ABu7en1D7I74t66bn2ghLS9-qdcWPDujOvJQ'; + + const verified = await verifyAttestationWithMetadata({ + statement: metadataStatement, + credentialPublicKey: isoBase64URL.toBuffer(credentialPublicKey), + x5c, + }); + + assertEquals(verified, true); +}); From af2ffde63b8dd1af63377bf6623021fcad8b1783 Mon Sep 17 00:00:00 2001 From: Matthew Miller Date: Thu, 5 Dec 2024 00:14:48 -0800 Subject: [PATCH 3/8] Fix time mocks in android-safetynet tests --- .../verifyAttestationAndroidSafetyNet.test.ts | 38 ++++++++++++++++--- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/packages/server/src/registration/verifications/verifyAttestationAndroidSafetyNet.test.ts b/packages/server/src/registration/verifications/verifyAttestationAndroidSafetyNet.test.ts index 29992efe..98756bfb 100644 --- a/packages/server/src/registration/verifications/verifyAttestationAndroidSafetyNet.test.ts +++ b/packages/server/src/registration/verifications/verifyAttestationAndroidSafetyNet.test.ts @@ -61,9 +61,20 @@ Deno.test('should verify Android SafetyNet attestation', async () => { rpIdHash, } = await getResponseValues(attestationAndroidSafetyNet); - // notBefore: 2017-06-15T00:00:42.000Z - // notAfter: 2021-12-15T00:00:42.000Z - const mockDate = new FakeTime(new Date('2021-11-15T00:00:42.000Z')); + // Faking time to something that'll satisfy all of these ranges: + // { + // notBefore: 2018-10-10T07:19:45.000Z, + // notAfter: 2019-10-09T07:19:45.000Z + // } + // { + // notBefore: 2017-06-15T00:00:42.000Z, + // notAfter: 2021-12-15T00:00:42.000Z + // } + // { + // notBefore: 1998-09-01T12:00:00.000Z, + // notAfter: 2028-01-28T12:00:00.000Z + // } + const mockDate = new FakeTime(new Date('2019-10-01T00:00:42.000Z')); const verified = await verifyAttestationAndroidSafetyNet({ attStmt, @@ -121,9 +132,24 @@ Deno.test('should validate response with cert path completed with GlobalSign R1 rpIdHash, } = await getResponseValues(safetyNetUsingGSR1RootCert); - // notBefore: 2006-12-15T08:00:00.000Z - // notAfter: 2021-12-15T08:00:00.000Z - const mockDate = new FakeTime(new Date('2021-11-15T00:00:42.000Z')); + // Faking time to something that'll satisfy all of these ranges: + // { + // notBefore: 2021-07-19T13:13:42.000Z, + // notAfter: 2021-10-17T13:13:41.000Z + // } + // { + // notBefore: 2020-08-13T00:00:42.000Z, + // notAfter: 2027-09-30T00:00:42.000Z + // } + // { + // notBefore: 2020-06-19T00:00:42.000Z, + // notAfter: 2028-01-28T00:00:42.000Z + // } + // { + // notBefore: 1998-09-01T12:00:00.000Z, + // notAfter: 2028-01-28T12:00:00.000Z + // } + const mockDate = new FakeTime(new Date('2021-10-15T00:00:42.000Z')); const verified = await verifyAttestationAndroidSafetyNet({ attStmt, From 3fad1158b25e564ae1db49d45aa8370723c108e9 Mon Sep 17 00:00:00 2001 From: Matthew Miller Date: Thu, 5 Dec 2024 00:15:51 -0800 Subject: [PATCH 4/8] Bump Deno version in CI --- .github/workflows/ciChecks.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/ciChecks.yml b/.github/workflows/ciChecks.yml index df49c06d..ea4b8e1a 100644 --- a/.github/workflows/ciChecks.yml +++ b/.github/workflows/ciChecks.yml @@ -16,7 +16,7 @@ jobs: strategy: matrix: node-version: [20, 22] - deno-version: ['v2.0.x'] + deno-version: ['v2.1.x'] steps: - uses: actions/checkout@v4 From 5a12b5b74587f25ee23e6abc640c287de8ae1345 Mon Sep 17 00:00:00 2001 From: Matthew Miller Date: Thu, 5 Dec 2024 00:23:16 -0800 Subject: [PATCH 5/8] Refine validateCertificatePath docstring --- packages/server/src/helpers/validateCertificatePath.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/server/src/helpers/validateCertificatePath.ts b/packages/server/src/helpers/validateCertificatePath.ts index 793d9d23..50543881 100644 --- a/packages/server/src/helpers/validateCertificatePath.ts +++ b/packages/server/src/helpers/validateCertificatePath.ts @@ -10,7 +10,7 @@ import { convertPEMToBytes } from './convertPEMToBytes.ts'; /** * Traverse an array of PEM certificates and ensure they form a proper chain * @param x5cPEMCerts Typically the result of `x5c.map(convertASN1toPEM)` - * @param trustAnchors Certificates that an attestation statement x5c may chain back to + * @param trustAnchors PEM-formatted certs that an attestation statement x5c may chain back to */ export async function validateCertificatePath( x5cPEMCerts: string[], From e0a9113c9f6334f009c955437e36c3006ef97433 Mon Sep 17 00:00:00 2001 From: Matthew Miller Date: Thu, 5 Dec 2024 00:28:39 -0800 Subject: [PATCH 6/8] Refine test comment for clarity on time mocking --- .../verifyAttestationWithMetadata.test.ts | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/server/src/metadata/verifyAttestationWithMetadata.test.ts b/packages/server/src/metadata/verifyAttestationWithMetadata.test.ts index 9cbf441b..6f7c17b6 100644 --- a/packages/server/src/metadata/verifyAttestationWithMetadata.test.ts +++ b/packages/server/src/metadata/verifyAttestationWithMetadata.test.ts @@ -6,8 +6,23 @@ import type { MetadataStatement } from '../metadata/mdsTypes.ts'; import { isoBase64URL } from '../helpers/iso/index.ts'; Deno.test('should verify attestation with metadata (android-safetynet)', async () => { - // notBefore: 2022-01-25T10:00:34.000Z, - // notAfter: 2022-04-25T10:00:33.000Z + // Faking time to something that'll satisfy all of these ranges: + // { + // notBefore: 2022-01-25T10:00:34.000Z, + // notAfter: 2022-04-25T10:00:33.000Z + // } + // { + // notBefore: 2020-08-13T00:00:42.000Z, + // notAfter: 2027-09-30T00:00:42.000Z + // } + // { + // notBefore: 2020-06-19T00:00:42.000Z, + // notAfter: 2028-01-28T00:00:42.000Z + // } + // { + // notBefore: 1998-09-01T12:00:00.000Z, + // notAfter: 2028-01-28T12:00:00.000Z + // } const fakedNow = new FakeTime(new Date('2022-02-01T00:00:00.000Z')); const metadataStatementJSONSafetyNet: MetadataStatement = { From dca51b313094d37b955c3158a7acbcd65b5bad30 Mon Sep 17 00:00:00 2001 From: Matthew Miller Date: Thu, 5 Dec 2024 00:39:29 -0800 Subject: [PATCH 7/8] Try to fix test flakiness --- .../verifyRegistrationResponse.test.ts | 49 ++++++++++++------- 1 file changed, 32 insertions(+), 17 deletions(-) diff --git a/packages/server/src/registration/verifyRegistrationResponse.test.ts b/packages/server/src/registration/verifyRegistrationResponse.test.ts index a6f7cdcc..bcc17638 100644 --- a/packages/server/src/registration/verifyRegistrationResponse.test.ts +++ b/packages/server/src/registration/verifyRegistrationResponse.test.ts @@ -503,26 +503,41 @@ Deno.test('should throw error if unsupported alg is used', async () => { mockDecodePubKey.restore(); }); -Deno.test('should not include authenticator info if not verified', async () => { - const mockVerifySignature = stub( - _verifySignatureInternals, - 'stubThis', - returnsNext([new Promise((resolve) => resolve(false))]), - ); +Deno.test( + 'should not include authenticator info if not verified', + { + /** + * CI likes to intermittently fail this test with the following: + * + * > An async operation to verify data was started in this test, but never completed. This is + * > often caused by not awaiting the result of a `crypto.subtle.verify` call + * + * I suspect it's something to do with how `_verifySignatureInternals.stubThis` is being mocked. + * This will disable this warning on an otherwise passing test. + */ + sanitizeOps: false, + }, + async () => { + const mockVerifySignature = stub( + _verifySignatureInternals, + 'stubThis', + returnsNext([new Promise((resolve) => resolve(false))]), + ); - const verification = await verifyRegistrationResponse({ - response: attestationFIDOU2F, - expectedChallenge: attestationFIDOU2FChallenge, - expectedOrigin: 'https://dev.dontneeda.pw', - expectedRPID: 'dev.dontneeda.pw', - requireUserVerification: false, - }); + const verification = await verifyRegistrationResponse({ + response: attestationFIDOU2F, + expectedChallenge: attestationFIDOU2FChallenge, + expectedOrigin: 'https://dev.dontneeda.pw', + expectedRPID: 'dev.dontneeda.pw', + requireUserVerification: false, + }); - assertFalse(verification.verified); - assertEquals(verification.registrationInfo, undefined); + assertFalse(verification.verified); + assertEquals(verification.registrationInfo, undefined); - mockVerifySignature.restore(); -}); + mockVerifySignature.restore(); + }, +); Deno.test('should throw an error if user verification is required but user was not verified', async () => { const mockParseAuthData = stub( From 7b27b8018652b84851d532d751bcdcce028ddc34 Mon Sep 17 00:00:00 2001 From: Matthew Miller Date: Thu, 5 Dec 2024 08:38:54 -0800 Subject: [PATCH 8/8] Try to be clearer about what's PEM-formatted --- .../src/helpers/validateCertificatePath.ts | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/packages/server/src/helpers/validateCertificatePath.ts b/packages/server/src/helpers/validateCertificatePath.ts index 50543881..f84fb400 100644 --- a/packages/server/src/helpers/validateCertificatePath.ts +++ b/packages/server/src/helpers/validateCertificatePath.ts @@ -9,23 +9,23 @@ import { convertPEMToBytes } from './convertPEMToBytes.ts'; /** * Traverse an array of PEM certificates and ensure they form a proper chain - * @param x5cPEMCerts Typically the result of `x5c.map(convertASN1toPEM)` - * @param trustAnchors PEM-formatted certs that an attestation statement x5c may chain back to + * @param x5cCertsPEM Typically the result of `x5c.map(convertASN1toPEM)` + * @param trustAnchorsPEM PEM-formatted certs that an attestation statement x5c may chain back to */ export async function validateCertificatePath( - x5cPEMCerts: string[], - trustAnchors: string[] = [], + x5cCertsPEM: string[], + trustAnchorsPEM: string[] = [], ): Promise { - if (trustAnchors.length === 0) { + if (trustAnchorsPEM.length === 0) { // We have no trust anchors to chain back to, so skip path validation return true; } let invalidSubjectAndIssuerError = false; let certificateNotYetValidOrExpiredErrorMessage = undefined; - for (const anchor of trustAnchors) { + for (const anchorPEM of trustAnchorsPEM) { try { - const certsWithTrustAnchor = x5cPEMCerts.concat([anchor]); + const certsWithTrustAnchor = x5cCertsPEM.concat([anchorPEM]); await _validatePath(certsWithTrustAnchor); // If we successfully validated a path then there's no need to continue. Reset any existing // errors that were thrown by earlier trust anchors @@ -59,22 +59,22 @@ export async function validateCertificatePath( * @param x5cCerts X.509 `x5c` certs in PEM string format * @param anchorCert X.509 trust anchor cert in PEM string format */ -async function _validatePath(x5cCertsWithTrustAnchor: string[]): Promise { - if (new Set(x5cCertsWithTrustAnchor).size !== x5cCertsWithTrustAnchor.length) { +async function _validatePath(x5cCertsWithTrustAnchorPEM: string[]): Promise { + if (new Set(x5cCertsWithTrustAnchorPEM).size !== x5cCertsWithTrustAnchorPEM.length) { throw new Error('Invalid certificate path: found duplicate certificates'); } // Make sure no certs are revoked, and all are within their time validity window - for (const certificate of x5cCertsWithTrustAnchor) { - const certInfo = getCertificateInfo(convertPEMToBytes(certificate)); + for (const certificatePEM of x5cCertsWithTrustAnchorPEM) { + const certInfo = getCertificateInfo(convertPEMToBytes(certificatePEM)); await assertCertNotRevoked(certInfo.parsedCertificate); - assertCertIsWithinValidTimeWindow(certInfo, certificate); + assertCertIsWithinValidTimeWindow(certInfo, certificatePEM); } // Make sure each x5c cert is issued by the next certificate in the chain - for (let i = 0; i < (x5cCertsWithTrustAnchor.length - 1); i += 1) { - const subjectPem = x5cCertsWithTrustAnchor[i]; - const issuerPem = x5cCertsWithTrustAnchor[i + 1]; + for (let i = 0; i < (x5cCertsWithTrustAnchorPEM.length - 1); i += 1) { + const subjectPem = x5cCertsWithTrustAnchorPEM[i]; + const issuerPem = x5cCertsWithTrustAnchorPEM[i + 1]; const subjectInfo = getCertificateInfo(convertPEMToBytes(subjectPem)); const issuerInfo = getCertificateInfo(convertPEMToBytes(issuerPem));