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.

Sign up for GitHub

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
+43 −5
Open

Add provider validation #156

Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
8494381
Add provider validation
marcoscaceres Aug 7, 2024
e78a79a
Update index.html
marcoscaceres Aug 9, 2024
740d78a
Update index.html
marcoscaceres Aug 9, 2024
2e3184a
Update index.html
marcoscaceres Aug 13, 2024
a97491b
Update index.html
marcoscaceres Aug 13, 2024
ae32976
Use range and index correctly
marcoscaceres Aug 13, 2024
62fdfc3
it's a set
marcoscaceres Aug 13, 2024
41e3dd4
Update index.html
marcoscaceres Aug 13, 2024
51d7112
Update index.html
marcoscaceres Aug 13, 2024
f87ba1e
Update index.html
marcoscaceres Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 43 additions & 5 deletions index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,38 @@ <h3>
<li>Let |providers| be |options|'s {{CredentialRequestOptions/digital}}'s
{{DigitalCredentialRequestOptions/providers}} member.
</li>
<li>If |providers| is empty, [=exception/throw=] a {{TypeError}}.
<li>If |providers| [=list/is empty=], [=exception/throw=] a
{{TypeError}}.
</li>
<li>Let |range| be [=list/get the indices=] of |providers|.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
</li>
<li>[=Set/For each=] |n| of the |range|:
<ol>
<li>Let |provider| be [=list/item=] at |providers|'s |n| index.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
</li>
<li>If |provider|'s {{DigitalCredentialsProvider/protocol}} does not
match a [=digital credentials registry/protocol identifier=] from the
[=digital credentials registry=], [=exception/throw=] a
{{TypeError}}.
</li>
<li>Let |dictionary| be the [=digital credentials registry/Web IDL
dictionary=] for |provider|'s
{{DigitalCredentialsProvider/protocol}}.
</li>
<li>Let |request| be |provider|'s
{{DigitalCredentialsProvider/request}} [=converted to an idl
value|converted=] to |dictionary|. [=exception/throw|Rethrow=] any
exceptions.
</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 •
edited
Loading

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!

{{TypeError}}.
</li>
<li>Set [=list/item=] at |providers|'s |n| index's
{{DigitalCredentialsProvider/request}} to |request|.
</li>
</ol>
</li>
<li>
<aside class="issue">
Expand Down Expand Up @@ -366,8 +397,9 @@ <h2 id="protocol-registry">
Registry of protocols for requesting digital credential
</h2>
<p>
The following is the registry of [=digital credential/exchange
protocols=] that are supported by this specification.
The following are <dfn data-lt="digital credentials registry">exchange
protocols from the digital credentials registry</dfn>, supported by this
specification.
marcoscaceres marked this conversation as resolved.
Show resolved Hide resolved
</p>
<p class="note" title="Official Registry" data-cite="w3c-process">
It is expected that this registry will be become a [=W3C registry=] in
Expand All @@ -392,13 +424,19 @@ <h3>
<thead>
<tr>
<th>
Protocol identifier
<dfn data-dfn-for="digital credentials registry">Protocol
identifier</dfn>
</th>
<th>
<dfn data-dfn-for="digital credentials registry">Web IDL
dictionary</dfn>
</th>
<th>
Description
</th>
<th>
Specification
<dfn data-dfn-for=
"digital credentials registry">Specification</dfn>
</th>
</tr>
</thead>
Expand Down