Prefer embedded LTV revocation and handle large national CRLs#14
Merged
Conversation
Fixes certificate_not_revoked_at_signing_time returning INDETERMINATE
("Failed to parse CRL data") for real Latvian eID signatures under
asn1js@^3.0.9, and makes the check offline and signing-time-correct.
- Revocation: evaluate "not revoked at signing time" from the signature's
embedded XAdES RevocationValues (OCSP/CRL captured at signing time) before
any live fetch; expose SignatureInfo.revocationValues and
RevocationResult.fromEmbedded
- crl: parseCRL() raises asn1js's DEFAULT_MAX_NODES cap via a bounded
fallback so national CRLs (~13k LV eID entries) parse
- xmlParser: fix InclusiveNamespaces "XPath evaluation failed" warning by
using xpath.useNamespaces instead of misusing select()'s `single` arg
- trusted-list: add top-level bundleId; carry forward last-known-good
services for unreachable territories; native http(s) fetch fallback for
endpoints that block undici's fingerprint (e.g. EE sr.riik.ee); refresh
bundled EU snapshot
- deps: declare asn1js, @peculiar/asn1-schema, @peculiar/asn1-x509
- prettier: ignore generated src/data/trusted-list.{ts,json}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fda1e9f552
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code review (P1): xades:RevocationValues live in UnsignedSignatureProperties and are not covered by the signature, so short-circuiting the revocation verdict to "good" from them let a tampered container pass a revoked signer with no network. Authenticating them offline would require verifying the OCSP/CRL signature and responder chain against a trusted issuer plus freshness, which isn't generally possible offline (the issuer cert may not be in the container). - Remove the embedded-revocation short-circuit; the verdict now comes only from live OCSP/CRL. When revocation can't be checked it is INDETERMINATE, never "good" - Drop RevocationCheckOptions.embeddedOCSP/embeddedCRL/atTime and RevocationResult.fromEmbedded; delete the embedded evaluator - Still parse and expose SignatureInfo.revocationValues as RAW, explicitly unvalidated material for consumers doing their own LTV validation - Add a regression test: offline revocation is indeterminate (not pass) while document/signature/certificate/timestamp/issuer checks still verify offline The original INDETERMINATE-on-large-CRL bug stays fixed via the parseCRL node-cap fallback on the live path (verified: live CRL returns "good").
When the container ships no certificate chain (as with the LV eID sample),
OCSP couldn't build a request ("could not find issuer") and fell back to
downloading the full ~668 KB CRL. The issuer cert is, however, bundled inside
the signature's RevocationValues OCSP response.
- extractCertsFromOCSPResponses(): recover certs embedded in OCSP responses
- resolveIssuerFromChain(): async issuer resolution that prefers a candidate
whose key actually signed the cert (so a tampered same-name issuer can't
misdirect the request); checkOCSP now uses it
- verification.ts feeds those embedded certs into the revocation chain
The embedded OCSP *status* is still not trusted — a fresh live OCSP query is
made. Verified end-to-end: revocation now answers via method "ocsp" instead of
"crl" for SampleFile, without downloading the CRL.
Owner
Author
|
@codex please re-review new code |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ec009b2cec
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Code review (P2): parseSignatureElement read RevocationValues, CertificateValues, SigningTime and SignatureTimeStamp from the whole document (xmlDoc), so in a multi-signature document every signature inherited the first signature's embedded material — pointing the new OCSP issuer resolution at another signature's certs. Query these within the current signatureElement instead. No change for the common one-signature-per-file case (the element is found either way); the fix matters when parseSignatureElement is used on documents with multiple ds:Signature elements. Adds a real-DOM test parsing a two-signature document and asserting each signature yields its own RevocationValues, CertificateValues and SigningTime.
Owner
Author
|
@codex please re-review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c68083f4cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
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
1 participant
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
Verifying a real Latvian eID signature returned
INDETERMINATEforcertificate_not_revoked_at_signing_time:Reproduction note
Only reproduces with
asn1js@^3.0.9(the latest within the existing^3.0.6range;npm install/npm ciland here). asn1js 3.0.9+ enforcesDEFAULT_MAX_NODES = 10000, and the LV eID ICA CRL has ~13k entries, sonew X509Crl()throws. The repo previously happened to resolve3.0.7(no cap), which masked it.What changed
Revocation
parseCRL()falls back toasn1js.fromBER(..., { maxNodes })+AsnParser.fromASN(CertificateList)+new X509Crl(certList)so national CRLs that exceed the node cap parse (bounded, still a DoS guard).RevocationValuesOCSP response, and issuer resolution prefers a candidate that actually signed the cert. The LV eID sample ships nocertificateChain, so OCSP previously failed ("could not find issuer") and fell back to the ~668 KB CRL; it now answers via a small live OCSP query (verified end-to-end:method: "ocsp", not"crl").RevocationValuesare parsed and exposed onSignatureInfo.revocationValues(raw base64 DER) but their status is never trusted — a fresh live query is always made (see Security).Security: embedded revocation status is not trusted (addresses review P1)
RevocationValueslive inUnsignedSignaturePropertiesand are not covered by the signature. Trusting theirgoodstatus to short-circuit the verdict would let a tampered container pass a revoked signer. The verdict therefore comes only from a live OCSP/CRL query; when revocation can't be checked the result isINDETERMINATE, nevergood. (The embedded issuer certificate is used only to address the live OCSP query, after verifying it actually signed the signer — a wrong/tampered cert yieldsunknown, notgood.) Document integrity, signature, certificate validity, timestamp, and issuer-trust checks remain fully offline-capable.XPath warning
InclusiveNamespaces"XPath evaluation failed" warning (was passing the namespace map asxpath.select'ssinglearg; now usesxpath.useNamespaces).Trusted list (Part B)
bundleId(wasnull); refreshed the EU LOTL snapshot.sr.riik.ee) block undici's TLS/HTTP2 fingerprint (andMozilla/*UAs) but accept Node's native client.Deps
asn1js,@peculiar/asn1-schema,@peculiar/asn1-x509(were transitive)..prettierignoreexcludes generatedsrc/data/trusted-list.{ts,json}(also fixes a pre-existingnpm run format:checkfailure).Not done (with reason)
Test plan
npm test— 147 passing / 24 suites, including:parseCRLon the real >10k-entry LV eID CRL (committed public, no-personal-data fixture)verifySignature(SampleFile)→certificate_not_revoked_at_signing_time = indeterminate(notpass), other checks still passmethod: "ocsp"and the signer CRL is never downloadednpx tsc --noEmitclean,npm run buildsucceeds,npm run format:checkclean