Skip to content

feat(signing): SigningProvider Protocol for KMS-backed signing (#283)#323

Merged
bokelley merged 2 commits intomainfrom
bokelley/issue-283
Apr 30, 2026
Merged

feat(signing): SigningProvider Protocol for KMS-backed signing (#283)#323
bokelley merged 2 commits intomainfrom
bokelley/issue-283

Conversation

@bokelley
Copy link
Copy Markdown
Contributor

Closes #283.

Summary

Adds a SigningProvider Protocol so the RFC 9421 request-signing profile no longer requires the private key to live in process memory. Operators using AWS KMS / GCP KMS / Azure Key Vault / Vault Transit can implement the Protocol against their managed key store without forking the signer.

The 2026-04-28 retriage on #283 green-lit this exact scope: Protocol + InMemorySigningProvider + async_sign_request() + pem_to_adcp_jwk + the RFC 8941 key_id escaping fix at signer.py:105.

What's added

  • SigningProvider Protocol (src/adcp/signing/provider.py) — async sign(signature_base: bytes) -> bytes, key_id() -> str, algorithm() -> Literal["ed25519", "ecdsa-p256-sha256"]. Contract docstring covers lazy init, public-key type-checking, rotation tripwire, key separation by adcp_use, and fingerprint-logging redaction.
  • InMemorySigningProvider — default adapter wrapping the existing in-process path. Validates Ed25519 vs. EC, EC curve = SECP256R1, and key_id non-empty at construction (closes the constructor-time alg/key/curve binding gap flagged in original triage and reaffirmed by security-reviewer this round).
  • async_sign_request(provider=...) — async entry point. Shares _prepare_signature with the sync path so both produce byte-identical Signature-Input. Sync sign_request is unchanged.
  • pem_to_adcp_jwk(pem, *, kid, purpose, password=None) — derives the public JWK from a PEM. Accepts PKCS#8 private OR SPKI public PEMs (the latter is what KMS adopters typically have, since the private half never leaves the managed store). Output is byte-shape-identical to generate_signing_keypair's public half, so the spec-required adcp_use claim and JWA alg value are correct by construction.
  • RFC 8941 §3.3.3 escaping fix (signer.py:_escape_sf_string) — applied to keyid, nonce, tag before interpolation into Signature-Input. Rejects anything outside printable ASCII 0x20-0x7E, including CRLF (HTTP-header-injection vector at non-httpx integrators) and lookalike Unicode quotes (parser-divergence vector against Go/JS verifiers).

Reviewer-flagged items addressed

This PR went through code-reviewer, ad-tech-protocol-expert, and security-reviewer. Their feedback is incorporated:

From Item Resolution
code-reviewer Drop temporal/historical framing in module docstring Done — "Adapter contract:" header instead.
code-reviewer Add ECDSA byte-identity test Done — test_sync_and_async_signature_input_identical_for_es256 (signature value skipped due to random k).
code-reviewer Rename payloadsignature_base on Protocol Done.
code-reviewer Match full PEM sentinel -----BEGIN PUBLIC KEY----- instead of substring Done.
security-reviewer _escape_sf_string must reject characters outside §3.3.3 grammar Done — raises ValueError on non-printable-ASCII; tests cover CRLF, NULL, and Unicode quote.
security-reviewer Move alg/key/curve binding check from sign-time to constructor-time on InMemorySigningProvider Done — covers Ed25519 vs. EC mismatch and non-SECP256R1 curve.
ad-tech-protocol-expert Confirm RFC 9421 alg casing, no pre-hash, P1363 encoding, RSA-PSS out of scope, JWK shape parity All confirmed in code. No spec violations found.

Test plan

  • pytest tests/conformance/signing/ — 375 passed, 6 skipped
  • 21 new tests in test_signing_provider.py covering: round-trip async sign+verify (ed25519 + es256), sync/async byte-identity (both algs), RFC 8941 escape with " and \, control-char + non-ASCII rejection, pem_to_adcp_jwk shape parity for both algs, SPKI public-PEM acceptance, RSA rejection, alg/key mismatch and wrong-curve rejection
  • ruff check — clean on changed files
  • mypy src/adcp/signing/ — clean
  • black --check — clean on changed files
  • Pre-commit hooks pass (black, ruff, mypy, bandit, detect-private-key)
  • CI green on PR

Notes

  • No spec change required — the Protocol is purely an SDK concern. AdCP already permits any signing path that produces a verifiable RFC 9421 signature against the keys at jwks_uri.
  • No KMS adapter shipped in this PR. The Protocol is the integration point; adapter implementations belong in adopter projects (or behind opt-in extras in a follow-up). This matches the reviewer guidance from the original triage.

🤖 Generated with Claude Code

bokelley and others added 2 commits April 30, 2026 11:27
Decouples the RFC 9421 request-signing profile from in-process key
storage so KMS / HSM / Vault deployments can plug in without forking
the signer or pulling private material out of the managed store at boot.

* `SigningProvider` Protocol — async `sign(signature_base) -> bytes`,
  `key_id() -> str`, `algorithm() -> Literal["ed25519","ecdsa-p256-sha256"]`.
  Contract docstring covers lazy init, public-key type-checking,
  rotation tripwire, key separation by `adcp_use`, and fingerprint
  redaction.
* `InMemorySigningProvider` — default adapter; validates Ed25519 vs.
  EC, EC curve = SECP256R1, and key_id non-empty at construction.
* `async_sign_request(provider=...)` — async entry point sharing the
  canonicalization spine (`_prepare_signature`) with the sync
  `sign_request`, so both paths produce byte-identical
  Signature-Input.
* `pem_to_adcp_jwk(pem, *, kid, purpose, password=None)` — derives
  the public JWK for KMS adopters whose private half never leaves the
  managed store. Accepts both PKCS#8 private and SPKI public PEMs.
* RFC 8941 §3.3.3 escaping fix at `signer.py:_escape_sf_string` —
  applies to `keyid`, `nonce`, `tag`. Rejects characters outside
  printable ASCII 0x20-0x7E to close header-injection / parser-
  divergence vectors at non-httpx integrators.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `label` kwarg on `sign_request` / `async_sign_request` is a public
input that lands unquoted in both the `Signature-Input` and
`Signature` headers. Without validation, a CRLF or other non-token
character in the label would inject extra header bytes (at non-httpx
integrators that don't sanitize embedded line terminators) or produce
a label that conformant verifiers parse differently from this
serializer.

Add `_validate_sf_key()` enforcing the RFC 8941 §3.1.2 token grammar:
must start with `[a-z*]`, then `[a-z0-9_\-.*]`. Applied at the entry
of `_prepare_signature` so both sync and async signers are covered.

Same parser-divergence / header-injection class as the
`keyid`/`nonce`/`tag` escaping fix already in this PR — just for the
remaining unguarded input.

Also adds the SPKI-public-key RSA rejection test for `pem_to_adcp_jwk`
that was missing from the previous coverage round.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@bokelley bokelley force-pushed the bokelley/issue-283 branch from 7dfa75a to c62a422 Compare April 30, 2026 15:27
@bokelley bokelley merged commit 4de648e into main Apr 30, 2026
11 of 12 checks passed
bokelley added a commit that referenced this pull request Apr 30, 2026
Brings in main's recent work since the foundation branch forked:

- 046b15e / d92cfb1 — PR #321 + #322 storyboard fixes (the same
  fixes my earlier cherry-pick commit 597d00a brought in; main's
  version supersedes since it includes additional fixture-dependent
  failure closures from #322).
- 4de648e — SigningProvider Protocol (PR #283 / #323).
- d3e1a0f — seller_agent PRODUCTS schema regression test.
- ce4c5df — handler-authoring docs expansion (salesagent migration
  patterns).
- e37930a — release 4.2.0.
- ce7e9ab — seed_creative_format scenario + force_session_status.
- 9244d46 — comply_test_controller dict return fix.

Conflicts resolved:

- examples/seller_agent.py — accepted main's version. Main has the
  PR #321 + #322 fixes plus storyboard fixture products
  (outdoor_display_q2, outdoor_video_q2, sports_preroll_q2, etc.)
  added directly to the PRODUCTS list. Foundation's cherry-pick
  commit 597d00a brought in only PR #321; main has the more
  complete state.

- tests/test_seller_agent_storyboard.py — accepted main's version
  (same rationale: more comprehensive coverage from #321 + #322
  combined).

Merged state verified:
- mypy clean across src/adcp/decisioning/ (13 files)
- ruff clean across src/adcp/decisioning/
- Full suite: 2588 passed, 17 skipped, 1 xfailed (+30 from main's
  added tests)

Foundation PR #316 is now up-to-date with main and CI should clear
on the next run.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bokelley added a commit that referenced this pull request Apr 30, 2026
Brings in main's recent work since the foundation branch forked:

- 046b15e / d92cfb1 — PR #321 + #322 storyboard fixes (the same
  fixes my earlier cherry-pick commit 597d00a brought in; main's
  version supersedes since it includes additional fixture-dependent
  failure closures from #322).
- 4de648e — SigningProvider Protocol (PR #283 / #323).
- d3e1a0f — seller_agent PRODUCTS schema regression test.
- ce4c5df — handler-authoring docs expansion.
- e37930a — release 4.2.0.
- ce7e9ab — seed_creative_format scenario + force_session_status.
- 9244d46 — comply_test_controller dict return fix.

Conflicts resolved by accepting main's version for
examples/seller_agent.py and tests/test_seller_agent_storyboard.py
— main has the PR #321 + #322 combined state, more complete than
the foundation's earlier cherry-pick.

Merged state: mypy clean, ruff clean, full suite 2588 passed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add SigningProvider abstraction for external key management (KMS/HSM/Vault)

1 participant