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

Add provider validation #156

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Add provider validation #156

wants to merge 10 commits into from

Conversation

marcoscaceres
Copy link
Collaborator

@marcoscaceres marcoscaceres commented Aug 7, 2024

Closes #152
Closes #130

The following tasks have been completed:

  • Modified Web platform tests (link)

Implementation commitment:

  • WebKit (link to issue)
  • Chromium (link to issue)
  • Gecko (link to issue)

Documentation and checks

  • Affects privacy
  • Affects security
  • Pinged MDN
  • Updated Explainer

Preview | Diff

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
@marcoscaceres
Copy link
Collaborator Author

You're the best @TallTed! Thank you again for those eagle eyes. 🦅

marcoscaceres and others added 7 commits August 13, 2024 16:41
Co-authored-by: Orie Steele <orie@or13.io>
Co-authored-by: Orie Steele <orie@or13.io>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
@marcoscaceres
Copy link
Collaborator Author

See openid/OpenID4VP#224 for a concrete example of what I'm expecting here...

index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
index.html Outdated Show resolved Hide resolved
marcoscaceres and others added 3 commits August 14, 2024 07:58
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
</li>
<li>Validate |request| against any validation rules set forth in the
corresponding [=digital credentials registry/specification=]. If any
of |request|'s members are invalid, [=exception/throw=] a
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can validate ALL aspects of the specification. If we're going to validate anything (EUDI folks would prefer we leave it all to the wallet), then we need to be clear about which extensibility points are reserved for the protocol.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right, but we can validate a lot of things when the request is passed to the browser. I agree that some aspects cannot be validated, and that can be left up to the wallet upon decrypting the request (if the request, or parts of it, is encrypted in the first place).

At the same time, we need the assurances of what to do when the user agents receive a request in a particular format.

Copy link
Collaborator

Choose a reason for hiding this comment

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

At the same time, we need the assurances of what to do when the user agents receive a request in a particular format.

My intuition is that there is a subset of the request that the browser cares about and should validate which is the query language, specifically, the presentation_definition parameter in OpenID4VP which takes shape as a Presentation Exchange.

That seems like something that the browser uses to help the user select which wallet has the credential that's being requested, as well as the privacy and security consequences of sharing that credential.

Beyond that, I think it would be harmful (from a security standpoint) to validate (e.g. drop an unknown parameter) the rest of the parameters.

Looking at the list of parameters in the Authorization Request we have the following, in addition to presentation_definition:

  • client_id_scheme
  • client_metadata
  • nonce
  • scope
  • response_mode

None of these seem like things that the browser needs to form an opinion on, right?

if the request, or parts of it, is encrypted in the first place

As soon as we introduce any parameter that is a DOMString (for example, nonce), verifiers and wallets can communicate freely through that channel. So, there is a escape hatch for any validation that holds the protocol designers and deployment back.

From a security standpoint, I think that's a feature, not a bug: if OpenID4VP (or developers running servers) figure out a safer way to do things, they should be able to develop that without having to ask for browsers to catch up. For example, say there is a production fire happening somewhere, you we wouldn't our browser users to wait until all browsers are patched and redeployed to fix the issue, right?

Copy link

Choose a reason for hiding this comment

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

I share much of @samuelgoto's intuition FWIW. But also just want to note that there's work underway in OpenID4VP towards developing a new query language alternative to PE openid/OpenID4VP#220

Copy link
Collaborator

Choose a reason for hiding this comment

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

I wrote down a proposal here: #161 (comment)

Copy link
Collaborator Author

@marcoscaceres marcoscaceres Aug 24, 2024

Choose a reason for hiding this comment

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

As soon as we introduce any parameter that is a DOMString (for example, nonce), verifiers and wallets can communicate freely through that channel.

I don’t think that’s true. We could certainly enforce restrictions or other checks if we see ab/misuse of a data structure.

Playing whack-a-mole is not fun, but it’s absolutely something we would do and have done in the past. All browsers do this.

Beyond that, I think it would be harmful (from a security standpoint) to validate (e.g. drop an unknown parameter) the rest of the parameters.

Potentially, yes. If the request was signed along with any non-standard parameters (which would get dropped by the browser before being passed to the wallet). At the same time, non-standard parameters shouldn’t be part of the request. An RP will likely need some means to check if the browser supports the request structure.

My intuition, and because of our security architecture, WebKit will drop anything non-standard.

For example, say there is a production fire happening somewhere, you we wouldn't our browser users to wait until all browsers are patched and redeployed to fix the issue, right?

“it depends”™️

but seriously… it’s really on a case by case basis. We definitely do this (fix browser bugs) for all other things on the web platform. This API is not different - or shouldn’t be.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Spoke @samuelgoto about how this would be implemented in practice, and agreeing with @RByers that this should happen in the wallet or as it's being passed to the wallet, and not for all the things that are passed in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll try to update this PR soon!

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.

Presentation request format validation Define error handling
7 participants