Enable non-breaking extensibility #382

selfissued · 2024-08-31T05:50:50Z

Fixes #375

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

bc-pi

one suggestion but otherwise LGTM

Applied Brian's suggestion Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>

paulbastian

@selfissued Any reason why Deferred Credential Response and Notification Response were omitted?

selfissued · 2024-09-08T20:27:28Z

I enabled additional Deferred Credential Response parameters in my latest commit.

I didn't add Notification Response parameters because the defined response is 204 No Content. But we could allow for Notification Response parameters if we choose. What do people think?

Sakurann

I would really like us to discuss the "both wallet and the issuer must understand the proof type" requirement. it we keep this requirement, i really don't understand why we don't have similar requirements for other issuer metadata that the wallet has to pre-discover from the issuer metadata
we are not enabling extensibility within credential format profiles?

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

jogu · 2024-09-10T19:56:02Z

I would really like us to discuss the "both wallet and the issuer must understand the proof type" requirement. it we keep this requirement, i really don't understand why we don't have similar requirements for other issuer metadata that the wallet has to pre-discover from the issuer metadata we are not enabling extensibility within credential format profiles?

Discussed on today's WG call, there was consensus on keeping the requirement.

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

Sakurann · 2024-09-12T09:03:14Z

I would really like us to discuss the "both wallet and the issuer must understand the proof type" requirement. it we keep this requirement, i really don't understand why we don't have similar requirements for other issuer metadata that the wallet has to pre-discover from the issuer metadata we are not enabling extensibility within credential format profiles?

Discussed on today's WG call, there was consensus on keeping the requirement.

Could you please elaborate why? could not find in wg call minutes

pmhsfelix · 2024-09-12T14:20:40Z

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


+Additional `authorization_details` data fields MAY be defined and used,
+as described in [@!RFC9396].
+The Wallet MUST ignore any unrecognized parameters.


Since the authorization_details in the authorization request is created by the wallet itself, does it make sense to state that "The Wallet MUST ignore any unrecognized parameters"? Perhaps this sentence should be in the token-response section, where the authorization_details is generated by the AS.

@pmhsfelix Thanks. I corrected the party that must ignore authorization_details extensions.

Should we also add the equivalent sentence on the authorization_details returned in the token response, where now is the wallet that MUST ignore unrecognized parameters put there by the AS?

selfissued · 2024-09-12T15:10:22Z

Could you please elaborate why? could not find in wg call minutes

As discussed in the call, the reason that both parties must understand the proof is that understanding it is integral to the security of the interaction. It's not like most extension points where not-understood values can be ignored. That's why people on the call were good with documenting that this particular extension point doesn't fit the normal must-ignore-if-not-understood pattern.

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

ju-cu · 2024-09-13T07:59:19Z

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


+Additional `authorization_details` data fields MAY be defined and used,
+as described in [@!RFC9396].
+The Credential Issuer MUST ignore any unrecognized parameters.


I wonder if this extensibility is actually compliant with RAR. Section 5 of RFC 9396 states that:

The AS MUST abort processing and respond with an error invalid_authorization_details to the client if [an object in the authorization_details structure] [...] is an object of known type but containing unknown fields,

Does the proposed extensibility mean that, when it comes to the openid_credential type, there are no "unknown fields" because by definition, any unrecognized field gets ignored and consequently all fields are expected and thus "known"?

Ouch, well spotted, I'd missed that. That's definitely awkward...

Thanks for pointing out this conflict, @ju-cu. I added language to the PR explicitly addressing this conflict.

why do we want to overwrite RAR language here though? I think we should rather stick to RAR and remove
the following sentence:

Additional authorization_details data fields MAY be defined and used
when the type value is openid_credential.

We're deviating from RAR for this type because that enables non-breaking extensibility when using the type openid_credential. Without this language, if RAR if followed strictly, if a new authorization_details parameter is used, it would cause an error in existing deployments. Meaning that this type would not be extensible.

@jogu's proposal is to define a claim for this authorization_details type as an alternative extensibility point - a claim that is an object that can contain any kind of additional fields.

@tlodderstedt WDYT?

I don't think anything is being overridden. If the openid_credential type is defined such that it can have additional data fields, then by definition it can have additional data fields. I think this can probably be noted with language that's less likely to be objectionable.

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

Co-authored-by: Judith <59833642+ju-cu@users.noreply.github.com>

ju-cu · 2024-09-13T20:51:33Z

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


+Additional Token Response parameters MAY be defined and used,
+as described in [@!RFC6749].
+The Wallet MUST ignore any unrecognized parameters.


Does that include unknown data fields in an openid_credential within the authorization_details structure?

