Skip to content

test(eudi/jwt): unit test VerifyCertificate against a root > CA > leaf PKI - #646

Open
dobby-coder[bot] wants to merge 3 commits into
masterfrom
test/eudi-jwt-verify-certificate-645
Open

test(eudi/jwt): unit test VerifyCertificate against a root > CA > leaf PKI#646
dobby-coder[bot] wants to merge 3 commits into
masterfrom
test/eudi-jwt-verify-certificate-645

Conversation

@dobby-coder

@dobby-coder dobby-coder Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Part of #645. This is Step 6 / Group D of the plan, which the issue marks as independent of the rest.

VerifyCertificate (eudi/jwt/verify.go) runs over every issuer leaf that arrives in an x5c header, and had no direct unit test.

What is here

New eudi/jwt/verify_test.go, six cases over a root > CA > end-entity hierarchy built with the existing testdata PKI helpers:

case expected
valid leaf, root in the pool no error
expired leaf x509: certificate has expired or is not yet valid
leaf without digitalSignature key usage end-entity certificate missing digitalSignature key usage
leaf revoked by a CRL from its issuer certificate is revoked by issuer ... in revocation list with number 1
leaf listed on a CRL from a CA with the same DN but a different subject key id no error
leaf whose root is absent from the pool x509: certificate signed by unknown authority

The fifth case is not in the plan's D1-D5 list. It pins the fact recorded under "Verified facts" that CRLs are matched on AuthorityKeyId plus issuer DN: both hierarchies use CN=CA CERT 0,..., so only the subject key id tells them apart. Drop it if you would rather keep to the listed five.

Two changes in testdata/testdata.go:

  • PkiOption_ExpiredEndEntity was declared and passed around, but CreateEndEntityCertificate never read it, so it produced a valid leaf. It now sets notBefore/notAfter in the past.
  • PkiOption_NoEndEntityDigitalSignatureKeyUsage is new and omits the keyUsage extension.

One knock-on change in eudi/openid4vp/verifier_validator_test.go: testParseAndVerifyAuthorizationRequestFailureExpiredX5C passed PkiOption_ExpiredEndEntity but also nil'd the x5c header and asserted the missing-x5c error, which made it a duplicate of testParseAndVerifyAuthorizationRequestFailureMissingX5C that never exercised expiry. That was unavoidable while PkiOption_ExpiredEndEntity was a no-op. It now drops the token modifier and asserts the real expiry error, matching the shape of its neighbours (...FailureRevokedX5C, ...FailureExpiredRoot, ...FailureExpiredIntermediate). The nil-x5c case stays covered by testParseAndVerifyAuthorizationRequestFailureMissingX5C.

What is not here

Steps 0-5 and 7 (the PKI re-chain, Group A x509 trust tests, Groups B and C, the auth-code spike) all need the eudi_pid_issuer_py stack running under Docker. This environment has no Docker daemon, so I could not capture the credential fixtures those tests assert on, nor run the verification gate the plan puts at the end of Step 1. Writing them blind would mean shipping assertions nobody has watched pass. Those steps are untouched.

Testing

  • go test ./eudi/... -count=1 passes.
  • Removing the PkiOption_ExpiredEndEntity block makes both the new expired-leaf test and the openid4vp expired-x5c test fail with "An error is expected but got nil"; removing the PkiOption_NoEndEntityDigitalSignatureKeyUsage block fails the key-usage test the same way. Restoring either makes them pass.
  • gofmt, go vet ./..., go fix -diff ./... and misspell are clean.
  • staticcheck (rebuilt against go1.26.4, since the container's default Go is older than go.mod's toolchain) reports only a pre-existing ST1022 on testdata/testdata.go:46, which CI does not see because ./... skips testdata.
  • go test ./... fails in the six packages that need Postgres, SMTP, Redis, the EUDI verifier or port 8080 (internal/sessiontest, irma, irma/irmaclient, keyshareserver, myirmaserver, keyshare/tasks). Same set fails on master here; CI supplies those services.

No changelog entry: test-only change, matching Step 8's "only if the PKI regen is noteworthy". The Changelog check needs the skip-changelog label to go green.

…f PKI

VerifyCertificate had no direct unit test. The integration suite reaches it
only through a live issuer container, which cannot produce an expired leaf or
a leaf without the digitalSignature key usage, so those branches were
unreachable from there.

Adds eudi/jwt/verify_test.go covering: a valid leaf, an expired leaf, a leaf
missing the digitalSignature key usage, a leaf revoked by a CRL from its
issuer, a CRL from an authority with the same DN but a different subject key
id (must not apply), and a leaf whose root is absent from the pool.

PkiOption_ExpiredEndEntity was declared but never honoured by
CreateEndEntityCertificate, so it now sets notBefore/notAfter in the past.
PkiOption_NoEndEntityDigitalSignatureKeyUsage is new and omits the keyUsage
extension.

Refs #645

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
testParseAndVerifyAuthorizationRequestFailureExpiredX5C also nil'd the x5c
header and asserted the missing-x5c message, making it a byte-for-byte
duplicate of testParseAndVerifyAuthorizationRequestFailureMissingX5C that
never exercised certificate expiry. That was unavoidable while
PkiOption_ExpiredEndEntity was a no-op; now that CreateEndEntityCertificate
honours it, the test can assert the real error.

