fix(mdocrical): stop enforcing isTrustAnchor, it's being removed from the standard - #151
Merged
Merged
Conversation
… the standard The Geneva 2026 event's live RICAL (geneva2026.mdoc.online) has isTrustAnchor absent on all 35 published certificateInfos entries, despite F.3.2.2 currently documenting it as Required. Word from the interop event organizers is that isTrustAnchor is being removed from the ISO/IEC 18013-5 standard entirely in a future edition - so enforcing it as a gate would deny every reader against any spec-current or spec-future RICAL, not just handle one non-conformant document as a special case. validateChainAgainstAnchors now builds its root pool from every CertificateInfo unconditionally; resolveCertificateInfo's matching no longer filters on IsTrustAnchor either. The field is still decoded (for wire compatibility and any future TrustConstraints use) but no longer read anywhere as a trust decision.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates the mdoc RICAL registry implementation to stop treating CertificateInfo.isTrustAnchor as a required gate for path validation, based on real-world RICALs omitting the field and expectations that the field will be removed from the standard.
Changes:
- Updated path validation to no longer filter root candidates on
IsTrustAnchor. - Updated
resolveCertificateInfoto no longer require the matched “root” to come fromIsTrustAnchorentries. - Added a regression test documenting trust behavior when
isTrustAnchoris not set/used.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/registry/mdocrical/registry.go | Removes IsTrustAnchor gating from chain validation/root selection and updates related documentation/errors. |
| pkg/registry/mdocrical/registry_test.go | Adds a test case intended to cover trusting chains even when isTrustAnchor is not enforced. |
Suppressed comments (1)
pkg/registry/mdocrical/registry_test.go:367
- In this fixture, IsTrustAnchor is not omitted; it is left at the zero value (false). The comment should reflect that to avoid suggesting the CBOR encoding differs from what buildSignedRical produces.
// IsTrustAnchor intentionally omitted (zero value: false).
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Raises new-code coverage on registry.go for PR #151 - no existing test exercised validateChainAgainstAnchors's if err != nil { continue } branch, which now matters more since every CertificateInfo entry is a candidate root regardless of isTrustAnchor.
Per Copilot review: adding every CertificateInfo to the root pool regardless of type let a reader/leaf entry enrolled solely to carry TrustConstraints (F.3.2.6) act as a validation anchor. Restrict Roots to certs with IsCA true; non-CA entries remain reachable via findFirstMatchingCertificateInfo's exact chain-member match. Also fixes test comment wording: the fixture's IsTrustAnchor is encoded as CBOR false (no omitempty tag), not absent from the wire - the field isn't consulted anywhere so the distinction is behaviorally moot, but the doc comment shouldn't claim otherwise.
Raises new-code coverage for the CA-only-roots fix (5e0da8a) - no existing test exercised the "skip non-CA CertificateInfo" path itself, only that a chain still validates via a separate genuine CA entry in the same RICAL.
Raises new-code coverage further - empty-chain and no-usable-cert are pre-existing defensive branches that Evaluate()'s higher-level tests don't reach directly (x5c always has >=1 cert, and every test RICAL so far had at least one parseable entry).
|
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.
Summary
geneva2026.mdoc.online) hasisTrustAnchorabsent on all 35 publishedcertificateInfosentries, despite F.3.2.2 currently documenting it as Required.isTrustAnchoris being removed from the ISO/IEC 18013-5 standard entirely in a future edition.validateChainAgainstAnchorsnow builds its root pool from everyCertificateInfounconditionally;resolveCertificateInfo's matching no longer filters onIsTrustAnchor. The field is still decoded (wire compatibility, possible futureTrustConstraintsuse) but never read as a gate.Found while live-debugging a Scytales reader-auth presentation failing against gdc - independently confirmed by directly fetching and CBOR-decoding the live published RICAL (dated 2026-08-30) and finding
isTrustAnchorabsent on every single entry, not just Scytales'.Test plan
go build ./...go test ./pkg/registry/mdocrical/...(all 6 passing, including newTestEvaluate_TrustedDespiteMissingIsTrustAnchor)golangci-lint run ./...(0 issues)