RFC 9396, Section 7.1 states that the AS may enrich authorization_details in the Token Response. Since we added extensibility for the openid_credential type, I think we should be clear on how the Wallet must treat a response with unknown data fields. We can add a normative statement here or under #(authorization-details. @pmhsfelix had the same concern in another comment and suggested that the Wallet MUST ignore any unrecognized data fields.

I'm not sure what you mean by "unknown data fields" or where you'd like them to be ignored when they occur. Can you maybe give an example?

FYI, I believe I addressed @pmhsfelix's comment in this commit in which I corrected the party that must ignore the not-understood parameter.

authorization_details can be used in two directions:

On an authorization request, produced by the Wallet and consumed by the AS.

On a token response, produced by the AS and consumed by the Wallet.
The later usage of authorization_details doesn't seem to be covered by the PR, i.e., there isn't any mention to the Wallet ignoring extra fields in the authorization_details produced by the AS and present in the token response.

Detail: regarding the use of authorization_details in the authorization request, should it be the Credential Issuer or the Authorization Server that must ignore them.

If you have better text in mind @pmhsfelix please make a change suggestion that I can apply to the PR. Thanks.

@selfissued
I can try to elaborate more on the issue. Using RAR, an Authorization Server MUST return authorization_details in the Token Response (see RFC 9396 Section 7). The authorization_details in the Token Response are not necessarily the same as in the Authorization or Token Request (see RFC 9396 Section 7.1). Technically, with the extension mechanism for the openid_credential type, there could be a discrepancy between the Wallet and the Authorization Server, where the Authorization Server enriches an openid_credential type in the authorization_details array of a Token Response with extra fields that the Wallet does not understand (RFC 9396 assumes that all possible fields of a type are known upfront, which is not the case here). To avoid any undefined behavior, I suggest, that the Wallet must ignore any unknown fields of an openid_credential type in an authorization_details array in a Token Response.

To be clear, the unrecognized parameters that the Wallet must ignore are not only parameters of the Token Response (as defined in RFC 6749) but also unrecognized data fields of the openid_credential type defined in this spec. The former is already defined, and the letter should also be defined imo.

Sakurann · 2024-09-16T16:35:51Z

Could you please elaborate why? could not find in wg call minutes

As discussed in the call, the reason that both parties must understand the proof is that understanding it is integral to the security of the interaction. It's not like most extension points where not-understood values can be ignored. That's why people on the call were good with documenting that this particular extension point doesn't fit the normal must-ignore-if-not-understood pattern.

I understand the thinking, but I still believe this is not enforceable. You can't stop the wallet from sending the proof type that the issuer does not support (just like for any other issuer metadata). it would be much better to paraphrase this to be an error condition - "issuer MUST return an error if the wallet uses a proof type not supported by the issuer.

I am sorry, but I am not comfortable merging this PR with this text. please separate this proof related language into an another PR, so that we could merge everything else meanwhile, thank you.

selfissued · 2024-09-17T01:05:55Z

I am sorry, but I am not comfortable merging this PR with this text. please separate this proof related language into an another PR, so that we could merge everything else meanwhile, thank you.

I deleted the text you're not comfortable with so that we can merge the rest.

Sakurann · 2024-09-17T16:56:39Z

@selfissued thank you! let us know when all other comments are addressed so that we can merge this PR.

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

Sakurann

RAR language is awkward and needs to be fixed

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

Co-authored-by: Judith <59833642+ju-cu@users.noreply.github.com>

Co-authored-by: Pedro Felix <pmhsfelix@gmail.com>

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

Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>

tlodderstedt · 2024-09-26T16:02:04Z

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


 Note: When processing the Authorization Request, the Credential Issuer MUST take into account that the `issuer_state` is not guaranteed to originate from this Credential Issuer in all circumstances. It could have been injected by an attacker.

+Additional Authorization Request parameters MAY be defined and used,


do we really need to state this again?

Definitely not technically necessary, but I think doing so will help implementers that aren't that familiar with OAuth2.

