Clarify what can go in client_metadata authz request parameter #233
Conversation
As per working group consensus agreed on 21st May call: #17 (comment) closes #17
Try to make it clearer that other values mustn't be used in client_metadata unless an extension to this specification explicitly allows them to be used.
jogu
force-pushed
the
clarify-client_metadata-parameter
branch
from
19ca39b
to
c4a7c30
Compare
|
|
||
| Values passed in `client_metadata` MUST NOT override authorative data the wallet is able to obtain about the client from other sources, for example those from an OpenID Federation Entity Statement. | ||
|
|
||
| Other metadata parameters defined in, for example, Section 4.3 and Section 2.1 of the OpenID Connect Dynamic Client Registration 1.0 [@!OpenID.Registration] specification or [@!RFC7591] MUST NOT be used unless a profile of this specification explicitly defines them as usable in the `client_metadata` parameter. |
There was a problem hiding this comment.
| Other metadata parameters defined in, for example, Section 4.3 and Section 2.1 of the OpenID Connect Dynamic Client Registration 1.0 [@!OpenID.Registration] specification or [@!RFC7591] MUST NOT be used unless a profile of this specification explicitly defines them as usable in the `client_metadata` parameter. | |
| Other metadata parameters MUST NOT be used unless a profile of this specification explicitly defines them as usable in the `client_metadata` parameter. |
per WG discussion. personally, I would like to see this sentence in line 264
There was a problem hiding this comment.
I'll apply this suggestion.
I can't see a way to have the text read nicely that moves the line up to line 264.
| * `authorization_encrypted_response_alg`: OPTIONAL. As defined in [@!JARM]. | ||
| * `authorization_encrypted_response_enc`: OPTIONAL. As defined in [@!JARM]. | ||
|
|
||
| Values passed in `client_metadata` MUST NOT override authorative data the wallet is able to obtain about the client from other sources, for example those from an OpenID Federation Entity Statement. |
There was a problem hiding this comment.
| Values passed in `client_metadata` MUST NOT override authorative data the wallet is able to obtain about the client from other sources, for example those from an OpenID Federation Entity Statement. | |
| Values passed in `client_metadata` MUST NOT override authoritative data the Wallet is able to obtain about the Client from other sources, for example those from an OpenID Federation Entity Statement. |
There was a problem hiding this comment.
@jogu here I would add an additional guidance for the implementers about how to deal with overlapping metadata, one obtained within the request and another one from an authoritative sources such as Federation or any other trusted metadata registry.
We may apply a rule based on the json object update
we have two objects in an hierarchy of priority: json_authoritative and json_ephemeral, the first from Federation and the latter from the request.
In [1]: json_authoritative = {"a": 1, "b": 2, "c": 3}
In [2]: json_ephemeral = {"a": 2, "b":1, "d":5}
In [3]: json_ephemeral.update(json_authoritative)
In [4]: json_ephemeral
Out[4]: {'a': 1, 'b': 2, 'd': 5, 'c': 3}therefore we may say that authoritative one must overwrite the ephemeral one.
As we may notice, the parameters that are not present in the authoritative but in the ephemeral, will be added to the authoritative one. The weakness of this approach is that the authoritative is therefore "updated" due to the merge with the ephemeral one, and this eluded the federation policies.
For this reason we should say something about this, and therefore distinguish two different moments:
- merge the two metadata having the federation one as priority
- apply again the trust chain policies
This might find space in the openid federation wallet architecure draft, for its specific purpose on this topic
There was a problem hiding this comment.
therefore we may say that authoritative one must overwrite the ephemeral one.
I think the text already says that?
There was a problem hiding this comment.
| Values passed in `client_metadata` MUST NOT override authorative data the wallet is able to obtain about the client from other sources, for example those from an OpenID Federation Entity Statement. | |
| Authoritative data the wallet is able to obtain about the client from other sources, for example those from an OpenID Federation Entity Statement, overrides the values passed in `client_metadata`. |
we could paraphrase like this, but I am hesitant to add anything more than that
There was a problem hiding this comment.
I think Giuseppe is asking: what happens if an encryption key is specified both in the request and in OpenID Federation?
I think this is a bigger topic that probably needs a specific issue and separate PR. This is problem that already exists without this PR.
There are two conflicting requirements:
- Some people want to use per-session encryption keys
- Some people want to ensure that only encryption keys from federation are used
I am pretty sure it is not possible to put an ephemeral key into federation metadata (it's just technically not feasible to have the federation metadata for a client potentially changing multiple times in a second?), so I'm not sure I see an obvious way to satisfy both sets of people.
There was a problem hiding this comment.
I'll apply something along the lines of Kristina's suggestions.
|
|
||
| Values passed in `client_metadata` MUST NOT override authorative data the wallet is able to obtain about the client from other sources, for example those from an OpenID Federation Entity Statement. | ||
|
|
||
| Other metadata parameters defined in, for example, Section 4.3 and Section 2.1 of the OpenID Connect Dynamic Client Registration 1.0 [@!OpenID.Registration] specification or [@!RFC7591] MUST NOT be used unless a profile of this specification explicitly defines them as usable in the `client_metadata` parameter. |
There was a problem hiding this comment.
| Other metadata parameters defined in, for example, Section 4.3 and Section 2.1 of the OpenID Connect Dynamic Client Registration 1.0 [@!OpenID.Registration] specification or [@!RFC7591] MUST NOT be used unless a profile of this specification explicitly defines them as usable in the `client_metadata` parameter. | |
| Other metadata parameters defined in, for example, Section 4.3 and Section 2.1 of the OpenID Connect Dynamic Client Registration 1.0 [@!OpenID.Registration] specification or [@!RFC7591] SHOULD NOT be used unless a profile of this specification explicitly defines them as usable in the `client_metadata` parameter. |
I have changed my mind on this, according to OAuth philosophy things that you don't understand you ignore. It would be better to not break this rule to facilitate extensions based on openid4vp
There was a problem hiding this comment.
I think what you are trying to get at, Giuseppe, is totally possible with a current text, you just need an extension or a profile says why it is ok to use a specific parameter. i would have a strong preference to keep it must not.
There was a problem hiding this comment.
I've changed to 'MUST be ignored" which I think better reflects the intent and extensibility. As Kristina says, it is very deliberate to make clear people must not just use other already defined client metadata values.
| @@ -727,7 +735,7 @@ The JWT response document MUST include `vp_token` and `presentation_submission` | |||
|
|
|||
| The key material used for encryption and signing SHOULD be determined using existing metadata mechanisms. | |||
|
|
|||
| To obtain Verifier's public key for the input to the key agreement to encrypt the Authorization Response, the Wallet MUST use `jwks` or `jwks_uri` claim within the `client_metadata` request parameter, or within the metadata defined in the Entity Configuration when [@!OpenID.Federation] is used, or other mechanisms. | |||
| To obtain Verifier's public key for the input to the key agreement to encrypt the Authorization Response, the Wallet MUST use `jwks` claim within the `client_metadata` request parameter, or within the metadata defined in the Entity Configuration when [@!OpenID.Federation] is used, or other mechanisms. | |||
There was a problem hiding this comment.
| To obtain Verifier's public key for the input to the key agreement to encrypt the Authorization Response, the Wallet MUST use `jwks` claim within the `client_metadata` request parameter, or within the metadata defined in the Entity Configuration when [@!OpenID.Federation] is used, or other mechanisms. | |
| To encrypt the Authorization Response, the Wallet uses the 'jwks` parameter within the `client_metadata` request parameter. This is REQUIRED if no other methods are available to retrieve the Verifier's metadata, such as through the trust chain described in [@!OpenID.Federation]. |
Generally metadata in Federation are only final metadata derived from the trust chain.
There was a problem hiding this comment.
This reads like encryption is mandatory, but we've not agreed that as a WG. Also I don't think 'securely encrypt' should be used, it's just encryption. As this is existing text (I only removed 'jwks_uri' from this sentence) I think we should handle changing this in a separate issue.
There was a problem hiding this comment.
how about paraphrasing like this?
| To obtain Verifier's public key for the input to the key agreement to encrypt the Authorization Response, the Wallet MUST use `jwks` claim within the `client_metadata` request parameter, or within the metadata defined in the Entity Configuration when [@!OpenID.Federation] is used, or other mechanisms. | |
| To obtain Verifier's public key for the input to the key agreement to encrypt the Authorization Response, the Wallet MUST use `jwks` claim within the `client_metadata` request parameter, if no other mechanisms such as the metadata defined in the Entity Configuration in [@!OpenID.Federation] are used. |
There was a problem hiding this comment.
@Sakurann I'm not sure how to read this. Does it mean "if the verifier has a jwks in its federation metadata it can't use ephemeral encryption keys"? If so I think that would be a normative change that I'm not sure I'm comfortable making in this PR.
There was a problem hiding this comment.
Applied @jogu 's suggestion here: https://github.com/openid/OpenID4VP/pull/233/files#r1752230536
There was a problem hiding this comment.
I'm somewhat confused as to what the latest suggestion here is, but I think if people think updates are needed to the text around how keys from Federation overlap with keys from client_metadata parameter then we should handle that in a separate issue please as that's outside of the scope of this PR and hasn't been discussed in the working group yet. This PR does not make anything worse in that area.
|
Thanks everyone for this comments - this is ready for re-review! I've applied most suggestions, other than some I've commented that I think we should handle separately if they're needed. |
Sakurann
requested review from
peppelinux,
bc-pi,
martijnharing,
Sakurann,
awoie and
paulbastian
|
Linking #251 here: We still have a dedicated section (Section 9) for on the current changes of the PR: I think this makes it a lot easier to understand 👍 |
|
I don't think there is a need to remove section 9. that one defines a new parameter and talks about discovery. |
|
|
||
| A public key to be used by the Wallet as an input to the key agreement to encrypt Authorization Response (see (#jarm)). It MAY be passed by the Verifier using the `jwks` or the `jwks_uri` claim within the `client_metadata` request parameter. | ||
| * `jwks`: OPTIONAL. A JWKS as defined in [@!RFC7591] that can contain one or more public keys with the `"use": "enc"` parameter to be used by the Wallet as an input to the key agreement to encrypt the Authorization Response (see (#jarm)). This allows the verifier to pass an empheral encryption key that is only used for this authorization request. Public keys included in the `jwks` parameter MUST NOT be used to verify the signature of signed Authorization Requests. |
There was a problem hiding this comment.
With ISO mdocs, this key is also used with ECDH + HMAC; not only for JARM. Does this language prevent the usage of this? I assume some similar usage is done also with SD-JWT VC @paulbastian ?
| * `jwks`: OPTIONAL. A JWKS as defined in [@!RFC7591] that can contain one or more public keys with the `"use": "enc"` parameter to be used by the Wallet as an input to the key agreement to encrypt the Authorization Response (see (#jarm)). This allows the verifier to pass an empheral encryption key that is only used for this authorization request. Public keys included in the `jwks` parameter MUST NOT be used to verify the signature of signed Authorization Requests. | |
| * `jwks`: OPTIONAL. A JWKS as defined in [@!RFC7591] that can contain one or more public keys with the `"use": "enc"` parameter to be used by the Wallet as an input to the key agreement to encrypt the Authorization Response (see (#jarm)). This allows the verifier to pass an empheral encryption key that is only used for this authorization request. Public keys included in the `jwks` parameter MUST NOT be used to verify the signature of signed Authorization Requests. |
There was a problem hiding this comment.
thats true, in Designated Verifier Signatures for JOSE, we don't define what the protocol layers above should use, but we definitly need something for RP public key and it makes sense to use this one
The text has been genericised a little to allow keys that don't have use: enc to allow for algorithms like: https://datatracker.ietf.org/doc/draft-bastian-dvs-jose/
|
I've applied all comments now (other than two about the overlap with federation that I believe aren't in scope nor agreed with WG yet and have responded saying so). @awoie could you check the new text please and approve if you're happy? It should allow for both the mdoc & jwt mac based signature you/Paul mentioned. |
As per working group consensus agreed on 21st May call:
#17 (comment)
closes #17