fix: verify certificate chains with the declared signature algorithm (fixes App Attest on Deno) - #64
Open
spendres wants to merge 1 commit into
Open
Conversation
`X509Certificate.prototype.verify()` is unreliable on non-Node runtimes for cross-curve ECDSA chains. Apple's App Attest chain always has a P-256 leaf signed ecdsa-with-SHA256 by a P-384 sub-CA, and Deno's node:crypto compatibility layer returns false for exactly that link, where Node returns true for the same bytes. It appears to derive the digest from the issuer key's curve (P-384 -> SHA-384) rather than reading the certificate's declared signatureAlgorithm (ecdsa-with-SHA256). Because every App Attest chain contains that link, verifyAttestation rejected 100% of genuine attestations on Deno, failing with "client CA certificate is not signed by Apple App Attestation CA 1" — which reads like a forged client certificate rather than a runtime defect. Filed upstream as denoland/deno#36309 Replace both chain checks with an isSignedBy() helper that takes the digest explicitly from the certificate's own signatureAlgorithm OID and verifies via crypto.verify(). This is correct on every runtime and, unlike a WebCrypto-based chain check, keeps verifyAttestation synchronous — so it is not a breaking change. It uses pkijs, already a dependency. An unrecognised signature-algorithm OID returns false, so an unverifiable certificate fails closed rather than being accepted. Verified: the existing suite passes unchanged (100% coverage maintained, plus a new case for the unsupported-algorithm branch), and a real production App Attest attestation that fails on Deno with the current code verifies successfully with this change on both Node 24.9.0 and Deno 2.9.3.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
verifyAttestationrejects every genuine App Attest attestation when run on Deno.X509Certificate.prototype.verify()is unreliable on non-Node runtimes for cross-curve ECDSA chains. Apple's App Attest chain always has a P-256 leaf signedecdsa-with-SHA256by a P-384 sub-CA ("Apple App Attestation CA 1"), and Deno'snode:cryptocompatibility layer returnsfalsefor exactly that link — where Node returnstruefor the same bytes:The P-384 → P-384 root→sub-CA link passes on both, so only the cross-curve link is affected. It appears the digest is derived from the issuer key's curve (P-384 → SHA-384) instead of read from the certificate's declared
signatureAlgorithm(ecdsa-with-SHA256).Since every App Attest chain contains that link, the failure is total, not intermittent. It surfaces at
src/verifyAttestation.js:110as:which reads like a forged or malformed client certificate rather than a runtime defect. That made it genuinely hard to diagnose — it disabled device attestation in our production service, and every test stayed green throughout because tests run under Node.
Filed upstream as denoland/deno#36309. Related: #28494 reported the same App Attest use case failing with
ERR_NOT_IMPLEMENTEDand was closed whenverify()was implemented; this is the follow-on, where the method now exists but returns the wrong answer.Change
Both chain checks go through a new
isSignedBy()helper that takes the digest explicitly from the certificate's ownsignatureAlgorithmOID and verifies withcrypto.verify().This is not a breaking change —
verifyAttestationstays synchronous. I specifically avoided the more obvious fix of using a WebCrypto-based chain check (e.g.pkijs'sCertificate.verify()), which is also correct on both runtimes but is async and would have forced a major version.Notes:
pkijs, already a dependency. No new dependencies.false, so an unverifiable certificate fails closed rather than being accepted.Verification
signatureAlgorithmOID while leaving the signed bytes intact.prettier --checkandeslintclean.Happy to adjust the OID table's scope or the naming if you'd prefer something different.