Drops the token modifier and asserts the expiry message, matching the shape
of the neighbouring revoked-x5c test. Verified non-vacuous: removing the
PkiOption_ExpiredEndEntity block from CreateEndEntityCertificate makes it
fail with "An error is expected but got nil".

Refs #645

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

@dobby-coder dobby-coder Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

VERDICT: approve

Rules Dobby 2 — cycle 1, sign-off

Rule sweep over 13 rules selected for a Go test-only irmago diff (CI-gate rules, PR-hygiene rules, Go/test-correctness rules), run as three Haiku batches. No rule violation survived verification, and no bug-severity findings. One documentation nit, which I fixed on this branch rather than looping the coder — details at the bottom.

What I verified rather than took on trust

The PR's central claim is that these six tests actually exercise VerifyCertificate, and that the two testdata/testdata.go changes are load-bearing. I checked that by mutation, not by reading:

mutation result
remove the PkiOption_ExpiredEndEntity block Test_VerifyCertificate_ExpiredLeaf_ReturnsError and ...FailureExpiredX5C both fail with "An error is expected but got nil"
remove the PkiOption_NoEndEntityDigitalSignatureKeyUsage block Test_VerifyCertificate_LeafWithoutDigitalSignatureKeyUsage_ReturnsError fails the same way
neutralise the AuthorityKeyId term in GetRevocationListsForIssuer Test_VerifyCertificate_LeafRevokedByCrlFromAnotherAuthority_ReturnsNoError fails

That third one settles the open question in the PR body. The sixth case is not in the plan's D1–D5 list, and it is worth keeping: it is the only test in the repo that pins CRL matching to AuthorityKeyId plus issuer DN rather than DN alone. Dropping it would leave that behaviour unpinned. Keep it.

Also checked:

  • go test ./eudi/... -count=1 — all 29 packages green. All six new tests pass individually.
  • gofmt -l clean on all three files; go vet ./eudi/... clean; go fix -diff ./eudi/... produces no modernizer diff (the check that fails on code which vets and gofmts cleanly).
  • The shared-helper semantics change is fully contained. CreateEndEntityCertificate has four callers; only verifier_validator_test.go:130 passed the previously-no-op PkiOption_ExpiredEndEntity, and this PR fixes exactly that one. getCaCertTemplate / CreateRootCertificate / CreateCaCertificate read only bits 8 (ExpiredIntermediate) and 32 (ExpiredRoot) — never bit 2 or the new bit 512 — so newVerifyTestPki threading one opts value through the whole hierarchy cannot expire a CA or root by accident. 512 collides with nothing.
  • certTemplate.KeyUsage = 0 genuinely omits the extension rather than emitting an empty one: crypto/x509 gates it on if template.KeyUsage != 0. The new option's doc comment is accurate.
  • No coverage lost in openid4vp. The nil-x5c path is still covered by testParseAndVerifyAuthorizationRequestFailureMissingX5C (unchanged, line 115), and the rewritten expired-x5c test now matches the shape of its three neighbours (...FailureRevokedX5C, ...FailureExpiredRoot, ...FailureExpiredIntermediate), all of which pass nil as the modifier and let the PkiOption do the work. This is a real de-duplication, not a rewrite.
  • closingIssuesReferences is empty, so merging leaves #645 open — correct for a Step-6-only slice of a nine-step plan. Reviewer rubenhensen is assigned. skip-changelog is applied and the changelog check is SKIPPED, appropriate for a test-only change.

Candidate findings I dropped

  • "The hard-coded DN CN=CA CERT 0,OU=Test Unit,O=Test Organization,C=NL is a brittle assertion." Not a defect — this is the established convention in this suite. verifier_validator_test.go:147 asserts the byte-identical DN string and predates this PR. Flagging it here would be inventing a new standard for one file.
  • "The PR should not be a draft." Handled by flipping it ready, not by a finding.

The one nit, already fixed

The body said the eudi/openid4vp expired-x5c test was "Left alone here" — contradicted by the PR's own second commit, which rewrote it. The "What is here" section also never mentioned that file. A reviewer reading the body would have mis-scoped the change. I rewrote that paragraph to describe what the commit actually does, and corrected the Testing bullet to cover both non-vacuity checks rather than just the expired-leaf one. No code touched.

Note for the maintainer

Steps 0–5 and 7 of #645 (the PKI re-chain, the Group A x509 trust tests, Groups B and C, the auth-code spike) are genuinely not here — they need the eudi_pid_issuer_py stack under Docker, which the build environment does not have. The PR body says so plainly and does not overclaim. #645 stays open for the rest.

Signing off. Flipping out of draft.

CI is fully green on 89c7902: test, both integration-test-* jobs, lint, build, docker-build and all nine release-artifact jobs pass; changelog is SKIPPED by the skip-changelog label.

@dobby-coder
dobby-coder Bot marked this pull request as ready for review July 28, 2026 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant