Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clarify what is mutually exclusive (client_metadata and pre-registered clients) #165

Merged
merged 2 commits into from
Jun 14, 2024

Conversation

Sakurann
Copy link
Collaborator

@Sakurann Sakurann commented May 3, 2024

fixes #96.

@@ -266,6 +266,8 @@ This specification defines the following new parameters:
`client_metadata_uri`:
: OPTIONAL. A string containing an HTTPS URL pointing to a resource where a JSON object with the Verifier metadata can be retrieved. The scheme used in the `client_metadata_uri` value MUST be `https`. The `client_metadata_uri` value MUST be reachable by the Wallet. It MUST NOT be present if `client_metadata` parameter is present.

`client-metadata` and `client_metadata_uri` parameters MUST NOT be used when the Wallet knows Client Identifier and corresponding Client metadata prior to the transaction (i.e., Client Identifier scheme is absent or has value `pre-registered`).
Copy link
Collaborator

@jogu jogu May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again it looks odd to me.

  1. I can't see why we'd single out pre-registered, it seems to me like the same concerns would apply to entity_id given that is very very similar to a pre-registered client.

  2. It overlaps with the discussion we had about what metadata goes into client_metadata parameter? #17 where people were saying the client_metadata could be used to pass ephemeral encryption keys, and I can't see why we'd want to disallow ephemeral encryptions keys for pre-registered clients.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on 1, I disageee. it is only pre-registered or when client id scheme is absent that requires wallet to know verifier's client metadata, entity_id might be similar, but it does not require wallet knowing a client_id and i think there are use-cases where it might be useful to use client metadata with entity_id scheme.

On 2, can we please take it step by step? i want to resolve #96 first because that sentence is confusing and this PR updates the text. once we have agreement on #17, we can update this.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think there are use-cases where it might be useful to use client metadata with entity_id scheme.

I think there's something that you're seeing that I'm not yet getting - I'm not sure which cases you are thinking of? Or can you explain a case where using client_metadata is very obviously wrong for pre-registered but not for any other client_id_scheme?

I think there are use cases where we need client metadata with pre_registered too? i.e. The ephemeral encryption key case, particularly once you're using https://datatracker.ietf.org/doc/draft-ietf-oauth-attestation-based-client-auth/ in combination with pre-registered client_ids.

I think I'd feel more comfortable removing the sentence at this stage. It feels like we're deciding on one part of #19 but without a full justification why it's sensible.

If we really want to keep the sentence we should fix it, as "Wallet knows Client Identifier and corresponding Client metadata prior to the transaction" is also true for entity_id the second time a wallet sees a particular verifier (federation requires the AS to register the client_id once it's seen it).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are use cases where we need client metadata with pre_registered too? i.e. The ephemeral encryption key case, particularly once you're using https://datatracker.ietf.org/doc/draft-ietf-oauth-attestation-based-client-auth/ in combination with pre-registered client_ids.

I agree with this point, although I think we also need to also go further here e.g

  1. Establish an order of precedent too, for example if there is metadata from registration that takes priority over what is specified in this parameter
  2. We also need to either make this parameter work with only a subset of metadata when the request isn't signed as its not authoritative in that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@tplooker tplooker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer we discuss this wider there is more discussion to be had on how this parameter is handled in different contexts.

@Sakurann
Copy link
Collaborator Author

Sakurann commented May 24, 2024

ok, in this PR let's simply delete the confusing paragraph for now, without introducing a clarification text (I agree it makes sense to use ephemeral encryption key with pre-registered clients). and I made a note in #17 (comment) that we should revisit if the usage of client metadata is affected by client_id_scheme or not to make sure this does not get forgotten.

@Sakurann Sakurann requested review from jogu and tplooker May 24, 2024 15:13
@Sakurann Sakurann merged commit bc85f29 into main Jun 14, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarification needed on SIOP and client_metadata paragraph
3 participants