test(cms): SKI-form harness, matchesSID canonicality fix, mutation-test target#14
Merged
Conversation
…argets
Audit-equivalent test workflow as Makefile targets so they're scripted
and reproducible:
- `make long-fuzz` — run every fuzz target for FUZZTIME each (default
10m; override e.g. FUZZTIME=30m or FUZZTIME=1h).
Anchored regexes (^Foo$) prevent multi-match.
- `make overnight-fuzz` — long-fuzz with FUZZTIME=1h (≈16h total at
current target count). For dedicated burn-in runs.
- `make mutation-test` — run gremlins mutation testing on pkg/cms;
installs the tool if missing. Surviving mutants
indicate test gaps.
- `make govulncheck` — surface stdlib/dependency CVEs reachable from
our call graph; installs the tool if missing.
The default target stays `help`, listing the new audit targets alongside
the existing build/test/lint/docker ones.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
compareBytes was a reimplementation of bytes.Compare with the same contract (lexicographic compare, -1/0/1). Switching to the stdlib function removes 22 lines of code, eliminates a small cluster of (false-positive) surviving mutants in mutation testing, and trims one ad-hoc helper from the package surface. Behavior is unchanged: bytes.Compare implements exactly the lexicographic byte ordering DER mandates for SET OF. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Closes the largest mutation-testing gap on this branch (the
SubjectKeyIdentifier path of matchesSID and the SignerInfo
version-vs-SID cross-check) by adding a from-scratch CMS construction
harness that the production signer cannot reach.
Finding & fix:
matchesSID previously expected EXPLICIT [0] wrapping of the SKI
(A0 <len> 04 <ski-len> <ski>): the code unconditionally tried
asn1.Unmarshal(sidRaw.Bytes, &keyID) which only succeeds when there's
a nested OCTET STRING TLV inside. RFC 5652's ASN.1 module declares
IMPLICIT TAGS, so the canonical encoding is 80 <len> <ski> with no
nested OCTET STRING — exactly what OpenSSL and
github.com/github/ietf-cms emit. As a result, go-cms could not verify
SKI-form SignerInfo produced by any standards-compliant CMS producer.
Fix: treat sidRaw.Bytes as the raw SKI value when the [0] tag is
primitive (canonical IMPLICIT form), and reject the constructed form
outright. Rejecting the EXPLICIT variant also closes a malleability
surface: under the old code, the same logical SID could be encoded
two different ways, breaking content-addressing built on CMS blob
hashes.
New test harness (pkg/cms/cms_builder_test.go):
- cmsBuildConfig + buildTestCMS: hand-assemble a CMS SignedData blob
with explicit control over SID form (IAS vs SKI), SignerInfo and
SignedData versions, EncapContentInfo OID, SKI corruption, IMPLICIT
vs EXPLICIT SKI wrapping, and Case-1 vs Case-2 SignedAttributes.
- The signature itself is always real Ed25519 over canonical-DER
SignedAttributes (or content for Case 2) — verifier behaviour we
observe is cryptographically authentic, not asn1-decode-passes-and-
stops.
SKI test suite (pkg/cms/ski_test.go, 8 tests):
- TestBuilderHappyPath_{IAS,SKI}: harness sanity checks.
- TestSKI_VersionMustBe3 / TestIAS_VersionMustBe1: prove the
SignerInfo version ↔ SID-form cross-check (verifier.go:216) rejects
SKI+v1 and IAS+v3. Previously LIVED in mutation testing because no
SKI test exercised the branch.
- TestSKI_KeyIdMismatchRejects: corrupt SKI bytes must fail
matchesSID equality.
- TestSKI_TamperResistance: SKI-form signature must still reject
detached-data tampering.
- TestSKI_RejectsExplicitWrapping: documents and locks in the
canonicality rejection.
- TestSKI_Case2: SKI × Case 2 intersection.
Validation:
- Full test suite passes under -race at 78.9% statement coverage
(+1.0 from main).
- Mutation testing: efficacy 76.36% → 79.76%, mutator coverage
83.77% → 85.76%. The version-vs-SID cross-check mutants in
matchesSID flip from LIVED to KILLED.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens pkg/cms CMS verification by fixing an interoperability bug in the SKI (SubjectKeyIdentifier) SignerIdentifier path (RFC 5652 canonical IMPLICIT encoding), and adds a test-only CMS construction harness plus targeted SKI tests to cover verifier branches the production signer cannot emit. It also adds Makefile targets intended to support reproducible audit-style fuzzing/mutation/vuln scanning workflows.
Changes:
- Fix
matchesSIDto accept canonical RFC 5652 SKI form ([0]IMPLICIT, primitive) and reject non-canonical EXPLICIT wrapping (malleability reduction). - Add
cms_builder_test.goharness andski_test.gosuite to exercise SKI + version cross-checks and Case 2 intersections. - Add audit-focused Makefile targets (
long-fuzz,overnight-fuzz,mutation-test,govulncheck) and refactor ordering compare tobytes.Compare.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/cms/verifier.go | Accept canonical SKI SID encoding, reject EXPLICIT wrapping; replace custom byte-compare helper with bytes.Compare. |
| pkg/cms/ski_test.go | Add SKI-focused tests (happy path, version cross-checks, mismatch/tamper/case2, explicit-wrapping rejection). |
| pkg/cms/cms_builder_test.go | Add a test-only CMS builder to construct SignedData/SI variants not reachable via production signing APIs. |
| Makefile | Add long-running fuzz/mutation/vulncheck targets and update help text. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Four fixes for issues Copilot flagged on PR #14: 1. Makefile help text: overnight-fuzz claimed "~12h total" but the target list has 16 entries × 1h = ~16h. Update help line to match the code comment. 2. mutation-test target: previously `which gremlins` then bare `gremlins`, which fails on fresh environments where GOBIN isn't on PATH even after `go install` succeeds. Resolve the install path via `go env GOBIN`/`GOPATH/bin` and invoke the binary by absolute path. 3. govulncheck target: same fix as (2). 4. cms_builder_test.go: clarify the SubjectKeyId derivation comment. RFC 5280 §4.2.1.2 method 1 hashes the *contents* of the subjectPublicKey BIT STRING (raw key bytes), not the DER encoding of the BIT STRING itself. The code was already correct; the comment is now accurate. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
Round 2 of the audit-equivalent hardening on
pkg/cms. Builds on PR #13:SignerIdentifier, which was untested and contained a real interop bug.make long-fuzz,make overnight-fuzz,make mutation-test,make govulncheck.compareByteswith stdlibbytes.Compare.Findings & fixes
matchesSIDrejected canonical RFC 5652 SKI form (real interop bug)Before this PR,
matchesSIDonly accepted EXPLICIT `[0]` wrapping of the SubjectKeyIdentifier (`A0 04 `). It tried `asn1.Unmarshal(sidRaw.Bytes, &keyID)`, which only succeeds when the SKI value is itself an OCTET STRING-tagged TLV.RFC 5652's ASN.1 module declares `IMPLICIT TAGS`, so the canonical encoding is `80 ` with no nested OCTET STRING. That's what OpenSSL and github.com/github/ietf-cms both emit. The consequence: go-cms could not verify SKI-form CMS produced by any standards-compliant CMS producer.
Fix: in `matchesSID`, treat `sidRaw.Bytes` as the raw SKI value when the `[0]` tag is primitive (canonical IMPLICIT), and reject the constructed/EXPLICIT form outright. Rejecting EXPLICIT also closes a small malleability surface — the same logical SID had two valid DER encodings under the old code.
What's added
CMS construction harness (`pkg/cms/cms_builder_test.go`)
Internal test-only `buildTestCMS` + `cmsBuildConfig` lets any future test hand-assemble a CMS SignedData blob with explicit control over fields the production signer doesn't expose:
Signatures produced are always real Ed25519 over canonical-DER `SignedAttributes` (Case 1) or content (Case 2) — verifier behavior we observe is authentic, not asn1-decode-and-stop.
SKI test suite (`pkg/cms/ski_test.go`, 8 tests)
Audit Makefile targets (`Makefile`)
Refactor (`pkg/cms/verifier.go`)
Validation
Full suite passes under `-race` across 5 `-count` repetitions. No regressions in existing tests.
Test plan
🤖 Generated with Claude Code