tlodderstedt · 2024-09-26T16:02:15Z

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


 When the Pre-Authorized Grant Type is used, it is RECOMMENDED that the Credential Issuer issues an Access Token valid only for the Credentials indicated in the Credential Offer (see (#credential-offer)). The Wallet SHOULD obtain a separate Access Token if it wants to request issuance of any Credentials that were not included in the Credential Offer, but were discoverable from the Credential Issuer's `credential_configurations_supported` metadata parameter.

+Additional Token Request parameters MAY be defined and used,


do we really need to state this again?

Sakurann

approve with an editorial suggestion

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

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

jogu · 2024-09-26T17:01:56Z

I believe all comments have been responded to and/or resolved now (if I missed any feel free to open issues) and we have 8 approvals so merging!

Enable non-breaking extensibility

93fe666

selfissued requested review from Sakurann, awoie, bc-pi, jogu, peppelinux, rohe and tplooker August 31, 2024 05:50

nemqe reviewed Sep 1, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

bc-pi approved these changes Sep 4, 2024

View reviewed changes

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

692fe51

Applied Brian's suggestion Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>

paulbastian reviewed Sep 5, 2024

View reviewed changes

Allow additional Deferred Credential Response parameters

9a00268

Sakurann requested changes Sep 10, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

jogu approved these changes Sep 10, 2024

View reviewed changes

paulbastian approved these changes Sep 10, 2024

View reviewed changes

Applied Joseph's suggestion

bea446e

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

Sakurann requested a review from nemqe September 12, 2024 09:03

pmhsfelix reviewed Sep 12, 2024

View reviewed changes

Corrected party that must ignore authorization_details extensions

6659b29

ju-cu reviewed Sep 13, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

ju-cu reviewed Sep 13, 2024

View reviewed changes

Address conflict with RAR text on handling unknown parameters

a1306e4

ju-cu reviewed Sep 13, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

Applied Judith's suggestion

6426aba

Co-authored-by: Judith <59833642+ju-cu@users.noreply.github.com>

ju-cu reviewed Sep 13, 2024

View reviewed changes

jogu added this to the Final 1.0 milestone Sep 16, 2024

Delete language about proof types being supported by both parties

d5c8203

c2bo approved these changes Sep 17, 2024

View reviewed changes

Sakurann approved these changes Sep 17, 2024

View reviewed changes

Sakurann mentioned this pull request Sep 19, 2024

Add extensibility to Credential Response #391

Merged

pmhsfelix reviewed Sep 19, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

Sakurann requested changes Sep 19, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

selfissued and others added 2 commits September 19, 2024 17:44

Applied Judith's suggestion

710b3b6

Co-authored-by: Judith <59833642+ju-cu@users.noreply.github.com>

Applied Pedro's suggestion

5dc2113

Co-authored-by: Pedro Felix <pmhsfelix@gmail.com>

peppelinux approved these changes Sep 24, 2024

View reviewed changes

bc-pi reviewed Sep 25, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

Applied Brian's suggestion

3a5b11c

Co-authored-by: Brian Campbell <71398439+bc-pi@users.noreply.github.com>

bc-pi approved these changes Sep 26, 2024

View reviewed changes

tlodderstedt approved these changes Sep 26, 2024

View reviewed changes

Sakurann approved these changes Sep 26, 2024

View reviewed changes

openid-4-verifiable-credential-issuance-1_0.md Outdated Show resolved Hide resolved

Applied Kristina's suggestion

628cc5a

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

nemqe approved these changes Sep 26, 2024

View reviewed changes

jogu merged commit 87bfe4a into openid:main Sep 26, 2024

pmhsfelix mentioned this pull request Oct 22, 2024

Introduce new query language (Approach 3) openid/OpenID4VP#266

Merged

6 tasks


		Note: When processing the Authorization Request, the Credential Issuer MUST take into account that the `issuer_state` is not guaranteed to originate from this Credential Issuer in all circumstances. It could have been injected by an attacker.

		Additional Authorization Request parameters MAY be defined and used,


		When the Pre-Authorized Grant Type is used, it is RECOMMENDED that the Credential Issuer issues an Access Token valid only for the Credentials indicated in the Credential Offer (see (#credential-offer)). The Wallet SHOULD obtain a separate Access Token if it wants to request issuance of any Credentials that were not included in the Credential Offer, but were discoverable from the Credential Issuer's `credential_configurations_supported` metadata parameter.

		Additional Token Request parameters MAY be defined and used,

Enable non-breaking extensibility #382

Enable non-breaking extensibility #382

Uh oh!

Conversation

selfissued commented Aug 31, 2024

Uh oh!

Uh oh!

bc-pi left a comment

Choose a reason for hiding this comment

Uh oh!

paulbastian left a comment

Choose a reason for hiding this comment

Uh oh!

selfissued commented Sep 8, 2024

Uh oh!

Sakurann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jogu commented Sep 10, 2024

Uh oh!

Sakurann commented Sep 12, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

selfissued commented Sep 12, 2024

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sakurann Sep 24, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

bc-pi Sep 25, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ju-cu Sep 17, 2024 • 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!

Sakurann commented Sep 16, 2024

Uh oh!

selfissued commented Sep 17, 2024

Uh oh!

Sakurann commented Sep 17, 2024

Uh oh!

Uh oh!

Sakurann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Sakurann Sep 24, 2024 •

edited

Loading

bc-pi Sep 25, 2024 •

edited

Loading

ju-cu Sep 17, 2024 •

edited

Loading