feat: add wallet attestation-based client authentication - #516
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces wallet attestation–based client authentication so wallets can authenticate without pre-registration in the static OAuth client map, by verifying a wallet-provider-signed attestation JWT against trusted provider JWKS.
Changes:
- Adds a
WalletAttestationVerifierandWalletProviderConfigto verify wallet-provider attestations via JWKS. - Extends OAuth PAR handling to fall back to attestation when
client_idis not in the static client map. - Extends configuration (
wallet_provider_trust,require_wallet_attestation) and request models to carry assertion parameters.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/openid4vci/authoriziation.go | Adds PAR request fields for carrying client assertion / attestation parameters. |
| pkg/oauth2/wallet_attestation_verifier.go | New verifier implementation for wallet-provider attestation JWTs using provider JWKS. |
| pkg/oauth2/wallet_attestation_verifier_test.go | Unit tests for the new wallet attestation verifier. |
| pkg/model/config.go | Adds config fields for trusted wallet providers and an enforcement flag. |
| internal/apigw/apiv1/handlers_oauth.go | Adds attestation fallback logic to PAR and token endpoints when client lookup fails. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
95f825e to
82d0389
Compare
Wallets authenticate using a provider-signed attestation JWT. The trust
decision is fully delegated to the go-trust PDP — the issuer only needs:
apigw:
trust:
pdp_url: "https://trust.siros.se/pdp"
delivery:
openid4vci:
accept_wallet_attestation: true
No per-provider enumeration. The PDP validates the wallet provider's
signature against its configured trust lists/federation anchors (OIDF,
ETSI TSL, etc.). If the PDP returns allowed_redirect_uris in
trust_metadata, those are enforced; otherwise PKCE is the sole
code-binding mechanism (RFC 9126 + S256).
Components:
- pkg/trust.WalletAttestationEvaluator: sends attestation to PDP
with Role=wallet-provider, returns WalletAttestationResult
- PAR/token endpoints: fall back to PDP evaluation when client not
in static map
- Sub binding: attestation sub must match presented client_id
- PARRequest: client_assertion fields with validation tags
Security model:
- PKCE mandatory (public clients always require code_challenge)
- DPoP sender-constrains access tokens
- PDP controls which providers are trusted (policy, not code)
- Redirect URI constraints are a PDP policy decision, not local config
82d0389 to
46300e6
Compare
When x5c is present, the wallet provider identity MUST be derived from the leaf certificate (SAN/CN), not from the self-asserted iss claim. A trusted wallet provider could set iss to a different provider's identifier and inherit that provider's SPOCP policy permissions. The x5c chain is the cryptographic proof of identity; iss is just a routing hint. New behavior: - x5c present: identity = cert SAN/CN. If iss is also present, it must match the cert identity or the attestation is rejected. - x5c absent: identity = iss claim (IETF draft format, PDP validates via JWKS lookup from the iss URL).
3fb8399 to
2a08d07
Compare
Update wallet attestation auth to use HTTP headers per draft-ietf-oauth-attestation-based-client-auth-04 §3.1: - Extract OAuth-Client-Attestation and OAuth-Client-Attestation-PoP headers at PAR and token endpoints - Add EvaluateWithPoP() that validates PoP JWT signature against WIA cnf.jwk, checks aud matches AS issuer URL, validates typ/iat/exp - Maintain backward compatibility: falls back to form-body client_assertion when headers are absent (legacy mode) - Add extractCNFKeyFromWIA() and parseECPublicKeyFromCNF() for key extraction with curve validation - Add comprehensive test coverage for PoP validation The PoP JWT must have: typ: oauth-client-attestation-pop+jwt aud: this AS's issuer URL (PublicURL from config) iat: present signature: valid against cnf.jwk from the WIA Ref: draft-ietf-oauth-attestation-based-client-auth-04 §3.1, §5.2
|
|
||
| - `trust.pdp_url` must be configured (PDP performs the trust decision) | ||
| - Wallet provider must be discoverable by the PDP (via OIDF, trust lists, or JWKS registry) | ||
| - Attestation JWT must contain `iss` (provider) and `sub` (wallet instance = `client_id`) |
There was a problem hiding this comment.
This doesn't match with the TS03 specification iiuc - identity is derived from the signature only. Iss is only checked and used for routing and authz using spocp if present.
- Require OAuth-Client-Attestation-PoP when OAuth-Client-Attestation header is present (reject without PoP in header mode, §3.1 compliance) - Legacy form-body client_assertion mode still works without PoP - Fail closed when x5c header present but no valid cert strings extracted - Fix issuerFromX5CLeaf docstring to document URI SAN preference - Use require.NoError in tests for key generation and SignedString - Update docs/TRUST_AND_IDENTITY.md to describe header mechanism as primary - Fix token endpoint: when client not in static map and wallet attestation evaluator is configured, defer to attestation instead of failing RFC 7523
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
docs/TRUST_AND_IDENTITY.md:85
- Implementation validates
OAuth-Client-Attestation/...-PoPon both PAR and token endpoints (token falls back to attestation whenclient_idisn’t in the static map). The doc currently states the headers are sent only in the PAR request, which may mislead integrators who then omit them on the token request and getinvalid_client.
1. Wallet sends `OAuth-Client-Attestation` (WIA JWT) and `OAuth-Client-Attestation-PoP` (PoP JWT) as HTTP headers in the PAR request per [draft-ietf-oauth-attestation-based-client-auth-04 §3.1](https://www.ietf.org/archive/id/draft-ietf-oauth-attestation-based-client-auth-04.html#section-3.1)
- Require exp claim on PoP JWT (jwt.WithExpirationRequired) - Use the code's actual granted scope for wallet-attestation policy authorization at the token endpoint instead of a wildcard, since pre-authorized_code flows never go through PAR - Fix test helper ignoring ecdsa PublicKey.ECDH() error - Use a numeric iat in test WIA claims instead of a placeholder string
|
Addressed the remaining open review threads (commit 04a7a31):
Left open, needs @leifj's call: the thread on All builds/tests green ( |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
internal/apigw/httpserver/endpoints_oauth.go:135
OAuth-Client-AttestationandOAuth-Client-Attestation-PoPare set afterBinding.Request, so they bypass any struct-tag validation. Adding a simple max-size check here (consistent withclient_assertion'smax=8192) would reduce the risk of oversized header inputs causing unnecessary work or memory pressure.
// Extract OAuth-Client-Attestation headers (draft-ietf-oauth-attestation-based-client-auth-04 §3.1)
request.ClientAttestation = c.GetHeader("OAuth-Client-Attestation")
request.ClientAttestationPoP = c.GetHeader("OAuth-Client-Attestation-PoP")
pkg/trust/wallet_attestation.go:185
- PoP validation only checks that
expis not in the past and thatiatis present. Without any freshness/max-lifetime constraint, a long-lived PoP can be replayed as a bearer credential for its fullexpwindow, weakening proof-of-possession semantics. Consider enforcing a maximum validity window (e.g.,exp-iat <= 5m) and rejectingiatvalues that are far in the past/future (beyond the existing leeway).
// exp is required (jwt.WithExpirationRequired) and already validated by jwt.Parse (with leeway)
// iat presence check
if claims.IssuedAt == nil {
return errors.New("PoP missing iat claim")
}
docs/TRUST_AND_IDENTITY.md:105
- The Requirements section currently states that the attestation JWT must contain
iss, but the implementation supports TS03-style attestations whereissmay be absent and provider identity is derived from thex5ccertificate chain. The doc should reflect thatsubis required, and that provider identity comes from eitheriss(draft format) orx5c(TS03), with consistency checks when both are present.
- `trust.pdp_url` must be configured (PDP performs the trust decision)
- Wallet provider must be discoverable by the PDP (via OIDF, trust lists, or JWKS registry)
- Attestation JWT must contain `iss` (provider) and `sub` (wallet instance = `client_id`)
docs/TRUST_AND_IDENTITY.md:90
- The documented sequence says the APIGW validates the PoP JWT before sending the WIA to the PDP, but the implementation calls
Evaluate(PDP trust decision) beforevalidateAttestationPoP. Please update the step order (or wording) so the doc matches the actual flow.
1. Wallet sends `OAuth-Client-Attestation` (WIA JWT) and `OAuth-Client-Attestation-PoP` (PoP JWT) as HTTP headers in the PAR request per [draft-ietf-oauth-attestation-based-client-auth-04 §3.1](https://www.ietf.org/archive/id/draft-ietf-oauth-attestation-based-client-auth-04.html#section-3.1)
2. APIGW looks up `client_id` in static client map → not found
3. APIGW validates the PoP JWT signature against the WIA's `cnf.jwk` and checks `aud` matches this AS
4. APIGW sends the WIA to go-trust PDP with `role=wallet-provider`
5. PDP validates wallet provider's signature against trust lists/federation
pkg/trust/wallet_attestation.go:283
parseAttestationIdentityintroduces several security-relevant branches (x5c vs iss selection, fail-closed behavior when x5c is malformed, and iss↔cert consistency checks), but the accompanying tests only cover cnf.jwk extraction and PoP validation. Adding unit tests for the identity parsing paths (e.g., x5c-only TS03, iss-only draft, and mismatched iss vs x5c identity) would help prevent regressions.
// parseAttestationIdentity extracts identity from a WIA JWT.
// Handles two formats:
// - If `iss` claim present: use as issuer (IETF draft format)
// - If x5c header present and no iss: derive issuer from leaf cert (TS03 format)
func parseAttestationIdentity(attestation string) (*attestationIdentity, error) {
internal/apigw/apiv1/client.go:158
- When
accept_wallet_attestationis set butapigw.trust.pdp_urlis empty, wallet attestation is silently disabled (theif ... && pdpURL != ""guard just skips initialization). Given the config comment and PR description say pdp_url is required for this feature, this should fail fast (or at least log an explicit warning) to avoid operators believing attestation is enabled when it isn't.
// Wallet attestation: enabled when accept_wallet_attestation + pdp_url are set
if cfg.APIGW.Delivery.OpenID4VCI.AcceptWalletAttestation && pdpURL != "" {
c.walletAttestationEvaluator = trust.NewWalletAttestationEvaluator(trustEvaluator)
internal/apigw/httpserver/endpoints_oauth.go:34
OAuth-Client-AttestationandOAuth-Client-Attestation-PoPare populated afterBinding.Request, so they bypass the usual request validation (size limits / printability). It would be easy to trigger extra parsing work or memory pressure with oversized header values. Consider enforcing the same max length you use forclient_assertion(8192) before assigning these headers onto the request.
This issue also appears on line 132 of the same file.
// Extract OAuth-Client-Attestation headers (draft-ietf-oauth-attestation-based-client-auth-04 §3.1)
request.ClientAttestation = c.GetHeader("OAuth-Client-Attestation")
request.ClientAttestationPoP = c.GetHeader("OAuth-Client-Attestation-PoP")
Per masv3971's suggestion - accept_wallet_attestation and
wallet_attestation_policy were top-level fields on
delivery.openid4vci, but wallet attestation is fundamentally a
trust-evaluation mechanism (it delegates to the PDP configured via
trust.pdp_url). Moved both into a new TrustConfig.WalletAttestation
struct:
apigw:
trust:
pdp_url: "https://trust.siros.se/pdp"
wallet_attestation:
enabled: true
policy:
rules: [...]
This config hasn't shipped yet (still unmerged in this PR), so the
move is free - no deployed config to break. Updated the one call site
(internal/apigw/apiv1/client.go), the docs/TRUST_AND_IDENTITY.md
example, and regenerated docs/CONFIGURATION.md.
|
Done (commit e02e3a8) — moved apigw:
trust:
pdp_url: "https://trust.siros.se/pdp"
wallet_attestation:
enabled: true
policy:
rules:
- "(wallet (attestation_source ios_app_attest)(scope pid)(issuer *))"This config hasn't shipped anywhere yet, so the move was free. Updated the one call site, the |
# Conflicts: # docs/CONFIGURATION.md
|
masv3971: "Since spocp is used both for endpoint access and oidc claims, i would like to harmonize them as much as possible. The goal should be that both endpoint rules and oidc rules can be processed by the same method." pkg/httphelpers.SafeEngine and pkg/issuance.PolicyEngine were nearly identical hand-rolled wrappers around *spocp.AdaptiveEngine + a mutex, each with its own copy of rule parsing/loading and (for httphelpers only) rule-shape validation. Extracted the shared machinery into pkg/spocputil (which already held the shared S-expression parser): - spocputil.Engine: the shared engine wrapper (QueryElement, RuleCount, ExportRules). httphelpers.SafeEngine is now a type alias for it, so every existing call site across internal/apigw and internal/verifier that references *httphelpers.SafeEngine keeps compiling unchanged. - spocputil.BuildEngine: parses inline rules + an optional rules file, validating each against a (tag, ordered dimension list) shape when one is given. Both BuildSPOCPEngine (endpoint access, tag "vc") and NewPolicyEngine (issuance policy, tag "credential") now call this instead of duplicating the loop. - spocputil.ValidateRuleElement: generalizes the old httphelpers-only validateRuleElement to an arbitrary tag/dimension list. This also closes an old deferred TODO (a Copilot comment on this same PR, acknowledged as "will add validation as a follow-up if needed") -- issuance policy rules loaded from a file now get the same shape validation inline rules already had. - spocputil.BuildTaggedQuery: generalizes the old fixed 6-argument BuildSPOCPQuery and issuance's BuildQuery into one query builder driven by an ordered dimension list. One real incompatibility surfaced while merging the two: issuance rules author wildcards as an empty dimension, e.g. "(org_id)" (TestEvaluate_WildcardRule, TestEvaluate_MultipleRules), while endpoint-access rules require an explicit "*" atom and reject empty dimensions (TestBuildSPOCPEngine_EmptyRequiredParts). Rather than silently break one domain's existing rule files to match the other, ValidateRuleElement takes a requireValue bool controlling which convention applies -- true for endpoint-access (unchanged behavior), false for issuance (unchanged behavior, now just validated when a QueryTemplate is configured instead of never validated at all). Deliberately did NOT touch pkg/trust.WalletAttestationPolicyEngine, a third, separate SPOCP usage site from an already-merged PR (SUNET#516) that masv3971's comment didn't mention. Added pkg/spocputil/engine_test.go for the new shared code directly. All existing pkg/httphelpers and pkg/issuance SPOCP tests pass unchanged. Regenerated docs/CONFIGURATION.md and updated IssuancePolicy.Rules/RulesFile's doc comments to describe the new validation.



Summary
Enable wallets to authenticate without pre-registration in the static client map, by presenting a wallet-provider-signed attestation JWT (WIA) plus a proof-of-possession JWT, validated via delegation to the go-trust PDP.
Changes
pkg/trust.WalletAttestationEvaluator: parses the attestation JWT, extracts wallet identity (from the x5c certificate chain when present, otherwise fromissper the IETF draft format), validates the PoP JWT againstcnf.jwk, and delegates the trust decision to the PDP (role=wallet-provider)config.OAuthServer.AcceptWalletAttestation(accept_wallet_attestation): enables the feature; requirestrust.pdp_urlconfig.WalletAttestationPolicy(wallet_attestation_policy): optional SPOCP rules gating which attestation tiers (attestation_source) may request which scopesclient_idis not found in the staticclientsmapOAuth-Client-Attestation+OAuth-Client-Attestation-PoPHTTP headers per draft-ietf-oauth-attestation-based-client-auth-04 §3.1; legacy form-bodyclient_assertion(no PoP) still accepted for backward compatibilityVerification Flow
client_assertionin legacy mode)client_idin the static client map → not foundcnf.jwkand checksaud/expiss, when present, is only used for routing and SPOCP authorization, and is cross-checked against the cert identity rather than trusted on its ownrole=wallet-provider; PDP validates the wallet provider against trust lists/federationConfiguration Example
The static
clientsmap remains as a backward-compatible fallback. Existing pre-registered clients work unchanged.Open question
There's an unresolved review thread on
docs/TRUST_AND_IDENTITY.mdabout whether the "Requirements" section correctly reflects TS03 (identity from signature/cert only,issused for routing/authz only) — flagging for @leifj to confirm before that doc section is finalized.Context
Phase 3c of the client-id-strategy plan — eliminates the need for static client registration when wallet providers issue attestations.