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

Presentation request format validation #152

Open
marcoscaceres opened this issue Aug 6, 2024 · 6 comments · May be fixed by #156
Open

Presentation request format validation #152

marcoscaceres opened this issue Aug 6, 2024 · 6 comments · May be fixed by #156

Comments

@marcoscaceres
Copy link
Collaborator

When ingesting any presentation format, presumably there will be a set of requirements/constraint that need to be satisfied. For example, expected_origins in OpenID4VP:

expected_origins: REQUIRED when signed requests defined in Appendix A.3.2 are used with the W3C Digital Credentials API [w3c.digital_credentials_api]. An array of strings, each string representing an origin of the Verifier that is making the request. The Wallet can detect replay of the request from a malicious Verifier by comparing values in this parameter to the origin asserted by the Web Platform.

Parsing those URLs may result in parsing error, which should reported a TypeError.

I think generally any validation error in the request format will result in a TypeError.

@marcoscaceres marcoscaceres linked a pull request Aug 7, 2024 that will close this issue
8 tasks
@RByers
Copy link
Member

RByers commented Aug 19, 2024

I think we need to work with the protocols to ensure there is clarity about which validation rules are enforced where in the stack. We've heard from EUDI designers that the browser/OS precisely validating the request is inconsistent with the requirements for EUDI. We captured this in the explainer with this text:

By separating the act of requesting from the specific protocol, we can enable flexibility and adaptability in both the protocol and credential formats. This way, the pace of changes in browsers won't hinder progress or block new developments.

While some validation may be OK, I don't think we can have the DC API just blindly say the user agent validates all properties of the request. Perhaps protocol specifications should be clear about which bits are OK to validate in the browser vs. which bits are expected to be validated by the wallet? In particular, where the points for future extensibility are?

@tlodderstedt
Copy link

I see the benefit of early syntax checks, I also see the challenges associated with it. Just checking whether a JSON request object is correct JSON is one thing, going deeper into the structure might result in additional obstacles for the deployment of any change.

I think the decision that needs to be made is, whether the introduction/modification of presentation protocols defined on top of DC API requires modifications to the browsers/platforms. I'm very hesitant about this as I don't see how this would work in a predictable/deterministic fashion.

@samuelgoto
Copy link
Collaborator

samuelgoto commented Aug 22, 2024

Just to cross-post, I expanded a bit some concerns I have in the PR itself: #156 (comment)

@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Aug 24, 2024

While some validation may be OK, I don't think we can have the DC API just blindly say the user agent validates all properties of the request.

note that’s not what we are proposing. We’re saying that the protocol spec is in charge of dictating how the members are validated, and which are be validated by the browser.

At the same time, even if the browser blindly passed all the data to a wallet, a wallet still needs to do all the validation: there’s no escaping having to do the validation somewhere.

This is supposed to serve as a forcing function to assure a high degree of privacy and security, as we would expect from any W3C standard that interacts with the browser, OS, or native applications.

Take again the example of expected_origins in openid4vp. As it currently stands, that member is underspecified and not implementable because it lacks any validation rules. See openid/OpenID4VP#224 for details.

That’s not to pick on openid4vp. I’m just highlighting the challenges that we face as either browser implementers or wallet implementers.

The browser is in a uniquely powerful position to report and fix developer errors, normalize data, and secure things. The net effect being maximized interoperability through well defined validation and data normalization behavior.

more concretely, imagine if every wallet treats
expected_origins differently. Like some reject if there’s a slash, or others allow file://, etc. that’s gonna be a nightmare scenario for everyone (users, developers, RPs) because a request will work in one wallet and not in another and developers may end up having no idea why… and it’s users who end up having a poor experience.

@marcoscaceres
Copy link
Collaborator Author

marcoscaceres commented Aug 24, 2024

Fixed the above to reference “expected_origins”, but my point stands with any URL that could be passed along to a wallet. Those needs to be checked as URLs are a source of security bugs when blindly dereferenced by the receiving application (be it the OS, or wallet).

Just as a counter example, let’s say the wallet tried to attack the RP through the response: browsers have ton of security infrastructure the prevents, for example, dereferencing things like file://, fetching from local network addresses, etc. it’s not realistic to expect native apps serving as wallets to have the same level of security (or operate under a “same origin” model), because they are operating under different assumptions and platform.

@marcoscaceres
Copy link
Collaborator Author

@tlodderstedt wrote:

I'm very hesitant about this as I don't see how this would work in a predictable/deterministic fashion.

See how we will handle it in WebKit:
openid/OpenID4VP#224 (comment)

it should be pretty straightforward. We did a lot of the same things in the Payment Request API and I think Web Auth does similar things.

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 a pull request may close this issue.

4 participants