-
Notifications
You must be signed in to change notification settings - Fork 36
Rephrase conditions to provide nonce in proof types based on presence of Nonce endpoint
#678
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…esence of nonce endpoint
paulbastian
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't judge for DI, but the fix on jwt proof_type looks right
Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>
jogu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still have the part here to solve: #676 (comment) (or we should raise a new issue for that, I don't have a strong opinion which).
I think we should apply the change to 1.0 as well.
|
Hi @awoie Please consider updating the definition of Currently the definition is
IMO this could be update to include
|
I was wondering about the same point - we should be clear where exactly nonce is required and where not. In the case of proof type |
My truth table ( 😄 ) for
|
For the attestation proof type we have this:
|
But yes, I agree with @babisRoutis. I will clean up the language again to clarify all of this since I agree it is language that can be misinterpreted. |
|
@jogu I applied the change to 1.0. Let me know if there is anything else needed wrt change log. Also added some language on pre-generated attestations and how a wallet could detect whether an issuer requires a nonce in the key attestation based on the current specification, e.g., invalid_nonce error. |
| * `proofPurpose`: REQUIRED. MUST be set to `authentication`. | ||
| * `domain`: REQUIRED. MUST be set to the Credential Issuer Identifier. | ||
| * `challenge`: REQUIRED when the Credential Issuer has provided a `c_nonce`. It MUST NOT be used otherwise. String, where the value is a server-provided `c_nonce`. It MUST be present when the issuer has a Nonce Endpoint as defined in (#nonce-endpoint). | ||
| * `challenge`: REQUIRED when the Credential Issuer has a Nonce Endpoint as defined in (#nonce-endpoint). It MUST NOT be used otherwise. String, where the value is a server-provided `c_nonce`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's somewhat inconsistent we say this is REQUIRED (but only when the Issuer has a Nonce Endpoint) while above for nonce we say it is OPTIONAL (but required when the Issuer has a Nonce Endpoint).
Potentially fixes #677 , potentially fixes #676
Note that I created the PR based on #676 (comment).
IMO, one implication is that for
nonceclaims in akey_attestationin ajwtproof, it means, the wallet decides whether to include it which is how I interpret the current version of the spec but wanted to point this out in case it is not obvious for readers of this PR. If the issuer insists on the presence which is unlikely, it could still provide a nonce error. To improve this behaviour, we could define a dedicated issuer metadata parameter, e.g.,require_nonce_in_key_attesatation_in_jwt_proofin a backward compatible way to improve this behaviour.