diff --git a/docs/adr/0017-aauth-agent-authentication.md b/docs/adr/0017-aauth-agent-authentication.md index 0b34b4d4ad..1e25523b8e 100644 --- a/docs/adr/0017-aauth-agent-authentication.md +++ b/docs/adr/0017-aauth-agent-authentication.md @@ -197,6 +197,30 @@ the tier-1 SpiceDB layer already uses. Optional tuning when unset, but an unparseable value is still a startup error rather than a silently-ignored default. +### 7. Discovery mode accepts any HTTPS issuer unless an allowlist is pinned + +`iss` is read from an unverified JWT claim in order to locate the key that +will verify it, so in discovery mode it is attacker-influenced input that +drives an outbound fetch. `HttpJwksResolver` bounds the damage structurally: +HTTPS only, DNS hosts only (IP literals and loopback names rejected outright +as SSRF attempts), a request timeout, and a streamed response-size cap that +holds even when `Content-Length` is absent or lies. + +What it does not do by default is bound *which* issuers may be resolved at +all. `A2A_GATEWAY_AAUTH_JWKS_ALLOWED_ISSUERS` takes a comma-separated exact +list (trailing slashes ignored) and rejects any other `iss` before a network +call is made; when it is unset, any issuer that is HTTPS-reachable and serves +a parseable well-known document can mint a token this gateway will +successfully verify. That default is deliberate and follows from the draft's +self-sovereign premise, that an agent needs no pre-registration, but it means +**verification success carries no admission decision on its own**. The +authority plane of [ADR#0037](./0037-agent-identity-governance.md) is what +decides whether a verified-but-unknown agent may act, and a deployment that +knows its federation partners should pin them here rather than rely on that +plane alone. This variable is part of the fail-loud inventory above only in +the sense that a malformed value is rejected; being unset is a valid, +documented posture, not a misconfiguration. + ## Consequences - Agents authenticate to the gateway with a self-sovereign, key-bound identity @@ -219,11 +243,24 @@ silently-ignored default. alongside whatever auth token the Person Server issued. - `-32118` is now reserved on the JSON-RPC-over-NATS error surface for AAuth denials specifically; no other gateway error path may reuse it. -- Replay protection today is `InMemoryReplayStore`, process-local. A - multi-node gateway deployment can have the same nonce accepted once per - node until a shared store (NATS [JetStream](../glossary/jetstream) KV, per the doc comment in - `trogon-aauth-person`) is wired in; single-node deployments are fully - protected, multi-node deployments are not yet. +- Replay protection today is `InMemoryReplayStore`, process-local, and it is + the *only* `ReplayStore` implementation in the workspace. A multi-node + gateway deployment has the same nonce accepted once per node; single-node + deployments are fully protected, multi-node deployments are not. A shared + store (NATS [JetStream](../glossary/jetstream) KV keyed with a per-key TTL + is the intended backend) has not been built. `AAuthIngress` is generic over + `S: ReplayStore` and `ReplayError::Backend` is reserved for it, so adding + one is a construction change rather than a signature change. +- Verification success is not admission. In discovery mode with no issuer + allowlist pinned (Decision 7), any HTTPS-reachable issuer can mint a token + that verifies. Deployments that rely on AAuth as an authorization boundary + rather than an authentication one are misreading it; that boundary is + [ADR#0037](./0037-agent-identity-governance.md)'s authority plane. +- The three algorithms this ADR admits (ES256, ES384, EdDSA) are the whole + verifier allowlist, and none of them is RS256. Federating this platform's + identities *out* to an external OIDC-consuming IdP is therefore not + reachable from this surface; that boundary is + [ADR#0053](./0053-external-oidc-federation-surface.md). - JWKS resolution is env-selected between a static file (`StaticJwks`) and live `.well-known/{dwk}` discovery (`HttpJwksResolver`, HTTPS-only, size- and timeout-capped, wrapped in `CachedJwksResolver`). Static deployments diff --git a/docs/adr/0036-agent-self-certifying-identity.md b/docs/adr/0036-agent-self-certifying-identity.md index 0b3079449c..8762a36ffa 100644 --- a/docs/adr/0036-agent-self-certifying-identity.md +++ b/docs/adr/0036-agent-self-certifying-identity.md @@ -94,6 +94,10 @@ These do not change the genesis anchor and are added when needed: replicated substrate (relays, a DID method, a transparency log) so parties outside this system can resolve it. Until then the agent's event stream is the authoritative key record. + [ADR#0053](./0053-external-oidc-federation-surface.md) settles the narrower + external-verifier case with an OIDC issuer and JWKS, which is what + non-AAuth parties actually consume; it does not close this layer, because + resolving the *identifier* is the part that still needs a DID method. - **Key rotation**: a later signed event authorized by the current key, with the identifier held stable by the resolution layer above. Pure self-certifying identifiers are immutable, so rotation and resolution arrive together. @@ -129,5 +133,6 @@ These do not change the genesis anchor and are added when needed: - [ADR#0037: Agent Identity Governance: Decentralized Verification under Governed Authority](./0037-agent-identity-governance.md) - [ADR#0038: Agent Identity Cryptographic Suite and Crypto-Agility](./0038-agent-identity-crypto-suite.md) - [ADR#0039: Self-Authenticating Event Provenance](./0039-self-authenticating-event-provenance.md) +- [ADR#0053: External OIDC Federation Surface for Agent Identity](./0053-external-oidc-federation-surface.md) - [Buzz: Nostr-based agent identity](https://github.com/block/buzz) - [ADR index](./index.md) diff --git a/docs/adr/0038-agent-identity-crypto-suite.md b/docs/adr/0038-agent-identity-crypto-suite.md index 7ba3bc8044..d15eea1486 100644 --- a/docs/adr/0038-agent-identity-crypto-suite.md +++ b/docs/adr/0038-agent-identity-crypto-suite.md @@ -133,12 +133,13 @@ decision this ADR makes: it is what lets a broken algorithm be survived by rebinding a new key and rotating verifier policy rather than by redesigning the identity format itself. -### 4. Conditional profiles, added as bound keys, never as root replacements +### 4. Conditional profiles, each with its trigger, never as root replacements -Two further curves are recorded as conditional profiles, each with the -trigger that would justify adopting it. Either arrives as an additional key -bound to the agent's identity through the stream-attested binding of -Decision 3, never as a replacement for the Ed25519 root: +Three further profiles are recorded as conditional, each with the trigger that +would justify adopting it. None of them replaces the Ed25519 root. The two +curve profiles arrive as an additional key bound to the agent's identity +through the stream-attested binding of Decision 3; the third is not agent-bound +at all, and its entry records where it lives instead: - **P-256 (ES256)**, carried in [COSE](https://www.rfc-editor.org/rfc/rfc9052) ([RFC 9052](https://www.rfc-editor.org/rfc/rfc9052) and @@ -155,8 +156,50 @@ Decision 3, never as a replacement for the Ed25519 root: is a governance question, recorded as a stance in [ADR#0037](./0037-agent-identity-governance.md), not a decision this ADR makes on its own. +- **RSA-2048 with RS256**, only where an external identity provider's + federation surface refuses everything else. This trigger is already met in + practice rather than hypothetical: Microsoft Entra's workload identity + federation supports only RS256-signed issuers, and additionally requires + that the published JWK set contain *nothing but* RSA signing keys, so EC + and OKP keys cannot merely sit alongside an added RSA one. Every algorithm + Decision 1 and the two profiles above name is excluded by that constraint. + This profile is adopted for one purpose only, signing assertions presented + to a third-party IdP, and it never becomes an agent's root anchor. It is + also the one profile here that is *not* bound to an agent identity through + Decision 3: the key is signing material belonging to the separate federation + issuer of [ADR#0053](./0053-external-oidc-federation-surface.md), whose + custody and rotation schedule are the issuer's and are governed apart from + agent key management. The agent identity an assertion speaks for travels in + that assertion's claims, not in the key that signs it, which is what keeps + one issuer key able to serve many agents without becoming any of their + anchors. The "only RSA keys" requirement additionally forces a *separate* + published key set rather than an extra key in the AAuth well-known + documents, which is why the surface itself is decided in + [ADR#0053](./0053-external-oidc-federation-surface.md) rather than here. -### 5. Post-quantum path +### 5. Where the implementation currently stands, and where it diverges + +Decision 1 names Ed25519 as the default and the two curves above as +conditional. The shipped code does not match that yet, and the divergence is +recorded here rather than left to be discovered: + +- `trogon-aauth-verify` admits `ES256 | ES384 | EdDSA`, which is the + allowlist Decision 3 requires and a superset of the default. Its PoP path + derives the algorithm from the confirmed key's own `kty`/`crv` rather than + from the presented header, so algorithm confusion is structurally + unavailable there, not merely filtered. +- `trogon-jwks-publisher` is P-256 only. `jwk_from_ec_pkcs8_pem` hardcodes + the curve and the Agent Provider mints with `ES256`. The Agent Provider can + therefore issue the *conditional* profile and cannot issue the declared + default. Closing this means adding Ed25519 issuance to the publisher, not + changing the decision above: P-256 issuance stays supported under its + hardware-custody trigger. + +Nothing about this divergence is load-bearing for the anchor, precisely +because Decision 3 makes every key and signature name its own algorithm. It +is recorded as a gap to close, not as an amendment to the default. + +### 6. Post-quantum path Every elliptic-curve scheme named above, Ed25519, P-256, and secp256k1 alike, falls to Shor's algorithm on a cryptographically relevant quantum computer; @@ -182,6 +225,19 @@ threat timeline, not an emergency response to an already-broken algorithm. presumes one fixed curve in the identifier itself, are rejected. Identifiers and keys in this platform are always algorithm-tagged, so an identifier never has to be reinterpreted if the algorithm it names is later retired. +- The verifier's allowlist is now enforced on three axes, not one: the key's + material (`kty`/`crv`) must match the algorithm's family, and where a + publisher declares `use`, `key_ops`, or `alg` on a JWK + ([RFC 7517](https://www.rfc-editor.org/rfc/rfc7517) sections 4.2 to 4.4) + those declarations are honored, so a key published for encryption or pinned + to a different algorithm is not conscripted into signature verification + because its curve happens to line up. Absent members stay permissive, since + they are optional in the RFC and a federated deployment resolves key sets + this platform did not publish. +- Adopting the RSA profile of Decision 4 means operating a second published + key set with its own rotation, because the IdP constraint that triggers it + forbids mixing key types in one document. That cost is the reason the + profile is conditional rather than default. - Private key custody is unaffected by this ADR and remains on the security plane defined by [ADR#0023](./0023-secret-management-and-key-custody-direction.md) and [ADR#0033](./0033-two-tier-key-custody-product-model.md). This ADR @@ -197,8 +253,10 @@ threat timeline, not an emergency response to an already-broken algorithm. - [ADR#0036: Agent Self-Certifying Cryptographic Identity](./0036-agent-self-certifying-identity.md) - [ADR#0037: Agent Identity Governance](./0037-agent-identity-governance.md) - [ADR#0039: Self-Authenticating Event Provenance](./0039-self-authenticating-event-provenance.md) +- [ADR#0053: External OIDC Federation Surface for Agent Identity](./0053-external-oidc-federation-surface.md) - [RFC 6979: Deterministic Usage of DSA and ECDSA](https://www.rfc-editor.org/rfc/rfc6979) - [RFC 7515: JSON Web Signature (JWS)](https://www.rfc-editor.org/rfc/rfc7515) +- [RFC 7517: JSON Web Key (JWK)](https://www.rfc-editor.org/rfc/rfc7517) - [RFC 7638: JSON Web Key (JWK) Thumbprint](https://www.rfc-editor.org/rfc/rfc7638) - [RFC 8032: Edwards-Curve Digital Signature Algorithm (EdDSA)](https://www.rfc-editor.org/rfc/rfc8032) - [RFC 8037: CFRG Elliptic Curve Diffie-Hellman and Signatures in JOSE](https://www.rfc-editor.org/rfc/rfc8037) diff --git a/docs/adr/0049-revocation-latency-target.md b/docs/adr/0049-revocation-latency-target.md index 69fdb0e1af..004a1617db 100644 --- a/docs/adr/0049-revocation-latency-target.md +++ b/docs/adr/0049-revocation-latency-target.md @@ -49,6 +49,13 @@ in dashboards. - The cache TTL plus jitter (at most 330 seconds) is the hard upper bound on staleness when the event path fails entirely; the alert on the event path exists precisely so the backstop is never the operative mechanism. +- The target is scoped to credentials this platform resolves. It does not + extend to standing that has been federated to an external identity + provider, which never consults this platform's revocation state; that + boundary is bounded by assertion TTL plus the remote provider's own cache + and is decided in + [ADR#0053](./0053-external-oidc-federation-surface.md). An offboarding that + must hold on both planes is not complete when this histogram says it is. - The numbers are working values. They are revisited once production stream metrics exist, and any change lands as an amendment to this ADR. diff --git a/docs/adr/0053-external-oidc-federation-surface.md b/docs/adr/0053-external-oidc-federation-surface.md new file mode 100644 index 0000000000..5bbe719c2f --- /dev/null +++ b/docs/adr/0053-external-oidc-federation-surface.md @@ -0,0 +1,233 @@ +--- +number: "0053" +slug: external-oidc-federation-surface +status: draft +date: 2026-08-07 +--- + +# ADR#0053: External OIDC Federation Surface for Agent Identity + +## Context + +[ADR#0036](./0036-agent-self-certifying-identity.md) anchors an +[agent](../glossary/agent) to a self-certifying key and defers a *global +resolution* layer, "publishing the identifier-to-current-key mapping to a +replicated substrate so parties outside this system can resolve it", with +root-key rotation deferred alongside it because the two arrive together. +[ADR#0038](./0038-agent-identity-crypto-suite.md) pins the suite and makes +crypto-agility structural. [ADR#0017](./0017-aauth-agent-authentication.md) +ships the verification path and a well-known publisher. None of them says how +an agent identity is presented to a party that is not this platform and does +not speak AAuth. + +That gap is now load-bearing, because the shape of the answer is not the one +the deferred layer implies. Every cloud identity provider that accepts an +external workload assertion, Microsoft Entra's workload identity federation +being the concrete case examined here, consumes exactly one thing: an OIDC +discovery document plus a JWKS, over HTTPS, at a stable issuer URL. It does +not resolve DIDs, read transparency logs, or fetch AAuth well-known +documents. The pragmatic substrate for +[ADR#0036](./0036-agent-self-certifying-identity.md)'s deferred layer is the +boring one that already has universal client support. + +Three facts about that surface make it a decision rather than an +implementation detail: + +1. **It is not the surface we already publish.** + `trogon-jwks-publisher` serves `GET /.well-known/{dwk}` for the four + filenames the AAuth draft registers, and each response is a bare `JwkSet`. + There is no `/.well-known/openid-configuration`, no `issuer` field, and no + `jwks_uri`. A key set and a discovery document both publish trust material + over HTTPS, and they are still different protocol surfaces: an IdP handed + an AAuth `dwk` URL as an issuer will fail discovery before it ever looks at + a key. + +2. **Our entire algorithm allowlist is excluded.** Entra's federation supports + only RS256-signed issuers and additionally requires the published key set + to contain *nothing but* RSA signing keys. `trogon-aauth-verify` admits + `ES256 | ES384 | EdDSA`; `trogon-jwks-publisher` mints ES256; + [ADR#0038](./0038-agent-identity-crypto-suite.md) makes Ed25519 the default + root. Every one of those is refused. The "nothing but RSA" half is the + sharper constraint, because it means an RSA key cannot simply be added + alongside the existing ones in a shared document. + +3. **The mapping, not the token, is where the trust decision lives.** An IdP + binds an external assertion to a governed identity through an exact, + case-sensitive match on `iss`, `sub`, and `aud`, with no wildcards and a + hard cap on how many such bindings one identity may hold. A valid assertion + proves which workload is running; it grants nothing. If the mapping is + wrong, a short-lived, correctly-signed token only lets the wrong workload + become the wrong principal faster. + +The platform also already holds a position this surface must not quietly +contradict. [ADR#0037](./0037-agent-identity-governance.md) requires that "the +signature is valid" and "the agent is currently authorized" never collapse +into one check, and [ADR#0049](./0049-revocation-latency-target.md) commits to +a p99 five-second revocation-to-invalidation target. Neither survives a +federation boundary unamended: a third-party IdP will not call this +platform's status list. + +## Decision + +### 1. The federation surface is a separate issuer, not an added key + +External OIDC federation is served by its own issuer origin, publishing its +own `/.well-known/openid-configuration` and its own `jwks_uri`, distinct from +the AAuth well-known documents of +[ADR#0017](./0017-aauth-agent-authentication.md). The two are not merged and +the AAuth `dwk` documents are never advertised as an OIDC issuer. + +This is forced, not stylistic. The IdP constraint that the key set contain +only RSA keys is incompatible with an AAuth document holding the EC and OKP +keys the mesh verifies against, so one document cannot serve both. Keeping +them separate also keeps the blast radius separate: the federation issuer is +internet-facing and consumed by parties outside our control, while the AAuth +documents serve the mesh. + +### 2. RS256 only on that surface, under the conditional profile + +Assertions minted for external federation are signed RS256 over RSA-2048 +keys, adopted as the conditional profile +[ADR#0038](./0038-agent-identity-crypto-suite.md) Decision 4 records. This +profile exists for this purpose and no other. It is never an agent's root +anchor, never enters the `did:key` identifier, and never widens +`trogon-aauth-verify`'s inbound allowlist: this platform *signs* RS256 here +and continues to refuse to *verify* it on the mesh. + +### 3. The binding is a first-class resource with exact-match semantics + +The mapping from a platform-attested identity to the external principal it may +assume is modeled as an explicit, enumerable, auditable resource, anchored in +the project hierarchy of +[ADR#0046](./0046-project-anchored-resource-hierarchy.md), with exact-match +semantics on issuer, subject, and audience. Not a pattern, not a policy +expression, not a wildcard. + +Pattern matching is rejected for this binding even where an IdP offers it. It +reduces the number of objects to manage and changes the risk model in the same +move: a subject pattern makes the security of the whole federation depend on +how tightly the upstream registration policy constrains what subjects can be +minted, which relocates the trust decision somewhere it is much harder to +audit. Enumerable bindings are the property worth paying object count for. + +### 4. One audience per assertion, minted at the moment of exchange + +An assertion minted for an external IdP carries exactly one audience, the one +that IdP requires. Tokens minted for any internal purpose are never presented +as external assertions and vice versa: a multi-audience bearer token is +replayable to every recipient it names. + +Because such an assertion is a bearer credential for its whole validity +window, it is requested only when an exchange is about to happen, never +cached; never logged, traced, or written to an event; and never placed +anywhere an agent's model context can reach it, which includes prompts, tool +arguments, and tool results. What *is* cached is the resource token the +exchange returns, keyed by its own lifetime, never the assertion that bought +it. + +This is a weaker posture than the platform holds for its own ingress, where +[ADR#0051](./0051-fully-bound-request-signing.md) binds a request token to its +target and payload with a nonce, and that asymmetry is inherent: the external +IdP's protocol defines what it accepts, and it accepts a bearer assertion. +The mitigations above are what is available, so they are requirements rather +than hardening. + +### 5. Revocation across the boundary is TTL plus unbinding, and it is slower + +[ADR#0049](./0049-revocation-latency-target.md)'s five-second target is +in-platform: it measures a revocation event reaching this platform's own +runtime projection. It does not extend across a federation boundary, because +the external IdP does not consult this platform's status list and +[ADR#0037](./0037-agent-identity-governance.md)'s mandatory status check +cannot be pushed onto it. + +Standing across the boundary is therefore bounded by two things only: the +assertion TTL, which is kept short and is the primary lever, and deleting the +binding of Decision 3, which propagates asynchronously through the IdP's own +caches and is not immediate. Both are measured and neither is presented as +equivalent to in-platform revocation. + +Both levers bound *future* exchanges: they stop the agent obtaining a new +external resource token. Neither reaches a token the external provider has +already issued, which stays valid until its own expiry or until that provider +revokes it, and no action on this platform shortens that. Offboarding an agent +must therefore revoke on both planes, and the residual window after the +platform-side revocation is the lifetime of the already-issued external token, +not the assertion TTL. + +### 6. Assertion validity is scoped, and delegated authority is not implied + +An assertion presented on this surface authenticates a *client*: it says which +platform-attested identity is calling. It never carries delegated user +authority. Where a downstream call acts on a person's behalf, that +authority comes from the person-linked principal +[ADR#0017](./0017-aauth-agent-authentication.md) Decision 5 already makes +authoritative, carried separately. This surface is not a shortcut around +delegated consent, and an integration that treats a successful federation +exchange as sufficient for an on-behalf-of call has widened authority without +anyone deciding to. + +### 7. Deferred, and named so it is not assumed + +Two things this decision deliberately does not settle: + +- **Which principal the downstream provider sees.** Today + [ADR#0032](./0032-model-route-and-credential-binding.md) Decision 4 brokers + hosted model access through a session-scoped proxy holding platform + credentials, so the provider's own audit log names the platform, not the + agent. Exchanging a per-agent assertion so the provider attributes calls to + the agent is the strictly better shape and is not built. Until it is, agent + attribution at a hosted provider is a platform-side record only. +- **Workload attestation.** Proof of possession + ([ADR#0017](./0017-aauth-agent-authentication.md), + [ADR#0036](./0036-agent-self-certifying-identity.md)) proves control of a + key, not that the process holding it is the runtime the platform expects; a + leaked private key satisfies it. Binding key use to an attested workload is + the deferred operational-key tier of + [ADR#0036](./0036-agent-self-certifying-identity.md), and this surface + inherits that limitation rather than fixing it. + +## Consequences + +- [ADR#0036](./0036-agent-self-certifying-identity.md)'s deferred global + resolution layer gains a concrete, unglamorous shape for the external case: + an OIDC issuer with a JWKS. It does not close the layer, because a DID + resolution story is still what makes the *identifier* portable; it closes + the narrower question of how a non-AAuth party verifies us today. +- A second published key set exists, with its own rotation schedule, its own + overlap window, and its own monitoring, and it must be coordinated with + SPIRE-style key rotation on the signing side and the IdP's cache on the + consuming side. Four rotation responsibilities that must not drift apart is + the standing operational cost of this decision. +- Federation failures need to be distinguishable in telemetry + ([ADR#0008](./0008-opentelemetry-observability.md)): a missing binding, an + unreachable or mismatched issuer, and IdP cache lag after a binding change + have different remediations, and the last class is expected to require + retry rather than a fix. +- The platform now signs with an algorithm it refuses to verify. That is + intentional and worth stating plainly, because the asymmetry looks like a + bug to anyone reading only one side of it. +- Adopting this surface expands what an attacker gains from compromising the + signing path: an assertion minted here is accepted by a third party under + the mapping's authority, and revoking it is slower than revoking anything + in-platform. + +## References + +- [ADR#0008: OpenTelemetry Observability](./0008-opentelemetry-observability.md) +- [ADR#0017: AAuth Agent Authentication over a Trogon NATS PoP Binding](./0017-aauth-agent-authentication.md) +- [ADR#0032: Model Route and Credential Binding](./0032-model-route-and-credential-binding.md) +- [ADR#0036: Agent Self-Certifying Cryptographic Identity](./0036-agent-self-certifying-identity.md) +- [ADR#0037: Agent Identity Governance: Decentralized Verification under Governed Authority](./0037-agent-identity-governance.md) +- [ADR#0038: Agent Identity Cryptographic Suite and Crypto-Agility](./0038-agent-identity-crypto-suite.md) +- [ADR#0046: Project-Anchored Resource Hierarchy for the Credential Platform](./0046-project-anchored-resource-hierarchy.md) +- [ADR#0049: Revocation Propagation Latency Target](./0049-revocation-latency-target.md) +- [ADR#0051: Fully Bound Per-Request Signing Contract](./0051-fully-bound-request-signing.md) +- [RFC 7517: JSON Web Key (JWK)](https://www.rfc-editor.org/rfc/rfc7517) +- [RFC 8414: OAuth 2.0 Authorization Server Metadata](https://www.rfc-editor.org/rfc/rfc8414) +- [RFC 8615: Well-Known Uniform Resource Identifiers](https://www.rfc-editor.org/rfc/rfc8615) +- [OpenID Connect Discovery 1.0](https://openid.net/specs/openid-connect-discovery-1_0.html) +- [Microsoft Entra: workload identity federation considerations](https://github.com/MicrosoftDocs/entra-docs/blob/main/docs/workload-id/workload-identity-federation-considerations.md) +- [SPIFFE Workload Identity and Entra Agent ID: the trust gap](https://dev.to/astaykov/your-spiffe-workload-can-authenticate-as-an-entra-agent-id-but-mind-the-trust-gap-3969) +- [From JWT-SVID to Entra Agent ID: a working SPIFFE PoC](https://dev.to/astaykov/from-jwt-svid-to-entra-agent-id-a-working-spiffe-poc-ic4) +- [ADR index](./index.md) diff --git a/docs/adr/index.md b/docs/adr/index.md index 6ea74c18ce..52e58b78a7 100644 --- a/docs/adr/index.md +++ b/docs/adr/index.md @@ -58,3 +58,4 @@ future implementation work. - [ADR#0050: Signed Proof-of-Possession as the Strongly Recommended Caller Authentication](./0050-signed-first-caller-authentication.md) - [ADR#0051: Fully Bound Per-Request Signing Contract](./0051-fully-bound-request-signing.md) - [ADR#0052: Cloud KMS Auto-Unseal Is Mandatory for Production OpenBao](./0052-cloud-kms-production-seal.md) +- [ADR#0053: External OIDC Federation Surface for Agent Identity (Draft)](./0053-external-oidc-federation-surface.md) diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/caller_jwt_header.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/caller_jwt_header.rs index b6b297fa85..4e65c7ec47 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/caller_jwt_header.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/caller_jwt_header.rs @@ -2,9 +2,15 @@ use std::fmt; use crate::jwt::{JwtError, MintedUserJwt}; -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] pub struct CallerJwtHeaderValue(String); +impl fmt::Debug for CallerJwtHeaderValue { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("CallerJwtHeaderValue").field(&"").finish() + } +} + impl CallerJwtHeaderValue { pub fn from_minted(jwt: &MintedUserJwt) -> Self { Self(jwt.as_str().to_owned()) diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/caller_jwt_header/tests.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/caller_jwt_header/tests.rs index 1ff330edd2..1366eee06d 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/caller_jwt_header/tests.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/caller_jwt_header/tests.rs @@ -41,3 +41,11 @@ fn display_redacts_value() { let header = CallerJwtHeaderValue::parse("a.b.c").unwrap(); assert_eq!(header.to_string(), ""); } + +#[test] +fn debug_does_not_leak_the_token() { + let header = CallerJwtHeaderValue::parse("hhh.ppp.sss").unwrap(); + let dbg = format!("{header:?}"); + assert!(!dbg.contains("ppp"), "{dbg}"); + assert!(dbg.contains(""), "{dbg}"); +} diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/constants.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/constants.rs index 3d79dfb75c..c53de0b29b 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/constants.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/constants.rs @@ -30,3 +30,19 @@ pub const NATS_JWT_PREFIX: &[u8] = b"eyJ"; /// Default minted user JWT TTL, in seconds, for the `a2a-auth-callout` binary. pub const DEFAULT_USER_JWT_TTL_SECS: u64 = 300; + +/// Signature algorithms an inbound OIDC ID token may assert. +/// +/// Scoped to the RSA family because [`crate::credentials::oidc`] only builds +/// decoding keys from RSA JWK components; an allowlist that admitted anything +/// else would name algorithms that cannot verify here anyway. The list exists +/// so the algorithm is a deployment decision rather than something the token +/// under verification chooses for itself. +pub(crate) const OIDC_ALLOWED_ALGORITHMS: [jsonwebtoken::Algorithm; 6] = [ + jsonwebtoken::Algorithm::RS256, + jsonwebtoken::Algorithm::RS384, + jsonwebtoken::Algorithm::RS512, + jsonwebtoken::Algorithm::PS256, + jsonwebtoken::Algorithm::PS384, + jsonwebtoken::Algorithm::PS512, +]; diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/credentials/oidc.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/credentials/oidc.rs index ff14d4c7b7..e5d95ca50c 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/credentials/oidc.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/credentials/oidc.rs @@ -1,12 +1,14 @@ +use std::fmt; use std::sync::Arc; +use crate::constants::OIDC_ALLOWED_ALGORITHMS; use crate::error::{AuthCalloutError, CredentialError}; use crate::jwt::{ AudienceAccount, ExternalSubject, UserJwtClaims, derive_caller_id, spicedb_principal_from_oidc_claims, }; use crate::permissions::IssuedPermissions; -use jsonwebtoken::jwk::{AlgorithmParameters, JwkSet}; -use jsonwebtoken::{DecodingKey, Validation, decode, decode_header}; +use jsonwebtoken::jwk::{AlgorithmParameters, JwkSet, KeyAlgorithm, KeyOperations, PublicKeyUse}; +use jsonwebtoken::{Algorithm, DecodingKey, Validation, decode, decode_header}; #[derive(Debug, Clone, PartialEq, Eq)] pub struct OidcIssuerUrl(String); @@ -50,9 +52,15 @@ impl OidcClientId { } } -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] pub struct BearerToken(String); +impl fmt::Debug for BearerToken { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("BearerToken").field(&"").finish() + } +} + impl BearerToken { pub fn new(token: impl Into) -> Self { Self(token.into()) @@ -189,6 +197,46 @@ fn url_origin(s: &str) -> Option<(String, String, Option)> { Some((scheme, host.to_ascii_lowercase(), port)) } +/// A JWK may verify an OIDC ID token under `alg` only when the key's own +/// advertised purpose permits verification and its declared algorithm, if any, +/// is the one asserted. +/// +/// RFC 7517 makes `use` (section 4.2), `key_ops` (section 4.3), and `alg` +/// (section 4.4) optional, so an absent member stays permissive: an external +/// IdP's JWKS is not ours to constrain beyond what it states. But a publisher +/// that does set them has declared what the key is for, and honoring that stops +/// a key published for encryption, or pinned to one RSA algorithm, from being +/// conscripted into verifying another. +/// +/// Mirrors `jwk_compatible_with_alg` in `trogon-aauth-verify`'s token +/// verifier. Kept separate rather than shared because the two crates compile +/// against different major versions of `jsonwebtoken`, so the `Jwk` types are +/// distinct. +fn jwk_permits_verification_with(jwk: &jsonwebtoken::jwk::Jwk, alg: jsonwebtoken::Algorithm) -> bool { + if let Some(public_key_use) = &jwk.common.public_key_use + && !matches!(public_key_use, PublicKeyUse::Signature) + { + return false; + } + if let Some(key_operations) = &jwk.common.key_operations + && !key_operations.iter().any(|op| matches!(op, KeyOperations::Verify)) + { + return false; + } + let Some(declared) = jwk.common.key_algorithm else { + return true; + }; + matches!( + (declared, alg), + (KeyAlgorithm::RS256, Algorithm::RS256) + | (KeyAlgorithm::RS384, Algorithm::RS384) + | (KeyAlgorithm::RS512, Algorithm::RS512) + | (KeyAlgorithm::PS256, Algorithm::PS256) + | (KeyAlgorithm::PS384, Algorithm::PS384) + | (KeyAlgorithm::PS512, Algorithm::PS512) + ) +} + impl JwksOidcVerifier { pub(crate) async fn fetch_jwks(&self) -> Result { match &self.jwks { @@ -239,7 +287,19 @@ impl JwksOidcVerifier { let jwk = jwks .find(kid) .ok_or_else(|| CredentialError::InvalidCredentials(format!("no JWK for kid {kid}")))?; + if !OIDC_ALLOWED_ALGORITHMS.contains(&header.alg) { + return Err(CredentialError::UnsupportedTokenAlgorithm { algorithm: header.alg }.into()); + } + if !jwk_permits_verification_with(jwk, header.alg) { + return Err(CredentialError::JwkNotPublishedForVerification { + kid: kid.clone(), + algorithm: header.alg, + } + .into()); + } let auds: Vec<&str> = self.expected_id_token_audiences.iter().map(String::as_str).collect(); + // Built from the header only after the allowlist above has vetted it, + // so the token cannot nominate its own algorithm. let mut validation = Validation::new(header.alg); validation.set_issuer(&[self.issuer.as_str()]); validation.set_audience(&auds); diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/credentials/oidc/tests.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/credentials/oidc/tests.rs index 58b925dcc7..69413fe6d2 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/credentials/oidc/tests.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/credentials/oidc/tests.rs @@ -38,7 +38,9 @@ fn test_jwks_and_encoding_key(rng: &mut OsRng) -> (JwkSet, jsonwebtoken::Encodin let jwk = Jwk { common: CommonParameters { public_key_use: Some(PublicKeyUse::Signature), - key_operations: Some(vec![KeyOperations::Sign]), + // `verify`, not `sign`: this is the *public* half of the pair, and + // RFC 7517 section 4.3 scopes `key_ops` to what this key can do. + key_operations: Some(vec![KeyOperations::Verify]), key_id: Some("test-kid".into()), x509_url: None, x509_chain: None, @@ -353,10 +355,10 @@ async fn verify_fails_with_non_rsa_jwk() { let jwks = JwkSet { keys: vec![ec_jwk] }; let verifier = JwksOidcVerifier::with_static_jwks(issuer, vec!["aud".into()], jwks); - // Craft a fake JWT whose kid matches the EC JWK; decode_header will succeed - // but decoding_key_for_jwk must reject the non-RSA key. - // We can't sign with the EC key easily, but we can make a header-only token - // that references the EC kid. decode_header just parses the header. + // The kid resolves to the EC JWK, but ES256 is outside the deployment's + // allow-list, so the token is refused on its algorithm before the key it + // points at is ever considered. A header-only token is enough: nothing + // past decode_header runs. let header_b64 = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(br#"{"alg":"ES256","kid":"ec-kid","typ":"JWT"}"#); let payload_b64 = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(b"{}"); @@ -366,10 +368,77 @@ async fn verify_fails_with_non_rsa_jwk() { .verify_internal(&BearerToken::new(fake_token), &AudienceAccount::new("acct")) .await .unwrap_err(); - assert!(matches!( - err, - AuthCalloutError::CredentialVerification(CredentialError::InvalidCredentials(_)) - )); + let AuthCalloutError::CredentialVerification(CredentialError::UnsupportedTokenAlgorithm { algorithm }) = err else { + panic!("expected UnsupportedTokenAlgorithm, got {err:?}"); + }; + assert_eq!(algorithm, jsonwebtoken::Algorithm::ES256); +} + +#[tokio::test] +async fn verify_fails_with_a_non_rsa_jwk_reached_under_an_allowed_algorithm() { + let issuer = OidcIssuerUrl::parse("https://issuer.example").unwrap(); + let ec_jwk = Jwk { + common: CommonParameters { + key_id: Some("ec-kid".into()), + ..Default::default() + }, + algorithm: AlgorithmParameters::EllipticCurve(EllipticCurveKeyParameters { + key_type: EllipticCurveKeyType::EC, + curve: jsonwebtoken::jwk::EllipticCurve::P256, + x: "dummyx".into(), + y: "dummyy".into(), + }), + }; + let jwks = JwkSet { keys: vec![ec_jwk] }; + let verifier = JwksOidcVerifier::with_static_jwks(issuer, vec!["aud".into()], jwks); + + // RS256 is allow-listed and the EC JWK declares no purpose of its own, so + // neither guard ahead of the key material refuses this token. What refuses + // it is the verifier's own RSA-only support, which the guards must not be + // allowed to mask. + let header_b64 = URL_SAFE_NO_PAD.encode(br#"{"alg":"RS256","kid":"ec-kid","typ":"JWT"}"#); + let payload_b64 = URL_SAFE_NO_PAD.encode(b"{}"); + let fake_token = format!("{header_b64}.{payload_b64}.sig"); + + let err = verifier + .verify_internal(&BearerToken::new(fake_token), &AudienceAccount::new("acct")) + .await + .unwrap_err(); + let AuthCalloutError::CredentialVerification(CredentialError::InvalidCredentials(message)) = err else { + panic!("expected InvalidCredentials, got {err:?}"); + }; + assert!(message.contains("must be RSA"), "{message}"); +} + +#[tokio::test] +async fn verify_fails_with_an_rsa_jwk_whose_components_do_not_decode() { + let issuer = OidcIssuerUrl::parse("https://issuer.example").unwrap(); + let broken_jwk = Jwk { + common: CommonParameters { + key_id: Some("broken-kid".into()), + ..Default::default() + }, + algorithm: AlgorithmParameters::RSA(RSAKeyParameters { + key_type: RSAKeyType::RSA, + n: "not base64url".into(), + e: "AQAB".into(), + }), + }; + let jwks = JwkSet { keys: vec![broken_jwk] }; + let verifier = JwksOidcVerifier::with_static_jwks(issuer, vec!["aud".into()], jwks); + + let header_b64 = URL_SAFE_NO_PAD.encode(br#"{"alg":"RS256","kid":"broken-kid","typ":"JWT"}"#); + let payload_b64 = URL_SAFE_NO_PAD.encode(b"{}"); + let fake_token = format!("{header_b64}.{payload_b64}.sig"); + + let err = verifier + .verify_internal(&BearerToken::new(fake_token), &AudienceAccount::new("acct")) + .await + .unwrap_err(); + let AuthCalloutError::CredentialVerification(CredentialError::InvalidCredentials(message)) = err else { + panic!("expected InvalidCredentials, got {err:?}"); + }; + assert!(message.contains("invalid RSA JWK components"), "{message}"); } #[tokio::test] @@ -406,3 +475,195 @@ async fn oidc_verifier_trait_delegates_to_verify_internal() { .expect("verify via trait"); assert_eq!(claims.sub.as_str(), "user-1"); } + +/// Signs `claims` as an RS256 token naming `test-kid`, the shape every guard +/// test below starts from before varying one thing about the JWK. +fn rs256_token_for(issuer: &OidcIssuerUrl, enc: &jsonwebtoken::EncodingKey) -> String { + #[derive(Serialize)] + struct IdClaims { + sub: String, + iss: String, + aud: String, + exp: u64, + } + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs(); + let id = IdClaims { + sub: "user-guard".into(), + iss: issuer.as_str().to_owned(), + aud: "a2a-client".into(), + exp: now + 600, + }; + let mut header = jsonwebtoken::Header::new(jsonwebtoken::Algorithm::RS256); + header.kid = Some("test-kid".into()); + jsonwebtoken::encode(&header, &id, enc).expect("encode") +} + +fn jwks_with_common(jwks: &JwkSet, mutate: impl FnOnce(&mut CommonParameters)) -> JwkSet { + let mut jwk = jwks.keys[0].clone(); + mutate(&mut jwk.common); + JwkSet { keys: vec![jwk] } +} + +#[tokio::test] +async fn verify_rejects_algorithm_outside_the_allowlist() { + let rng = &mut OsRng; + let (jwks, _) = test_jwks_and_encoding_key(rng); + let issuer = OidcIssuerUrl::parse("https://issuer.example").unwrap(); + let verifier = JwksOidcVerifier::with_static_jwks(issuer.clone(), vec!["a2a-client".into()], jwks); + + #[derive(Serialize)] + struct IdClaims { + sub: String, + iss: String, + aud: String, + exp: u64, + } + let now = std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .unwrap() + .as_secs(); + let id = IdClaims { + sub: "user-hs".into(), + iss: issuer.as_str().to_owned(), + aud: "a2a-client".into(), + exp: now + 600, + }; + let mut header = jsonwebtoken::Header::new(jsonwebtoken::Algorithm::HS256); + header.kid = Some("test-kid".into()); + let token = + jsonwebtoken::encode(&header, &id, &jsonwebtoken::EncodingKey::from_secret(b"shared")).expect("encode hs256"); + + let err = verifier + .verify_internal(&BearerToken::new(token), &AudienceAccount::new("acct")) + .await + .unwrap_err(); + let AuthCalloutError::CredentialVerification(CredentialError::UnsupportedTokenAlgorithm { algorithm }) = err else { + panic!("expected UnsupportedTokenAlgorithm, got {err:?}"); + }; + assert_eq!(algorithm, jsonwebtoken::Algorithm::HS256); +} + +#[tokio::test] +async fn verify_rejects_a_jwk_published_for_encryption() { + let rng = &mut OsRng; + let (jwks, enc) = test_jwks_and_encoding_key(rng); + let issuer = OidcIssuerUrl::parse("https://issuer.example").unwrap(); + let token = rs256_token_for(&issuer, &enc); + let jwks = jwks_with_common(&jwks, |common| { + common.public_key_use = Some(PublicKeyUse::Encryption); + common.key_operations = None; + }); + let verifier = JwksOidcVerifier::with_static_jwks(issuer, vec!["a2a-client".into()], jwks); + + let err = verifier + .verify_internal(&BearerToken::new(token), &AudienceAccount::new("acct")) + .await + .unwrap_err(); + let AuthCalloutError::CredentialVerification(CredentialError::JwkNotPublishedForVerification { kid, algorithm }) = + err + else { + panic!("expected JwkNotPublishedForVerification, got {err:?}"); + }; + assert_eq!(kid, "test-kid"); + assert_eq!(algorithm, jsonwebtoken::Algorithm::RS256); +} + +#[tokio::test] +async fn verify_rejects_a_jwk_whose_key_ops_omit_verify() { + let rng = &mut OsRng; + let (jwks, enc) = test_jwks_and_encoding_key(rng); + let issuer = OidcIssuerUrl::parse("https://issuer.example").unwrap(); + let token = rs256_token_for(&issuer, &enc); + let jwks = jwks_with_common(&jwks, |common| { + common.key_operations = Some(vec![KeyOperations::Encrypt]); + }); + let verifier = JwksOidcVerifier::with_static_jwks(issuer, vec!["a2a-client".into()], jwks); + + let err = verifier + .verify_internal(&BearerToken::new(token), &AudienceAccount::new("acct")) + .await + .unwrap_err(); + assert!(matches!( + err, + AuthCalloutError::CredentialVerification(CredentialError::JwkNotPublishedForVerification { .. }) + )); +} + +#[tokio::test] +async fn verify_rejects_a_jwk_pinned_to_a_different_rsa_algorithm() { + let rng = &mut OsRng; + let (jwks, enc) = test_jwks_and_encoding_key(rng); + let issuer = OidcIssuerUrl::parse("https://issuer.example").unwrap(); + let token = rs256_token_for(&issuer, &enc); + let jwks = jwks_with_common(&jwks, |common| { + common.key_algorithm = Some(jsonwebtoken::jwk::KeyAlgorithm::PS512); + }); + let verifier = JwksOidcVerifier::with_static_jwks(issuer, vec!["a2a-client".into()], jwks); + + let err = verifier + .verify_internal(&BearerToken::new(token), &AudienceAccount::new("acct")) + .await + .unwrap_err(); + let AuthCalloutError::CredentialVerification(CredentialError::JwkNotPublishedForVerification { kid, algorithm }) = + err + else { + panic!("expected JwkNotPublishedForVerification, got {err:?}"); + }; + assert_eq!(kid, "test-kid"); + assert_eq!(algorithm, jsonwebtoken::Algorithm::RS256); +} + +#[tokio::test] +async fn verify_accepts_a_jwk_that_declares_the_asserted_algorithm() { + let rng = &mut OsRng; + let (jwks, enc) = test_jwks_and_encoding_key(rng); + let issuer = OidcIssuerUrl::parse("https://issuer.example").unwrap(); + let token = rs256_token_for(&issuer, &enc); + let jwks = jwks_with_common(&jwks, |common| { + common.key_algorithm = Some(jsonwebtoken::jwk::KeyAlgorithm::RS256); + }); + let verifier = JwksOidcVerifier::with_static_jwks(issuer, vec!["a2a-client".into()], jwks); + + let claims = verifier + .verify_internal(&BearerToken::new(token), &AudienceAccount::new("acct")) + .await + .expect("declared alg matches the asserted one"); + assert_eq!(claims.sub.as_str(), "user-guard"); +} + +#[tokio::test] +async fn verify_accepts_a_jwk_that_declares_no_purpose_at_all() { + let rng = &mut OsRng; + let (jwks, enc) = test_jwks_and_encoding_key(rng); + let issuer = OidcIssuerUrl::parse("https://issuer.example").unwrap(); + let token = rs256_token_for(&issuer, &enc); + let jwks = jwks_with_common(&jwks, |common| { + common.public_key_use = None; + common.key_operations = None; + common.key_algorithm = None; + }); + let verifier = JwksOidcVerifier::with_static_jwks(issuer, vec!["a2a-client".into()], jwks); + + let claims = verifier + .verify_internal(&BearerToken::new(token), &AudienceAccount::new("acct")) + .await + .expect("absent RFC 7517 members stay permissive"); + assert_eq!(claims.sub.as_str(), "user-guard"); +} + +#[test] +fn bearer_token_debug_does_not_leak_the_assertion() { + let token = BearerToken::new("hhh.ppp.sss"); + let dbg = format!("{token:?}"); + assert!(!dbg.contains(token.as_str()), "{dbg}"); + // Each segment separately: a Debug that printed only the header, or only + // the signature, would still disclose the assertion piecewise. + for segment in ["hhh", "ppp", "sss"] { + assert!(!dbg.contains(segment), "{dbg}"); + } + assert!(dbg.contains(""), "{dbg}"); + assert_eq!(token.as_str(), "hhh.ppp.sss"); +} diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/denial_category.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/denial_category.rs index 13de8d7d2e..bbfd42daf0 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/denial_category.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/denial_category.rs @@ -47,7 +47,13 @@ impl DenialCategory { CredentialError::UnknownAccount(_) => Self::UnknownAccount, CredentialError::VerifierUnavailable { .. } => Self::VerifierUnavailable, CredentialError::InvalidRequest(_) => Self::InvalidRequest, - CredentialError::InvalidCredentials(_) => Self::InvalidCredentials, + // The algorithm and JWK-policy rejections stay behind the same + // wire category as any other bad credential. The typed variants + // exist so this side can count them apart, not so a caller can + // learn which check refused it. + CredentialError::InvalidCredentials(_) + | CredentialError::UnsupportedTokenAlgorithm { .. } + | CredentialError::JwkNotPublishedForVerification { .. } => Self::InvalidCredentials, } } } diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/error.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/error.rs index 8d28c3eb13..a81ce25d17 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/error.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/error.rs @@ -22,6 +22,23 @@ pub enum CredentialError { /// The verifier ran and refused the credential material itself. #[error("credential verification failed: {0}")] InvalidCredentials(String), + /// The token nominated an `alg` outside the verifier's allowlist. Kept + /// apart from [`Self::InvalidCredentials`] because an algorithm-confusion + /// attempt is a different signal from a merely bad signature, and a + /// rejection counter should be able to tell them apart without matching + /// on message text. + #[error("credential verification failed: unsupported token algorithm {algorithm:?}")] + UnsupportedTokenAlgorithm { algorithm: jsonwebtoken::Algorithm }, + /// The JWK named by `kid` verified as well-formed but its own `alg`, + /// `use`, or `key_ops` do not permit verifying signatures with the + /// token's algorithm. + #[error( + "credential verification failed: JWK for kid {kid:?} is not published for verifying {algorithm:?} signatures" + )] + JwkNotPublishedForVerification { + kid: String, + algorithm: jsonwebtoken::Algorithm, + }, } impl From for AuthCalloutError { diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/jwt/mod.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/jwt/mod.rs index 9877879b2e..95dc3d6233 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/jwt/mod.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/jwt/mod.rs @@ -166,9 +166,15 @@ impl SpiceDbPrincipal { } /// HS256 User JWT minted for bridge/gateway consumption (inner `nats.jwt` on wire responses). -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] pub struct MintedUserJwt(String); +impl fmt::Debug for MintedUserJwt { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("MintedUserJwt").field(&"").finish() + } +} + impl MintedUserJwt { /// Wrap a token after validating compact-JWT shape (three non-empty /// dot-separated segments, non-empty input after trimming). Mirrors the diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/jwt/tests.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/jwt/tests.rs index e5070da7eb..a2ceda5e2c 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/jwt/tests.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/jwt/tests.rs @@ -417,3 +417,11 @@ fn ensure_fresh_rejects_not_yet_valid_token() { let err = token.ensure_fresh().unwrap_err(); assert!(matches!(err, JwtError::Decode(ref m) if m.contains("not yet valid"))); } + +#[test] +fn minted_user_jwt_debug_does_not_leak_the_token() { + let minted = MintedUserJwt::new("hhh.ppp.sss").expect("valid shape"); + let dbg = format!("{minted:?}"); + assert!(!dbg.contains("ppp"), "{dbg}"); + assert!(dbg.contains(""), "{dbg}"); +} diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/wire/callout_auth_response_claims.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/wire/callout_auth_response_claims.rs index 8b90fe2926..80abe2acab 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/wire/callout_auth_response_claims.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/wire/callout_auth_response_claims.rs @@ -1,3 +1,5 @@ +use std::fmt; + use nats_jwt_rs::authorization::AuthResponse; use nkeys::{KeyPair, XKey}; @@ -6,11 +8,21 @@ use crate::error::AuthCalloutError; use crate::jwt::MintedUserJwt; /// Callout-signed authorization **response** JWT for `$SYS.REQ.USER.AUTH` reply. -#[derive(Debug, Clone)] +#[derive(Clone)] pub struct CalloutAuthResponseClaims { encoded: String, } +/// `encoded` carries the minted user JWT inside `nats.jwt`, so printing it +/// would put a usable credential wherever the response is traced. +impl fmt::Debug for CalloutAuthResponseClaims { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_struct("CalloutAuthResponseClaims") + .field("encoded", &"") + .finish() + } +} + impl CalloutAuthResponseClaims { pub fn success( request: &ServerAuthRequestClaims, diff --git a/rsworkspace/crates/a2a/a2a-auth-callout/src/wire/callout_auth_response_claims/tests.rs b/rsworkspace/crates/a2a/a2a-auth-callout/src/wire/callout_auth_response_claims/tests.rs index fc811ae342..3d40843ebe 100644 --- a/rsworkspace/crates/a2a/a2a-auth-callout/src/wire/callout_auth_response_claims/tests.rs +++ b/rsworkspace/crates/a2a/a2a-auth-callout/src/wire/callout_auth_response_claims/tests.rs @@ -111,3 +111,17 @@ fn into_wire_bytes_errors_when_server_xkey_but_no_account_xkey() { let err = resp.into_wire_bytes(&req, None).unwrap_err(); assert!(matches!(err, AuthCalloutError::WireFormat(_))); } + +#[test] +fn debug_does_not_leak_the_encoded_response() { + let (req, callout) = fixture_request(); + let resp = CalloutAuthResponseClaims::success( + &req, + &MintedUserJwt::new("eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiJ4In0.c2ln").unwrap(), + &callout, + ) + .unwrap(); + let dbg = format!("{resp:?}"); + assert!(!dbg.contains(resp.as_jwt_str()), "{dbg}"); + assert!(dbg.contains(""), "{dbg}"); +} diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs index 92227d3372..6d607347a3 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env.rs @@ -5,6 +5,11 @@ //! (returns the safer disabled / shorter-deadline default) when the //! env value is missing or malformed -- callers should branch on the //! resulting state rather than re-parse the env at the dispatch site. +//! +//! [`gateway_tier3_signing_pubkey`] is the exception, and deliberately +//! so: for a code-signing key the disabled default is *not* the safer +//! one, so it reports a malformed value as its own state instead of +//! folding it into "not configured". use std::time::{Duration, SystemTime, UNIX_EPOCH}; @@ -34,26 +39,56 @@ pub fn gateway_audit_publish_enabled(env: &E) -> bool { parse_bool_flag(env, ENV_GATEWAY_AUDIT_PUBLISH) } -/// Tier-3 wasm bundle signing public key, if configured. Returns -/// `None` for unset / empty / unparseable values -- the gateway -/// then refuses to verify signatures rather than half-trusting an -/// invalid pubkey. -pub fn gateway_tier3_signing_pubkey(env: &E) -> Option { +/// What `A2A_GATEWAY_TIER3_SIGNING_PUBKEY` says about bundle signing. +/// +/// The three states are kept distinct because two of them look alike +/// and mean opposite things. "Unset" is an operator who never opted +/// into signing, and unsigned bundles are the expected posture. +/// "Invalid" is an operator who *did* opt in and mistyped the key; +/// collapsing that into "no pubkey configured" would silently execute +/// unverified wasm on a deployment that asked for verification. +#[derive(Debug)] +pub enum Tier3SigningKey { + /// Var unset or blank: bundle signing was never requested. + NotConfigured, + /// A usable verifying key. + Configured(Ed25519PublicKey), + /// Var set to something that is not a valid ed25519 pubkey. + Invalid, +} + +impl Tier3SigningKey { + /// The key to hand the wasm substrate, or `None` when signing was + /// never configured. [`Self::Invalid`] has no such projection on + /// purpose: callers have to handle it before they can get here. + pub fn into_configured(self) -> Option { + match self { + Self::Configured(pubkey) => Some(pubkey), + Self::NotConfigured | Self::Invalid => None, + } + } +} + +/// Reads the tier-3 wasm bundle signing public key. +/// +/// Unlike the other helpers in this module, an unusable value here is +/// not folded into the disabled default: see [`Tier3SigningKey`]. +pub fn gateway_tier3_signing_pubkey(env: &E) -> Tier3SigningKey { let Ok(raw) = env.var(ENV_GATEWAY_TIER3_SIGNING_PUBKEY) else { - return None; + return Tier3SigningKey::NotConfigured; }; let trimmed = raw.trim(); if trimmed.is_empty() { - return None; + return Tier3SigningKey::NotConfigured; } match Ed25519PublicKey::from_hex(trimmed) { - Ok(pubkey) => Some(pubkey), + Ok(pubkey) => Tier3SigningKey::Configured(pubkey), Err(err) => { warn!( error = %err, - "{ENV_GATEWAY_TIER3_SIGNING_PUBKEY} invalid; tier-3 bundle signing disabled", + "{ENV_GATEWAY_TIER3_SIGNING_PUBKEY} is not a valid ed25519 pubkey", ); - None + Tier3SigningKey::Invalid } } } diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs index ca105dea22..b8fb8bb954 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/env/tests.rs @@ -30,37 +30,51 @@ fn audit_publish_enables_on_truthy_value() { } #[test] -fn tier3_signing_pubkey_returns_none_when_unset() { +fn tier3_signing_pubkey_is_not_configured_when_unset() { let env = InMemoryEnv::new(); - assert!(gateway_tier3_signing_pubkey(&env).is_none()); + assert!(matches!( + gateway_tier3_signing_pubkey(&env), + Tier3SigningKey::NotConfigured + )); } #[test] -fn tier3_signing_pubkey_returns_none_for_empty_string() { +fn tier3_signing_pubkey_is_not_configured_for_empty_string() { // An empty value must surface as "no pubkey configured" rather // than a half-trusted invalid pubkey. Operators clearing the // env var to disable signing rely on this. let env = InMemoryEnv::new(); env.set(ENV_GATEWAY_TIER3_SIGNING_PUBKEY, " "); - assert!(gateway_tier3_signing_pubkey(&env).is_none()); + assert!(matches!( + gateway_tier3_signing_pubkey(&env), + Tier3SigningKey::NotConfigured + )); } #[test] -fn tier3_signing_pubkey_returns_none_for_invalid_hex() { +fn tier3_signing_pubkey_is_invalid_not_unconfigured_for_bad_hex() { + // The distinction that matters: an operator who typo'd the key + // asked for verification and must not silently get none. let env = InMemoryEnv::new(); env.set(ENV_GATEWAY_TIER3_SIGNING_PUBKEY, "not-hex"); - assert!(gateway_tier3_signing_pubkey(&env).is_none()); + assert!(matches!(gateway_tier3_signing_pubkey(&env), Tier3SigningKey::Invalid)); +} + +#[test] +fn tier3_signing_pubkey_invalid_has_no_configured_projection() { + assert!(Tier3SigningKey::Invalid.into_configured().is_none()); + assert!(Tier3SigningKey::NotConfigured.into_configured().is_none()); } #[test] fn tier3_signing_pubkey_parses_valid_hex() { // Test ed25519 pubkey from the a2a-redaction fixtures (32-byte - // hex). Asserts the success path produces a `Some(_)` without + // hex). Asserts the success path produces a usable key without // hard-coding the inner type's debug shape. let env = InMemoryEnv::new(); let hex = "abababababababababababababababababababababababababababababababab"; env.set(ENV_GATEWAY_TIER3_SIGNING_PUBKEY, hex); - assert!(gateway_tier3_signing_pubkey(&env).is_some()); + assert!(gateway_tier3_signing_pubkey(&env).into_configured().is_some()); } #[test] diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/policy_stack.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/policy_stack.rs index bb615cabe1..b541b785b5 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/policy_stack.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/policy_stack.rs @@ -22,9 +22,11 @@ use crate::policy::tier2_cel::{RealTier2CelEvaluator, Tier2CompiledBundle}; use crate::policy::tier3_redaction::load_tier3_manifests_from_bundle; use crate::policy::wasmtime_substrate::{Tier2State, WasmtimeSubstrate}; use crate::policy::{NoopTier3RedactionGate, RealTier3RedactionGate, Tier3RedactionGate, Tier3SkillManifest}; -use crate::runtime::env::{gateway_tier2_cel_enabled, gateway_tier3_signing_pubkey}; +use crate::runtime::env::{Tier3SigningKey, gateway_tier2_cel_enabled, gateway_tier3_signing_pubkey}; -use crate::constants::{ENV_POLICY_BUNDLE_DIR, ENV_POLICY_SKILLS, ENV_TIER3_REDACTION_ENABLED}; +use crate::constants::{ + ENV_GATEWAY_TIER3_SIGNING_PUBKEY, ENV_POLICY_BUNDLE_DIR, ENV_POLICY_SKILLS, ENV_TIER3_REDACTION_ENABLED, +}; /// The dispatch path's view of the policy stack. Each field is /// independently-replaceable in tests: @@ -63,10 +65,11 @@ impl GatewayPolicyStack { /// Boot the policy stack from environment variables. /// /// Returns the Noop stack when no bundle directory is configured, -/// when the directory is empty, or when the Wasmtime substrate fails -/// to load -- in every failure path the gateway still serves traffic -/// but applies no policy, with a warning logged so operators can -/// diagnose the misconfiguration. +/// when the directory is empty, when the configured bundle signing +/// pubkey is unusable, or when the Wasmtime substrate fails to load +/// -- in every failure path the gateway still serves traffic but +/// applies no policy, with a warning logged so operators can diagnose +/// the misconfiguration. /// /// Reads: /// - `A2A_GATEWAY_POLICY_BUNDLE_DIR` -- root of the policy bundle layout @@ -92,7 +95,15 @@ pub fn gateway_policy_stack_from_env(env: &E) -> GatewayPolicyStack } let bundle_path = WasmBundlePath::new(dir); - let tier3_signing_pubkey = gateway_tier3_signing_pubkey(env); + let signing_key = gateway_tier3_signing_pubkey(env); + if matches!(signing_key, Tier3SigningKey::Invalid) { + // Booting without the pubkey would run the very bundles this + // operator asked to have verified, unverified. A typo in the + // key must cost policy enforcement, not bundle authenticity. + warn!("{ENV_GATEWAY_TIER3_SIGNING_PUBKEY} is set but unusable; refusing to load unverified bundles"); + return GatewayPolicyStack::noop(); + } + let tier3_signing_pubkey = signing_key.into_configured(); let tier2_cel_active = gateway_tier2_cel_enabled(env); let tier2 = load_tier2_state(&bundle_path, tier2_cel_active); diff --git a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/policy_stack/tests.rs b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/policy_stack/tests.rs index d2679615b6..b65c5b6a74 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/src/runtime/policy_stack/tests.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/src/runtime/policy_stack/tests.rs @@ -57,3 +57,28 @@ fn from_env_returns_substrate_when_bundle_dir_set_to_any_path() { assert!(stack.substrate.is_some()); assert!(stack.tier3_manifests.is_empty()); } + +#[test] +fn from_env_returns_noop_when_signing_pubkey_is_unusable() { + // A bundle dir plus a mistyped signing pubkey must not boot a + // substrate that would execute those bundles unverified. Losing + // policy enforcement is the correct cost of the typo. + let env = InMemoryEnv::new(); + env.set(ENV_POLICY_BUNDLE_DIR, "/tmp/policy-bundle-for-boot-test"); + env.set(ENV_GATEWAY_TIER3_SIGNING_PUBKEY, "not-hex"); + let stack = gateway_policy_stack_from_env(&env); + assert!(stack.substrate.is_none()); + assert!(stack.tier3_manifests.is_empty()); +} + +#[test] +fn from_env_returns_substrate_when_signing_pubkey_is_valid() { + let env = InMemoryEnv::new(); + env.set(ENV_POLICY_BUNDLE_DIR, "/tmp/policy-bundle-for-boot-test"); + env.set( + ENV_GATEWAY_TIER3_SIGNING_PUBKEY, + "abababababababababababababababababababababababababababababababab", + ); + let stack = gateway_policy_stack_from_env(&env); + assert!(stack.substrate.is_some()); +} diff --git a/rsworkspace/crates/a2a/a2a-gateway/tests/aauth_roundtrip.rs b/rsworkspace/crates/a2a/a2a-gateway/tests/aauth_roundtrip.rs index 3e030baaf8..b53f8b2d82 100644 --- a/rsworkspace/crates/a2a/a2a-gateway/tests/aauth_roundtrip.rs +++ b/rsworkspace/crates/a2a/a2a-gateway/tests/aauth_roundtrip.rs @@ -80,7 +80,7 @@ fn mint_agent_jwt(ap_signing: &SigningKey, ap_kid: &str, ap_iss: &str, agent: &A "iat": now - 5, "exp": now + 600, "dwk": DWK_AGENT, - "cnf": Cnf { jwk: agent.jwk_val.clone() }, + "cnf": Cnf::public(agent.jwk_val.clone()).expect("test fixture is a public jwk"), }); encode(&header, &claims, &enc).expect("encode agent jwt") } diff --git a/rsworkspace/crates/a2a/a2a-identity-types/src/jwt.rs b/rsworkspace/crates/a2a/a2a-identity-types/src/jwt.rs index f6d97fab8c..aebe74e8b7 100644 --- a/rsworkspace/crates/a2a/a2a-identity-types/src/jwt.rs +++ b/rsworkspace/crates/a2a/a2a-identity-types/src/jwt.rs @@ -10,9 +10,15 @@ use crate::error::JwtError; /// Compact JWT string suitable for header transport. Validates shape on /// construction (3 dotted segments) without verifying signature. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] pub struct CallerJwtHeaderValue(String); +impl fmt::Debug for CallerJwtHeaderValue { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("CallerJwtHeaderValue").field(&"").finish() + } +} + impl CallerJwtHeaderValue { /// Builds a header value from a [`MintedUserJwt`]. The minted JWT is already /// shape-validated at construction, so this is infallible. @@ -54,9 +60,15 @@ impl fmt::Display for CallerJwtHeaderValue { /// User JWT minted for bridge/gateway consumption; carried as the inner /// `nats.jwt` on wire responses. Validates shape on construction but does not /// verify the signature — that lives gateway-side. -#[derive(Debug, Clone, PartialEq, Eq)] +#[derive(Clone, PartialEq, Eq)] pub struct MintedUserJwt(String); +impl fmt::Debug for MintedUserJwt { + fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { + f.debug_tuple("MintedUserJwt").field(&"").finish() + } +} + impl MintedUserJwt { /// Constructs a minted JWT from an already-shape-valid compact JWT string. /// Returns an error if the input is not three non-empty `.`-separated diff --git a/rsworkspace/crates/a2a/a2a-identity-types/src/jwt/tests.rs b/rsworkspace/crates/a2a/a2a-identity-types/src/jwt/tests.rs index c75c911e34..036d3a8272 100644 --- a/rsworkspace/crates/a2a/a2a-identity-types/src/jwt/tests.rs +++ b/rsworkspace/crates/a2a/a2a-identity-types/src/jwt/tests.rs @@ -125,3 +125,16 @@ fn ensure_fresh_rejects_future_nbf() { let err = MintedUserJwt::new(token).unwrap().ensure_fresh().unwrap_err(); assert!(matches!(err, JwtError::Decode(ref msg) if msg.contains("not yet valid"))); } + +#[test] +fn debug_does_not_leak_bearer_credentials() { + let minted = MintedUserJwt::new("hhh.ppp.sss").expect("valid shape"); + let minted_dbg = format!("{minted:?}"); + assert!(!minted_dbg.contains("ppp"), "{minted_dbg}"); + assert!(minted_dbg.contains(""), "{minted_dbg}"); + + let header = CallerJwtHeaderValue::from_minted(&minted); + let header_dbg = format!("{header:?}"); + assert!(!header_dbg.contains("ppp"), "{header_dbg}"); + assert!(header_dbg.contains(""), "{header_dbg}"); +} diff --git a/rsworkspace/crates/a2a/a2a-redaction/src/signed_bundle/verify.rs b/rsworkspace/crates/a2a/a2a-redaction/src/signed_bundle/verify.rs index 6ecca25a5e..dd7780e392 100644 --- a/rsworkspace/crates/a2a/a2a-redaction/src/signed_bundle/verify.rs +++ b/rsworkspace/crates/a2a/a2a-redaction/src/signed_bundle/verify.rs @@ -1,4 +1,3 @@ -use ed25519_dalek::Verifier; use sha2::{Digest, Sha256}; use super::digest::Sha256Digest; @@ -88,8 +87,13 @@ pub fn verify_signed_bundle( let signature = envelope.signature_bytes(&skill_id)?; let message = sign_bundle_digest(envelope.version, &skill_id, expected_manifest, expected_wasm); + // `verify_strict` rather than `verify`: this is a code-signing + // decision, so the permissive checks are the wrong default. Strict + // rejects small-order and non-canonically-encoded key/nonce points, + // which is what closes the gap where one signature validates under + // more than one public key. verifying_key - .verify(&message, &signature.dalek_signature()?) + .verify_strict(&message, &signature.dalek_signature()?) .map_err(|_| SignatureVerificationError::SignatureVerificationFailed { skill_id: skill_id.to_string(), }) diff --git a/rsworkspace/crates/aauth/trogon-aauth-as/src/error.rs b/rsworkspace/crates/aauth/trogon-aauth-as/src/error.rs index f8befee3bd..05ca39b798 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-as/src/error.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-as/src/error.rs @@ -82,6 +82,8 @@ pub enum MintError { Encode(#[source] jsonwebtoken::errors::Error), #[error("ttl overflowed i64 when added to iat ({iat} + {ttl_secs})")] TtlOverflow { iat: i64, ttl_secs: i64 }, + #[error("confirmation key rejected: {0}")] + Cnf(#[from] trogon_identity_types::aauth::CnfError), } /// Top-level error for one AS token-endpoint evaluation, per "Token Endpoint diff --git a/rsworkspace/crates/aauth/trogon-aauth-as/src/mint.rs b/rsworkspace/crates/aauth/trogon-aauth-as/src/mint.rs index 32670bfa0c..993a718b37 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-as/src/mint.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-as/src/mint.rs @@ -129,9 +129,7 @@ pub fn mint_auth_jwt( consent_id: inputs.consent_id.map(str::to_string), resource: inputs.resource.map(str::to_string), act: inputs.act.clone(), - cnf: Some(Cnf { - jwk: inputs.cnf_jwk.clone(), - }), + cnf: Some(Cnf::public(inputs.cnf_jwk.clone())?), }; let mut header = Header::new(alg); diff --git a/rsworkspace/crates/aauth/trogon-aauth-as/src/pending/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-as/src/pending/tests.rs index c3916ef4e7..1798a28ff2 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-as/src/pending/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-as/src/pending/tests.rs @@ -13,9 +13,8 @@ fn verified_request() -> VerifiedRequest { iat: 0, exp: 1000, dwk: "aauth-agent.json".into(), - cnf: Cnf { - jwk: serde_json::json!({"kty": "EC"}), - }, + cnf: Cnf::public(serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA", "y": "BBB"})) + .expect("test fixture is a public jwk"), ps: None, }; let resource = ResourceClaims { diff --git a/rsworkspace/crates/aauth/trogon-aauth-as/src/policy/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-as/src/policy/tests.rs index 4ae4539417..dab31b45f0 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-as/src/policy/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-as/src/policy/tests.rs @@ -13,9 +13,8 @@ fn agent_claims() -> AgentClaims { iat: 0, exp: 1000, dwk: "aauth-agent.json".into(), - cnf: Cnf { - jwk: serde_json::json!({"kty": "EC"}), - }, + cnf: Cnf::public(serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA", "y": "BBB"})) + .expect("test fixture is a public jwk"), ps: None, } } diff --git a/rsworkspace/crates/aauth/trogon-aauth-as/src/request/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-as/src/request/tests.rs index 1255a147e7..cac9901db6 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-as/src/request/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-as/src/request/tests.rs @@ -11,9 +11,8 @@ fn as_token_context_carries_all_verified_inputs() { iat: 0, exp: 1000, dwk: "aauth-agent.json".into(), - cnf: Cnf { - jwk: serde_json::json!({"kty": "EC"}), - }, + cnf: Cnf::public(serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA", "y": "BBB"})) + .expect("test fixture is a public jwk"), ps: None, }; let resource = ResourceClaims { diff --git a/rsworkspace/crates/aauth/trogon-aauth-as/src/server.rs b/rsworkspace/crates/aauth/trogon-aauth-as/src/server.rs index 0056ab64d2..3e8dd4656b 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-as/src/server.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-as/src/server.rs @@ -273,7 +273,7 @@ fn cnf_jwk_for(verified: &VerifiedRequest) -> serde_json::Value { .as_ref() .unwrap_or(&verified.agent_claims) .cnf - .jwk + .jwk() .clone() } diff --git a/rsworkspace/crates/aauth/trogon-aauth-as/src/test_support.rs b/rsworkspace/crates/aauth/trogon-aauth-as/src/test_support.rs index b5cd8a0c82..d4ddcf3d69 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-as/src/test_support.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-as/src/test_support.rs @@ -80,7 +80,7 @@ pub fn mint_agent_jwt( "iat": now - 5, "exp": now + 600, "dwk": DWK_AGENT, - "cnf": Cnf { jwk: agent_jwk.clone() }, + "cnf": Cnf::public(agent_jwk.clone()).expect("test fixture is a public jwk"), }); if let Some(parent) = parent_agent { claims["parent_agent"] = serde_json::Value::String(parent.to_string()); diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/src/agent/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-person/src/agent/tests.rs index 5a8f4f0de7..3204a66718 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/src/agent/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/src/agent/tests.rs @@ -46,9 +46,7 @@ fn mint_agent_jwt(fixture: &KeyFixture, iss: &str, sub: &str, kid: &str) -> Stri iat: now - 5, exp: now + 600, dwk: "aauth-agent.json".to_string(), - cnf: Cnf { - jwk: fixture.jwk.clone(), - }, + cnf: Cnf::public(fixture.jwk.clone()).expect("test fixture is a public jwk"), ps: None, }; let mut header = Header::new(Algorithm::ES256); diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/src/error.rs b/rsworkspace/crates/aauth/trogon-aauth-person/src/error.rs index 6cd9f69c66..b0ecb29c26 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/src/error.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/src/error.rs @@ -73,6 +73,8 @@ pub enum MintError { Encode(#[source] jsonwebtoken::errors::Error), #[error("ttl overflowed i64 when added to iat ({iat} + {ttl_secs})")] TtlOverflow { iat: i64, ttl_secs: i64 }, + #[error("confirmation key rejected: {0}")] + Cnf(#[from] trogon_identity_types::aauth::CnfError), } /// Failures operating on a pending request, per "Clarification Chat" and diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/src/http/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-person/src/http/tests.rs index ff8e18f910..916090df63 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/src/http/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/src/http/tests.rs @@ -55,9 +55,7 @@ fn mint_agent_jwt(fixture: &KeyFixture, iss: &str, sub: &str, kid: &str) -> Stri iat: now - 5, exp: now + 600, dwk: "aauth-agent.json".to_string(), - cnf: Cnf { - jwk: fixture.jwk.clone(), - }, + cnf: Cnf::public(fixture.jwk.clone()).expect("test fixture is a public jwk"), ps: None, }; let mut header = Header::new(Algorithm::ES256); @@ -531,9 +529,7 @@ async fn seed_pending(store: &InMemoryStore, phase: PendingPhase) -> String { iat: now_unix() - 5, exp: now_unix() + 600, dwk: "aauth-agent.json".to_string(), - cnf: Cnf { - jwk: agent_fixture.jwk.clone(), - }, + cnf: Cnf::public(agent_fixture.jwk.clone()).expect("test fixture is a public jwk"), ps: None, }; let resource = ResourceClaims { diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/src/mint.rs b/rsworkspace/crates/aauth/trogon-aauth-person/src/mint.rs index bcfb0d8124..7171e9bd49 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/src/mint.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/src/mint.rs @@ -133,9 +133,7 @@ pub fn mint_auth_jwt( consent_id: inputs.consent_id.map(str::to_string), resource: inputs.resource.map(str::to_string), act: inputs.act.clone(), - cnf: Some(Cnf { - jwk: inputs.cnf_jwk.clone(), - }), + cnf: Some(Cnf::public(inputs.cnf_jwk.clone())?), }; let mut header = Header::new(alg); diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/src/pending/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-person/src/pending/tests.rs index f42df4d3b9..0f0f331d7b 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/src/pending/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/src/pending/tests.rs @@ -11,9 +11,8 @@ fn agent_claims() -> AgentClaims { iat: 0, exp: 1000, dwk: "aauth-agent.json".to_string(), - cnf: Cnf { - jwk: serde_json::json!({"kty": "EC"}), - }, + cnf: Cnf::public(serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA", "y": "BBB"})) + .expect("test fixture is a public jwk"), ps: None, } } diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/src/server.rs b/rsworkspace/crates/aauth/trogon-aauth-person/src/server.rs index c3d20fff4d..ebc0390f95 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/src/server.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/src/server.rs @@ -206,7 +206,7 @@ where cnf_jwk: verified .subagent_claims .as_ref() - .map_or(&verified.agent_claims.cnf.jwk, |s| &s.cnf.jwk) + .map_or(verified.agent_claims.cnf.jwk(), |s| s.cnf.jwk()) .clone(), scope: &scope, act, diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/src/server/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-person/src/server/tests.rs index b77386ca58..e34bd2af34 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/src/server/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/src/server/tests.rs @@ -50,9 +50,7 @@ fn mint_agent_jwt(fixture: &KeyFixture, iss: &str, sub: &str, kid: &str) -> Stri iat: now - 5, exp: now + 600, dwk: "aauth-agent.json".to_string(), - cnf: Cnf { - jwk: fixture.jwk.clone(), - }, + cnf: Cnf::public(fixture.jwk.clone()).expect("test fixture is a public jwk"), ps: None, }; let mut header = Header::new(Algorithm::ES256); @@ -891,9 +889,8 @@ async fn respond_to_clarification_on_terminal_pending_returns_gone() { iat: 0, exp: 1000, dwk: "aauth-agent.json".to_string(), - cnf: Cnf { - jwk: serde_json::json!({"kty": "EC"}), - }, + cnf: Cnf::public(serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA", "y": "BBB"})) + .expect("test fixture is a public jwk"), ps: None, }, trogon_identity_types::aauth::ResourceClaims { diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/src/store/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-person/src/store/tests.rs index 54af8ca281..5203966188 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/src/store/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/src/store/tests.rs @@ -12,9 +12,8 @@ fn agent_claims() -> AgentClaims { iat: 0, exp: 1000, dwk: "aauth-agent.json".to_string(), - cnf: Cnf { - jwk: serde_json::json!({"kty": "EC"}), - }, + cnf: Cnf::public(serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA", "y": "BBB"})) + .expect("test fixture is a public jwk"), ps: None, } } diff --git a/rsworkspace/crates/aauth/trogon-aauth-person/tests/person_server_e2e.rs b/rsworkspace/crates/aauth/trogon-aauth-person/tests/person_server_e2e.rs index d302fc739b..58405e86ba 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-person/tests/person_server_e2e.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-person/tests/person_server_e2e.rs @@ -64,7 +64,7 @@ fn mint_agent_jwt(ap_signing: &SigningKey, ap_kid: &str, ap_iss: &str, agent: &A "iat": now - 5, "exp": now + 600, "dwk": DWK_AGENT, - "cnf": Cnf { jwk: agent.jwk_val.clone() }, + "cnf": Cnf::public(agent.jwk_val.clone()).expect("test fixture is a public jwk"), }); encode(&header, &claims, &enc).expect("encode agent jwt") } diff --git a/rsworkspace/crates/aauth/trogon-aauth-sdk/src/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-sdk/src/tests.rs index cd2004ba66..526c1dfc78 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-sdk/src/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-sdk/src/tests.rs @@ -41,7 +41,7 @@ fn mint_agent_jwt(ap_signing: &SigningKey, agent_jwk_val: &serde_json::Value, no "iat": now - 5, "exp": now + 600, "dwk": DWK_AGENT, - "cnf": Cnf { jwk: agent_jwk_val.clone() }, + "cnf": Cnf::public(agent_jwk_val.clone()).expect("test fixture is a public jwk"), }); encode(&header, &claims, &enc).expect("encode agent jwt") } diff --git a/rsworkspace/crates/aauth/trogon-aauth-sdk/src/verify_response.rs b/rsworkspace/crates/aauth/trogon-aauth-sdk/src/verify_response.rs index f625c69822..fcee083a33 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-sdk/src/verify_response.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-sdk/src/verify_response.rs @@ -111,7 +111,7 @@ pub fn verify_auth_claims( .cnf .as_ref() .ok_or(VerifyResponseError::ConfirmationClaimMissing)?; - let token_jkt = jwk_thumbprint(&cnf.jwk).map_err(VerifyResponseError::ConfirmationKeyThumbprint)?; + let token_jkt = jwk_thumbprint(cnf.jwk()).map_err(VerifyResponseError::ConfirmationKeyThumbprint)?; let own_jkt = jwk_thumbprint(own_agent_jwk).map_err(VerifyResponseError::ConfirmationKeyThumbprint)?; if token_jkt != own_jkt { return Err(VerifyResponseError::ConfirmationKeyMismatch); diff --git a/rsworkspace/crates/aauth/trogon-aauth-sdk/src/verify_response/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-sdk/src/verify_response/tests.rs index 921feac56c..090c68eb25 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-sdk/src/verify_response/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-sdk/src/verify_response/tests.rs @@ -85,7 +85,7 @@ fn valid_claims(agent_jwk: serde_json::Value) -> AuthClaims { consent_id: None, resource: None, act: None, - cnf: Some(Cnf { jwk: agent_jwk }), + cnf: Some(Cnf::public(agent_jwk).expect("test fixture is a public jwk")), } } diff --git a/rsworkspace/crates/aauth/trogon-aauth-verify/Cargo.toml b/rsworkspace/crates/aauth/trogon-aauth-verify/Cargo.toml index 841110cc96..d0c2bf5f97 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-verify/Cargo.toml +++ b/rsworkspace/crates/aauth/trogon-aauth-verify/Cargo.toml @@ -18,8 +18,10 @@ async-trait = { workspace = true } base64 = { workspace = true } # AAuth tokens rely on JWS shapes (esp. EC `kid`-based selection) that the # crate's parser only models cleanly from jsonwebtoken 10.x; keep the -# version pin local to this crate so main's other jsonwebtoken 9.3 -# consumers (auth-callout, etc.) are not forced to upgrade in lockstep. +# version pin local to this crate so the workspace's other jsonwebtoken +# consumers (auth-callout, on 11.x) are not forced to move in lockstep. +# The cost is two majors linked into a2a-gateway, which also means +# verification hardening cannot be shared between the two paths as code. jsonwebtoken = { version = "=10.4.0", features = ["rust_crypto"] } # rustls-tls only, never native-tls, per ADR#0015. reqwest = { workspace = true, features = ["json", "rustls-tls"] } diff --git a/rsworkspace/crates/aauth/trogon-aauth-verify/src/http_pop.rs b/rsworkspace/crates/aauth/trogon-aauth-verify/src/http_pop.rs index 7114cb7b53..5ef4d6f5fd 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-verify/src/http_pop.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-verify/src/http_pop.rs @@ -25,6 +25,17 @@ //! - `aauth-mission` (OPTIONAL): the raw `AAuth-Mission` header field value, //! required to be covered only when the request carries that header. //! +//! ## Body integrity +//! +//! Covering `content-digest` binds the *header value* to the signature; on +//! its own that says nothing about the bytes in `body`. Whenever the header +//! is present this verifier therefore also recomputes SHA-256 over the +//! request body and compares, exactly as [`crate::nats_pop`] does for its +//! own profile. Without that second half a captured request could have its +//! body replaced (or removed entirely, which drops it out of the +//! has-a-body branch) while the untouched digest header kept the signature +//! valid. +//! //! `Signature-Input`, `Signature`, and `Signature-Key` are parsed only in the //! single-member Dictionary shape the draft's examples use throughout //! (`sig=(...)`, `sig=:...:`, `sig=jwt;jwt="..."`) -- this verifier does not @@ -48,7 +59,10 @@ //! NATS PoP profile ([`crate::nats_pop`]), which mints an explicit nonce //! header -- the HTTP profile has none to reuse. +use base64::Engine; +use base64::engine::general_purpose::{STANDARD, STANDARD_NO_PAD, URL_SAFE_NO_PAD}; use jsonwebtoken::{Algorithm, DecodingKey, crypto::verify, jwk::Jwk}; +use sha2::{Digest, Sha256}; use trogon_identity_types::aauth::headers; use crate::constants::HTTP_SECURITY_HEADERS; @@ -159,6 +173,15 @@ pub enum HttpPopError { /// versa the header is covered but absent. #[error("content-digest header required but missing")] MissingContentDigest, + /// `Content-Digest` was present but carried no `sha-256` entry, or that + /// entry was not a decodable Byte Sequence. Refused rather than skipped, + /// because skipping an unparseable digest is indistinguishable from + /// having no body integrity at all. + #[error("content-digest header has no decodable sha-256 entry")] + UnsupportedContentDigest, + /// `Content-Digest` did not match SHA-256 over the request body. + #[error("content-digest does not match the request body")] + ContentDigestMismatch, /// The `AAuth-Mission` header is covered by the signature but absent /// from the request, or present but absent from coverage while a mission /// claim is expected -- see [`crate::mission`] for claim-level checks. @@ -257,8 +280,9 @@ impl HttpPopVerifier { /// (Server)" (#verification): extracts `Signature-Key`, `Signature-Input`, /// `Signature`; verifies required component coverage; verifies the JWT /// layer of the presented token (agent or auth); verifies the HTTP - /// Message Signature against the token's `cnf.jwk`; and enforces - /// freshness plus the replay tuple from (#freshness-and-replay). + /// Message Signature against the token's `cnf.jwk`; recomputes + /// `Content-Digest` over the body; and enforces freshness plus the replay + /// tuple from (#freshness-and-replay). pub async fn verify(&self, req: &HttpRequest) -> Result { if self.max_skew_secs < 0 { return Err(HttpPopError::NegativeMaxSkew(self.max_skew_secs)); @@ -300,7 +324,7 @@ impl HttpPopVerifier { .verify_agent(&jwt) .await .map_err(HttpPopError::Agent)?; - let cnf_jwk = verified.claims.cnf.jwk.clone(); + let cnf_jwk = verified.claims.cnf.jwk().clone(); let jkt = verified.jkt.clone(); (cnf_jwk, jkt, VerifiedPresenter::Agent(verified)) } @@ -313,19 +337,20 @@ impl HttpPopVerifier { let cnf = verified.claims.cnf.clone().ok_or(HttpPopError::InvalidConfirmationKey( InvalidConfirmationKeyError::MissingConfirmationClaim, ))?; - let jkt = crate::jkt::jwk_thumbprint(&cnf.jwk).map_err(|e| { + let jkt = crate::jkt::jwk_thumbprint(cnf.jwk()).map_err(|e| { HttpPopError::InvalidConfirmationKey(InvalidConfirmationKeyError::StructurallyIncomplete(e)) })?; let presenter = VerifiedPresenter::Auth(VerifiedAuthPresenter { auth: verified, jkt: jkt.clone(), }); - (cnf.jwk, jkt, presenter) + (cnf.jwk().clone(), jkt, presenter) } }; let base = build_signature_base(req, &parsed_input)?; verify_signature_with_jwk(&cnf_jwk, base.as_bytes(), &sig_b64)?; + verify_content_digest(req)?; let replay_key = format!( "http-pop:{jkt}:{}:{}:{}:{}", @@ -502,6 +527,90 @@ fn verify_covered_components(req: &HttpRequest, components: &[String]) -> Result Ok(()) } +/// Recomputes SHA-256 over the request body and compares it against the +/// `Content-Digest` header. +/// +/// Keyed on the header being *present* rather than on +/// [`HttpRequest::has_body`], so that stripping the body from a captured +/// request does not slip past by falling out of the has-a-body branch. The +/// header cannot itself be stripped: [`verify_covered_components`] requires it +/// to be covered whenever a body is present, and [`build_signature_base`] +/// fails to reconstruct the base for a covered field that is missing. +fn verify_content_digest(req: &HttpRequest) -> Result<(), HttpPopError> { + let Some(raw) = req.header(headers::CONTENT_DIGEST) else { + return Ok(()); + }; + let supplied = parse_sha256_content_digest(raw).ok_or(HttpPopError::UnsupportedContentDigest)?; + let expected = Sha256::digest(req.body.as_deref().unwrap_or(&[])); + if supplied.as_slice() != expected.as_slice() { + return Err(HttpPopError::ContentDigestMismatch); + } + Ok(()) +} + +/// Extracts the raw `sha-256` digest bytes from an RFC 9530 `Content-Digest` +/// value, a Structured Fields Dictionary of algorithm keys to Byte Sequences +/// (`sha-256=::`). Other algorithm entries are skipped rather than +/// rejected, since a peer is free to send additional ones. +/// +/// Two RFC 8941 Dictionary rules matter for interop and are honoured here. +/// A member's Item may carry parameters (`sha-256=:...:;q=1`), which are not +/// part of the Byte Sequence and are ignored. A repeated key resolves to its +/// *last* occurrence, so the scan cannot return early: taking the first would +/// make this verifier disagree with any RFC-compliant peer about which digest +/// a duplicated `sha-256` member names. +/// +/// Last-wins ranges over the members this reader can read. A `sha-256` member +/// it cannot fails the whole value rather than yielding to a later one, which +/// is what an RFC-compliant peer does too: RFC 8941 discards a field that does +/// not parse, and a discarded `Content-Digest` leaves nothing to check the body +/// against. Skipping ahead would instead let a sender bury a member this +/// verifier cannot read under a trailing one it can. +/// +/// So what a member is keyed on, not whether it carries a Byte Sequence, +/// decides between skipping it and failing. RFC 8941 reads a member with no +/// `=` as the Boolean true, which is a `sha-256` naming no digest once it wins +/// last, and a peer is still free to key its own algorithms however it likes. +/// +/// This is a targeted reader for one Byte Sequence member, not a general +/// Structured Fields parser. It does not model Inner Lists, and a parameter +/// whose value is a String containing `,` or `;` would split wrongly; no +/// parameter defined for `Content-Digest` takes such a value. +fn parse_sha256_content_digest(raw: &str) -> Option> { + let mut last = None; + for member in raw.split(',') { + let (key, value) = match member.split_once('=') { + Some((key, value)) => (key, Some(value)), + None => (member, None), + }; + // A Boolean member's parameters sit on the key, an Item's on its value. + let key = key.split(';').next().unwrap_or(key); + if !key.trim().eq_ignore_ascii_case("sha-256") { + continue; + } + // The Byte Sequence ends at its closing colon; anything after that is + // the parameter list. + let value = value?.trim_start(); + let inner = value.strip_prefix(':')?.split_once(':')?.0; + last = Some(decode_base64_any_alphabet(inner)?); + } + last +} + +/// Decodes a digest that may arrive in any of the base64 alphabets seen in +/// practice. RFC 8941 Byte Sequences are standard padded base64, which is +/// what a conformant third-party client sends, while +/// [`crate::nats_pop::content_digest_sha256`] emits URL-safe unpadded. Both +/// have to decode to the same 32 bytes, so comparison happens on the decoded +/// digest rather than on the header string. +fn decode_base64_any_alphabet(encoded: &str) -> Option> { + STANDARD + .decode(encoded) + .or_else(|_| STANDARD_NO_PAD.decode(encoded)) + .or_else(|_| URL_SAFE_NO_PAD.decode(encoded)) + .ok() +} + /// Builds the RFC 9421 canonical signature base for the supported component /// subset (see module docs). For each covered component identifier, in the /// order given by `Signature-Input`, appends `"": \n`; then a diff --git a/rsworkspace/crates/aauth/trogon-aauth-verify/src/http_pop/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-verify/src/http_pop/tests.rs index 7585dc5c3f..a7c1f29052 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-verify/src/http_pop/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-verify/src/http_pop/tests.rs @@ -170,37 +170,239 @@ async fn verify_accepts_ed25519_agent_presenter() { assert!(matches!(result, VerifiedPresenter::Agent(_))); } +/// Signs a request carrying `body` with a matching, covered `Content-Digest`. +fn signed_body_request( + fixture: &crate::test_support::EcFixture, + jwt: &str, + body: &[u8], + digest: String, +) -> HttpRequest { + let mut req = base_request(&signature_key_header(jwt)); + req.body = Some(body.to_vec()); + req.headers.push((headers::CONTENT_DIGEST.to_string(), digest)); + let mut components = REQUIRED_COMPONENTS.to_vec(); + components.push("content-digest"); + sign_request(fixture, &mut req, 1000, &components); + req +} + #[tokio::test(flavor = "current_thread")] -async fn verify_rejects_tampered_body_content_digest_not_recomputed() { +async fn verify_accepts_body_matching_content_digest() { let fixture = p256_fixture("k1"); let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); let verifier = verifier_at(jwks, 1000, "resource.example"); - let body = br#"{"scope":"data.read"}"#.to_vec(); - let digest = crate::nats_pop::content_digest_sha256(&body); - let mut req = base_request(&signature_key_header(&jwt)); - req.body = Some(body); - req.headers.push((headers::CONTENT_DIGEST.to_string(), digest)); - let mut components = REQUIRED_COMPONENTS.to_vec(); - components.push("content-digest"); - sign_request(&fixture, &mut req, 1000, &components); + let body = br#"{"scope":"data.read"}"#; + let digest = crate::nats_pop::content_digest_sha256(body); + let req = signed_body_request(&fixture, &jwt, body, digest); + + let result = verifier.verify(&req).await.expect("matching digest verifies"); + assert!(matches!(result, VerifiedPresenter::Agent(_))); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_accepts_rfc9530_padded_base64_content_digest() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // RFC 8941 Byte Sequences are standard padded base64, which is what a + // conformant third-party client sends; our own emitter uses URL-safe + // unpadded. Both encode the same digest and both must be accepted. + let body = br#"{"scope":"data.read"}"#; + let padded = format!("sha-256=:{}:", STANDARD.encode(Sha256::digest(body))); + let req = signed_body_request(&fixture, &jwt, body, padded); + + verifier.verify(&req).await.expect("padded base64 digest verifies"); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_rejects_tampered_body_against_covered_content_digest() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); - // Tamper with the body after signing without updating content-digest. - // The digest header itself is covered and unmodified, so the signature - // still verifies -- catching a body/digest mismatch is the caller's - // responsibility (recompute and compare), since #covered-components only - // requires that content-digest be *covered*, not that this crate - // recompute it against a body it is never given as a signed component. + let body = br#"{"scope":"data.read"}"#; + let digest = crate::nats_pop::content_digest_sha256(body); + let mut req = signed_body_request(&fixture, &jwt, body, digest); + + // Swap the body after signing, leaving the covered digest header intact. + // The signature still verifies over the untouched header, so only + // recomputing the digest against the body catches this. req.body = Some(br#"{"scope":"data.write"}"#.to_vec()); - let recomputed = crate::nats_pop::content_digest_sha256(req.body.as_ref().unwrap()); - let supplied = req.header(headers::CONTENT_DIGEST).unwrap().to_string(); - assert_ne!(recomputed, supplied, "tampering must be visible via digest mismatch"); - - verifier - .verify(&req) - .await - .expect("signature over untouched digest header still verifies"); + + let err = verifier.verify(&req).await.unwrap_err(); + assert!(matches!(err, HttpPopError::ContentDigestMismatch)); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_rejects_stripped_body_against_covered_content_digest() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + let body = br#"{"scope":"data.read"}"#; + let digest = crate::nats_pop::content_digest_sha256(body); + let mut req = signed_body_request(&fixture, &jwt, body, digest); + + // Dropping the body entirely falls out of the has-a-body branch, so a + // coverage-only check would let it through. + req.body = None; + + let err = verifier.verify(&req).await.unwrap_err(); + assert!(matches!(err, HttpPopError::ContentDigestMismatch)); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_accepts_a_parameterized_sha256_content_digest_item() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // RFC 8941 lets any Dictionary member's Item carry parameters. They are + // not part of the Byte Sequence, so a peer that sends them must still + // interoperate rather than be read as an unsupported digest. + let body = br#"{"scope":"data.read"}"#; + let parameterized = format!("sha-256=:{}:;q=1", STANDARD.encode(Sha256::digest(body))); + let req = signed_body_request(&fixture, &jwt, body, parameterized); + + verifier.verify(&req).await.expect("parameterized item verifies"); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_accepts_a_content_digest_carrying_a_bare_key_member() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // RFC 8941 spells a Dictionary member with no `=` as the Boolean true, so + // one carries no Byte Sequence to compare a body against. Reading past it + // keeps an unknown member from displacing the `sha-256` beside it. + let body = br#"{"scope":"data.read"}"#; + let with_bare_key = format!("unixsum, sha-256=:{}:", STANDARD.encode(Sha256::digest(body))); + let req = signed_body_request(&fixture, &jwt, body, with_bare_key); + + verifier.verify(&req).await.expect("a bare-key member is stepped over"); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_rejects_a_bare_sha256_member_trailing_a_readable_one() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // A trailing bare `sha-256` is the Boolean true, and last-wins hands the + // key to it: an RFC-compliant peer is left with no digest to check. Keeping + // the earlier Byte Sequence would let a sender show this verifier a body it + // has agreed to and every other component nothing at all. + let body = br#"{"scope":"data.read"}"#; + let live = STANDARD.encode(Sha256::digest(body)); + let displaced = format!("sha-256=:{live}:, sha-256"); + let req = signed_body_request(&fixture, &jwt, body, displaced); + + let err = verifier.verify(&req).await.unwrap_err(); + assert!(matches!(err, HttpPopError::UnsupportedContentDigest)); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_rejects_a_parameterized_bare_sha256_member() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // The same displacement dressed as a parameterized member: `sha-256;q=1` + // splits at the parameter's `=`, so the algorithm has to be read off the + // key ahead of the `;` for the member to be recognised as the Boolean it is. + let body = br#"{"scope":"data.read"}"#; + let live = STANDARD.encode(Sha256::digest(body)); + let displaced = format!("sha-256=:{live}:, sha-256;q=1"); + let req = signed_body_request(&fixture, &jwt, body, displaced); + + let err = verifier.verify(&req).await.unwrap_err(); + assert!(matches!(err, HttpPopError::UnsupportedContentDigest)); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_resolves_a_duplicated_sha256_member_to_the_last_one() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // RFC 8941 Dictionary parsing keeps the last member for a repeated key. + // Reading the first instead would let a peer show one digest to this + // verifier and a different one to every other RFC-compliant component. + let body = br#"{"scope":"data.read"}"#; + let stale = STANDARD.encode(Sha256::digest(br#"{"scope":"data.write"}"#)); + let live = STANDARD.encode(Sha256::digest(body)); + let duplicated = format!("sha-256=:{stale}:, sha-256=:{live}:"); + let req = signed_body_request(&fixture, &jwt, body, duplicated); + + verifier.verify(&req).await.expect("last duplicate member wins"); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_rejects_when_the_last_duplicated_sha256_member_mismatches() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // The mirror of the case above: a correct leading member must not rescue + // a trailing one that does not match the body. + let body = br#"{"scope":"data.read"}"#; + let live = STANDARD.encode(Sha256::digest(body)); + let stale = STANDARD.encode(Sha256::digest(br#"{"scope":"data.write"}"#)); + let duplicated = format!("sha-256=:{live}:, sha-256=:{stale}:"); + let req = signed_body_request(&fixture, &jwt, body, duplicated); + + let err = verifier.verify(&req).await.unwrap_err(); + assert!(matches!(err, HttpPopError::ContentDigestMismatch)); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_rejects_a_readable_sha256_member_trailing_an_unreadable_one() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // Last-wins ranges over the members this verifier can read. RFC 8941 + // discards a field carrying a member that does not parse, so a `sha-256` + // whose Byte Sequence cannot be decoded refuses the request rather than + // yielding to whatever follows it. Recovering would let a sender bury an + // unreadable member under a trailing readable one. + let body = br#"{"scope":"data.read"}"#; + let live = STANDARD.encode(Sha256::digest(body)); + let unreadable = format!("sha-256=:not base64!:, sha-256=:{live}:"); + let req = signed_body_request(&fixture, &jwt, body, unreadable); + + let err = verifier.verify(&req).await.unwrap_err(); + assert!(matches!(err, HttpPopError::UnsupportedContentDigest)); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_rejects_content_digest_without_sha256_entry() { + let fixture = p256_fixture("k1"); + let jwt = agent_jwt(&fixture, "k1", "agent-provider.example"); + let jwks = jwks_with_key("agent-provider.example", fixture.jwk.clone()); + let verifier = verifier_at(jwks, 1000, "resource.example"); + + // Only algorithms this verifier cannot check: refused rather than + // skipped, since skipping is indistinguishable from no body integrity. + let body = br#"{"scope":"data.read"}"#; + let req = signed_body_request(&fixture, &jwt, body, "sha-512=:YWJj:".to_string()); + + let err = verifier.verify(&req).await.unwrap_err(); + assert!(matches!(err, HttpPopError::UnsupportedContentDigest)); } #[tokio::test(flavor = "current_thread")] diff --git a/rsworkspace/crates/aauth/trogon-aauth-verify/src/nats_pop.rs b/rsworkspace/crates/aauth/trogon-aauth-verify/src/nats_pop.rs index 678adad15e..d80d85318b 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-verify/src/nats_pop.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-verify/src/nats_pop.rs @@ -264,7 +264,7 @@ impl NatsPopVerifier { // and a later valid retry from the same agent is wrongly rejected as // a replay. let canonical = envelope.canonical_base(req.subject, req.reply, &verified_agent.jkt); - verify_signature_with_jwk(&verified_agent.claims.cnf.jwk, canonical.as_bytes(), sig)?; + verify_signature_with_jwk(verified_agent.claims.cnf.jwk(), canonical.as_bytes(), sig)?; // Replay protection only fires once the signature has authenticated // the request. Derive the TTL using saturating arithmetic and floor diff --git a/rsworkspace/crates/aauth/trogon-aauth-verify/src/replay.rs b/rsworkspace/crates/aauth/trogon-aauth-verify/src/replay.rs index 5a9e3ce8ec..7724722504 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-verify/src/replay.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-verify/src/replay.rs @@ -1,8 +1,14 @@ //! Replay-protection store: dedup `jti` and PoP nonces with TTLs. //! -//! The production-grade implementation lives in `trogon-aauth-person` backed by -//! NATS JetStream KV (TTL per key). This module provides the trait + an in-memory -//! variant used by the gateway when no shared store is configured. +//! This module provides the [`ReplayStore`] trait and [`InMemoryReplayStore`], +//! its only implementation today. The in-memory store is process-local, so a +//! multi-replica gateway accepts the same nonce once per replica: replay +//! protection is complete for a single-process deployment and partial for any +//! other. A shared backend (NATS JetStream KV, keyed with a per-key TTL, is +//! the intended one) has not been built yet; [`ReplayError::Backend`] exists +//! so it can be added without changing this trait, and `AAuthIngress` is +//! already generic over `S: ReplayStore` so wiring one in is a construction +//! change rather than a signature change. use std::collections::HashMap; use std::sync::Mutex; @@ -24,14 +30,18 @@ pub enum ReplayError { /// failed-closed. #[error("replay store mutex poisoned")] MutexPoisoned, - /// Pluggable backends (NATS JetStream KV, Redis, etc.) surface their own - /// typed source error here instead of being flattened to a String. + /// Reserved for shared backends (NATS JetStream KV, Redis, etc.) so they + /// surface their own typed source error here instead of being flattened + /// to a String. No such backend ships today. #[error("replay store backend failure")] Backend(#[source] Box), } -/// Best-effort in-memory replay protection. Suitable for a single-process gateway -/// or unit tests. Multi-instance deployments should use the JetStream-backed store. +/// Best-effort in-memory replay protection: complete for a single-process +/// gateway or unit tests, and partial for a multi-replica deployment, where +/// each replica keeps its own map and a nonce is therefore accepted once per +/// replica. There is no shared-store implementation to fall back to yet; see +/// the module docs. pub struct InMemoryReplayStore { inner: Mutex>, clock: Box i64 + Send + Sync>, diff --git a/rsworkspace/crates/aauth/trogon-aauth-verify/src/token.rs b/rsworkspace/crates/aauth/trogon-aauth-verify/src/token.rs index edc96446d1..98bb9aeb7b 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-verify/src/token.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-verify/src/token.rs @@ -2,7 +2,9 @@ use jsonwebtoken::{ Algorithm, DecodingKey, Validation, decode, decode_header, - jwk::{AlgorithmParameters, EllipticCurve, Jwk, JwkSet}, + jwk::{ + AlgorithmParameters, CommonParameters, EllipticCurve, Jwk, JwkSet, KeyAlgorithm, KeyOperations, PublicKeyUse, + }, }; use serde::Deserialize; use trogon_identity_types::aauth::{AgentClaims, AuthClaims, ResourceClaims, TYP_AGENT, TYP_AUTH, TYP_RESOURCE}; @@ -173,7 +175,7 @@ impl TokenVerifier { .await?; let claims: AgentClaims = serde_json::from_value(claims_raw.clone()).map_err(|_| TokenError::MissingClaim("agent claims"))?; - let jkt = crate::jkt::jwk_thumbprint(&claims.cnf.jwk).map_err(|e| { + let jkt = crate::jkt::jwk_thumbprint(claims.cnf.jwk()).map_err(|e| { TokenError::InvalidClaim(match e { crate::jkt::JktError::MissingKty => "cnf.jwk.kty", crate::jkt::JktError::MissingField(f) => f, @@ -271,13 +273,13 @@ impl TokenVerifier { // None of its error variants indicate key material that parses // structurally but is cryptographically invalid -- that case is // caught below by `DecodingKey::from_jwk`. - let jkt = crate::jkt::jwk_thumbprint(&cnf.jwk).map_err(RequestContextError::StructurallyIncompleteKey)?; + let jkt = crate::jkt::jwk_thumbprint(cnf.jwk()).map_err(RequestContextError::StructurallyIncompleteKey)?; // jwk_thumbprint already validates presence of the type-specific // required members (crv/x/y for EC, crv/x for OKP, n/e for RSA); a // JWK that reaches this point but still cannot be parsed into a // `jsonwebtoken` decoding key is invalid key material, not merely // structurally incomplete. - let parsed_jwk: Jwk = serde_json::from_value(cnf.jwk.clone()) + let parsed_jwk: Jwk = serde_json::from_value(cnf.jwk().clone()) .map_err(|e| RequestContextError::InvalidKeyMaterial(InvalidKeyMaterialSourceError::Deserialize(e)))?; DecodingKey::from_jwk(&parsed_jwk) .map_err(|e| RequestContextError::InvalidKeyMaterial(InvalidKeyMaterialSourceError::DecodingKey(e)))?; @@ -406,8 +408,62 @@ fn pick_jwk<'a>(set: &'a JwkSet, alg: Algorithm, kid: Option<&str>) -> Option<&' None } +/// A JWK may verify a signature under `alg` only when its key material matches +/// that algorithm's family *and* the key's own advertised purpose permits +/// verification. +/// +/// RFC 7517 makes `use` (section 4.2), `key_ops` (section 4.3), and `alg` +/// (section 4.4) optional, so an absent member stays permissive: the AAuth +/// draft does not require them and a federated deployment resolves JWKS +/// documents this platform did not publish. But a publisher that *does* set +/// them has declared what the key is for, and honoring that declaration stops +/// a key published for encryption, or pinned to a different algorithm, from +/// being conscripted into signature verification merely because its curve +/// lines up. fn jwk_compatible_with_alg(jwk: &Jwk, alg: Algorithm) -> bool { - match (&jwk.algorithm, alg) { + jwk_purpose_permits_verification(&jwk.common) + && jwk_declared_alg_matches(&jwk.common, alg) + && jwk_material_matches_alg(&jwk.algorithm, alg) +} + +/// Rejects a key whose `use` names something other than signatures, or whose +/// `key_ops` enumerates operations without including `verify`. RFC 7517 says +/// the two members SHOULD NOT appear together; when a publisher sets both +/// anyway, each is checked independently and either one can reject. +fn jwk_purpose_permits_verification(common: &CommonParameters) -> bool { + if let Some(public_key_use) = &common.public_key_use + && !matches!(public_key_use, PublicKeyUse::Signature) + { + return false; + } + if let Some(key_operations) = &common.key_operations + && !key_operations.iter().any(|op| matches!(op, KeyOperations::Verify)) + { + return false; + } + true +} + +/// Rejects a key whose own `alg` names a different algorithm than the one the +/// JWT header claims, closing the gap where a publisher pins a key to one +/// algorithm and a token asserts another from the same key family. +fn jwk_declared_alg_matches(common: &CommonParameters, alg: Algorithm) -> bool { + let Some(declared) = common.key_algorithm else { + return true; + }; + matches!( + (declared, alg), + (KeyAlgorithm::ES256, Algorithm::ES256) + | (KeyAlgorithm::ES384, Algorithm::ES384) + | (KeyAlgorithm::EdDSA, Algorithm::EdDSA) + ) +} + +/// The key-material check: `kty`/`crv` must be the pair the algorithm is +/// defined over, which is what blocks cross-family confusion between the +/// three algorithms [`parse_typ`] admits. +fn jwk_material_matches_alg(parameters: &AlgorithmParameters, alg: Algorithm) -> bool { + match (parameters, alg) { (AlgorithmParameters::EllipticCurve(ec), Algorithm::ES256) => ec.curve == EllipticCurve::P256, (AlgorithmParameters::EllipticCurve(ec), Algorithm::ES384) => ec.curve == EllipticCurve::P384, (AlgorithmParameters::OctetKeyPair(okp), Algorithm::EdDSA) => okp.curve == EllipticCurve::Ed25519, diff --git a/rsworkspace/crates/aauth/trogon-aauth-verify/src/token/tests.rs b/rsworkspace/crates/aauth/trogon-aauth-verify/src/token/tests.rs index ba1e7e97d8..436620ffde 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-verify/src/token/tests.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-verify/src/token/tests.rs @@ -95,6 +95,100 @@ fn jwk_compatible_with_alg_rejects_mismatched_family() { assert!(!jwk_compatible_with_alg(&ec.jwk, Algorithm::EdDSA)); } +#[test] +fn jwk_compatible_with_alg_rejects_encryption_use() { + let mut ec = p256_fixture("p256-k1"); + ec.jwk.common.public_key_use = Some(jsonwebtoken::jwk::PublicKeyUse::Encryption); + assert!( + !jwk_compatible_with_alg(&ec.jwk, Algorithm::ES256), + "a key published for encryption must not verify signatures" + ); +} + +#[test] +fn jwk_compatible_with_alg_rejects_unrecognized_use() { + let mut ec = p256_fixture("p256-k1"); + ec.jwk.common.public_key_use = Some(jsonwebtoken::jwk::PublicKeyUse::Other("tls".into())); + assert!(!jwk_compatible_with_alg(&ec.jwk, Algorithm::ES256)); +} + +#[test] +fn jwk_compatible_with_alg_rejects_key_ops_without_verify() { + let mut ec = p256_fixture("p256-k1"); + ec.jwk.common.public_key_use = None; + ec.jwk.common.key_operations = Some(vec![jsonwebtoken::jwk::KeyOperations::Encrypt]); + assert!(!jwk_compatible_with_alg(&ec.jwk, Algorithm::ES256)); +} + +#[test] +fn jwk_compatible_with_alg_accepts_key_ops_containing_verify() { + let mut ec = p256_fixture("p256-k1"); + ec.jwk.common.public_key_use = None; + ec.jwk.common.key_operations = Some(vec![ + jsonwebtoken::jwk::KeyOperations::Verify, + jsonwebtoken::jwk::KeyOperations::Encrypt, + ]); + assert!(jwk_compatible_with_alg(&ec.jwk, Algorithm::ES256)); +} + +#[test] +fn jwk_compatible_with_alg_rejects_declared_alg_mismatch() { + // Material lines up (EC/P-256 is exactly what ES256 is defined over), so + // only the key's own `alg` member can catch this mislabeling. + let mut ec = p256_fixture("p256-k1"); + ec.jwk.common.key_algorithm = Some(jsonwebtoken::jwk::KeyAlgorithm::ES384); + assert!(!jwk_compatible_with_alg(&ec.jwk, Algorithm::ES256)); +} + +#[test] +fn jwk_compatible_with_alg_rejects_declared_encryption_alg() { + // `alg: "ECDH-ES"` is a key-agreement algorithm; jsonwebtoken folds any + // algorithm it does not model into UNKNOWN_ALGORITHM, which must not be + // treated as "compatible with whatever the header claims". + let mut ec = p256_fixture("p256-k1"); + ec.jwk.common.key_algorithm = Some(jsonwebtoken::jwk::KeyAlgorithm::UNKNOWN_ALGORITHM); + assert!(!jwk_compatible_with_alg(&ec.jwk, Algorithm::ES256)); +} + +#[test] +fn jwk_compatible_with_alg_accepts_absent_purpose_members() { + // `use`, `key_ops` and `alg` are all optional in RFC 7517; a JWKS that + // omits them stays verifiable, which is what keeps third-party issuers + // working. + let mut ec = p256_fixture("p256-k1"); + ec.jwk.common.public_key_use = None; + ec.jwk.common.key_operations = None; + ec.jwk.common.key_algorithm = None; + assert!(jwk_compatible_with_alg(&ec.jwk, Algorithm::ES256)); +} + +#[tokio::test(flavor = "current_thread")] +async fn verify_resource_rejects_key_published_for_encryption() { + // End-to-end: an issuer whose only P-256 key is marked `use: enc` has no + // key eligible to verify with, even though the curve matches. + let mut fixture = p256_fixture("p256-k1"); + fixture.jwk.common.public_key_use = Some(jsonwebtoken::jwk::PublicKeyUse::Encryption); + let mut header = jsonwebtoken::Header::new(Algorithm::ES256); + header.typ = Some(TYP_RESOURCE.into()); + header.kid = Some("p256-k1".into()); + let claims = serde_json::json!({ + "iss": "iss.example", + "aud": "ps.example", + "jti": "j1", + "iat": 1000, + "exp": 9999999999_i64, + "dwk": "aa-resource", + "agent": "agent-1", + "agent_jkt": "abc", + "scope": "read", + }); + let jwt = jsonwebtoken::encode(&header, &claims, &fixture.signing).expect("encode"); + let jwks = jwks_with_key("iss.example", fixture.jwk); + let v = TokenVerifier::new(jwks, SystemTimeSource); + let err = v.verify_resource(&jwt, "ps.example").await.unwrap_err(); + assert!(matches!(err, TokenError::NoCompatibleJwk), "got {err:?}"); +} + #[tokio::test(flavor = "current_thread")] async fn assert_freshness_uses_supplied_clock() { let now = Arc::new(std::sync::atomic::AtomicI64::new(1000)); diff --git a/rsworkspace/crates/aauth/trogon-aauth-verify/tests/nats_pop_roundtrip.rs b/rsworkspace/crates/aauth/trogon-aauth-verify/tests/nats_pop_roundtrip.rs index 88c4ac93aa..191f615f87 100644 --- a/rsworkspace/crates/aauth/trogon-aauth-verify/tests/nats_pop_roundtrip.rs +++ b/rsworkspace/crates/aauth/trogon-aauth-verify/tests/nats_pop_roundtrip.rs @@ -87,7 +87,7 @@ async fn end_to_end_nats_pop_verifies() { "iat": now - 5, "exp": now + 600, "dwk": DWK_AGENT, - "cnf": Cnf { jwk: agent_jwk_val.clone() }, + "cnf": Cnf::public(agent_jwk_val.clone()).expect("test fixture is a public jwk"), }); let agent_jwt = encode(&header, &claims, &enc_key).expect("encode agent jwt"); diff --git a/rsworkspace/crates/channel/trogon-channel/src/store.rs b/rsworkspace/crates/channel/trogon-channel/src/store.rs index a739f2f3b2..4ef8b465fd 100644 --- a/rsworkspace/crates/channel/trogon-channel/src/store.rs +++ b/rsworkspace/crates/channel/trogon-channel/src/store.rs @@ -172,11 +172,17 @@ pub struct ChannelStore { /// through would let a momentary read failure reconfigure live storage. async fn ensure_bucket(js: &jetstream::Context, bucket: String) -> Result { match js.get_key_value(&bucket).await { - Ok(store) => return Ok(store), - Err(source) if is_get_key_value_not_found(&source) => {} - Err(source) => return Err(ChannelStoreError::OpenBucket { bucket, source }), + Ok(store) => Ok(store), + Err(source) if is_get_key_value_not_found(&source) => create_bucket(js, bucket).await, + Err(source) => Err(ChannelStoreError::OpenBucket { bucket, source }), } +} +/// The half of [`ensure_bucket`] that runs once the bucket has been found +/// missing, split out so the interleaving it exists to survive can be staged +/// rather than raced for: another replica creating the same bucket, with a +/// config of its own, in the window this call opens. +async fn create_bucket(js: &jetstream::Context, bucket: String) -> Result { info!(bucket = %bucket, "Creating channel KV bucket"); match js .create_key_value(jetstream::kv::Config { diff --git a/rsworkspace/crates/channel/trogon-channel/src/store/tests.rs b/rsworkspace/crates/channel/trogon-channel/src/store/tests.rs index 3388158ad9..80b767cfa2 100644 --- a/rsworkspace/crates/channel/trogon-channel/src/store/tests.rs +++ b/rsworkspace/crates/channel/trogon-channel/src/store/tests.rs @@ -573,39 +573,47 @@ async fn a_claim_that_cannot_be_read_back_takes_the_conversation_record_with_it( /// `ensure_is_idempotent_under_concurrent_creation` races two *identical* /// configs, and neither side ever takes this arm: `STREAM.CREATE` only /// errors when the stream that beat it has a different config, and an -/// identical race succeeds silently on both sides. Racing a bare create -/// against `ensure_bucket`'s own get-then-create for the same bucket name -/// reliably loses that race instead: the bare create skips the get's extra -/// round trip, so its differently-configured bucket already exists by the -/// time this store's own create is rejected. +/// identical race succeeds silently on both sides. Reaching the arm needs a +/// differently-configured bucket to exist by the time the create runs, and +/// which of two concurrent calls the scheduler and the server put first is +/// not something a test gets to decide, so the halves of `ensure_bucket` are +/// called in the order the losing interleaving would have produced. #[tokio::test] async fn a_bucket_created_with_a_different_config_between_the_get_and_the_create_is_still_opened() { let server = JetStreamTestServer::start().await; let js = server.jetstream().await; let bucket = "conflict".to_string(); - // `ensure_bucket` is private and reachable only through `ChannelStore::ensure`, - // so it is called directly here to race a single bucket instead of all four. - let racing_create = js.create_key_value(jetstream::kv::Config { + // Matched rather than `expect_err`ed because a `Store` is not `Debug`. + let Err(missing) = js.get_key_value(&bucket).await else { + panic!("the bucket must be absent for the create half to be what runs next"); + }; + assert!( + is_get_key_value_not_found(&missing), + "expected the bucket to read as absent, got {missing:?}" + ); + + js.create_key_value(jetstream::kv::Config { bucket: bucket.clone(), history: 1, storage: jetstream::stream::StorageType::File, ..Default::default() - }); - - let (ours, theirs) = tokio::join!(ensure_bucket(&js, bucket.clone()), racing_create); + }) + .await + .expect("the other replica's create must land for there to be anything to recover"); - ours.expect("ensure_bucket must recover the bucket the race left behind"); - theirs.expect("the racing create must succeed for there to be anything to recover"); + create_bucket(&js, bucket.clone()) + .await + .expect("create_bucket must open the bucket the replica that beat it left behind"); let mut stream = js .get_stream(format!("KV_{bucket}")) .await - .expect("the bucket the race created must still be there"); + .expect("the bucket the other replica created must still be there"); let info = stream.info().await.expect("stream info"); assert_eq!( info.config.max_messages_per_subject, 1, - "the surviving config must be the racing create's, not ensure_bucket's own attempt" + "the surviving config must be the winner's, not the rejected create's" ); } @@ -641,12 +649,12 @@ async fn a_bucket_whose_subject_space_is_already_claimed_fails_to_create() { ); } -/// Line 105's arm: the already-exists recovery read can itself fail. Racing -/// a stream into existence with `max_messages_per_subject` below the minimum -/// a real KV config ever produces (`kv_to_stream_config` floors it at 1) -/// forces exactly that: the name conflict sends `ensure_bucket` to recover by -/// reading the bucket back, and that read rejects what it finds as not a -/// valid KV store rather than returning it. +/// `create_bucket`'s already-exists arm recovers by reading the bucket back, +/// and that read can itself fail. A stream claiming the bucket's name with +/// `max_messages_per_subject` below the minimum a real KV config ever +/// produces (`kv_to_stream_config` floors it at 1) forces exactly that: the +/// name conflict sends the create off to recover, and the recovery read +/// rejects what it finds as not a valid KV store rather than returning it. #[tokio::test] async fn a_recovery_read_that_also_fails_surfaces_as_a_bucket_read_failure() { let server = JetStreamTestServer::start().await; @@ -655,19 +663,17 @@ async fn a_recovery_read_that_also_fails_surfaces_as_a_bucket_read_failure() { // Plain `create_stream`, not `create_key_value`, because the KV wrapper // floors `max_messages_per_subject` at 1 and could never produce this. - let racing_create = js.create_stream(jetstream::stream::Config { + js.create_stream(jetstream::stream::Config { name: format!("KV_{bucket}"), subjects: vec![format!("$KV.{bucket}.>")], max_messages_per_subject: 0, ..Default::default() - }); - - let (ours, theirs) = tokio::join!(ensure_bucket(&js, bucket.clone()), racing_create); - - theirs.expect("the racing create must win for there to be a conflicting bucket to recover"); + }) + .await + .expect("the conflicting bucket must exist for there to be a recovery read to fail"); - let Err(error) = ours else { - panic!("ensure_bucket must fail when its own recovery read also fails"); + let Err(error) = create_bucket(&js, bucket.clone()).await else { + panic!("create_bucket must fail when its own recovery read also fails"); }; assert!( matches!(error, ChannelStoreError::OpenBucket { .. }), diff --git a/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/constants.rs b/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/constants.rs index 35e0b40abf..ca2b29bcfc 100644 --- a/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/constants.rs +++ b/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/constants.rs @@ -16,6 +16,8 @@ pub const HEADER_IDEMPOTENCY_KEY: &str = "idempotency-key"; pub const HEADER_INSTANCE: &str = "x-gitlab-instance"; pub const NATS_HEADER_EVENT: &str = "X-GitLab-Event"; +pub const NATS_HEADER_WEBHOOK_ID: &str = "Webhook-Id"; +pub const NATS_HEADER_IDEMPOTENCY_KEY: &str = "Idempotency-Key"; pub const NATS_HEADER_WEBHOOK_UUID: &str = "X-GitLab-Webhook-UUID"; pub const NATS_HEADER_EVENT_UUID: &str = "X-GitLab-Event-UUID"; pub const NATS_HEADER_INSTANCE: &str = "X-GitLab-Instance"; diff --git a/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/server.rs b/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/server.rs index 6e7982c11c..856960dfb1 100644 --- a/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/server.rs +++ b/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/server.rs @@ -5,8 +5,8 @@ use super::GitLabSigningToken; use super::config::GitlabConfig; use super::constants::{ HEADER_EVENT, HEADER_EVENT_UUID, HEADER_IDEMPOTENCY_KEY, HEADER_INSTANCE, HEADER_WEBHOOK_UUID, HTTP_BODY_SIZE_MAX, - NATS_HEADER_EVENT, NATS_HEADER_EVENT_UUID, NATS_HEADER_INSTANCE, NATS_HEADER_REJECT_REASON, - NATS_HEADER_WEBHOOK_UUID, + NATS_HEADER_EVENT, NATS_HEADER_EVENT_UUID, NATS_HEADER_IDEMPOTENCY_KEY, NATS_HEADER_INSTANCE, + NATS_HEADER_REJECT_REASON, NATS_HEADER_WEBHOOK_ID, NATS_HEADER_WEBHOOK_UUID, }; use super::signature; use axum::{ @@ -17,7 +17,7 @@ use std::pin::Pin; use tracing::{info, instrument, warn}; use trogon_nats::NatsToken; use trogon_nats::jetstream::{ - ClaimCheckPublisher, JetStreamContext, JetStreamPublisher, ObjectStorePut, PublishOutcome, + ClaimCheckPublisher, JetStreamContext, JetStreamPublisher, ObjectStorePut, ProvisionedStreamField, PublishOutcome, }; use trogon_semconv::span::GITLAB_WEBHOOK; use trogon_std::NonZeroDuration; @@ -40,12 +40,15 @@ impl RejectReason { async fn publish_unroutable( publisher: &ClaimCheckPublisher, subject_prefix: &NatsToken, + verified: &signature::VerifiedWebhook, reason: RejectReason, body: Bytes, ack_timeout: NonZeroDuration, ) -> StatusCode { let subject = format!("{subject_prefix}.unroutable"); let mut headers = async_nats::HeaderMap::new(); + headers.insert(async_nats::header::NATS_MESSAGE_ID, verified.webhook_id.as_str()); + headers.insert(NATS_HEADER_WEBHOOK_ID, verified.webhook_id.as_str()); headers.insert(NATS_HEADER_REJECT_REASON, reason.as_str()); let outcome = publisher @@ -81,12 +84,26 @@ struct AppState { } pub async fn provision(js: &C, config: &GitlabConfig) -> Result<(), C::Error> { - js.get_or_create_stream(async_nats::jetstream::stream::Config { - name: config.stream_name.as_str().to_owned(), - subjects: vec![format!("{}.>", config.subject_prefix)], - max_age: config.stream_max_age.into(), - ..Default::default() - }) + // Reconciled rather than created-if-absent: `duplicate_window` is the + // replay bound paired with the signature timestamp tolerance below, so a + // stream provisioned before that pairing existed would otherwise keep + // JetStream's default window and leave replays live past it. The listed + // fields are what this source owns; placement and limits stay the + // operator's. + js.create_or_reconcile_stream( + async_nats::jetstream::stream::Config { + name: config.stream_name.as_str().to_owned(), + subjects: vec![format!("{}.>", config.subject_prefix)], + duplicate_window: config.timestamp_tolerance.into(), + max_age: config.stream_max_age.into(), + ..Default::default() + }, + &[ + ProvisionedStreamField::Subjects, + ProvisionedStreamField::DuplicateWindow, + ProvisionedStreamField::MaxAge, + ], + ) .await?; let stream = config.stream_name.as_str(); @@ -135,16 +152,20 @@ async fn handle_webhook_inner( headers: HeaderMap, body: Bytes, ) -> StatusCode { - if let Err(e) = signature::verify(&headers, &body, &state.signing_token, state.timestamp_tolerance) { - warn!(reason = %e, "GitLab webhook signature validation failed"); - return StatusCode::UNAUTHORIZED; - } + let verified = match signature::verify(&headers, &body, &state.signing_token, state.timestamp_tolerance) { + Ok(verified) => verified, + Err(e) => { + warn!(reason = %e, "GitLab webhook signature validation failed"); + return StatusCode::UNAUTHORIZED; + } + }; let Some(raw_event) = headers.get(HEADER_EVENT).and_then(|v| v.to_str().ok()) else { warn!("Missing X-GitLab-Event header"); return publish_unroutable( &state.publisher, &state.subject_prefix, + &verified, RejectReason::MissingEventHeader, body, state.nats_ack_timeout, @@ -163,6 +184,7 @@ async fn handle_webhook_inner( return publish_unroutable( &state.publisher, &state.subject_prefix, + &verified, RejectReason::InvalidEventToken, body, state.nats_ack_timeout, @@ -187,8 +209,14 @@ async fn handle_webhook_inner( let mut nats_headers = async_nats::HeaderMap::new(); nats_headers.insert(NATS_HEADER_EVENT, raw_event); - if let Some(id) = idempotency_key { - nats_headers.insert(async_nats::header::NATS_MESSAGE_ID, id); + // Dedup on the signed `webhook-id`, not on `idempotency-key`. Both + // identify a delivery, but only the former is inside the Standard + // Webhooks signed content, so it is the only one an attacker replaying a + // captured request cannot vary to defeat the JetStream duplicate window. + nats_headers.insert(async_nats::header::NATS_MESSAGE_ID, verified.webhook_id.as_str()); + nats_headers.insert(NATS_HEADER_WEBHOOK_ID, verified.webhook_id.as_str()); + if let Some(key) = idempotency_key { + nats_headers.insert(NATS_HEADER_IDEMPOTENCY_KEY, key); } if let Some(uuid) = webhook_uuid { nats_headers.insert(NATS_HEADER_WEBHOOK_UUID, uuid); diff --git a/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/server/tests.rs b/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/server/tests.rs index d9a1ba777a..23dde659c4 100644 --- a/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/server/tests.rs +++ b/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/server/tests.rs @@ -168,6 +168,40 @@ async fn provision_creates_stream() { assert_eq!(streams[0].name, "GITLAB"); assert_eq!(streams[0].subjects, vec!["gitlab.>"]); assert_eq!(streams[0].max_age, Duration::from_secs(3600)); + assert_eq!(streams[0].duplicate_window, Duration::from_secs(300)); +} + +#[tokio::test] +async fn provision_widens_the_dedup_window_on_an_already_provisioned_stream() { + let _guard = tracing_guard(); + let js = MockJetStreamContext::new(); + // A deployment that provisioned GITLAB before the dedup window was paired + // with the signature tolerance: JetStream's own default, not ours. The + // replica count and storage tier are an operator's, set out of band. + js.get_or_create_stream(async_nats::jetstream::stream::Config { + name: "GITLAB".to_owned(), + duplicate_window: Duration::from_secs(120), + num_replicas: 3, + storage: async_nats::jetstream::stream::StorageType::Memory, + max_bytes: 1_024, + ..Default::default() + }) + .await + .unwrap(); + + provision(&js, &test_config()).await.unwrap(); + + let streams = js.created_streams(); + assert_eq!(streams.len(), 1, "reconciled in place rather than duplicated"); + assert_eq!(streams[0].name, "GITLAB"); + assert_eq!(streams[0].duplicate_window, Duration::from_secs(300)); + assert_eq!(streams[0].num_replicas, 3, "operator-set replica count survives"); + assert_eq!( + streams[0].storage, + async_nats::jetstream::stream::StorageType::Memory, + "operator-set storage tier survives" + ); + assert_eq!(streams[0].max_bytes, 1_024, "operator-set limit survives"); } #[tokio::test] @@ -216,6 +250,14 @@ async fn valid_webhook_publishes_to_nats_and_returns_200() { .headers .get(async_nats::header::NATS_MESSAGE_ID) .map(|v| v.as_str()), + Some("msg_123"), + ); + assert_eq!( + messages[0].headers.get(NATS_HEADER_WEBHOOK_ID).map(|v| v.as_str()), + Some("msg_123"), + ); + assert_eq!( + messages[0].headers.get(NATS_HEADER_IDEMPOTENCY_KEY).map(|v| v.as_str()), Some("idem-key-test"), ); } @@ -359,7 +401,7 @@ async fn empty_body_publishes_successfully() { } #[tokio::test] -async fn missing_idempotency_key_skips_dedup_id() { +async fn dedup_id_comes_from_signed_webhook_id_not_idempotency_key() { let _guard = tracing_guard(); let publisher = MockJetStreamPublisher::new(); @@ -385,9 +427,17 @@ async fn missing_idempotency_key_skips_dedup_id() { assert_eq!(resp.status(), StatusCode::OK); let messages = publisher.published_messages(); + assert_eq!( + messages[0] + .headers + .get(async_nats::header::NATS_MESSAGE_ID) + .map(|v| v.as_str()), + Some("msg_123"), + "dedup must not depend on the unsigned Idempotency-Key header" + ); assert!( - messages[0].headers.get(async_nats::header::NATS_MESSAGE_ID).is_none(), - "should not set Nats-Msg-Id when Idempotency-Key is absent" + messages[0].headers.get(NATS_HEADER_IDEMPOTENCY_KEY).is_none(), + "absent Idempotency-Key is forwarded as absent" ); } diff --git a/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/signature.rs b/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/signature.rs index 50109599a0..7566f59a57 100644 --- a/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/signature.rs +++ b/rsworkspace/crates/platform/trogon-gateway/src/source/gitlab/signature.rs @@ -5,7 +5,7 @@ use super::GitLabSigningToken; use super::constants::HEADER_NAMES; use crate::source::standard_webhooks; -pub use crate::source::standard_webhooks::SignatureError; +pub use crate::source::standard_webhooks::{SignatureError, VerifiedWebhook}; #[cfg(test)] pub use crate::source::standard_webhooks::{WebhookId, WebhookTimestamp}; @@ -14,7 +14,7 @@ pub fn verify( body: &[u8], signing_token: &GitLabSigningToken, timestamp_tolerance: NonZeroDuration, -) -> Result<(), SignatureError> { +) -> Result { standard_webhooks::verify( headers, body, @@ -22,7 +22,6 @@ pub fn verify( timestamp_tolerance, HEADER_NAMES, ) - .map(|_| ()) } #[cfg(test)] diff --git a/rsworkspace/crates/platform/trogon-identity-types/src/aauth/mod.rs b/rsworkspace/crates/platform/trogon-identity-types/src/aauth/mod.rs index 8d09681a20..2facd4d57e 100644 --- a/rsworkspace/crates/platform/trogon-identity-types/src/aauth/mod.rs +++ b/rsworkspace/crates/platform/trogon-identity-types/src/aauth/mod.rs @@ -28,11 +28,106 @@ pub mod person_server; pub use delegation::Act; /// Public-key confirmation claim (`cnf`) as carried in `aa-agent+jwt`. -#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +/// +/// Issuer-side construction goes through [`Cnf::public`], which is the only +/// constructor; the field is private so no caller can assemble one around it. +/// Deserialization is deliberately exempt: a peer's inbound `cnf` is parsed as +/// sent, because what a peer put in its own confirmation claim is not this +/// type's call to reject, and verification reads only the public parameters. +#[derive(Clone, PartialEq, Eq, Serialize, Deserialize)] pub struct Cnf { /// Embedded JWK. Stored as serde_json::Value so this crate avoids depending on /// `jsonwebtoken`. Verifier-side parses into `jsonwebtoken::jwk::Jwk`. - pub jwk: Value, + jwk: Value, +} + +/// Prints only the members that say *which* key this is, never the key. +/// +/// [`Cnf::public`] refuses private key material, but the deserialization path +/// above accepts whatever a peer sent, so a `Cnf` reached by that path may hold +/// a private scalar. Anything that logs a claim set at debug level would then +/// write it out, and a derived `Debug` gives no warning that this is what it +/// does. The peer's own key is theirs to mishandle; writing it into this +/// platform's logs is not. +impl std::fmt::Debug for Cnf { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + let mut out = f.debug_struct("Cnf"); + for member in crate::constants::JWK_DESCRIPTIVE_MEMBERS { + if let Some(value) = self.jwk.get(member) { + out.field(member, value); + } + } + out.finish_non_exhaustive() + } +} + +impl Cnf { + /// Build a confirmation claim, refusing any JWK that carries private or + /// symmetric key material, or that is missing the public members its key + /// type needs to be usable. + /// + /// Two failures are guarded here and they fail in opposite directions. A + /// caller that passes a full keypair instead of its public half publishes + /// the private key to every party that sees the token, with a valid + /// signature over it; that mistake is easy to make (JWK serializers + /// include `d` unless asked not to) and impossible to walk back once a + /// token is issued. A caller that passes an incomplete JWK instead mints a + /// token whose confirmation key can never satisfy a proof of possession, + /// so every request bound to it fails at the resource with no indication + /// that the fault is in the token rather than the request. + pub fn public(jwk: Value) -> Result { + let Some(members) = jwk.as_object() else { + return Err(CnfError::NotAnObject); + }; + let Some(kty) = members.get("kty").and_then(Value::as_str) else { + return Err(CnfError::MissingKeyType); + }; + if kty.eq_ignore_ascii_case(crate::constants::KTY_OCT) { + return Err(CnfError::SymmetricKey); + } + for member in crate::constants::JWK_PRIVATE_MEMBERS { + if members.contains_key(member) { + return Err(CnfError::PrivateKeyMaterial { member }); + } + } + let (kty, required): (&'static str, &[&'static str]) = match kty { + crate::constants::KTY_EC => (crate::constants::KTY_EC, &crate::constants::JWK_REQUIRED_EC_MEMBERS), + crate::constants::KTY_RSA => (crate::constants::KTY_RSA, &crate::constants::JWK_REQUIRED_RSA_MEMBERS), + crate::constants::KTY_OKP => (crate::constants::KTY_OKP, &crate::constants::JWK_REQUIRED_OKP_MEMBERS), + other => { + return Err(CnfError::UnsupportedKeyType { kty: other.to_owned() }); + } + }; + for member in required { + if members.get(*member).and_then(Value::as_str).is_none_or(str::is_empty) { + return Err(CnfError::UnusablePublicMember { kty, member }); + } + } + Ok(Self { jwk }) + } + + /// The embedded JWK, as it will appear in the issued token. + #[must_use] + pub fn jwk(&self) -> &Value { + &self.jwk + } +} + +/// Rejections from [`Cnf::public`]. +#[derive(Clone, Debug, PartialEq, Eq, thiserror::Error)] +pub enum CnfError { + #[error("cnf.jwk must be a JSON object")] + NotAnObject, + #[error("cnf.jwk must name a kty")] + MissingKeyType, + #[error("cnf.jwk must not be a symmetric key")] + SymmetricKey, + #[error("cnf.jwk carries private key material in member {member:?}")] + PrivateKeyMaterial { member: &'static str }, + #[error("cnf.jwk names key type {kty:?}, which cannot carry a confirmation key")] + UnsupportedKeyType { kty: String }, + #[error("cnf.jwk of type {kty} needs a non-empty string member {member:?}")] + UnusablePublicMember { kty: &'static str, member: &'static str }, } /// Claims for an `aa-agent+jwt`. Issued by an Agent Provider at bootstrap. diff --git a/rsworkspace/crates/platform/trogon-identity-types/src/aauth/tests.rs b/rsworkspace/crates/platform/trogon-identity-types/src/aauth/tests.rs index db2bed62b8..a5f3a26bfc 100644 --- a/rsworkspace/crates/platform/trogon-identity-types/src/aauth/tests.rs +++ b/rsworkspace/crates/platform/trogon-identity-types/src/aauth/tests.rs @@ -44,9 +44,8 @@ fn agent_claims_serde() { iat: 100, exp: 200, dwk: DWK_AGENT.into(), - cnf: Cnf { - jwk: serde_json::json!({"kty": "EC", "crv": "P-256", "x": "X", "y": "Y"}), - }, + cnf: Cnf::public(serde_json::json!({"kty": "EC", "crv": "P-256", "x": "X", "y": "Y"})) + .expect("test fixture is a public jwk"), ps: Some("https://ps.example".into()), }; let j = serde_json::to_value(&c).unwrap(); @@ -238,3 +237,167 @@ fn split_header_skips_empty_segments_from_stray_semicolons() { let raw = "requirement=clarification;; ;"; assert_eq!(Requirement::parse(raw), Requirement::Clarification); } + +#[test] +fn cnf_public_accepts_an_ec_public_jwk() { + let jwk = serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA", "y": "BBB"}); + let cnf = Cnf::public(jwk.clone()).expect("public jwk accepted"); + assert_eq!(cnf.jwk(), &jwk); +} + +#[test] +fn cnf_public_rejects_ec_private_scalar() { + // The mistake this guards: serializing a keypair instead of its public + // half puts `d` in a token that is handed to every resource server. + let jwk = serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA", "y": "BBB", "d": "SECRET"}); + assert_eq!( + Cnf::public(jwk).unwrap_err(), + CnfError::PrivateKeyMaterial { member: "d" } + ); +} + +#[test] +fn cnf_public_rejects_rsa_crt_parameters() { + for member in ["p", "q", "dp", "dq", "qi", "oth"] { + let mut jwk = serde_json::json!({"kty": "RSA", "n": "AAA", "e": "AQAB"}); + jwk[member] = serde_json::json!("SECRET"); + assert_eq!( + Cnf::public(jwk).unwrap_err(), + CnfError::PrivateKeyMaterial { member }, + "{member} must be refused" + ); + } +} + +#[test] +fn cnf_public_rejects_okp_private_scalar() { + let jwk = serde_json::json!({"kty": "OKP", "crv": "Ed25519", "x": "AAA", "d": "SECRET"}); + assert_eq!( + Cnf::public(jwk).unwrap_err(), + CnfError::PrivateKeyMaterial { member: "d" } + ); +} + +#[test] +fn cnf_public_rejects_symmetric_keys_by_kty() { + // Checked before the member scan so an `oct` key is refused even when + // `k` is absent: there is no public half of a symmetric key to carry. + let jwk = serde_json::json!({"kty": "oct"}); + assert_eq!(Cnf::public(jwk).unwrap_err(), CnfError::SymmetricKey); + let upper = serde_json::json!({"kty": "OCT", "k": "SECRET"}); + assert_eq!(Cnf::public(upper).unwrap_err(), CnfError::SymmetricKey); +} + +#[test] +fn cnf_public_rejects_non_object_jwk() { + for value in [ + serde_json::json!("not-a-jwk"), + serde_json::json!(null), + serde_json::json!([{"kty": "EC"}]), + ] { + assert_eq!(Cnf::public(value).unwrap_err(), CnfError::NotAnObject); + } +} + +#[test] +fn cnf_public_accepts_the_other_supported_key_types() { + for jwk in [ + serde_json::json!({"kty": "RSA", "n": "AAA", "e": "AQAB"}), + serde_json::json!({"kty": "OKP", "crv": "Ed25519", "x": "AAA"}), + ] { + Cnf::public(jwk.clone()).unwrap_or_else(|e| panic!("{jwk} must be accepted, got {e}")); + } +} + +#[test] +fn cnf_public_rejects_a_jwk_with_no_kty() { + let jwk = serde_json::json!({"crv": "P-256", "x": "AAA", "y": "BBB"}); + assert_eq!(Cnf::public(jwk).unwrap_err(), CnfError::MissingKeyType); + let non_string = serde_json::json!({"kty": 256, "x": "AAA"}); + assert_eq!(Cnf::public(non_string).unwrap_err(), CnfError::MissingKeyType); +} + +#[test] +fn cnf_public_rejects_a_key_type_it_cannot_check() { + // Anything outside the three known types would sail past the member scan + // with nothing verified, so it is refused rather than waved through. + let jwk = serde_json::json!({"kty": "ec", "crv": "P-256", "x": "AAA", "y": "BBB"}); + assert_eq!( + Cnf::public(jwk).unwrap_err(), + CnfError::UnsupportedKeyType { kty: "ec".to_owned() } + ); +} + +#[test] +fn cnf_public_rejects_a_jwk_missing_a_member_its_key_type_needs() { + // The failure this guards is silent at issuance: the token verifies, and + // every proof of possession against it fails at the resource instead. + let cases = [ + (serde_json::json!({"kty": "EC", "x": "AAA", "y": "BBB"}), "EC", "crv"), + (serde_json::json!({"kty": "EC", "crv": "P-256", "y": "BBB"}), "EC", "x"), + (serde_json::json!({"kty": "EC", "crv": "P-256", "x": "AAA"}), "EC", "y"), + (serde_json::json!({"kty": "RSA", "e": "AQAB"}), "RSA", "n"), + (serde_json::json!({"kty": "RSA", "n": "AAA"}), "RSA", "e"), + (serde_json::json!({"kty": "OKP", "x": "AAA"}), "OKP", "crv"), + (serde_json::json!({"kty": "OKP", "crv": "Ed25519"}), "OKP", "x"), + ]; + for (jwk, kty, member) in cases { + assert_eq!( + Cnf::public(jwk).unwrap_err(), + CnfError::UnusablePublicMember { kty, member }, + "{kty} without {member} must be refused" + ); + } +} + +#[test] +fn cnf_public_rejects_a_required_member_that_is_present_but_unusable() { + // Present-but-empty and present-but-not-a-string are the same defect as + // absent: nothing a verifier can build a key from. + for x in [serde_json::json!(""), serde_json::json!(0), serde_json::json!(null)] { + let jwk = serde_json::json!({"kty": "EC", "crv": "P-256", "x": x, "y": "BBB"}); + assert_eq!( + Cnf::public(jwk).unwrap_err(), + CnfError::UnusablePublicMember { kty: "EC", member: "x" } + ); + } +} + +#[test] +fn cnf_still_deserializes_a_peer_supplied_confirmation_claim() { + // The verifier read path must stay lenient: rejecting a peer's own `cnf` + // at parse time is not this type's call, and the guard is issuer-side. + let raw = r#"{"jwk":{"kty":"EC","crv":"P-256","x":"AAA","y":"BBB","d":"THEIRS"}}"#; + let cnf: Cnf = serde_json::from_str(raw).expect("inbound cnf parses"); + assert!(cnf.jwk().get("d").is_some()); +} + +#[test] +fn cnf_debug_does_not_print_the_key_it_holds() { + // Reached through the lenient inbound path, so `d` is present: this is + // exactly the shape whose Debug output must stay clean. + let raw = r#"{"jwk":{"kty":"EC","crv":"P-256","kid":"peer-1","x":"PUBX","y":"PUBY","d":"THEIRS"}}"#; + let cnf: Cnf = serde_json::from_str(raw).expect("inbound cnf parses"); + + let printed = format!("{cnf:?}"); + for secret in ["THEIRS", "PUBX", "PUBY"] { + assert!(!printed.contains(secret), "{printed}"); + } + // Still says which key it is, or the redaction costs every log line its + // diagnostic value. + assert!(printed.contains("peer-1"), "{printed}"); + assert!(printed.contains("P-256"), "{printed}"); +} + +#[test] +fn cnf_debug_omits_members_it_does_not_recognise() { + // The allow-list is the point: a member added to a future key type must be + // withheld until someone decides it is safe to print. + let raw = r#"{"jwk":{"kty":"OKP","crv":"Ed25519","x":"AAA","some_future_member":"UNVETTED"}}"#; + let cnf: Cnf = serde_json::from_str(raw).expect("inbound cnf parses"); + + let printed = format!("{cnf:?}"); + assert!(!printed.contains("UNVETTED"), "{printed}"); + assert!(!printed.contains("some_future_member"), "{printed}"); + assert!(printed.contains(".."), "{printed}"); +} diff --git a/rsworkspace/crates/platform/trogon-identity-types/src/constants.rs b/rsworkspace/crates/platform/trogon-identity-types/src/constants.rs index 207cd365d1..e9fe4ce8d9 100644 --- a/rsworkspace/crates/platform/trogon-identity-types/src/constants.rs +++ b/rsworkspace/crates/platform/trogon-identity-types/src/constants.rs @@ -4,6 +4,37 @@ /// Maximum number of entries allowed in an `act` delegation chain. pub const MAX_ACT_CHAIN_DEPTH: usize = 8; +/// JWK members that carry private key material, across every key type AAuth +/// can encounter: `d` for EC (RFC 7518 Section 6.2.2), RSA (Section 6.3.2), +/// and OKP (RFC 8037 Section 2); the remaining RSA CRT parameters; and `k`, +/// which *is* the secret for a symmetric `oct` key (Section 6.4.1). +pub const JWK_PRIVATE_MEMBERS: [&str; 8] = ["d", "p", "q", "dp", "dq", "qi", "oth", "k"]; + +/// JWK members that describe a key without being any part of one: they name +/// which key is meant and what it is for, and none of them is key material of +/// either half. This is an allow-list rather than the inverse of +/// [`JWK_PRIVATE_MEMBERS`] because a member this crate has never heard of must +/// stay unprinted by default. +pub const JWK_DESCRIPTIVE_MEMBERS: [&str; 5] = ["kty", "crv", "kid", "alg", "use"]; + +/// `kty` value for a symmetric key. Never valid in a confirmation claim: a +/// proof-of-possession key that both parties must hold is not a proof of +/// possession, and publishing it in a token discloses it. +pub const KTY_OCT: &str = "oct"; + +/// `kty` values an issuer may put in a confirmation claim. +pub const KTY_EC: &str = "EC"; +pub const KTY_RSA: &str = "RSA"; +pub const KTY_OKP: &str = "OKP"; + +/// Public members each key type requires before the JWK can verify anything. +/// EC per RFC 7518 Section 6.2.1, RSA per Section 6.3.1, OKP per RFC 8037 +/// Section 2. A confirmation key missing any of these is syntactically a JWK +/// but cannot be used to check a proof of possession. +pub const JWK_REQUIRED_EC_MEMBERS: [&str; 3] = ["crv", "x", "y"]; +pub const JWK_REQUIRED_RSA_MEMBERS: [&str; 2] = ["n", "e"]; +pub const JWK_REQUIRED_OKP_MEMBERS: [&str; 2] = ["crv", "x"]; + /// `typ` header value identifying an agent identity token. pub const TYP_AGENT: &str = "aa-agent+jwt"; /// `typ` header value identifying a resource challenge token. diff --git a/rsworkspace/crates/platform/trogon-jwks-publisher/src/provider.rs b/rsworkspace/crates/platform/trogon-jwks-publisher/src/provider.rs index a2e4997af3..14e9acca85 100644 --- a/rsworkspace/crates/platform/trogon-jwks-publisher/src/provider.rs +++ b/rsworkspace/crates/platform/trogon-jwks-publisher/src/provider.rs @@ -235,10 +235,11 @@ pub enum PersonServerUrlError { /// public confirmation key. pub struct AgentTokenRequest { pub sub: AgentIdentifier, - /// Agent's public JWK, embedded verbatim into `cnf.jwk` per RFC 7800. - /// Kept as `serde_json::Value` to match `Cnf::jwk`'s type -- this crate - /// does not validate JWK shape beyond what the caller already produced - /// (e.g. via `trogon-aauth-sdk`'s public key derivation). + /// Agent's public JWK, embedded into `cnf.jwk` per RFC 7800. Kept as + /// `serde_json::Value` to match `Cnf::jwk`'s type; [`Cnf::public`] + /// rejects private and symmetric key material at mint time, but the + /// remaining shape is whatever the caller produced (e.g. via + /// `trogon-aauth-sdk`'s public key derivation). pub agent_jwk: serde_json::Value, pub ttl: TokenTtl, pub ps: Option, @@ -251,6 +252,8 @@ pub enum ProviderError { Encode(#[source] jsonwebtoken::errors::Error), #[error("system clock is before unix epoch")] ClockBeforeEpoch, + #[error("confirmation key rejected: {0}")] + Cnf(#[from] trogon_identity_types::aauth::CnfError), } /// Agent Provider: mints `aa-agent+jwt` tokens under a fixed signing key, @@ -288,7 +291,7 @@ impl AgentProvider { iat, exp, dwk: DWK_AGENT.to_string(), - cnf: Cnf { jwk: req.agent_jwk }, + cnf: Cnf::public(req.agent_jwk)?, ps: req.ps.map(PersonServerUrl::into_inner), }; diff --git a/rsworkspace/crates/platform/trogon-jwks-publisher/src/provider/tests.rs b/rsworkspace/crates/platform/trogon-jwks-publisher/src/provider/tests.rs index bd3aa11713..f53a8c5f00 100644 --- a/rsworkspace/crates/platform/trogon-jwks-publisher/src/provider/tests.rs +++ b/rsworkspace/crates/platform/trogon-jwks-publisher/src/provider/tests.rs @@ -182,7 +182,7 @@ fn mint_produces_header_and_claims_per_spec() { assert_eq!(claims.iss, "https://ap.example"); assert_eq!(claims.sub, sub.as_str()); assert_eq!(claims.dwk, DWK_AGENT); - assert_eq!(claims.cnf.jwk, test_agent_jwk()); + assert_eq!(claims.cnf.jwk(), &test_agent_jwk()); assert_eq!(claims.ps.as_deref(), Some("https://ps.example")); assert!(claims.exp > claims.iat); assert_eq!(claims.exp - claims.iat, 3600); @@ -238,3 +238,26 @@ fn base64_decode(segment: &str) -> Vec { .decode(segment) .expect("valid base64url") } + +#[test] +fn mint_refuses_to_publish_private_key_material_in_cnf() { + // Passing the full keypair instead of its public half would sign the + // agent's private scalar into a token every resource server receives. + let key = AgentProviderKey::new( + test_encoding_key(), + KeyId::new("ap-key-1").expect("kid"), + ProviderIssuer::new("https://ap.example").expect("iss"), + ); + let provider = AgentProvider::new(key); + let mut jwk = test_agent_jwk(); + jwk["d"] = serde_json::json!("evZzL1gdAFr88hb2OF_2NxApJCzGCEDdfSp6VQO30hw"); + let req = AgentTokenRequest { + sub: AgentIdentifier::new("aauth:assistant-v2@agent.example").expect("sub"), + agent_jwk: jwk, + ttl: TokenTtl::new(3600).expect("ttl"), + ps: None, + }; + + let err = provider.mint(req).expect_err("private key material refused"); + assert!(matches!(err, ProviderError::Cnf(_)), "got {err:?}"); +} diff --git a/rsworkspace/crates/platform/trogon-jwks-publisher/src/publisher.rs b/rsworkspace/crates/platform/trogon-jwks-publisher/src/publisher.rs index d1417be9ce..b5a5082e41 100644 --- a/rsworkspace/crates/platform/trogon-jwks-publisher/src/publisher.rs +++ b/rsworkspace/crates/platform/trogon-jwks-publisher/src/publisher.rs @@ -7,7 +7,7 @@ //! host service mounts at its own root so `GET /.well-known/{dwk}` resolves //! against a configured set of `JwkSet`s. -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; use axum::Router; use axum::extract::{Path, State}; @@ -54,6 +54,12 @@ pub enum PublisherError { UnknownDwk(String), #[error("dwk filename {0:?} was registered more than once")] DuplicateDwk(String), + #[error("dwk {dwk:?} cannot be published: {source}")] + Unpublishable { + dwk: String, + #[source] + source: PublishableJwkSetError, + }, #[error("invalid EC PKCS8 PEM for kid {kid:?}: {source}")] InvalidPem { kid: String, @@ -71,6 +77,74 @@ fn is_known_dwk(dwk: &str) -> bool { known_dwk_filenames().contains(&dwk) } +/// Why a [`JwkSet`] cannot be published as it stands. Names no dwk: which +/// document a set was registered under is the registrar's context, not the +/// set's own. +#[derive(Debug, thiserror::Error)] +pub enum PublishableJwkSetError { + #[error("key id {kid:?} is published more than once")] + DuplicateKeyId { kid: String }, + #[error("{keys} keys are published and at least one omits `kid`; only a single-key set may omit it")] + MissingKeyId { keys: usize }, +} + +/// A [`JwkSet`] every consumer can actually select a key from. +/// +/// A published set is selected from by `kid`: [`JwkSet::find`] matches the JWT +/// header's `kid` against `common.key_id`, and `trogon-aauth-verify`'s +/// `pick_jwk` does the same, falling back to the sole compatible key only when +/// the set holds exactly one. +/// +/// That makes two shapes unusable. Keys sharing a `kid` resolve silently to +/// whichever copy is enumerated first, so a rotation that reuses the previous +/// `kid` keeps every consumer pinned to the retired key. A key with no `kid` +/// cannot be selected at all once the set holds more than one, which is exactly +/// the state a rotation overlap creates. Both fail at the consumer, remotely, +/// with nothing to see on this side, so they are refused at startup instead -- +/// the same reason the dwk filename is checked at registration rather than left +/// to surface as a 404. +/// +/// Holding that as a type rather than a check run on the way past means a set +/// reaching [`JwksPublisherConfig`] has already been through it, and an +/// unselectable one has no way to be represented there at all. An empty set +/// (`{"keys":[]}`) is publishable: it is a legitimate discovery state, not a +/// misconfiguration. +#[derive(Debug, Clone)] +pub struct PublishableJwkSet(JwkSet); + +impl TryFrom for PublishableJwkSet { + type Error = PublishableJwkSetError; + + fn try_from(set: JwkSet) -> Result { + let multi_key = set.keys.len() > 1; + let mut seen: HashSet<&str> = HashSet::new(); + for jwk in &set.keys { + match jwk.common.key_id.as_deref() { + Some(kid) => { + if !seen.insert(kid) { + return Err(PublishableJwkSetError::DuplicateKeyId { kid: kid.to_owned() }); + } + } + None if multi_key => { + return Err(PublishableJwkSetError::MissingKeyId { keys: set.keys.len() }); + } + None => {} + } + } + Ok(Self(set)) + } +} + +impl PublishableJwkSet { + /// The underlying set, for serialization. Borrowed rather than owned so + /// the validated value cannot be taken apart and put back together with + /// the invariant broken. + #[must_use] + pub fn as_jwk_set(&self) -> &JwkSet { + &self.0 + } +} + /// Build a public EC P-256 JWK from a PKCS8 PEM private key, mirroring /// `trogon-aauth-sdk`'s `public_jwk` helper but returning a typed /// `jsonwebtoken::jwk::Jwk` (this crate already depends on `jsonwebtoken` for @@ -113,7 +187,7 @@ fn base64_url(bytes: impl AsRef<[u8]>) -> String { #[derive(Debug, Default)] pub struct JwksPublisherConfigBuilder { max_age: CacheMaxAge, - entries: HashMap, + entries: HashMap, } impl JwksPublisherConfigBuilder { @@ -125,9 +199,9 @@ impl JwksPublisherConfigBuilder { } } - /// Register a pre-built `JwkSet` under a dwk filename. An empty `JwkSet` - /// (`{"keys":[]}`) is accepted -- it is a legitimate discovery state, not - /// an error. + /// Register a pre-built `JwkSet` under a dwk filename. This is the one + /// boundary where a raw set is converted into a [`PublishableJwkSet`]; + /// past here the configuration holds nothing else. pub fn with_jwk_set(mut self, dwk: impl Into, set: JwkSet) -> Result { let dwk = dwk.into(); if !is_known_dwk(&dwk) { @@ -136,6 +210,10 @@ impl JwksPublisherConfigBuilder { if self.entries.contains_key(&dwk) { return Err(PublisherError::DuplicateDwk(dwk)); } + let set = PublishableJwkSet::try_from(set).map_err(|source| PublisherError::Unpublishable { + dwk: dwk.clone(), + source, + })?; self.entries.insert(dwk, set); Ok(self) } @@ -156,12 +234,12 @@ impl JwksPublisherConfigBuilder { } } -/// Validated publisher configuration: dwk filename -> `JwkSet`, plus the -/// `Cache-Control` max-age applied to every discovery response. +/// Validated publisher configuration: dwk filename -> [`PublishableJwkSet`], +/// plus the `Cache-Control` max-age applied to every discovery response. #[derive(Clone)] pub struct JwksPublisherConfig { max_age: CacheMaxAge, - entries: HashMap, + entries: HashMap, } /// Build the `GET /.well-known/{dwk}` discovery router. Mount this into a @@ -198,7 +276,7 @@ async fn serve_dwk(State(config): State, Path(dwk): Path Jwk { + jwk_from_ec_pkcs8_pem(TEST_PEM, kid).expect("valid pem") +} + +fn unkeyed() -> Jwk { + let mut jwk = keyed("k1"); + jwk.common.key_id = None; + jwk +} + +#[test] +fn builder_accepts_a_rotation_overlap_with_distinct_key_ids() { + let cfg = JwksPublisherConfigBuilder::new(CacheMaxAge::new(60)) + .with_jwk_set( + DWK_AGENT, + JwkSet { + keys: vec![keyed("current"), keyed("previous")], + }, + ) + .expect("distinct kids stay selectable") + .build(); + assert_eq!(cfg.entries.get(DWK_AGENT).expect("present").as_jwk_set().keys.len(), 2); +} + +#[test] +fn builder_rejects_a_set_that_repeats_a_key_id() { + let err = JwksPublisherConfigBuilder::new(CacheMaxAge::new(60)) + .with_jwk_set( + DWK_AGENT, + JwkSet { + keys: vec![keyed("same"), keyed("same")], + }, + ) + .unwrap_err(); + assert!( + matches!( + &err, + PublisherError::Unpublishable { + dwk, + source: PublishableJwkSetError::DuplicateKeyId { kid }, + } if dwk == DWK_AGENT && kid.as_str() == "same" + ), + "{err}" + ); +} + +#[test] +fn a_set_that_repeats_a_key_id_cannot_be_built_at_all() { + // The registrar is not the only way in: the invariant belongs to the type, + // so converting the set directly refuses it just the same. + let err = PublishableJwkSet::try_from(JwkSet { + keys: vec![keyed("same"), keyed("same")], + }) + .unwrap_err(); + assert!( + matches!(&err, PublishableJwkSetError::DuplicateKeyId { kid } if kid.as_str() == "same"), + "{err}" + ); +} + +#[test] +fn builder_rejects_a_multi_key_set_holding_an_unidentified_key() { + let err = JwksPublisherConfigBuilder::new(CacheMaxAge::new(60)) + .with_jwk_set( + DWK_AGENT, + JwkSet { + keys: vec![keyed("current"), unkeyed()], + }, + ) + .unwrap_err(); + assert!( + matches!( + &err, + PublisherError::Unpublishable { + dwk, + source: PublishableJwkSetError::MissingKeyId { keys: 2 }, + } if dwk == DWK_AGENT + ), + "{err}" + ); +} + +#[test] +fn builder_allows_a_lone_key_without_a_key_id() { + // `pick_jwk` resolves a one-key set without consulting `kid`, and RFC 7517 + // section 4.5 leaves the member optional, so this shape stays publishable. + let cfg = JwksPublisherConfigBuilder::new(CacheMaxAge::new(60)) + .with_jwk_set(DWK_AGENT, JwkSet { keys: vec![unkeyed()] }) + .expect("a sole key needs no kid") + .build(); + assert_eq!(cfg.entries.get(DWK_AGENT).expect("present").as_jwk_set().keys.len(), 1); +} diff --git a/rsworkspace/crates/platform/trogon-nats/src/jetstream/client.rs b/rsworkspace/crates/platform/trogon-nats/src/jetstream/client.rs index deec432e93..86ff7512c4 100644 --- a/rsworkspace/crates/platform/trogon-nats/src/jetstream/client.rs +++ b/rsworkspace/crates/platform/trogon-nats/src/jetstream/client.rs @@ -10,7 +10,7 @@ use bytes::Bytes; use super::message::{JsAck, JsAckWith, JsDoubleAck, JsDoubleAckWith, JsMessageRef}; use super::traits::{ JetStreamContext, JetStreamCreateKeyValue, JetStreamGetKeyValue, JetStreamGetStream, JetStreamPublishMessage, - JetStreamPublisher, + JetStreamPublisher, ProvisionedStreamField, reconciled_stream_config, }; #[derive(Clone)] @@ -38,6 +38,33 @@ impl JetStreamContext for NatsJetStreamClient { ) -> Result { self.context.get_or_create_stream(config).await } + + async fn create_or_reconcile_stream + Send>( + &self, + desired: S, + owned: &[ProvisionedStreamField], + ) -> Result<(), async_nats::jetstream::context::CreateStreamError> { + let desired = desired.into(); + // A lookup that fails for any reason falls through to create, which + // errors on a name already in use. Failing provisioning is the right + // outcome for a stream we could not read: the alternative is updating + // it from a config we never reconciled against. + let Ok(stream) = self.context.get_stream(&desired.name).await else { + self.context.create_stream(desired).await?; + return Ok(()); + }; + let live = &stream.cached_info().config; + let reconciled = reconciled_stream_config(live, &desired, owned); + // An update would send the whole config back, so a stream already + // holding what we declare is one we leave alone rather than one we + // rewrite identically: no write, no window for an operator's + // concurrent edit to fall into. + if reconciled == *live { + return Ok(()); + } + self.context.update_stream(&reconciled).await?; + Ok(()) + } } pub type PublishError = async_nats::jetstream::context::PublishError; diff --git a/rsworkspace/crates/platform/trogon-nats/src/jetstream/mocks.rs b/rsworkspace/crates/platform/trogon-nats/src/jetstream/mocks.rs index 4053630865..bbe9c5f685 100644 --- a/rsworkspace/crates/platform/trogon-nats/src/jetstream/mocks.rs +++ b/rsworkspace/crates/platform/trogon-nats/src/jetstream/mocks.rs @@ -32,7 +32,7 @@ use super::traits::{ JetStreamGetRawMessage, JetStreamGetStream, JetStreamGetStreamInfo, JetStreamKeyValueCreateWithTtl, JetStreamKeyValueDeleteExpectRevision, JetStreamKeyValueStatus, JetStreamKeyValueUpdate, JetStreamKvCreate, JetStreamKvEntry, JetStreamKvGet, JetStreamKvKeys, JetStreamLastRawMessageBySubject, JetStreamPublishMessage, - JetStreamPublisher, JetStreamSubjectPurger, + JetStreamPublisher, JetStreamSubjectPurger, ProvisionedStreamField, reconciled_stream_config, }; use crate::mocks::MockError; @@ -210,10 +210,24 @@ impl JsDoubleAckWith for MockJsMessage { } } +/// Why a [`MockJetStreamContext`] refused to provision. +/// +/// Its own type rather than the crate's shared [`MockError`]: a test that +/// armed `fail_next` should be able to name the refusal it asked for instead +/// of matching on the prose of a message. +#[derive(Debug, Clone, Copy, PartialEq, Eq, thiserror::Error)] +pub enum MockStreamProvisionError { + #[error("simulated stream creation failure")] + Creation, + #[error("simulated stream reconciliation failure")] + Reconciliation, +} + #[derive(Clone, Debug)] pub struct MockJetStreamContext { created_streams: Arc>>, should_fail: Arc>, + stream_writes: Arc>, } impl MockJetStreamContext { @@ -221,6 +235,7 @@ impl MockJetStreamContext { Self { created_streams: Arc::new(Mutex::new(Vec::new())), should_fail: Arc::new(Mutex::new(false)), + stream_writes: Arc::new(Mutex::new(0)), } } @@ -228,9 +243,26 @@ impl MockJetStreamContext { self.created_streams.lock().unwrap().clone() } + /// How many times a stream config was written. A reconcile that finds the + /// stream already holding what the caller declares writes nothing, so it + /// does not count here. + pub fn stream_writes(&self) -> usize { + *self.stream_writes.lock().unwrap() + } + pub fn fail_next(&self) { *self.should_fail.lock().unwrap() = true; } + + fn take_failure(&self) -> bool { + let mut flag = self.should_fail.lock().unwrap(); + if *flag { + *flag = false; + true + } else { + false + } + } } impl Default for MockJetStreamContext { @@ -240,24 +272,49 @@ impl Default for MockJetStreamContext { } impl JetStreamContext for MockJetStreamContext { - type Error = MockError; + type Error = MockStreamProvisionError; type Stream = (); - async fn get_or_create_stream + Send>(&self, config: S) -> Result<(), MockError> { + async fn get_or_create_stream + Send>( + &self, + config: S, + ) -> Result<(), MockStreamProvisionError> { let config = config.into(); - let should_fail = { - let mut flag = self.should_fail.lock().unwrap(); - if *flag { - *flag = false; - true - } else { - false - } - }; - if should_fail { - return Err(MockError("simulated stream creation failure".to_string())); + if self.take_failure() { + return Err(MockStreamProvisionError::Creation); } self.created_streams.lock().unwrap().push(config); + *self.stream_writes.lock().unwrap() += 1; + Ok(()) + } + + /// Reconciles into the stored config for a matching name rather than + /// replacing it, so a test can observe that a field the caller does not + /// own survives provisioning, and records no write when the reconcile + /// changes nothing. + async fn create_or_reconcile_stream + Send>( + &self, + desired: S, + owned: &[ProvisionedStreamField], + ) -> Result<(), MockStreamProvisionError> { + let desired = desired.into(); + if self.take_failure() { + return Err(MockStreamProvisionError::Reconciliation); + } + { + let mut streams = self.created_streams.lock().unwrap(); + match streams.iter_mut().find(|existing| existing.name == desired.name) { + Some(existing) => { + let reconciled = reconciled_stream_config(existing, &desired, owned); + if reconciled == *existing { + return Ok(()); + } + *existing = reconciled; + } + None => streams.push(desired), + } + } + *self.stream_writes.lock().unwrap() += 1; Ok(()) } } diff --git a/rsworkspace/crates/platform/trogon-nats/src/jetstream/mocks/tests.rs b/rsworkspace/crates/platform/trogon-nats/src/jetstream/mocks/tests.rs index 8b05b02066..2a1bdf57d0 100644 --- a/rsworkspace/crates/platform/trogon-nats/src/jetstream/mocks/tests.rs +++ b/rsworkspace/crates/platform/trogon-nats/src/jetstream/mocks/tests.rs @@ -74,8 +74,43 @@ async fn mock_context_records_stream_creation() { async fn mock_context_fails_when_configured() { let ctx = MockJetStreamContext::new(); ctx.fail_next(); - let result = ctx.get_or_create_stream(stream::Config::default()).await; - assert!(result.is_err()); + let err = ctx.get_or_create_stream(stream::Config::default()).await.unwrap_err(); + assert_eq!(err, MockStreamProvisionError::Creation); +} + +#[tokio::test] +async fn mock_context_names_a_reconcile_refusal_apart_from_a_creation_one() { + let ctx = MockJetStreamContext::new(); + ctx.fail_next(); + let err = ctx + .create_or_reconcile_stream(stream::Config::default(), &[ProvisionedStreamField::Subjects]) + .await + .unwrap_err(); + assert_eq!(err, MockStreamProvisionError::Reconciliation); +} + +#[tokio::test] +async fn reconciling_a_stream_that_already_matches_writes_nothing() { + let ctx = MockJetStreamContext::new(); + let declared = stream::Config { + name: "RECONCILED".to_owned(), + subjects: vec!["reconciled.>".to_owned()], + duplicate_window: Duration::from_secs(300), + ..Default::default() + }; + let owned = [ + ProvisionedStreamField::Subjects, + ProvisionedStreamField::DuplicateWindow, + ]; + + ctx.create_or_reconcile_stream(declared.clone(), &owned).await.unwrap(); + assert_eq!(ctx.stream_writes(), 1); + + // The second boot finds the stream already holding what it declares. An + // update would resend the whole config for nothing, and every resend is a + // chance to land on top of an operator's concurrent edit. + ctx.create_or_reconcile_stream(declared, &owned).await.unwrap(); + assert_eq!(ctx.stream_writes(), 1); } #[tokio::test] diff --git a/rsworkspace/crates/platform/trogon-nats/src/jetstream/mod.rs b/rsworkspace/crates/platform/trogon-nats/src/jetstream/mod.rs index 00cfd49d7e..96de58973f 100644 --- a/rsworkspace/crates/platform/trogon-nats/src/jetstream/mod.rs +++ b/rsworkspace/crates/platform/trogon-nats/src/jetstream/mod.rs @@ -38,7 +38,8 @@ pub use traits::{ JetStreamGetRawMessage, JetStreamGetStream, JetStreamGetStreamInfo, JetStreamKeyValueCreateWithTtl, JetStreamKeyValueDeleteExpectRevision, JetStreamKeyValueStatus, JetStreamKeyValueUpdate, JetStreamKvCreate, JetStreamKvEntry, JetStreamKvGet, JetStreamKvKeys, JetStreamLastRawMessageBySubject, JetStreamPublishMessage, - JetStreamPublisher, JetStreamSubjectPurger, JsMessageOf, PurgeOutcome, + JetStreamPublisher, JetStreamSubjectPurger, JsMessageOf, ProvisionedStreamField, PurgeOutcome, + reconciled_stream_config, }; #[cfg(any(test, feature = "test-support"))] @@ -46,5 +47,5 @@ pub use mocks::{ AckKindSnapshot, AckKindValue, MockJetStreamConsumer, MockJetStreamConsumerFactory, MockJetStreamContext, MockJetStreamKvClient, MockJetStreamKvStore, MockJetStreamPublishMessage, MockJetStreamPublisher, MockJetStreamPurger, MockJetStreamStream, MockJsMessage, MockKvEntryOutcome, MockKvGetOutcome, MockObjectStore, - MockPublishedOutboundMessage, + MockPublishedOutboundMessage, MockStreamProvisionError, }; diff --git a/rsworkspace/crates/platform/trogon-nats/src/jetstream/traits.rs b/rsworkspace/crates/platform/trogon-nats/src/jetstream/traits.rs index ea11df3f19..cecce3d749 100644 --- a/rsworkspace/crates/platform/trogon-nats/src/jetstream/traits.rs +++ b/rsworkspace/crates/platform/trogon-nats/src/jetstream/traits.rs @@ -12,6 +12,46 @@ use futures::Stream; use std::error::Error; use std::future::{Future, IntoFuture}; +/// A [`stream::Config`] field a service that provisions a stream is +/// authoritative for. +/// +/// Everything absent from this enum belongs to whoever runs the cluster: +/// placement, storage tier, retention limits. Naming the owned fields as a +/// closed set, rather than handing reconciliation a merge closure, is what +/// makes the split hold -- there is no variant for `num_replicas`, so no +/// provisioning code can write one back. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum ProvisionedStreamField { + Subjects, + DuplicateWindow, + MaxAge, +} + +impl ProvisionedStreamField { + fn reconcile(self, live: &mut stream::Config, declared: &stream::Config) { + match self { + Self::Subjects => live.subjects.clone_from(&declared.subjects), + Self::DuplicateWindow => live.duplicate_window = declared.duplicate_window, + Self::MaxAge => live.max_age = declared.max_age, + } + } +} + +/// The config a stream should hold once the `owned` fields of `declared` are +/// applied to `live`, leaving every other field as the server reports it. +#[must_use] +pub fn reconciled_stream_config( + live: &stream::Config, + declared: &stream::Config, + owned: &[ProvisionedStreamField], +) -> stream::Config { + let mut reconciled = live.clone(); + for field in owned { + field.reconcile(&mut reconciled, declared); + } + reconciled +} + pub trait JetStreamContext: Send + Sync + Clone + 'static { type Error: Error + Send + Sync; type Stream: Send; @@ -20,6 +60,36 @@ pub trait JetStreamContext: Send + Sync + Clone + 'static { &self, config: S, ) -> impl Future> + Send; + + /// Create `desired` when the stream is absent; otherwise read the live + /// config, apply the fields named by `owned`, and write the result back. + /// + /// Two behaviours are wrong here and this method is the narrow path + /// between them. [`Self::get_or_create_stream`] returns an existing stream + /// untouched, so a setting that carries a security property -- a + /// `duplicate_window` sized to a signature timestamp tolerance, say -- + /// keeps whatever value it was first created with and the declared config + /// never takes effect. Sending `desired` wholesale to `STREAM.UPDATE` + /// instead reconciles the *entire* config, so every field the caller left + /// at `Default::default()` overwrites what the operator set: replica + /// count, storage tier, and retention limits all roll back on the next + /// boot. + /// + /// [`ProvisionedStreamField`] is what separates the two, and it is a + /// closed set precisely so a caller cannot widen its own authority. + /// + /// Implementations must write nothing when every named field already + /// matches what the server reports. `STREAM.UPDATE` carries the whole + /// config and accepts no expected-revision, so any write races an + /// operator editing the same stream and one of the two changes is lost. + /// JetStream offers no conditional form that would close that window, so + /// not writing is what keeps it shut, and a boot against an + /// already-reconciled stream is the case that actually happens. + fn create_or_reconcile_stream + Send>( + &self, + desired: S, + owned: &[ProvisionedStreamField], + ) -> impl Future> + Send; } pub trait JetStreamKeyValueStatus: Send + Sync + Clone + 'static { diff --git a/rsworkspace/crates/platform/trogon-nats/src/jetstream/traits/tests.rs b/rsworkspace/crates/platform/trogon-nats/src/jetstream/traits/tests.rs index b0806b3a89..fb964d9427 100644 --- a/rsworkspace/crates/platform/trogon-nats/src/jetstream/traits/tests.rs +++ b/rsworkspace/crates/platform/trogon-nats/src/jetstream/traits/tests.rs @@ -1,6 +1,7 @@ -use super::PurgeOutcome; +use super::{ProvisionedStreamField, PurgeOutcome, reconciled_stream_config}; use async_nats::jetstream::stream; use serde_json::json; +use std::time::Duration; fn purge_response(success: bool) -> stream::PurgeResponse { serde_json::from_value(json!({ "success": success, "purged": 0_u64 })).unwrap() @@ -16,3 +17,47 @@ fn purge_response_outcome_reflects_success_field() { assert!(purge_response(true).is_success()); assert!(!purge_response(false).is_success()); } + +fn operator_managed() -> stream::Config { + stream::Config { + name: "OPERATED".to_owned(), + subjects: vec!["stale.>".to_owned()], + duplicate_window: Duration::from_secs(120), + num_replicas: 3, + storage: stream::StorageType::Memory, + max_bytes: 1_024, + ..Default::default() + } +} + +#[test] +fn reconciling_applies_only_the_named_fields() { + let declared = stream::Config { + name: "OPERATED".to_owned(), + subjects: vec!["fresh.>".to_owned()], + duplicate_window: Duration::from_secs(300), + max_age: Duration::from_secs(3_600), + ..Default::default() + }; + + let reconciled = reconciled_stream_config( + &operator_managed(), + &declared, + &[ProvisionedStreamField::Subjects, ProvisionedStreamField::MaxAge], + ); + + assert_eq!(reconciled.subjects, vec!["fresh.>"]); + assert_eq!(reconciled.max_age, Duration::from_secs(3_600)); + // Unnamed, so the declared value never reaches the server, defaults least + // of all: this is the roll-back a whole-config update would have caused. + assert_eq!(reconciled.duplicate_window, Duration::from_secs(120)); + assert_eq!(reconciled.num_replicas, 3); + assert_eq!(reconciled.storage, stream::StorageType::Memory); + assert_eq!(reconciled.max_bytes, 1_024); +} + +#[test] +fn reconciling_nothing_leaves_the_live_config_alone() { + let live = operator_managed(); + assert_eq!(reconciled_stream_config(&live, &stream::Config::default(), &[]), live); +}