Attempt to clarify intro text about key attestations #524
Conversation
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
The singular "key" was being used in a way that could be confusing given a single key attestation can be used to attest multiple keys. closes #463
c2bo
approved these changes
May 28, 2025
Sakurann
reviewed
May 28, 2025
| @@ -919,10 +919,10 @@ This specification defines the following proof types: | |||
|
|
|||
| There are two ways to convey key attestations (as defined in (#keyattestation)) of the cryptographic key material during Credential issuance: | |||
|
|
|||
| - The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession of the key and adds the key attestation in the JOSE header. | |||
| - The Wallet uses the `attestation` proof type in the Credential Request with the key attestation without a proof of possession of the key itself. | |||
| - The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession for one of the attested keys and adds the key attestation in the JOSE header. | |||
There was a problem hiding this comment.
i think this is super important
Suggested change
| - The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession for one of the attested keys and adds the key attestation in the JOSE header. | |
| - The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession for one of the attested keys and adds the key attestation with all of the attested keys in the JOSE header. |
There was a problem hiding this comment.
I'm not too keen on this wording, I think it becomes a bit ambiguous and could be read as saying to add the JWKs to the JOSE header in addition to the attestation. I do think it's important to make clear that the key attestation can contain multiple keys though - would adding that just before the list, or in the paragraph below the list work for you? I've made a suggestion for adding it below.
There was a problem hiding this comment.
I agree with @jogu, that suggestion is clearer than this proposed language.
Sakurann
reviewed
May 28, 2025
| - The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession of the key and adds the key attestation in the JOSE header. | ||
| - The Wallet uses the `attestation` proof type in the Credential Request with the key attestation without a proof of possession of the key itself. | ||
| - The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession for one of the attested keys and adds the key attestation in the JOSE header. | ||
| - The Wallet uses the `attestation` proof type in the Credential Request to provide a key attestation without a proof of possession of any of the keys. |
There was a problem hiding this comment.
Suggested change
| - The Wallet uses the `attestation` proof type in the Credential Request to provide a key attestation without a proof of possession of any of the keys. | |
| - The Wallet uses the `attestation` proof type in the Credential Request to provide a key attestation for all of the attested keys without a proof of possession of any of those keys. |
Sakurann
reviewed
May 28, 2025
Co-authored-by: Kristina <[email protected]>
jogu
commented
May 28, 2025
|
|
||
| Depending on the Wallet's implementation, the `attestation` may avoid unnecessary End-User interaction during Credential issuance, as the key itself does not necessarily need to perform signature operations. | ||
| Depending on the Wallet's implementation, the `attestation` may avoid unnecessary End-User interaction during Credential issuance, as the key to which the Credential will be bound does not necessarily need to perform signature operations. |
There was a problem hiding this comment.
Suggested change
| Depending on the Wallet's implementation, the `attestation` may avoid unnecessary End-User interaction during Credential issuance, as the key to which the Credential will be bound does not necessarily need to perform signature operations. | |
| Depending on the Wallet's implementation, the `attestation` may avoid unnecessary End-User interaction during Credential issuance, as the key(s) to which the Credential(s) will be bound does not necessarily need to perform signature operations, and one key attestation can be used to attest multiple keys. |
| @@ -919,10 +919,10 @@ This specification defines the following proof types: | |||
|
|
|||
| There are two ways to convey key attestations (as defined in (#keyattestation)) of the cryptographic key material during Credential issuance: | |||
|
|
|||
| - The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession of the key and adds the key attestation in the JOSE header. | |||
| - The Wallet uses the `attestation` proof type in the Credential Request with the key attestation without a proof of possession of the key itself. | |||
| - The Wallet uses the `jwt` proof type in the Credential Request to create a proof of possession for one of the attested keys and adds the key attestation in the JOSE header. | |||
There was a problem hiding this comment.
I'm not too keen on this wording, I think it becomes a bit ambiguous and could be read as saying to add the JWKs to the JOSE header in addition to the attestation. I do think it's important to make clear that the key attestation can contain multiple keys though - would adding that just before the list, or in the paragraph below the list work for you? I've made a suggestion for adding it below.
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.
The singular "key" was being used in a way that could be confusing given a single key attestation can be used to attest multiple keys.
closes #463