Skip to content
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

Fix c_nonce language; may fix #331 #365

Merged
merged 19 commits into from
Aug 20, 2024
Merged

Fix c_nonce language; may fix #331 #365

merged 19 commits into from
Aug 20, 2024

Conversation

awoie
Copy link
Contributor

@awoie awoie commented Jul 16, 2024

May fix #331.

  • removed c_nonce from batch (was MUST)
  • c_nonce only enforced if issuer policy requires it
  • made it clear that c_nonce is optional
  • fixed c_nonce language for ldp proof type

out of scope of this PR:

Copy link
Collaborator

@Sakurann Sakurann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nits here and there but overall looks good.

@@ -1070,9 +1070,11 @@ Cache-Control: no-store

### Credential Issuer Provided Nonce {#issuer-provided-nonce}

The Credential Issuer that requires the Client to send a key proof of possession of the key material for the Credential to be bound to (`proof`) MAY receive a Credential Request without such a key proof or with an invalid key proof. In such a case, the Credential Issuer MUST provide the Client with a `c_nonce` defined in (#credential-response) in a Credential Error Response using `invalid_proof` error code defined in (#credential-error-response).
The Credential Issuer that requires the Client to send a key proof of possession of the key material for the Credential to be bound to (`proof` or `proofs`) MAY receive a Credential Request or Batch Credential Request without a or with an invalid server-provided `c_nonce` value included in the `proof` or `proofs` parameter. In such a case, the Credential Issuer MAY provide the Client with a `c_nonce` defined in (#credential-response) in a Credential Error Response or Batch Credential Error Response using `invalid_proof` error code defined in (#credential-error-response).
Copy link
Member

@peppelinux peppelinux Jul 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
The Credential Issuer that requires the Client to send a key proof of possession of the key material for the Credential to be bound to (`proof` or `proofs`) MAY receive a Credential Request or Batch Credential Request without a or with an invalid server-provided `c_nonce` value included in the `proof` or `proofs` parameter. In such a case, the Credential Issuer MAY provide the Client with a `c_nonce` defined in (#credential-response) in a Credential Error Response or Batch Credential Error Response using `invalid_proof` error code defined in (#credential-error-response).
The Credential Issuer MAY accept a Credential Request or Batch Credential Request that lacks a valid `c_nonce` value in the `proof` or `proofs` parameter. In such cases, the Credential Issuer MAY respond with a Credential Error Response or Batch Credential Error Response, providing a `c_nonce` as specified in (#credential-response) and using the `invalid_proof` error code as defined in (#credential-error-response).

TODO: remove the bach endpoint ref when the related PR will be merged!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not sound right to me. If the credential issuer accepts the proof(s) without a nonce, it is a bit misleading to say that under this condition, the issuer may provide a c_nonce.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have just reworded what I understood from your text trying to simplify the reading.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why is Batch still in here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs to be removed

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peppelinux i updated the text to make it more readable. please also check @Sakurann

Copy link
Contributor

@jogu jogu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some trivial suggestions but basically looks good to me, thanks Oliver!

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved
awoie and others added 3 commits August 7, 2024 10:09
Co-authored-by: Joseph Heenan <[email protected]>
Co-authored-by: Joseph Heenan <[email protected]>
Co-authored-by: Joseph Heenan <[email protected]>
@Sakurann Sakurann merged commit 3f88f5e into main Aug 20, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Is c_nonce required in proof or not?
8 participants