Skip to content

Commit df383ff

Browse files
vvillait88claude
vvillait88
and
claude
committed
hardening: round-2 reviewer findings (security + test gaps)
Mirrors node-commerce hardening for cross-language parity. - verify_ucp_profile: wrap UnsupportedHeaderError (RFC 7515 §4.1.11 unrecognized `crit`) into UCPVerificationError('unrecognized_critical_header'). - verify_ucp_profile: malformed JWKS shape (non-dict, missing keys array) now emits UCPVerificationError('malformed_jwks') instead of cryptic AttributeError. Same for non-dict signed_profile input. - verify_ucp_profile: reject JWKs with use != 'sig' as 'unusable_key' (RFC 7517 §4.2). - sign_ucp_profile: enforce kid in profile.signing_keys[] at sign-time. - UCPSigningKey.from_jwk: reject `oct` symmetric keys + missing `kid`/`kty` with typed ValueError instead of bare KeyError. - Drop 3 redundant `del joserfc` statements. - Cross-language fixture corpus: 6 → 12 fixtures (minimal, ES256-rails, extras-int, capability, unicode, multi-key per language). - Fix vacuous key-order-independence test (json.dumps preserves order on Python 3.7+); now hand-constructs reverse insertion-order dict. - README KMS claim rewritten — `OKPKey`/`ECKey` import key MATERIAL, they don't wrap remote signers. Tests: 773 pass, 3 skipped. ruff + ty clean. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent 7e6d6ed commit df383ff

11 files changed

Lines changed: 501 additions & 19 deletions

File tree

‎README.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ jwks = build_jwks_response([key.public_jwk])
230230

231231
`sign_ucp_profile` rejects profiles containing `float` values: cross-language float canonicalization is not stable, so use decimal strings (e.g. `"9.99"`) for any monetary or fractional fields you put in `extras`.
232232

233-
**HSM / KMS-backed signing.** `signing_key` accepts any joserfc `Key` subclass — including remote signers wrapped via `joserfc.jwk.OKPKey`/`ECKey`. The signing key never has to leave the HSM.
233+
**Persisting the private JWK.** Mint once via `generate_ucp_signing_key()`, serialize via `key.private_key.as_dict(private=True)`, store in your secret manager. On each container start, read the secret, `OKPKey.import_key(jwk_dict)` (or `ECKey.import_key` for ES256) to re-hydrate. Remote-signer flows (KMS-backed asymmetric keys) require subclassing the joserfc Key to delegate the sign hook; `OKPKey`/`ECKey` themselves only carry local key material.
234234

235235
**Key rotation.** Mint a new key with a new `kid`, add the public JWK to your JWKS endpoint alongside the old one, then sign new profiles with the new key. Drop the old JWK after your verifier-side cache TTL has elapsed.
236236

‎agentscore_commerce/identity/ucp.py‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,28 @@ def from_jwk(cls, jwk: dict[str, Any]) -> UCPSigningKey:
6868
Routes the JWK's known fields (kid/kty/alg/use/crv) onto the dataclass and
6969
captures any other fields (x/y/n/e/etc.) into ``extras``. Use this when
7070
publishing the output of :func:`generate_ucp_signing_key` directly.
71+
72+
Rejects symmetric (``oct``) keys and JWKs missing required fields with a
73+
typed ``ValueError`` rather than a bare ``KeyError``.
7174
"""
75+
if not isinstance(jwk, dict):
76+
msg = f"UCPSigningKey.from_jwk expected a dict; got {type(jwk).__name__}."
77+
raise ValueError(msg)
78+
if "kid" not in jwk:
79+
msg = "UCPSigningKey.from_jwk: JWK missing required field `kid`."
80+
raise ValueError(msg)
81+
if "kty" not in jwk:
82+
msg = "UCPSigningKey.from_jwk: JWK missing required field `kty`."
83+
raise ValueError(msg)
84+
if jwk["kty"] not in {"OKP", "EC", "RSA"}:
85+
msg = (
86+
f"UCPSigningKey.from_jwk: kty={jwk['kty']!r} is not a supported "
87+
"asymmetric key type (expected OKP, EC, or RSA). Symmetric `oct` "
88+
"keys are rejected because they cannot publicly verify a JWS in "
89+
"the trust-mode UCP flow."
90+
)
91+
raise ValueError(msg)
92+
7293
known = {"kid", "kty", "alg", "use", "crv"}
7394
return cls(
7495
kid=jwk["kid"],

‎agentscore_commerce/identity/ucp_jwks.py‎

Lines changed: 55 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,9 @@ def __init__(
5656
"signature_invalid",
5757
"body_mismatch",
5858
"malformed_jws",
59+
"malformed_jwks",
60+
"unrecognized_critical_header",
61+
"unusable_key",
5962
],
6063
message: str,
6164
) -> None:
@@ -105,7 +108,7 @@ def generate_ucp_signing_key(*, kid: str, alg: Literal["EdDSA", "ES256"] = "EdDS
105108
# key.private_key — persist securely
106109
# key.public_jwk — publish at /.well-known/jwks.json
107110
"""
108-
joserfc = _load_joserfc()
111+
_load_joserfc()
109112

