feat(openid4vci): client compliance (issue #3152)#4058
Draft
JorisHeadease wants to merge 9 commits intofeature/openid4vci-v1from
Draft
feat(openid4vci): client compliance (issue #3152)#4058JorisHeadease wants to merge 9 commits intofeature/openid4vci-v1from
JorisHeadease wants to merge 9 commits intofeature/openid4vci-v1from
Conversation
|
Coverage Impact ⬆️ Merging this pull request will increase total coverage on Modified Files with Diff Coverage (8)
🤖 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
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. |
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