Repository navigation
fix(crypto): canonicalize agent cards per RFC 8785 - #94
Merged
Merged
Conversation
CreateCanonicalJSON was not RFC 8785. Its comment stated the assumption
plainly: "encoding/json sorts map keys by default, providing
canonicalization". It does not. A2A specification 8.4.1 requires the signed
payload to be canonicalized with the JSON Canonicalization Scheme, and the
differences are not cosmetic. Cards signed by a spec-compliant signer failed
verification here, which is worse than not verifying at all: it fails closed
on valid cards while giving the appearance of a working trust check.
Three defects, all reproduced before fixing:
1. Field presence. The function marshalled the AgentCard struct before
canonicalizing. AgentCapabilities declares plain bools with omitempty, so a
sender's explicit "streaming": false was dropped, and capabilities
canonicalized to {}. Spec 8.4.1 requires an explicitly set optional field
to survive even when it holds a default. Conversely, fields without
omitempty were emitted as zero values the sender never sent.
2. HTML escaping. Go's encoder escapes '<', '>' and '&'; RFC 8785 emits them
literally. An ampersand in a URL query string was enough to change the
payload, and query strings with ampersands are ordinary.
3. Key ordering. Go sorts map keys by UTF-8 byte order, RFC 8785 by UTF-16
code unit. These diverge above U+FFFF. Latent rather than urgent, but
fixed by the same change.
The fix canonicalizes the document that actually arrived rather than the
struct's rendering of it. AgentCard now retains its source bytes on decode
(UnmarshalJSON), CreateCanonicalJSON prefers those bytes, strips `signatures`,
and hands the result to github.com/gowebpki/jcs. Cards built in code carry no
source document and still fall back to the struct; that path is documented as
only as faithful as the struct tags allow.
Callers are unchanged: VerifyAgentCardSignatures and pkg/scoring keep their
signatures.
Testing. The previous canonical_test.go could not have caught any of this: on
mismatch it called t.Logf rather than t.Errorf, so it passed unconditionally.
It is replaced with eight assertions, including the worked example published
in spec 8.4.1 compared byte for byte.
Adds an interop fixture in pkg/crypto/interop_test.go: an agent card signed by
an implementation independent of this package, carrying both an explicitly-set
false and an ampersand in a URL. It verifies end to end, and a tampered copy is
rejected. All four of the new canonicalization tests and both interop tests
fail against the previous implementation and pass against this one.
Full suite green. Note pkg/agentcard now holds an unexported raw field, so
AgentCard is no longer struct-comparable between a decoded and a constructed
card; the round-trip test compares content instead.
Not addressed here: key resolution trust anchoring. validateHeader does not
bind the jku to the agent identity being verified, which is a design decision
the spec leaves to the deployment. Tracked in #91.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
AgentCard.Raw() currently exposes the internal backing slice, allowing mutation of verification-critical retained bytes after decode.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes Agent Card signature verification interop by canonicalizing the received Agent Card JSON per RFC 8785 (JCS) after removing the top-level signatures field, instead of relying on Go’s encoding/json behavior.
Changes:
- Implement RFC 8785 canonicalization via
github.com/gowebpki/jcs, using retained source JSON bytes when available. - Add comprehensive canonicalization tests (including the spec worked example) and an end-to-end interop fixture signed by an independent implementation.
- Extend
pkg/agentcard.AgentCardto retain its original decoded JSON bytes for verification fidelity.
File summaries
| File | Description |
|---|---|
| pkg/crypto/canonical.go | Switch canonicalization to RFC 8785 (JCS) and canonicalize from preserved source JSON when available. |
| pkg/crypto/canonical_test.go | Replace ineffective test with multiple assertions covering key ordering, HTML escaping, field presence, etc. |
| pkg/crypto/interop_test.go | Add interop verification fixture and tamper test to ensure end-to-end correctness across implementations. |
| pkg/agentcard/types.go | Retain raw decoded JSON bytes via UnmarshalJSON, expose via Raw() for verifier canonicalization. |
| pkg/agentcard/types_test.go | Update round-trip expectations and add tests for raw retention and copying behavior. |
| go.mod | Add JCS dependency. |
| go.sum | Record JCS module sums. |
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Raw() handed out the backing array, so a caller could mutate the retained source document in place and change what a later verification canonicalizes. Inconsistent with the decode path, which already copies the caller's buffer for the same reason. Now both directions copy. Raised by copilot-pull-request-reviewer on #94. The new test fails against the previous implementation. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Codecov flagged the UnmarshalJSON error branch as uncovered. It is reachable: any card that arrives as a wrong-typed field hits it. Asserts the property that matters rather than the line: a document that fails to decode leaves the card as it was, so decoding into a reused variable cannot leave one card's retained bytes attached to another card's fields. A verifier canonicalizes those bytes, so the two must never diverge. Covers both failure sites, which differ: malformed syntax is rejected by encoding/json before UnmarshalJSON is called, while a type mismatch is syntactically valid and fails inside it. UnmarshalJSON now at 100%. The branches still uncovered in canonical.go are marshal and transform errors that valid inputs cannot produce. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
CI Security Scanning has failed on every run since 27 May, on main and on every branch, and no code change can fix it. Reported in #74, where the list was seven stdlib CVEs; it is now ten. govulncheck reports fixes land in go1.25.11 (net/textproto, crypto/x509) and go1.25.13 (net/http), and golang.org/x/net v0.55.0 (GO-2026-5026). Bumps the go directive to 1.25.13 so actions/setup-go picks it up via go-version-file, and x/net to v0.55.0. go mod tidy carried crypto, sys and text along with it. Build and full test suite green on 1.25.13: 22 packages, no failures. Closes #74. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Canonicalization now prefers retained raw bytes, so mutating a decoded AgentCard before verification can yield “valid” results for bytes that no longer match the in-memory struct, and this needs explicit documentation in-code to avoid misuse.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 1
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes address the canonicalization defects directly, preserve on-the-wire semantics via raw-byte retention, and are backed by strong spec/interop regression tests.
Review details
- Files reviewed: 6/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
govulncheck reports GO-2026-6061 against google.golang.org/[email protected]: vulnerabilities in the xDS RBAC authorization engine and the HTTP/2 transport server. Both are reachable from this module, through pkg/pop/session_cache.go and cmd/capiscio/rpc.go. v1.82.1 is the fixed version named in the advisory. It also drops the go.opentelemetry.io/otel/sdk/metric indirect dependency, which grpc no longer requires. Build, vet and the full test suite pass unchanged. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Verify Protobuf has been failing before it runs any check: No github_token supplied, API requests will be subject to stricter rate limiting Resolving the download URL for the current platform... API rate limit exceeded for 128.24.163.82. bufbuild/buf-setup-action resolves its release download through the GitHub API. Unauthenticated, that quota is shared across every job on the runner's IP, so the step fails on load rather than on anything in this repository. It reproduced identically on two consecutive runs. github.token is already available to the job and needs no new permissions. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The canonicalization logic now matches the RFC 8785/A2A requirements and is backed by strong, interop-focused tests that would have caught the prior defects.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
A JSON `null` decoded into an AgentCard satisfies encoding/json, so the
retained document became the four bytes "null". Canonicalizing that
returned "null" and no error: a signable payload for something that is
not an agent card.
Reject it in stripSignatures instead. A nil map is the only way
decoding into map[string]interface{} succeeds on a non-object, so the
guard is one comparison.
Also covers the RFC 8785 number range. A literal outside the IEEE 754
double range has no canonical form; jcs already errors on it, and there
is now a test pinning that rather than an assumption in a comment. The
package comment records both refusals.
CreateCanonicalJSON statement coverage 76.9% -> 92.3%. What remains is
the json.Marshal branch on the programmatic path, which AgentCard has no
unmarshalable field to reach.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Raised in review. Because the received bytes take precedence over the struct, a caller that decodes a card and then mutates its fields still gets the payload of the original document. A signature reported valid says the sender signed what was received, not what the struct now holds. That is the correct behaviour for a verifier, but it is a trap if read the other way round, so the contract is now written down: treat a decoded AgentCard as immutable up to verification, and re-encode and re-decode a card that must change. Comment only. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
It changes a security-critical canonicalization/signature-verification path and introduces new cryptographic canonicalization behavior/dependencies that warrant final human review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
go.mod:3
- go.mod now requires Go 1.25.13, but the CI protobuf job still pins actions/setup-go to 1.24.0. Even if the protobuf step doesn’t currently run
go, this version drift can cause confusing CI failures if any Go tooling is later added (or ifmake protostarts invoking Go-based plugins). Consider switching that job togo-version-file: go.mod(or bumping the pinned version) for consistency with the module toolchain.
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Contributor
There was a problem hiding this comment.
🔵 Needs a closer look
Canonicalization currently accepts JSON with trailing non-whitespace by ignoring it during decode, which can let invalid JSON texts verify successfully.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
pkg/crypto/canonical.go:87
- stripSignatures uses json.Decoder.Decode once and does not verify the input contains exactly one JSON value; any trailing non-whitespace after the object would be silently ignored, so canonicalization (and signature verification) could succeed on an invalid JSON text that differs from what was signed.
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
Both raised in review. Decode stops at the end of the first JSON value and ignores whatever follows. On a signature path that means a document could carry bytes the payload never covers. json.Unmarshal rejects trailing data before the raw bytes are retained, so this is not reachable through AgentCard today, verified in both directions: via json.Unmarshal and via a direct call to the exported UnmarshalJSON. The guard holds the guarantee in the function rather than in a caller's invariant, which is the same mistake class this PR exists to fix. Separately, the protobuf job pinned actions/setup-go to 1.24.0 while go.mod requires 1.25.13. The toolchain directive papers over it today. Every other job in the file already uses go-version-file, so this one now does too. Co-Authored-By: Claude Opus 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
stripSignatures uses json.Decoder.More() as a trailing-data guard, which is not a robust top-level trailing-token check and can miss certain malformed trailers despite the comment claiming the invariant is enforced here.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 1
- Review effort level: Lite
Raised in review, and correct. json.Decoder.More() reports whether
another element follows inside an array or object. At the top level it
answers false for a stray closing bracket, so the guard added in the
previous commit let `{"a":1}}` and `{"a":1}]` through and canonicalized
them as valid. Reproduced both before changing anything.
Reading a token and requiring io.EOF rejects any trailing byte,
including those two. Both are now in the table alongside the cases that
already worked.
The guard was introduced as defense in depth for something unreachable
through AgentCard today, and it was itself wrong. Worth stating plainly:
"unreachable" made me careless about whether the guard held.
Co-Authored-By: Claude Opus 5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01N2T85WUJCKKUgn4K1VwrTD
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes align canonicalization with RFC 8785/A2A requirements, preserve received-document fidelity for verification, and are backed by robust regression/interop tests plus CI unblocking updates.
Review details
- Files reviewed: 7/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Closes #35. Also closes #74.
CreateCanonicalJSONwas not RFC 8785, despite a comment claiming it was: "encoding/json sorts map keys by default, providing canonicalization". Spec 8.4.1 requires JCS, and the differences are not cosmetic — correctly signed cards failed verification here. That is worse than not verifying at all, because it fails closed on valid cards while looking like a working trust check.@jank6r reported this in #35 back in February, including that cards signed by the official A2A Python SDK failed against us. The report was correct and we left it sitting. Apologies.
What was wrong
Three defects, all reproduced before fixing:
omitemptyonAgentCapabilities' plain bools dropped an explicit"streaming": false, socapabilitiescanonicalized to{}. 8.4.1 requires explicitly set optional fields to survive at their defaults. Fields withoutomitemptyhad the opposite problem — zero values the sender never sent.<,>,&; RFC 8785 emits them literally. One ampersand in a query string changes the payload.Note on the fix suggested in #35: adding
omitemptyeverywhere solves half of (1) and creates its mirror image, sinceomitemptyon a plain bool can't distinguish absent from explicitly false. Struct tags are the wrong layer for a field presence rule.What changed
Canonicalize the document that arrived, not the struct's rendering of it.
AgentCardretains its source bytes on decode,CreateCanonicalJSONprefers those, stripssignatures, and hands off togowebpki/jcs. Programmatic cards have no source document and still fall back to the struct.Callers are unchanged.
Raw()returns a copy so a caller can't mutate what a later verification canonicalizes.Two consequences worth knowing:
stripSignaturesrejects two things it used to accept. A JSONnulldecoded fine and canonicalized to the literal"null"— a signable payload for something that isn't an agent card. And trailing data after the object was ignored. Both found by actually checking the uncovered branches instead of assuming they were unreachable.Testing
The old test file couldn't have caught any of this: on mismatch it called
t.Logfinstead oft.Errorf, so it passed unconditionally. That's how this survived.Replaced with twelve assertions, including 8.4.1's worked example compared byte for byte. Added
interop_test.go: a card signed by an implementation independent of this package, carrying both an explicitfalseand an ampersand in a URL, plus the canonical payload that signer computed. Verifies end to end; a tampered copy is rejected.Every new canonicalization and interop test fails against the old implementation.
CI
Security Scanning and Verify Protobuf were failing on this branch and had to be fixed before it could go green. Three commits, none related to the canonicalization change, each reviewable alone:
go1.25.13 +x/netv0.55.0 (chore: bump Go toolchain to fix govulncheck stdlib vulnerabilities #74) — versions from govulncheck's own "Fixed in" linesgrpcv1.82.1 — clears GO-2026-6061, reachable viapkg/pop/session_cache.goandcmd/capiscio/rpc.gobuf-setup-action— Verify Protobuf was dying on load, rate-limited resolving its own download against the anonymous GitHub API. Reproduced identically twice.Correction. An earlier version of this description, and the squash commit message, said CI had been red on
mainsince 27 May. That is wrong.main's last run before this one (27 May, run 335) was green on Test, Lint, Security Scanning and Verify Protobuf; its only failure wasTrigger E2E Tests. govulncheck reads a live advisory database, so the same commit passes and later fails as advisories publish — the #74 CVEs postdate 27 May, and nothing was pushed tomainin between to record the transition. The branch failures were real; the 27 May regression story was not.Trigger E2E Testsstill fails onmainand is unrelated to this change. Tracked separately.Review notes
pkg/agentcardgains an unexportedrawfield, soAgentCardis no longer struct-comparable between a decoded and a constructed card. Round-trip test compares content.gowebpki/jcsv1.0.1, the reference Go RFC 8785 implementation.CreateCanonicalJSONis at 92.3% statements. What's left isjson.Marshalfailing on the programmatic path, whichAgentCardhas no unmarshalable field to reach.