110113
if alg == "EdDSA":
111114
from joserfc.jwk import OKPKey # type: ignore[import-not-found]
@@ -125,8 +128,6 @@ def generate_ucp_signing_key(*, kid: str, alg: Literal["EdDSA", "ES256"] = "EdDS
125128
public_jwk.setdefault("alg", alg)
126129
public_jwk.setdefault("use", "sig")
127130

128-
del joserfc
129-
130131
return GeneratedUCPKey(private_key=priv, public_jwk=public_jwk)
131132

132133

@@ -198,19 +199,30 @@ def sign_ucp_profile(
198199
profile = build_ucp_profile(..., signing_keys=[UCPSigningKey(**key.public_jwk)])
199200
signed = sign_ucp_profile(profile.to_dict(), signing_key=key.private_key, kid='merchant-2026-05')
200201
"""
201-
joserfc = _load_joserfc()
202+
_load_joserfc()
202203
from joserfc import jws # type: ignore[import-not-found]
203204
from joserfc.jws import JWSRegistry # type: ignore[import-not-found]
204205

206+
# Sign-time kid sanity check: the profile's `signing_keys[]` MUST contain
207+
# a JWK with the matching kid; otherwise verifiers can't resolve the
208+
# public key and the profile is dead-on-arrival.
209+
declared_kids = [
210+
k.get("kid") if isinstance(k, dict) else getattr(k, "kid", None) for k in profile.get("signing_keys", [])
211+
]
212+
if kid not in declared_kids:
213+
msg = (
214+
f"sign_ucp_profile: kid {kid!r} is not present in profile.signing_keys[] "
215+
f"(declared kids: {declared_kids!r}). Verifiers will not find the key."
216+
)
217+
raise ValueError(msg)
218+
205219
canonical_body = _canonicalize_profile(profile)
206220
header = {"alg": alg, "kid": kid, "typ": _UCP_TYP}
207221
# joserfc treats EdDSA as "not recommended" by default; UCP §6 explicitly accepts
208222
# both EdDSA and ES256, so allow both.
209223
registry = JWSRegistry(algorithms=list(_ALLOWED_ALGS))
210224
signature = jws.serialize_compact(header, canonical_body, signing_key, registry=registry)
211225

212-
del joserfc
213-
214226
return {**profile, "signature": signature}
215227

216228

@@ -251,11 +263,25 @@ def verify_ucp_profile(
251263
252264
ok = verify_ucp_profile(signed, build_jwks_response([key.public_jwk]))
253265
"""
254-
joserfc = _load_joserfc()
266+
_load_joserfc()
255267
from joserfc import jws # type: ignore[import-not-found]
256268
from joserfc.jwk import KeySet # type: ignore[import-not-found]
257269
from joserfc.jws import JWSRegistry # type: ignore[import-not-found]
258270

271+
# JWKS shape guard so a malformed argument emits a typed UCPVerificationError
272+
# rather than a confusing kid_not_found / AttributeError.
273+
if not isinstance(jwks, dict) or not isinstance(jwks.get("keys"), list):
274+
raise UCPVerificationError(
275+
"malformed_jwks",
276+
f"UCP verifier expected JWKS shape {{'keys': [...]}}; got {type(jwks).__name__}.",
277+
)
278+
279+
if not isinstance(signed_profile, dict):
280+
raise UCPVerificationError(
281+
"no_signature",
282+
f"UCP verifier expected a profile dict; got {type(signed_profile).__name__}.",
283+
)
284+
259285
sig = signed_profile.get("signature")
260286
if not sig:
261287
raise UCPVerificationError(
@@ -294,6 +320,13 @@ def verify_ucp_profile(
294320
"duplicate_kid",
295321
f"JWKS contains {len(matches)} keys with kid={kid!r}; expected exactly one.",
296322
)
323+
# RFC 7517 §4.2: reject keys not intended for signature verification.
324+
matched_use = matches[0].get("use")
325+
if matched_use is not None and matched_use != "sig":
326+
raise UCPVerificationError(
327+
"unusable_key",
328+
f"JWK with kid={kid!r} has use={matched_use!r}; expected 'sig'.",
329+
)
297330

298331
stripped = {k: v for k, v in signed_profile.items() if k != "signature"}
299332
expected_payload = _canonicalize_profile(stripped)
@@ -303,14 +336,26 @@ def verify_ucp_profile(
303336
try:
304337
obj = jws.deserialize_compact(sig, key_set, registry=registry)
305338
except Exception as exc:
306-
# joserfc raises various subclasses (BadSignatureError, DecodeError, ...).
307-
# Wrap in our own type so callers don't need to import joserfc internals.
308-
from joserfc.errors import BadSignatureError, DecodeError # type: ignore[import-not-found]
339+
# joserfc raises various subclasses. Wrap in our own type so callers
340+
# don't need to import joserfc internals.
341+
from joserfc.errors import ( # type: ignore[import-not-found]
342+
BadSignatureError,
343+
DecodeError,
344+
UnsupportedHeaderError,
345+
)
309346

310347
if isinstance(exc, BadSignatureError):
311348
raise UCPVerificationError("signature_invalid", f"UCP signature verification failed: {exc}") from exc
312349
if isinstance(exc, DecodeError):
313350
raise UCPVerificationError("malformed_jws", f"Malformed JWS: {exc}") from exc
351+
# RFC 7515 §4.1.11 / RFC 8725 §3.10: a verifier MUST reject any JWS
352+
# whose `crit` header carries an extension the implementation doesn't
353+
# understand.
354+
if isinstance(exc, UnsupportedHeaderError):
355+
raise UCPVerificationError(
356+
"unrecognized_critical_header",
357+
f"UCP signing rejected unrecognized critical header: {exc}",
358+
) from exc
314359
raise
315360

316361
# Compare the bytes that were actually signed against the canonical body of the
@@ -323,7 +368,6 @@ def verify_ucp_profile(
323368
"UCP profile body does not match the signed payload (tampered or non-canonical).",
324369
)
325370

326-
del joserfc
327371
return True
328372

329373

‎tests/fixtures/cross-lang/node-capability.json‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
{
2+
"profile": {
3+
"version": "2026-04-17",
4+
"spec": "https://ucp.dev/",
5+
"services": [
6+
{
7+
"type": "rest",
8+
"url": "https://c.example.com"
9+
}
10+
],
11+
"capabilities": [
12+
{
13+
"name": "agentscore-identity",
14+
"schema": "https://agentscore.sh/schema/identity/1",
15+
"version": "1",
16+
"kyc_required": true
17+
}
18+
],
19+
"payment_handlers": [
20+
{
21+
"name": "tempo",
22+
"config": {
23+
"rail": "tempo-mainnet",
24+
"chain_id": 4217
25+
}
26+
}
27+
],
28+
"signing_keys": [
29+
{
30+
"kid": "node-capability-EdDSA",
31+
"alg": "EdDSA",
32+
"use": "sig",
33+
"crv": "Ed25519",
34+
"kty": "OKP",
35+
"x": "8zz-L1N_SZ0EUmciU1IzuxBuGd67MSg-OemKm6ofmgg"
36+
}
37+
],
38+
"name": "Capability Merchant",
39+
"signature": "eyJhbGciOiJFZERTQSIsImtpZCI6Im5vZGUtY2FwYWJpbGl0eS1FZERTQSIsInR5cCI6InVjcC1wcm9maWxlK2p3cyJ9.eyJjYXBhYmlsaXRpZXMiOlt7Imt5Y19yZXF1aXJlZCI6dHJ1ZSwibmFtZSI6ImFnZW50c2NvcmUtaWRlbnRpdHkiLCJzY2hlbWEiOiJodHRwczovL2FnZW50c2NvcmUuc2gvc2NoZW1hL2lkZW50aXR5LzEiLCJ2ZXJzaW9uIjoiMSJ9XSwibmFtZSI6IkNhcGFiaWxpdHkgTWVyY2hhbnQiLCJwYXltZW50X2hhbmRsZXJzIjpbeyJjb25maWciOnsiY2hhaW5faWQiOjQyMTcsInJhaWwiOiJ0ZW1wby1tYWlubmV0In0sIm5hbWUiOiJ0ZW1wbyJ9XSwic2VydmljZXMiOlt7InR5cGUiOiJyZXN0IiwidXJsIjoiaHR0cHM6Ly9jLmV4YW1wbGUuY29tIn1dLCJzaWduaW5nX2tleXMiOlt7ImFsZyI6IkVkRFNBIiwiY3J2IjoiRWQyNTUxOSIsImtpZCI6Im5vZGUtY2FwYWJpbGl0eS1FZERTQSIsImt0eSI6Ik9LUCIsInVzZSI6InNpZyIsIngiOiI4enotTDFOX1NaMEVVbWNpVTFJenV4QnVHZDY3TVNnLU9lbUttNm9mbWdnIn1dLCJzcGVjIjoiaHR0cHM6Ly91Y3AuZGV2LyIsInZlcnNpb24iOiIyMDI2LTA0LTE3In0.YmiTy87alEbVfAEXYzXYkBrsbO_kHqgTSlv3gKuzy6Oere-pJl0PmZ8zGW2uTyjaGC9OFbjLUIzowY3jnJmGAg"
40+
},
41+
"jwks": {
42+
"keys": [
43+
{
44+
"kid": "node-capability-EdDSA",
45+
"alg": "EdDSA",
46+
"use": "sig",
47+
"crv": "Ed25519",
48+
"kty": "OKP",
49+
"x": "8zz-L1N_SZ0EUmciU1IzuxBuGd67MSg-OemKm6ofmgg"
50+
}
51+
]
52+
},
53+
"alg": "EdDSA",
54+
"kid": "node-capability-EdDSA",
55+
"generator": "node"
56+
}

‎tests/fixtures/cross-lang/node-multikey.json‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,64 @@
1+
{
2+
"profile": {
3+
"version": "2026-04-17",
4+
"spec": "https://ucp.dev/",
5+
"services": [
6+
{
7+
"type": "rest",
8+
"url": "https://mk.example.com"
9+
}
10+
],
11+
"capabilities": [],
12+
"payment_handlers": [
13+
{
14+
"name": "tempo",
15+
"config": {
16+
"rail": "tempo-mainnet"
17+
}
18+
}
19+
],
20+
"signing_keys": [
21+
{
22+
"kid": "node-multikey-old",
23+
"alg": "EdDSA",
24+
"use": "sig",
25+
"crv": "Ed25519",
26+
"kty": "OKP",
27+
"x": "qSF9p7IJIFIKxoFl6Od1G8qj65Prx35EnN44zMxJs6U"
28+
},
29+
{
30+
"kid": "node-multikey-new",
31+
"alg": "EdDSA",
32+
"use": "sig",
33+
"crv": "Ed25519",
34+
"kty": "OKP",
35+
"x": "9Al1EZLHjgl02MWGtIGaStOPnR9cBc0WXNYuGbU5r-g"
36+
}
37+
],
38+
"name": "Multi-Key Merchant",
39+
"signature": "eyJhbGciOiJFZERTQSIsImtpZCI6Im5vZGUtbXVsdGlrZXktbmV3IiwidHlwIjoidWNwLXByb2ZpbGUrandzIn0.eyJjYXBhYmlsaXRpZXMiOltdLCJuYW1lIjoiTXVsdGktS2V5IE1lcmNoYW50IiwicGF5bWVudF9oYW5kbGVycyI6W3siY29uZmlnIjp7InJhaWwiOiJ0ZW1wby1tYWlubmV0In0sIm5hbWUiOiJ0ZW1wbyJ9XSwic2VydmljZXMiOlt7InR5cGUiOiJyZXN0IiwidXJsIjoiaHR0cHM6Ly9tay5leGFtcGxlLmNvbSJ9XSwic2lnbmluZ19rZXlzIjpbeyJhbGciOiJFZERTQSIsImNydiI6IkVkMjU1MTkiLCJraWQiOiJub2RlLW11bHRpa2V5LW9sZCIsImt0eSI6Ik9LUCIsInVzZSI6InNpZyIsIngiOiJxU0Y5cDdJSklGSUt4b0ZsNk9kMUc4cWo2NVByeDM1RW5ONDR6TXhKczZVIn0seyJhbGciOiJFZERTQSIsImNydiI6IkVkMjU1MTkiLCJraWQiOiJub2RlLW11bHRpa2V5LW5ldyIsImt0eSI6Ik9LUCIsInVzZSI6InNpZyIsIngiOiI5QWwxRVpMSGpnbDAyTVdHdElHYVN0T1BuUjljQmMwV1hOWXVHYlU1ci1nIn1dLCJzcGVjIjoiaHR0cHM6Ly91Y3AuZGV2LyIsInZlcnNpb24iOiIyMDI2LTA0LTE3In0.jXm8ZRUWa9_BUYfSv_PCJNqIWbAYf39DUwOdqvExMPTLWDoDzNAwoIleWfyiAGXMOyK0J-0DPeeCFTzmOPMnBQ"
40+
},
41+
"jwks": {
42+
"keys": [
43+
{
44+
"kid": "node-multikey-old",
45+
"alg": "EdDSA",
46+
"use": "sig",
47+
"crv": "Ed25519",
48+
"kty": "OKP",
49+
"x": "qSF9p7IJIFIKxoFl6Od1G8qj65Prx35EnN44zMxJs6U"
50+
},
51+
{
52+
"kid": "node-multikey-new",
53+
"alg": "EdDSA",
54+
"use": "sig",
55+
"crv": "Ed25519",
56+
"kty": "OKP",
57+
"x": "9Al1EZLHjgl02MWGtIGaStOPnR9cBc0WXNYuGbU5r-g"
58+
}
59+
]
60+
},
61+
"alg": "EdDSA",
62+
"kid": "node-multikey-new",
63+
"generator": "node"
64+
}

‎tests/fixtures/cross-lang/node-unicode.json‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
{
2+
"profile": {
3+
"version": "2026-04-17",
4+
"spec": "https://ucp.dev/",
5+
"services": [
6+
{
7+
"type": "rest",
8+
"url": "https://日本.example.com"
9+
}
10+
],
11+
"capabilities": [],
12+
"payment_handlers": [
13+
{
14+
"name": "tempo",
15+
"config": {
16+
"note": "メモ"
17+
}
18+
}
19+
],
20+
"signing_keys": [
21+
{
22+
"kid": "node-unicode-EdDSA",
23+
"alg": "EdDSA",
24+
"use": "sig",
25+
"crv": "Ed25519",
26+
"kty": "OKP",
27+
"x": "mxtclpNy58uer_3ivEk9HfPp5_6zXtYUpc_ItTLz0sA"
28+
}
29+
],
30+
"name": "Café 日本 🍷 Merchant",
31+
"signature": "eyJhbGciOiJFZERTQSIsImtpZCI6Im5vZGUtdW5pY29kZS1FZERTQSIsInR5cCI6InVjcC1wcm9maWxlK2p3cyJ9.eyJjYXBhYmlsaXRpZXMiOltdLCJuYW1lIjoiQ2Fmw6kg5pel5pysIPCfjbcgTWVyY2hhbnQiLCJwYXltZW50X2hhbmRsZXJzIjpbeyJjb25maWciOnsibm90ZSI6IuODoeODoiJ9LCJuYW1lIjoidGVtcG8ifV0sInNlcnZpY2VzIjpbeyJ0eXBlIjoicmVzdCIsInVybCI6Imh0dHBzOi8v5pel5pysLmV4YW1wbGUuY29tIn1dLCJzaWduaW5nX2tleXMiOlt7ImFsZyI6IkVkRFNBIiwiY3J2IjoiRWQyNTUxOSIsImtpZCI6Im5vZGUtdW5pY29kZS1FZERTQSIsImt0eSI6Ik9LUCIsInVzZSI6InNpZyIsIngiOiJteHRjbHBOeTU4dWVyXzNpdkVrOUhmUHA1XzZ6WHRZVXBjX0l0VEx6MHNBIn1dLCJzcGVjIjoiaHR0cHM6Ly91Y3AuZGV2LyIsInZlcnNpb24iOiIyMDI2LTA0LTE3In0.21NUepmkXaXs6cRnPBgheUR0F7EoqysnAkOEqiaqZEG8OEGMkegGsVeEvtaxEjpQZfC4KAeTqvjy6Vc-FSDzBg"
32+
},
33+
"jwks": {
34+
"keys": [
35+
{
36+
"kid": "node-unicode-EdDSA",
37+
"alg": "EdDSA",
38+
"use": "sig",
39+
"crv": "Ed25519",
40+
"kty": "OKP",
41+
"x": "mxtclpNy58uer_3ivEk9HfPp5_6zXtYUpc_ItTLz0sA"
42+
}
43+
]
44+
},
45+
"alg": "EdDSA",
46+
"kid": "node-unicode-EdDSA",
47+
"generator": "node"
48+
}

0 commit comments

Comments
 (0)