group `credential_response_encryption` parameters in the Issuer Metadata by paulbastian · Pull Request #153 · openid/OpenID4VCI

paulbastian · 2023-12-14T18:37:18Z

Closes #152

I want to note that we don't have an example for the usage of this parameter in the issuer metadata yet.
Should we add an example?

peppelinux · 2023-12-17T16:23:28Z

@paulbastian yes an example would improve this PR

Sakurann

i don't think the current description is sufficient, and would like to see it expanded. having an example would be nice, but not mandatory to merge this PR IMO

Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>

bc-pi

I am in favor of grouping these parameters but I think the case of a credential issuer being able to encrypt a credential response and publishing the supported alg/enc values while not requiring the credential response be encrypted should be retained.

awoie

Generally looks good. I made some comments. Please accept them. I'm ok if jwk_values_supported does not get accepted but I think it might be useful. Especially with brainpool etc.

Sakurann · 2023-12-20T17:19:17Z

@bc-pi I don't see anything in the PR that mandates the encryption when these metadata parameters are present.

bc-pi · 2023-12-20T21:15:28Z

@bc-pi I don't see anything in the PR that mandates the encryption when these metadata parameters are present.

These bits of text, for example, that use the word "require" read that way to me.

The Credential Issuer MAY require encrypted responses by including credential_response_encryption object in the Credential Issuer Metadata.

credential_response_encryption: OPTIONAL. Object containing information whether the Credential Issuer requires encryption of the Credential and Batch Credential Response on top of TLS.

Sakurann · 2023-12-21T03:18:54Z

@bc-pi I see your point - I made two suggestions to address them. changing "requires" to "supports" made a difference.

Sakurann

assuming something like my suggested changes will be accepted

Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>

Sakurann · 2023-12-21T16:29:55Z

to clarify, the existence of the encryption related parameters signal that issuer supports the encryption, and the existence of the boolean signals that the issuer requires encryption

selfissued · 2023-12-31T19:37:40Z

I plan to review this after the disposition of the set of suggested changes is known.

…uer-metadata

paulbastian · 2024-01-04T13:08:40Z

@bc-pi @selfissued please review

Co-authored-by: Joseph Heenan <joseph@heenan.me.uk>

Sakurann · 2024-01-04T14:57:09Z

+* `credential_response_encryption`: OPTIONAL. Object containing information whether the Credential Issuer supports encryption of the Credential and Batch Credential Response on top of TLS.
+  * `alg_values_supported`: REQUIRED. Array containing a list of the JWE [@!RFC7516] encryption algorithms (`alg` values) [@!RFC7518] supported by the Credential and Batch Credential Endpoint to encode the Credential or Batch Credential Response in a JWT [@!RFC7519].
+  * `enc_values_supported`: REQUIRED. Array containing a list of the JWE [@!RFC7516] encryption algorithms (`enc` values) [@!RFC7518] supported by the Credential and Batch Credential Endpoint to encode the Credential or Batch Credential Response in a JWT [@!RFC7519].
+  * `encryption_required`: REQUIRED. Boolean value specifying whether the Credential Issuer requires the additional encryption on top of TLS for the Credential Response. If the value is `true`, the Credential Issuer requires the encryption for every Credential Response and therefore the Wallet MUST provide encryption keys in the Credential Request. If the value is `false`, the Wallet MAY chose whether it provides encryption keys or not.


I think it logically makes more sense to bring this parameter before enc_values_supported and alg_values_supported when describing?

Sakurann · 2024-01-04T15:17:57Z

DCP WG call - Jan-04-2024 agreed to merge once Brian's approval is in. agreed encryption_required is a good trade-off as a parameter name.

bc-pi · 2024-01-04T15:21:20Z

Brian's approval is in

group credential_response_encryption parameters in the Issuer Metadata

bf0f63b

paulbastian linked an issue Dec 14, 2023 that may be closed by this pull request

Merge Credential Response encryption Issuer Metadata #152

Closed

paulbastian requested review from Sakurann and awoie December 14, 2023 21:33

peppelinux reviewed Dec 15, 2023

View reviewed changes

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

Sakurann reviewed Dec 18, 2023

View reviewed changes

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

Sakurann requested changes Dec 18, 2023

View reviewed changes

Update openid-4-verifiable-credential-issuance-1_0.md

a3cf2db

Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>

paulbastian commented Dec 18, 2023

View reviewed changes

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

paulbastian added 2 commits December 19, 2023 16:23

add example for credential_response_encryption in metadata

7338f89

Update openid-4-verifiable-credential-issuance-1_0.md

16b6df8

paulbastian commented Dec 19, 2023

View reviewed changes

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

bc-pi requested changes Dec 19, 2023

View reviewed changes

awoie approved these changes Dec 19, 2023

View reviewed changes

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

Comment thread examples/credential_issuer_metadata_jwt_vc_json.json Outdated

Sakurann reviewed Dec 21, 2023

View reviewed changes

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

Sakurann approved these changes Dec 21, 2023

View reviewed changes

Sakurann added this to the ID-1 milestone Dec 21, 2023

paulbastian and others added 2 commits December 21, 2023 15:20

Update openid-4-verifiable-credential-issuance-1_0.md

06f367f

Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>

Apply suggestions from code review

3dcfc91

Co-authored-by: Kristina <52878547+Sakurann@users.noreply.github.com>

bc-pi reviewed Dec 21, 2023

View reviewed changes

Comment thread openid-4-verifiable-credential-issuance-1_0.md Outdated

awoie mentioned this pull request Dec 21, 2023

Indicate supported curves for encryption in Credential Issuer metadata #168

Open

selfissued self-requested a review December 31, 2023 19:37

Sakurann mentioned this pull request Jan 4, 2024

Proofread Metadata section #184

Merged

Merge branch 'main' into 152-merge-credential-response-encryption-iss…

905534b

…uer-metadata

fix merging errors

fce45ff

jogu requested changes Jan 4, 2024

View reviewed changes

Comment thread examples/credential_issuer_metadata_jwt_vc_json.json Outdated

Update examples/credential_issuer_metadata_jwt_vc_json.json

a4b0b4a

Co-authored-by: Joseph Heenan <joseph@heenan.me.uk>

paulbastian requested a review from jogu January 4, 2024 14:55

Sakurann reviewed Jan 4, 2024

View reviewed changes

Sakurann requested a review from bc-pi January 4, 2024 14:57

jogu approved these changes Jan 4, 2024

View reviewed changes

selfissued approved these changes Jan 4, 2024

View reviewed changes

bc-pi approved these changes Jan 4, 2024

View reviewed changes

Sakurann merged commit d8724f3 into main Jan 4, 2024

Uh oh!

Conversation

paulbastian commented Dec 14, 2023

Uh oh!

Uh oh!

peppelinux commented Dec 17, 2023

Uh oh!

Uh oh!

Sakurann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bc-pi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

awoie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sakurann commented Dec 20, 2023

Uh oh!

bc-pi commented Dec 20, 2023

Uh oh!

Uh oh!

Sakurann commented Dec 21, 2023

Uh oh!

Sakurann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sakurann commented Dec 21, 2023

Uh oh!

selfissued commented Dec 31, 2023

Uh oh!

paulbastian commented Jan 4, 2024

Uh oh!

Uh oh!

Sakurann Jan 4, 2024

Choose a reason for hiding this comment

Uh oh!

Sakurann commented Jan 4, 2024

Uh oh!

bc-pi commented Jan 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

bc-pi left a comment •

edited

Loading

awoie left a comment •

edited

Loading