Enable non-breaking extensibility #382
Conversation
selfissued
requested review from
rohe,
jogu,
peppelinux,
tplooker,
awoie,
Sakurann and
bc-pi
Applied Brian's suggestion Co-authored-by: Brian Campbell <[email protected]>
There was a problem hiding this comment.
@selfissued Any reason why Deferred Credential Response and Notification Response were omitted?
|
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? |
There was a problem hiding this comment.
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 <[email protected]>
Could you please elaborate why? could not find in wg call minutes |
| @@ -446,6 +449,10 @@ The request parameter `authorization_details` defined in Section 2 of [@!RFC9396 | |||
| * `credential_configuration_id`: REQUIRED when `format` parameter is not present. String specifying a unique identifier of the Credential being described in the `credential_configurations_supported` map in the Credential Issuer Metadata as defined in (#credential-issuer-parameters). The referenced object in the `credential_configurations_supported` map conveys the details, such as the format, for issuance of the requested Credential. This specification defines Credential Format specific Issuer Metadata in (#format-profiles). It MUST NOT be present if `format` parameter is present. | |||
| * `format`: REQUIRED when `credential_configuration_id` parameter is not present. String identifying the format of the Credential the Wallet needs. This Credential Format Identifier determines further claims in the authorization details object needed to identify the Credential type in the requested format. This specification defines Credential Format Profiles in (#format-profiles). It MUST NOT be present if `credential_configuration_id` parameter is present. | |||
|
|
|||
| Additional `authorization_details` data fields MAY be defined and used, | |||
| as described in [@!RFC9396]. | |||
| The Wallet MUST ignore any unrecognized parameters. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@pmhsfelix Thanks. I corrected the party that must ignore authorization_details extensions.
There was a problem hiding this comment.
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?
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. |
| @@ -446,6 +449,10 @@ The request parameter `authorization_details` defined in Section 2 of [@!RFC9396 | |||
| * `credential_configuration_id`: REQUIRED when `format` parameter is not present. String specifying a unique identifier of the Credential being described in the `credential_configurations_supported` map in the Credential Issuer Metadata as defined in (#credential-issuer-parameters). The referenced object in the `credential_configurations_supported` map conveys the details, such as the format, for issuance of the requested Credential. This specification defines Credential Format specific Issuer Metadata in (#format-profiles). It MUST NOT be present if `format` parameter is present. | |||
| * `format`: REQUIRED when `credential_configuration_id` parameter is not present. String identifying the format of the Credential the Wallet needs. This Credential Format Identifier determines further claims in the authorization details object needed to identify the Credential type in the requested format. This specification defines Credential Format Profiles in (#format-profiles). It MUST NOT be present if `credential_configuration_id` parameter is present. | |||
|
|
|||
| Additional `authorization_details` data fields MAY be defined and used, | |||
| as described in [@!RFC9396]. | |||
| The Credential Issuer MUST ignore any unrecognized parameters. | |||
There was a problem hiding this comment.
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_detailsto 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"?
There was a problem hiding this comment.
Ouch, well spotted, I'd missed that. That's definitely awkward...
There was a problem hiding this comment.
Thanks for pointing out this conflict, @ju-cu. I added language to the PR explicitly addressing this conflict.
There was a problem hiding this comment.
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_detailsdata fields MAY be defined and used
when thetypevalue isopenid_credential.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
Co-authored-by: Judith <[email protected]>
| @@ -655,6 +673,10 @@ In addition to the response parameters defined in [@!RFC6749], the Authorization | |||
|
|
|||
| Note: The `credential_identifiers` parameter cannot be used when the `scope` parameter is used in the Authorization Request to request issuance of a Credential. | |||
|
|
|||
| Additional Token Response parameters MAY be defined and used, | |||
| as described in [@!RFC6749]. | |||
| The Wallet MUST ignore any unrecognized parameters. | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ofauthorization_detailsdoesn't seem to be covered by the PR, i.e., there isn't any mention to the Wallet ignoring extra fields in theauthorization_detailsproduced 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.
There was a problem hiding this comment.
If you have better text in mind @pmhsfelix please make a change suggestion that I can apply to the PR. Thanks.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
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. |
I deleted the text you're not comfortable with so that we can merge the rest. |
|
@selfissued thank you! let us know when all other comments are addressed so that we can merge this PR. |
Co-authored-by: Judith <[email protected]>
Co-authored-by: Pedro Felix <[email protected]>
Co-authored-by: Brian Campbell <[email protected]>
| @@ -526,6 +533,10 @@ This specification defines the following request parameters that can be supplied | |||
|
|
|||
| 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, | |||
There was a problem hiding this comment.
do we really need to state this again?
There was a problem hiding this comment.
Definitely not technically necessary, but I think doing so will help implementers that aren't that familiar with OAuth2.
| @@ -605,6 +616,10 @@ If the Token Request contains a scope value related to Credential issuance and t | |||
|
|
|||
| 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, | |||
There was a problem hiding this comment.
do we really need to state this again?
Co-authored-by: Kristina <[email protected]>
|
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! |
Fixes #375