feat(signing): add SigningProvider abstraction for KMS/HSM-backed signing - #481
Open
sujanchalla0510 wants to merge 1 commit into
Open
feat(signing): add SigningProvider abstraction for KMS/HSM-backed signing#481sujanchalla0510 wants to merge 1 commit into
sujanchalla0510 wants to merge 1 commit into
Conversation
…ning Closes adcontextprotocol#99. adcp/v3/signing's Signer required the private key to live in process memory. Add a context-aware SigningProvider interface so operators can keep the key in a KMS/HSM/Vault instead: Sign(ctx, payload) ([]byte, error), KeyID() string, Algorithm() Algorithm, PublicKey(ctx) (crypto.PublicKey, error). InMemorySigningProvider wraps the existing behavior and stays the default — SignerOptions.KeyID/PrivateKey keep working unchanged; the new SignerOptions.Provider field is purely additive, no breaking change. SignRequest threads r.Context() into the provider's Sign call (both the direct-call and RoundTripper paths). Incorporates the design constraints from this issue's own maintainer triage thread (three review rounds, JS reference-implementation learnings cited in the comments): Algorithm() returns the typed signing.Algorithm (RFC 9421 wire value), not a bare string; provider-backed errors surface as a typed *SigningError (Code is safe to log, the raw backend SDK error is reachable only via errors.Unwrap/errors.As, never via Detail — KMS/GCP/ Vault error strings routinely embed resource ARNs/paths); the interface ships PublicKey up front since adding it later would be breaking, which unlocks NewPublicJWKFromProvider (jwks_uri publication straight from a provider, adcp_use required per adcp#2423 key-separation) and AssertProviderPublicKeyMatchesSPKI (startup tripwire against a managed key store silently rotating the key backing a kid). Ships a worked AWS KMS provider (adcp/v3/signing/awskms) against aws-sdk-go-v2/service/kms's real Sign/GetPublicKey APIs for an ECC_NIST_P256 key (AWS KMS has no Ed25519 asymmetric signing), converting KMS's DER-encoded ECDSA signature to the AdCP profile's fixed-width IEEE P1363 wire format. Isolated as its own go.mod — same pattern as registry/redisstore and registry/glidestore — so importing adcp/v3/signing never pulls in a cloud SDK (AGENTS.md: "Zero unnecessary dependencies"). PublicKey is lazily fetched and cached success-only (not sync.Once), per a production incident cited in the issue's triage history (eager KMS calls before a listener binds can hang startup on retryer backoff). Tested end-to-end via a fake SignAPI (no live AWS needed) covering request construction, DER->P1363 mapping, the SigningError contract, and a full sign+publish+verify round trip through the real adcp/v3/signing verifier. This PR's go.mod for the awskms module pins adcp/v3 at the currently published v3.0.0 tag, which predates SigningProvider (added here, in the same PR) — the same bootstrap gap any first submodule consuming brand-new sibling code has. Verified build/vet/test locally via `cp go.work.example go.work` (this repo's documented mechanism for exactly this); resolves itself with the next adcp/v3 tag. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc
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
Closes #99. Adds a
SigningProviderabstraction toadcp/v3/signingso an AdCP agent's RFC 9421 signing key no longer has to live in process memory — it can stay in AWS KMS / GCP KMS / Azure Key Vault / Vault Transit and only ever leave that boundary as an already-computed signature.This issue got three rounds of maintainer triage (see the issue thread) with specific, binding design constraints before it was marked implementation-ready. This PR follows that spec precisely — see "Design constraints from the triage thread" below for how each one was resolved, including the one left explicitly open for the PR author (
PublicKey()on the interface).Non-breaking
SignerOptions.KeyID+PrivateKeykeep working exactly as before —NewSignerwraps them internally in a newInMemorySigningProvider. The newSignerOptions.Providerfield is the alternative path. No existing caller (including theadcp/v3/webhookpackage, which also callssigning.NewSigner) needed a single-line change; the full existing test suite passes untouched.SignRequestnow runs the signing operation againstr.Context()instead of a context-free helper — this applies to both the directSignRequestcall and theRoundTripper/signingTransport.RoundTrippath (the issue/triage only called out the RoundTrip path explicitly; this PR covers both sinceSignRequestis the one place that actually signs).Design constraints from the triage thread
The issue's three-round triage (2026-04-25 → 2026-04-28) flagged the original proposal's
Algorithm() stringas a wire-breaking blocker and left one design question open for whoever picked this up. Resolved here:Algorithm()returnssigning.Algorithm, notstring. It's the RFC 9421algsig-param wire value ("ed25519"/"ecdsa-p256-sha256"), validated againstAlgorithm.Allowed()inNewSignerat construction time, not per-request.Error()unwrapped.SigningProvider.Sign/PublicKeyimplementations should return*signing.SigningError—Codeis a stable, loggable string; the raw backend SDK error (which can embed KMS key ARNs, GCP resource paths, Vault mount paths) is reachable only viaerrors.Unwrap/errors.As, never viaDetailorError().adcp/v3/signing/awskmsfollows this for every error path, with a dedicated test (TestSignPropagatesKMSError) asserting the raw AWS error text never appears inError().NewSignerintegration is exactly the spec from the triage thread:SignerOptions.Provider SigningProvider,NewSigneracceptsProvider != nil || PrivateKey != niland short-circuits whenProvideris set.Algorithm.Allowed(), not implemented.r.Context()threading — done, and extended to coverSignRequestdirectly, not only theRoundTripperpath the triage comment named.tmproto/signing.gountouched — zero-dep root-module path, correctly out of scope.KeyID()stability contract documented on the interface.PublicKey(ctx) (crypto.PublicKey, error)on the interface — resolved: yes, added. The triage thread flagged this as a "resolve before freezing the interface, adding it later is breaking" decision left to the PR. Adding it now unlocks two safeguards the maintainer's own comment asked for ifPublicKey()shipped:NewPublicJWKFromProvider(ctx, provider, kid, adcpUse)— builds thejwks_uripublication JWK straight from a provider instead of hand-assemblingkty/crv/alg(a real source of drift per the triage comment).adcpUseis a required parameter, not defaulted, because the spec requires distinct key material per signing purpose (spec(webhooks): unify webhook signing on RFC 9421 profile adcp#2423). Tested against the real verifier, including a negative case proving a JWK published under the wrongadcp_useis correctly rejected (CodeKeyPurposeInvalid) — the same failure class as spec conformance vector008-wrong-adcp-use.AssertProviderPublicKeyMatchesSPKI(ctx, provider, expectedSPKI)— a startup tripwire: pin the expected SPKI bytes alongside deployed code, call once after the listener binds, fail loudly if a managed key store silently rotated the key backing akidinstead of quietly signing with a key no verifier will accept.SigningProvidersupport, and the PostgresReplayStoregap (already being addressed by a parallel PR against signing: Postgres ReplayStore reference adapter (adcp/signing/pgreplay) #54/feat(signing): add Postgres-backed ReplayStore for distributed verifier deployments #105).AWS KMS: dependency isolation
Per
AGENTS.md's "Zero unnecessary dependencies" rule (root module has zero deps; sub-modules add deps only where needed) and the issue's own framing (a real cloud KMS SDK is unavoidably heavy), the AWS KMS provider ships as its own Go module:adcp/v3/signing/awskms, with its owngo.modpulling inaws-sdk-go-v2/service/kms. This mirrorsregistry/redisstoreandregistry/glidestore, which isolatego-redis/glidethe same way from the dependency-freeregistrymodule.adcp/v3/signingitself gained zero new imports — confirmed bygo build ./.../go vet ./...at the repo root and inadcp/v3with nogo.moddiff there.The provider only supports ECDSA-P256 (
AlgES256): AWS KMS has no Ed25519/EdDSA asymmetric signing key spec as of this writing, only RSA and NIST/SECG curves, soAlgorithm()always returnsAlgES256. It signs a locally-computed SHA-256 digest via KMS'sSignAPI (MessageType=DIGEST,SigningAlgorithm=ECDSA_SHA_256) and converts the ASN.1 DER-encoded response into the fixed-width 64-byte IEEE P1363 (r||s) encoding the AdCP profile'sSignatureheader requires — the same conversionInMemorySigningProvider's ECDSA path does locally, verified byte-for-byte against a real key in tests.PublicKeyfetches via KMS'sGetPublicKeyand caches only a successful result (a mutex-guarded cache, notsync.Once) — per a production-incident lesson cited directly in the issue's triage thread: eagerly calling KMS before a listener binds can hang process startup indefinitely on the AWS SDK retryer's backoff with no visible error, andsync.Oncewould permanently poison the cache on one transient failure.New()itself never calls KMS.What's tested vs. what requires live AWS (honest accounting)
Tested, real, no live AWS needed (10 tests,
-raceclean):Providerdepends onSignAPI— a two-method interface (Sign,GetPublicKey) matching the exact subset of*kms.Clientit calls, satisfiable by*kms.Clientitself in production and by a fake in tests — the pattern the AWS SDK for Go v2 documents for unit-testing service clients. Against that fake:Signsends KMS (KeyId,MessageType=DIGEST,SigningAlgorithm=ECDSA_SHA_256, and a SHA-256 digest of the payload — not the raw payload);ecdsa.PrivateKey/ecdsa.Verify;GetPublicKey→x509.ParsePKIXPublicKeymapping;TestPublicKeyFetchesAndCaches,TestPublicKeyRetriesAfterFailure);*signing.SigningErrorcontract on every failure path (KMS error, mismatched response algorithm, malformed DER,GetPublicKeyfailure) — including the explicit "raw KMS error text never appears inError()" assertion;awskms.Provider→signing.NewSigner→SignRequest→NewPublicJWKFromProvider→signing.VerifyRequest, against a fake KMS backend that actually signs with a real ECDSA key (not canned bytes), so the whole chain is a genuine cryptographic round trip.Not tested here, and said so plainly rather than overclaiming: an actual round trip against live AWS KMS — IAM permissions, key policy, throttling/retry behavior under real network conditions, and confirming KMS's actual
GetPublicKey/Signresponse encodings match what this package assumes (RFC 3279 §2.2.3 DER for the signature, X.509 SubjectPublicKeyInfo for the public key — both per AWS's own API documentation, but not independently verified against a live key in this PR). That requires an AWS account and is out of scope for an automated test suite;adcp/v3/signing/awskms's package doc describes the manual verification recipe (wire a realkms.NewFromConfigclient into the same code path the tests exercise against the fake, sign, then verify withsigning.VerifyRequestSignature— a successful verify is the end-to-end proof).Known bootstrap gap (disclosed)
adcp/v3/signing/awskms/go.modrequiresgithub.com/adcontextprotocol/adcp-go/adcp/v3at the currently-publishedv3.0.0tag — which predatesSigningProvider(added in this same PR). This meansgo buildin theawskmsmodule against a fresh module-mode checkout won't resolvesigning.SigningProvideruntil a newadcp/v3tag is cut post-merge (at which point bumping therequireline is a trivial follow-up, matching howregistry/redisstore/registry/glidestorealready pin their parent module).I verified
go build/go vet/go test -raceall pass forawskmslocally viacp go.work.example go.work(this repo's documented mechanism for cross-module development against unreleased local changes) — not glossing over this, just disclosing it plainly.adcp/v3/signing/awskmsis added togo.work.example'suse()block and passesscripts/check-goworkexample.sh. I did not wire a CI step for this new module: doing so today would require either exercising an untested workspace-mode CI mechanism (andgo.work.example's owngo 1.26.2directive is already lower than some sibling modules'go 1.27.0requirement — a pre-existing, unrelated issue I hit and worked around locally but didn't fix here) or accepting a step that fails until the tag bump lands. Once a newadcp/v3tag ships, a completely ordinarycd adcp/v3/signing/awskms && go test ./...CI step — no workspace mode needed — becomes trivial to add, exactly matching every other module's CI step.Test plan
go build ./.../go vet ./...— repo root: clean, no new deps (confirmed viagit diff go.mod go.sum— no diff)go build ./.../go vet ./.../go test -race -count=1 ./...—adcp/v3module: clean, full existing suite (includingadcp/v3/webhook, which also callssigning.NewSigner) passes untouchedgo build ./.../go vet ./.../go test -race -count=1 ./...—adcp/v3/signing/awskmsmodule (viago.work, see above): clean, 10/10 new tests passgolangci-lint run ./...—adcp/v3(0 new issues; 3 pre-existingstaticcheckfindings injwk.go, unrelated to this PR, from a newer Go 1.26 deprecation the repo hasn't addressed yet) andadcp/v3/signing/awskms(0 issues)gofmt -lclean on every changed/new filebash scripts/check-goworkexample.shpasses after adding the new moduleFiles
adcp/v3/signing/provider.go—SigningProvider,InMemorySigningProvider,NewPublicJWKFromProvider,AssertProviderPublicKeyMatchesSPKIadcp/v3/signing/errors.go—SigningError,SigningErrorCodeadcp/v3/signing/sign.go—Signer/NewSigner/SignRequestrefactored ontoSigningProvideradcp/v3/signing/provider_test.go,adcp/v3/signing/awskms/provider_test.go— new testsadcp/v3/signing/awskms/— new module (provider, doc, go.mod/go.sum)adcp/v3/signing/README.md,MIGRATION.md,doc.go, rootREADME.md,go.work.example— docs + module registration🤖 Generated with Claude Code
https://claude.ai/code/session_01NFth3sHBrJbHuFeA8Cegdc