feat(signing): ship signing/signingtest subpackage + ObserveOnly mode - #479
Open
sujanchalla0510 wants to merge 1 commit into
Open
Conversation
Every consumer writing a handler test that expects signed requests had to reverse-engineer the ~30-line keypair/JWK/StaticJWKSResolver/replay-store pattern in middleware_test.go. signingtest.NewTestAgent + SignAndSend collapse that into a two-line setup and a one-line send. Separately, MiddlewareOptions.ObserveOnly implements the spec's warn_for shadow-mode rollout stop (between supported_for and required_for): verification still runs, but a failing request reaches next.ServeHTTP with no VerifiedSigner in its context instead of getting a 401, and the failure is logged at INFO. A partial or malformed Signature/Signature-Input pair still hard-rejects even under ObserveOnly, per the spec's explicit carve-out that such a pair can't be safely read as signed or unsigned traffic. MIGRATION.md's step-B guidance, which previously described this as a not-yet-landed OnReject shim, is updated to use the real API. Built against adcp/v3/signing — the active module per README's "Modules & versioning" table; adcp/signing (v2) is frozen for security backports only, so this feature does not touch it. Closes adcontextprotocol#53
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 #53.
Summary
adcp/v3/signing/signingtest—NewTestAgent(t)builds a matched Ed25519*signing.Signer+signing.MiddlewareOptions(a freshStaticJWKSResolverseeded with the signer's public key, a freshNewMemoryReplayStore(0), no revocation).SignAndSend(t, signer, handler, req)signsreq(withCoverContentDigest: true) and delivers it tohandlerin-process viahttptest.NewRecorder, returning the*http.Response. Together these collapse the ~30-line keypair/JWK/resolver/replay-store patternmiddleware_test.gocurrently hand-rolls into two lines of setup + one line to send.MiddlewareOptions.ObserveOnly bool— a real behavioral branch inMiddleware(), not a passthrough field. When true,VerifyRequestSignaturestill runs; on failure the request is logged atslog.LevelInfo(vs the normalslog.LevelWarn) with anobserve_only=trueattribute and passed tonext.ServeHTTPwith noVerifiedSignerin its context — i.e., treated exactly as unsigned. On success, behavior is unchanged.ObserveOnly↔ spec mappingThis maps to the AdCP transport spec's
warn_forrollout stop (supported_for → warn_for → required_for, see Transport capability advertisement). Per spec: "A missing signature or a well-formed signature that fails verification or body binding MUST NOT establish verified-signer identity ... A partial or malformed Signature / Signature-Input pair always hard-rejects."ObserveOnlyimplements that split precisely, not just "log and let everything through":RequiredForop, ...) is observed: INFO log, request passes through unauthenticated.Signature/Signature-Inputheader pair — one header present without the other, or either header present but unparseable (*Error{Code: CodeHeaderMalformed}) — still hard-rejects with 401 even underObserveOnly, because the spec says it "cannot be safely interpreted as either signed or unsigned traffic."adcp/v3/signing/MIGRATION.md's "Step B —warn_for" section previously described this exact mode as not yet landed, with anOnReject-based logs-and-passes-through shim as the interim workaround (and a "delete this before enablingRequiredFor" warning). This PR replaces that shim guidance with the realObserveOnlyAPI throughout the migration guide (bootstrap → step B → common pitfalls → pre-enforcement checklist).Module note
This targets
adcp/v3/signing, notadcp/signing. Per the repo'sREADME.md"Modules & versioning" table andMIGRATING.md,adcp(the pre-v3 module) is frozen at v2.1.1 and receives security backports only —adcp/v3is the actively developed module for AdCP 3.x. A new DX/feature package belongs there, soadcp/signing(the frozen copy) is untouched by this PR.Testing
signingtest's own tests (adcp/v3/signing/signingtest/signingtest_test.go) proveNewTestAgent+SignAndSendproduce a request a realsigning.Middleware-wrapped handler accepts (VerifiedSignerFromContextpopulated, correct algorithm), that the wired verifier is real and not a stub (rejects unsigned traffic on aRequiredForop, rejects a replayed signature), and thatSignAndSendfails fast on a non-absolute request URL (verified via a re-exec'd subprocess, since a subtest'st.Fatalfcan't be observed without failing the parent test/package).middleware_test.gogets three new tests:ObserveOnlylets an unsigned request to aRequiredForoperation through (INFO log, noVerifiedSigner);ObserveOnlylets a well-formed-but-cryptographically-invalid signature through, with a matchingObserveOnly=falsesubtest proving the existing 401 behavior is unchanged; andObserveOnlystill hard-rejects a malformedSignature/Signature-Inputpair (WARN log, 401,WWW-Authenticate: ... request_signature_header_malformed), proving the spec carve-out is real.go build ./...,go vet ./...,gofmt -l, andgolangci-lint run ./signing/...are clean for every file this PR touches. The threestaticcheckfindingsgolangci-lintreports inadcp/v3/signing/jwk.go(deprecatedecdsa.PublicKey.X/.Y/.PrivateKey.Dfield access, Go 1.26) are pre-existing onmainand untouched by this PR.go test ./...passes for both theadcp/v3module (this change) and the root module (unaffected).On item 5 of the issue ("refactor an existing boilerplate-heavy test as a demonstration")
I looked for one and came up empty, honestly reported: the request-signing profile's
StaticJWKSResolver/NewMemoryReplayStoreboilerplate pattern this issue targets appears nowhere else in the repo outsideadcp/signingandadcp/v3/signingthemselves.adcp/v3/webhook's test files (signing_test.go,publisher_test.go,e2e_test.go) use the webhook-signing profile via a different, already-existing dedicated helper (webhookKeypair) and a different verifier entry point (webhook.HTTPHandler, notsigning.Middleware) — not a good refactor target forsigningtest, which is scoped to the request-signing/signing.Middlewarepath the issue describes.I also couldn't refactor
adcp/v3/signing/middleware_test.go's ownTestMiddlewareEndToEndSignAndVerify(the literal ~30-line pattern quoted in the issue) in place: that file ispackage signing(internal, white-box), andsigningtestimportssigning— importingsigningtestfrom insidepackage signingwould be a cyclic import. Instead,signingtest's ownTestNewTestAgentSignAndSendRoundTripreproduces the same scenario (sign, verify via middleware, assert onVerifiedSignerFromContext) using the new two-line + one-line helpers, as the concrete demonstration of the line-count reduction, and the package'sREADME.md/doc.gocross-reference it for discoverability.Test plan
cd adcp/v3 && go build ./... && go vet ./... && go test ./...cd adcp/v3 && golangci-lint run ./signing/...go build ./... && go vet ./... && go test ./...at repo root (unaffected — confirms no accidental frozen-module edits)git statusclean in the working tree aside from the files this PR touches