Repository navigation
feat(signing): accept any well-formed adcp_use purpose in keygen - #1710
Merged
Merged
Conversation
generate_signing_keypair, pem_to_adcp_jwk and adcp-keygen --purpose took a closed Literal["request-signing", "webhook-signing"] and refused every other value, so the SDK could not mint or publish a key for response-signing (the designated-task response JWS profile, core/response-payload-jws-envelope.json), governance-signing (which governance.py already verifies), or a surface the vocabulary does not name yet. The helpers they delegate to already took a str. purpose is now validated by syntax instead of membership: a lowercase hyphenated token of at most 64 characters. Malformed values (empty, upper case, underscores, whitespace, leading/trailing/double hyphens, non-str) still raise ValueError, and the CLI exits 2 before writing a file. An open vocabulary is safe because purpose matching is fail-closed: a verifier accepts a key only when adcp_use equals the purpose of the surface it verifies, so a key under any other purpose verifies nothing there. A test pins that against the request and webhook verifiers. adcp.signing exports ADCP_USE_REQUEST, ADCP_USE_WEBHOOK (marked deprecated upstream), and the new ADCP_USE_RESPONSE and ADCP_USE_GOVERNANCE, so callers need not retype literals. The docstrings no longer say request- and webhook-signing keys must be distinct for every verifier: 3.3 webhook verifiers accept request-signing keys, and new webhook signers publish request-signing. Backport of #1708 to 9.x. Closes #1677 on main. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![aao-secretariat[bot]](https://avatars.githubusercontent.com/u/221690821?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ladon verdict: Approve
Approve.
Additive change: feat(signing) widens keygen purpose from a closed Literal to a syntactically-validated open token (lowercase hyphenated, ≤64 chars) and adds four ADCP_USE_* constant exports. Correct non-breaking semver prefix for a purely additive public-surface change.
Checks confirmed:
- Open-vocabulary safety holds: verifier.py:849-857 fails closed (
adcp_usemust equal the verified surface's purpose), so a key under any other purpose authorizes nothing. - Validation ordering is safe (non-str short-circuits before regex/length); CLI exits 2 before writing.
- Tests cover open acceptance, malformed rejection, CLI, and the fail-closed property against request and webhook verifiers.
Decision table: no critical/high/medium findings. gated_paths is false; high_risk is true only via (modified) reasons on src/adcp/signing/**, but with no medium-or-higher finding the modifications are presumed safe (rows 4/5 do not fire). No author team gate, no prior escalation. Falls through to row 9 — approve.
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.
Backport of #1708 (into
next) to SDK 9.x. #1677 asked to check whether 9.x needs the same fix. It does:mainhas the same closedLiteral["request-signing", "webhook-signing"]ongenerate_signing_keypair,pem_to_adcp_jwkandadcp-keygen --purpose. The change is additive (Literalwidened tostr), so it isn't breaking.Change
purposeis validated syntactically (a lowercase hyphenated token of at most 64 characters) instead of by membership. That makesresponse-signing,governance-signingand any not-yet-named purpose expressible. Malformed input still raisesValueError, and the CLI exits 2 before writing.adcp.signingexportsADCP_USE_REQUEST,ADCP_USE_WEBHOOK,ADCP_USE_RESPONSEandADCP_USE_GOVERNANCE.Differences from #1708:
main's keygen still uses the hand-rolledcryptographyJWK export (no joserfc), andgovernance.pykeeps its string literal (cosmetic only, left out to keep the backport minimal).Tests
These run against
main:test_keygen_programmatic.py,test_signing_provider.py,test_keygen.py(75 passed). They include the fail-closed property: a key under any non-request purpose is refused by the request and webhook verifiers.Closes #1677
🤖 Generated with Claude Code