OID4VC: Nonce validation, .well-known path, readme#3151
Conversation
weiiv
commented
May 26, 2026
- conditional nonce in 4 different mode through configure
- .well-known path to align with v1.0 spec
- readme for nonce configure
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
|
Hi @jamshale, when you have a moment, could you please take a look at this small PR and approve if everything looks good? Thanks so much! -Ivan |
|
Hey @weiiv -- I had Claude review the PR, and the response requires your review and response, I think. A couple of things seem odd. Can you take a look, please? Sorry for the long description...typical AI. :-( This PR makes three related changes to the OID4VC plugin: Conditional nonce endpoint — a new OID4VCI_ENABLE_NONCE_ENDPOINT config flag lets operators disable the /nonce endpoint. When disabled, nonce validation falls back to comparing the c_nonce from the access token directly rather than using DB-based single-use redemption. Issues
The previous version used psql variable placeholders (:ADMIN_DB_USER, :ADMIN_DB_PASSWORD, :ADMIN_DB_NAME). This PR replaces them with hardcoded values auth_server_admin / auth_server_admin. Even in demo/test infrastructure, committing hardcoded credentials to git is bad practice and will trigger secret-scanning tools. This should either revert to variables or at minimum be clearly scoped to the demo folder with a strong warning.
26 lines are deleted that validated JWT proofs in the notification endpoint. The removed comment says "OID4VCI 1.0 §11: if the request body includes a proof…the notification endpoint MUST validate the JWT proof." Removing this is a spec compliance regression — it isn't gated on enable_nonce_endpoint. The PR description doesn't mention this change.
The old code registered both GET and POST for /nonce. This PR only registers POST. If any existing wallets use GET /nonce, they'll get a 405. The change has no comment or mention in the PR body.
Old behavior: DB redemption if no c_nonce; direct comparison if c_nonce present. New behavior: DB redemption if enable_nonce_endpoint=True, ignoring any c_nonce from the token. The old code branched on whether c_nonce was provided at the call site. The new code always ignores c_nonce when the endpoint is enabled, even if the caller passed one. This is a semantic change that could silently break existing callers that pass c_nonce and expect it to be used.
The test is updated only for enable_nonce_endpoint=True. The False path (direct c_nonce comparison, no DB hit) has no test. Both the token endpoint's nonce-storage path and handle_proof_of_posession's comparison path should have coverage. Minor observations
|
Signed-off-by: Ivan Wei <ivan.wei@ontario.ca>
|
Hi @swcurran, please see my answer below:
Fixed. init.sql is now fully parameterized (:ADMIN_DB_USER, :'ADMIN_DB_PASSWORD', :ADMIN_DB_NAME).
Intentional. OID4VCI 1.0 §11.1 defines the notification body as notification_id, event, event_description only — there is no proof field to validate.
Intentional. OID4VCI 1.0 §7.1 mandates POST /nonce only.
Intentional. When both an external AS issues c_nonce and /nonce is enabled, the wallet puts the /nonce-issued value in its proof (per spec §F.4), but the old "branch on c_nonce presence" compares it against the AS's c_nonce — they never match. Branching on enable_nonce_endpoint is the only correct discriminator. Sole caller is credential.py:277. Docstring added.
Added test_handle_pop_no_nonce_endpoint and test_get_token_nonce_behavior in test_public_routes.py.
Fixed. Uses |