feat(openid4vci): client compliance (issue #3152)#4058
Draft
JorisHeadease wants to merge 28 commits into
Draft
Conversation
Replace draft-era error codes (unsupported_credential_type, unsupported_credential_format) with the complete set of 7 Credential Endpoint error codes from OpenID4VCI v1.0 Section 8.3.1.2.
Core structural changes for OpenID4VCI v1.0 alignment:
- Metadata uses credential_configurations_supported map keyed by
credential_configuration_id (replaces credentials_supported array)
- Credential offers reference configuration IDs instead of inline
credential definitions (credential_configuration_ids field)
- Typed grant structs replace untyped maps in offers
- Credential requests use credential_configuration_id
- Issuer matches credentials to configurations via findCredentialConfigID
- Config IDs generated as {CredentialType}_{format}
- InvalidNonce used for nonce errors (was InvalidProof in draft)
- server_error returns HTTP 500 (was incorrectly 400)
Wallet-side changes for v1.0 alignment: - Holder resolves credential_configuration_id from issuer metadata instead of using inline credential definitions from offers - Credential requests use credential_configuration_id (v1.0 preferred) - Typed grant structs replace untyped map access - ServerError used for upstream failures (not InvalidRequest) - API handler returns non-pointer Credential in response
- Update OpenAPI schemas for v1.0 field names and structures - Update error code documentation for Credential Endpoint - Remove PreAuthorizedGrantAnonymousAccessSupported from VP authorization server metadata (belongs in VCI issuer metadata only per Section 12.3)
Three spec compliance fixes found during detailed v1.0 review:
- Credential response: use `credentials` array of wrapper objects
with `credential` key per Section 8.3
- Credential request: use `proofs` (plural) with
`{"jwt": ["..."]}` structure per Section 8.2.1
- Error response: remove c_nonce/c_nonce_expires_in fields (wallet
should use Nonce Endpoint), make c_nonce optional in holder
Update auth/ module OpenID4VCI client code for v1.0 compliance: - Use Nonce Endpoint instead of c_nonce from token response - Add credential_configuration_id to credential request and session - Change CredentialResponseEntry.Credential to json.RawMessage - Add invalid_nonce retry logic in callback - Add RequestNonce to IAM client interface - Remove c_nonce_expires_in from NonceResponse - Reject non-string @context/type entries in holder metadata parsing - Regenerate mocks and OpenAPI generated code
- Move nil body check before first field access in RequestOpenid4VCICredentialIssuance - Add comma-ok assertion on nonce claim type in issuer validateProof - Validate format field presence in holder resolveCredentialConfiguration - Add iss claim to holder proof JWT for consistency with auth module - Add tests: nil OwnDID, empty credentials, non-string nonce, missing format
…epcopyMap Restore original comments for unchanged OAuth2 error codes. Replace JSON round-trip deepcopy with direct map copy matching the master approach.
Revert deepcopyMap to JSON round-trip: shallow copy is insufficient for nested maps like credential_definition. Remove resolved TODO about credential validation (already done via ValidateDefinitionWithCredential).
- Restore defensive panics in deepcopyMap for unmarshalable data - Make CredentialOffer.Grants a pointer with omitempty (OPTIONAL per Section 4.1.1) - Return false for non-string types in matchesCredential instead of silently skipping - Add iss claim validation in issuer proof verification per v1.0 Appendix F.1, with dedicated test case - Validate non-empty c_nonce from Nonce Endpoint responses - Remove redundant WithContext in RequestNonce
Remove draft-era Format and CredentialDefinition fields from CredentialRequest (v1.0 uses credential_configuration_id only). Rename CredentialConfigurationId(s) to CredentialConfigurationID(s) per Go naming convention for acronyms. JSON wire format unchanged.
Change Credential field from map[string]interface{} to
json.RawMessage to support any credential format (JSON-LD objects,
JWT strings). Fixes Copilot review finding and aligns vcr module
with auth module's type.
Delete duplicate CredentialRequest, CredentialResponse, and CredentialResponseEntry types from auth/client/iam. Use the canonical types from vcr/openid4vci as single source of truth. Replace local jwtTypeOpenID4VCIProof const with openid4vci.JWTTypeOpenID4VCIProof.
Contributor
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (12)
🤖 Increase coverage with AI coding...🚦 See full report on Qlty Cloud » 🛟 Help
|
Validate authorization_details entries per v1.0 Section 5.1.1: - type must be "openid_credential" - credential_configuration_id is required and must exist in issuer credential_configurations_supported - Inject locations field when authorization_servers is present - Sanitize entries to only known keys to prevent arbitrary JSON passthrough - Reject multiple entries (single credential issuance only) Add CredentialConfigurationsSupported to OpenIDCredentialIssuerMetadata. Also fix nil context usage throughout openid4vci_test.go: use context.Background() for method calls and gomock.Any() for mock expectations.
Check holder's signing algorithm against the issuer's advertised proof_signing_alg_values_supported (v1.0 Appendix F.1) in both the authorization code flow and pre-authorized code flow. Shared validation logic extracted to openid4vci.ValidateProofSigningAlg.
Detect transaction_id in credential responses (v1.0 Section 8.3) and return a clear error instead of a generic "no credentials" message. The transaction_id value is logged at warn level but excluded from error messages to prevent leaking issuer-internal state.
Use Pushed Authorization Requests when the AS metadata advertises a pushed_authorization_request_endpoint. All authorization parameters are POSTed server-to-server; the browser redirect carries only client_id and the returned request_uri. Falls back to query parameters when PAR is not advertised.
When the token response includes authorization_details with credential_identifiers (v1.0 Section 6.2), use credential_identifier instead of credential_configuration_id in the credential request (Section 8.2). Adds GetRaw to TokenResponse for accessing non-string additional parameters.
Add scope as an alternative to authorization_details for requesting credentials (v1.0 Section 5.1.2). The scope is resolved against the issuer's credential_configurations_supported metadata. Both scope and authorization_details can be provided simultaneously per the spec. Only a single scope value is supported, consistent with the single-entry restriction for authorization_details.
When the credential issuer's metadata contains signed_metadata (v1.0 Section 12.2.3), verify the JWT signature using the issuer's key from the DID document. Validates typ header, required claims (sub, iat), and compares metadata claims against the unsigned metadata. Rejects metadata if verification fails; proceeds without it if absent (field is OPTIONAL).
The proof JWT audience (aud) must be the Credential Issuer Identifier per v1.0 Section 8.2.1.1, not the Authorization Server issuer. These differ when the credential issuer delegates to a separate AS.
JorisHeadease
force-pushed
the
feature/openid4vci-client-compliance
branch
from
bcb7311 to
6b9902b
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Implements OpenID4VCI v1.0 (ID1) client-side compliance improvements, aligning the Nuts node’s OID4VCI wallet/client flows with issuer metadata requirements and interoperability expectations from issue #3152.
Changes:
- Add support for PAR (RFC 9126), scope-based credential requests, and token-response
credential_identifiers→credential_identifiercredential requests. - Add signed issuer metadata (
signed_metadata) verification and proof signing algorithm validation against issuer metadata. - Detect deferred issuance (
transaction_id) and fail fast with a clear “not supported” error.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
vcr/openid4vci/types_test.go |
Adds tests for deferred responses and proof signing algorithm extraction/validation helpers. |
vcr/openid4vci/types.go |
Extends OID4VCI types with transaction_id, credential_identifier, and proof-alg validation helpers. |
vcr/openid4vci/issuer_client_test.go |
Adds coverage for deferred issuance error handling in issuer client. |
vcr/openid4vci/issuer_client.go |
Detects transaction_id and returns “deferred issuance not supported”. |
vcr/holder/openid_test.go |
Adds tests for proof signing algorithm validation during holder issuance flow. |
vcr/holder/openid.go |
Validates holder proof signing algorithm against issuer metadata before requesting credentials. |
docs/_static/auth/v2.yaml |
Updates API schema: makes authorization_details optional and documents scope. |
crypto/jwx.go |
Adds JWTTyp helper to read JWT typ header without validation. |
auth/oauth/types_test.go |
Adds tests for TokenResponse.GetRaw. |
auth/oauth/types.go |
Adds GetRaw, PAR metadata field, and OID4VCI issuer metadata extensions (signed_metadata, configurations). |
auth/client/iam/openid4vp_test.go |
Adds tests for signed_metadata verification and PAR; adapts credential request signature changes. |
auth/client/iam/openid4vp.go |
Adds PAR method and updates VerifiableCredentials call signature to include credential_identifier. |
auth/client/iam/mock.go |
Updates mocks for new PAR and updated VerifiableCredentials signature. |
auth/client/iam/interface.go |
Extends IAM client interface with PAR and credential_identifier support. |
auth/client/iam/client.go |
Implements PAR call and signed_metadata verification; sends credential_identifier in credential requests. |
auth/api/iam/session.go |
Stores issuer-supported proof signing algorithms in session for later validation. |
auth/api/iam/openid4vci_test.go |
Adds extensive test coverage for new OID4VCI behaviors (PAR, scope, identifiers, alg validation, deferred). |
auth/api/iam/openid4vci.go |
Adds scope support, authorization_details validation/sanitization, PAR flow, credential_identifier usage, deferred detection, and alg validation. |
auth/api/iam/generated.go |
Updates generated request body: authorization_details optional and adds scope. |
Comments suppressed due to low confidence (2)
auth/client/iam/client.go:37
- Import block is not gofmt-sorted (third-party imports are mixed with stdlib). This will fail gofmt/lint checks and makes diffs noisier. Please run gofmt (or re-order imports into stdlib / third-party groups).
import (
"bytes"
"context"
stdcrypto "crypto"
"encoding/json"
"errors"
"fmt"
"github.com/lestrrat-go/jwx/v2/jws"
"github.com/lestrrat-go/jwx/v2/jwt"
"github.com/nuts-foundation/nuts-node/crypto"
"github.com/nuts-foundation/nuts-node/vdr/resolver"
"io"
"net/http"
"net/url"
"strings"
"time"
auth/client/iam/openid4vp_test.go:40
- Import block is not gofmt-sorted (stdlib and third-party imports are interleaved). This commonly breaks CI formatting/lint checks; please run gofmt or re-order the imports.
import (
"context"
"crypto/ecdsa"
"crypto/tls"
"encoding/json"
"errors"
"fmt"
"github.com/lestrrat-go/jwx/v2/jwa"
"github.com/lestrrat-go/jwx/v2/jws"
"github.com/lestrrat-go/jwx/v2/jwt"
"github.com/nuts-foundation/nuts-node/http/client"
test2 "github.com/nuts-foundation/nuts-node/test"
"github.com/nuts-foundation/nuts-node/vcr/credential"
"github.com/nuts-foundation/nuts-node/vdr/didsubject"
"net/http"
"net/http/httptest"
"net/url"
"testing"
"time"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Require credential_endpoint to be present in the signed_metadata JWT rather than silently skipping validation when absent. The spec requires all metadata parameters to be present as top-level claims in the JWT. Based on Copilot PR review feedback.
Contributor
Author
|
PR ready, keeping it as draft until #4057 is merged in. |
JorisHeadease
force-pushed
the
feature/openid4vci-v1
branch
from
2474568 to
a72d50a
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Client-side OpenID4VCI v1.0 compliance improvements per issue #3152, building on the wire format migration in #4057.
Planned commits
authorization_detailsagainst issuer metadata (Section 5.1.1)proof_signing_alg_values_supported(Appendix F.1)credential_identifiersin token response (Section 6.2)signed_metadata(Section 11.2)Skipped
DPoP (Section 6 + 7) The credential request's proof JWT (Section 8.2) already covers the same threat surface: it's signed by the wallet's DID key, verified against the DID document, and includes a nonce. A stolen access token alone is useless without the wallet's private key. DPoP would add a second PoP layer with no practical security gain. Worth revisiting if an external issuer requires it.
credential_identifiers_supportedNot part of the v1 spec?11.3
pre-authorized_grant_anonymous_access_supportedThe issue notes this is "incorrectly added to our metadata since we do not supportpre-authorized_codeflow". This was already addressed in the v1 wire format migration (feat(openid4vci): migrate to v1.0 spec #4057). The field was restored because we do support thepre-authorizedcode flow on the issuer side.Notification endpoint (Section 10) Deferred, depends on credential lifecycle management.
Closes #3152