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

Which options are required? #292

Closed
aljones15 opened this issue May 19, 2022 · 11 comments · Fixed by #370
Closed

Which options are required? #292

aljones15 opened this issue May 19, 2022 · 11 comments · Fixed by #370
Assignees
Labels
ready for PR Issue ready to be resolved via a Pull Request

Comments

@aljones15
Copy link
Contributor

aljones15 commented May 19, 2022

Many VC API endpoints take in options.
None of them state that options are optional.

The /presentations/verify rules in fact appear to require options:

options [object]
Options for specifying how the LinkedDataProof is verified. The options object MUST be an object of the following form:

/credentials/verify has no normative statement for options, but also does not specify if options must be there or not. There is a Json Schema.

/credentials/derive has no normative statement for options, but does have a json schema.

/presentations/prove has no normative statement for options, but does have a json schema.

/credentials/issue has no normative statement for options, but does have a json schema.

So my questions/comments here are:

  1. /credentials/issue has a schema for options, but it seems reasonable to send a request to this endpoint with no options and to simply accept the issuer's default values.
  2. Why does presentations/verify have a normative MUST but no other endpoint does? I would imagine verifying a presentation probably does require supplying the challenge and domain, but is it always required? Neither domain nor challenge are normative. Are they required?
  3. Language should be added to specify how to handle a request where options is not present in the request.
  4. Normative statements should be added to which options are required in endpoints that require options.
  5. Because none of the properties in options are normative, does that mean we should supply an empty object for options?
@jandrieu
Copy link
Contributor

jandrieu commented Jun 21, 2022

For each of the above endpoints,

Which component(s) are hosting the endpoint?

And which components call it?

Without the specific call defined (from one component to another), it's not clear to me how to have a concrete discussion.

@msporny
Copy link
Contributor

msporny commented Jun 21, 2022

The group discussed this on the 2022-06-21 call. @jandrieu noted that we need to identify which part of the ecosystem the endpoint is sitting on. Manu suggested that we mark each API endpoint with which components on which they're expected to exist.

Need for PR: Add a descriptor to each endpoint that lists which components its expected to be attached to.

/credentials/issue is something that's exposed on Issuer Service.

There was a discussion about whether it should be on the Issuer App, Dave noted. Shawn noted that it is something that's exposed to a developer and thus not something that should be on the Issuer App. Joe noted that there is no real difference between an endpoint for a human web page and a JSON endpoint. Business rules specific to an application are the Issuer App vs. generic services provided (which is the Issuer Service). DaveL noted that business logic acts as a gating factor wrt. whether you use the service or not.

Need for PR: Remove challenge, domain, and credentialStatus from the list of settable options for the /credentials/issue Issuer Service endpoint.

Manu asked if the options object itself was optional, or if we had any mandatory options. Dave, Dmitri, Kayode, and Manu asserted that we have no mandatory options and the options object is completely optional (and can be omitted).

Need for PR: Ensure that when there are no mandatory options (which should be true for all endpoints), that the options object can be omitted in the VC API.

@msporny
Copy link
Contributor

msporny commented Jun 21, 2022

Discussion around /credentials/verify which is on the Verifier Service was discussed on the 2022-06-21 call.

Dmitri proposed to start from the Verifier App UI and work backward from that. Verification seems to be viewed as binary (pass/fail). Dmitri suggested that we have the verifier API return back a list of events and pass/fail on each one. We can imagine a wallet define overall status (verified or not), but then provide details (key was revoked, credential was revoked). This affects the options object -- change verify return status code, but also object -- similar to checks performed, but with more detail.

DavidC noted that if you get a pass, you don't need an event log. Only get it when something goes wrong. If everything is ok, why do you want an empty event log? Dmitri noted that different verifier services perform different checks, it informs the user what is meant by 'verified' -- check for issuer key revocation? Not all services will do that, hence the level of detail. DavidC noted that might mean there are bad implementations that don't do a good job. Dmitri noted we didn't want the mechanism to shore up non-conforming implementations -- implementations leave a lot of leeway, business logic wise, within us, with DL, when using physical DL for age verification, the verifier looks at DOB but also in some cases, expiration. Some venues will say "valid license, but because it's expired, it's not valid"... others will allow it. Joe noted that these are good points, make it optional to include event log on success and always include on failure -- looking at something from debugging standpoint, in production, you might not want extra traffic.

Manu asked if we were differentiating between verification vs. validation -- crypto and status is verification, validation is everything else. Joe noted that validation endpoint at verifier service might not be good, might need to be on Verifier App. Dmitri noted it's an important distinction to make -- two separate endpoints, verifier needs to make two API calls? Joe asked if App calls service for validation or App is responsible for validation? DavidC said verifier service only does verification... validation is something else. Ted noted that validation is business logic, verification is crypto and basic structure. Separation of those two endpoints feels optional... might want one thing to deal w/ what options passed depends on what happens. Dave and Joe agreed that validation doesn't happen at verifier service, which may or may not expose the endpoint. Dmitri noted that steps for verifier need to be normatively defined.

PR needed: Define steps for verification at Verifier Service using normative language -- Check to make sure the structure of the VC is valid, check credentialSchema (if applicable), the verification method has not been revoked, check the credentialStatus has not been revoked

PR needed: Create an issue marker to note that checking whether current time is between issuance date and expiration date is still up for debate wrt. whether that is a check that's done during verification or during validation.

/credentials/verify is on the Verifier Service,

PR needed: Add options.eventLog (optional value), not mandatory, that provides the checks performed. By default that's turned off on successful verification. However, the eventLog is provided on failed verification.

@msporny msporny added the ready for PR Issue ready to be resolved via a Pull Request label Jun 21, 2022
@msporny
Copy link
Contributor

msporny commented Jun 28, 2022

This was discussed on the 2022-06-28 telecon:

For /presentations/verify options, we should clearly list that you don't have to specify challenge or domain. There was a concern about how VC-JWTs support challenge/domain vs. using the aud field. @mavarley noted that they plan on implementing but have no current suggestions. We should contact the Traceability cohort to ask them if they have other options they pass during presentation verification (/cc @mprorock @OR13 @mkhraisha). @jandrieu noted that both challenge and domain are not normatively specified anywhere and that these properties are tribal knowledge at this point. This item should be raised in the VC2WG as a point of discussion, noting that challenge and domain are a party of presentation validation (and not verification).

PR needed: Create an issue marker noting that challenge and domain are not normatively defined at present, but should be discussed in the VC Data Model (at the very least) and a protocol specification (normatively).

Discussion around /credentials/derive -- @msporny asked if anyone had implemented. Avast has implemented, but noted that the endpoint might not be in the right place. Rolson noted that this was implemented in 2021 and not really looked at since. Manu asked if we should mark an issue beside the endpoint. Rolson noted they implemented because they needed holder APIs, end to end testing. Manu suggested that we put an issue marker in the spec noting that the feature isn't being actively developed, but we expect that it will be actively developed once there is a new BBS+ Signature spec to implement. We should reach out to MATTR for an expected timeline and put that timeline in the spec. (/cc @tplooker)

PR needed: Add an issue marker to /credentials/derive to note that it is not being actively developed and provide a timeline for when we feel it will be under development again.

Discussion around /presentations/prove -- There was concern over the word "prove" not being the right verb to use in this situation. Options discussed included create, issue, sign, prepare, assemble, and prove. This was raised as issue w3c/vc-data-model#887 in the VCWG. (/cc @OR13)

@mavarley
Copy link
Contributor

Wanted to link in issue #188 where the question was asked about making challenge/domain required.
I'll also note that https://github.com/w3c-ccg/traceability-interop does not currently mandate domain/challenge be required, probably linked to the issue above.

@OR13
Copy link
Contributor

OR13 commented Jun 29, 2022

@msporny
Copy link
Contributor

msporny commented Jun 29, 2022

only challenge is required, domain is optional afaik.

The question that was raised on the call was: What about VC-JWT -- how is that community covering challenge/domain -- how is the aud property being used (if at all) for presentations?

@OR13
Copy link
Contributor

OR13 commented Jun 29, 2022

VC-JWT support was added only recently, this is the test suite for it:

We are not currently testing / requiring support for VP-JWT.

In Transmute libraries, we handle these properties as the TR suggests... domain => aud and challenge => nonce.

Here are tests from our codebase that covers this behavior:

https://github.com/transmute-industries/verifiable-data/tree/main/packages/vc.js/src/vc-jwt/__tests__/aud-and-nonce

(again Traceability does not currently maintain interop tests for VP-JWT).

@msporny
Copy link
Contributor

msporny commented Jun 29, 2022

@David-Chadwick wrote:

I can answer a couple of questions that were asked about the verification of JWTs during the meeting.

The aud field contains the intended recipient of the VP. So if the recipient is calling a verification service, then it should pass its (authenticated) name and the verification service can verify if the JWT aud field matches this. If it does not, then the verification service should reject the verification because the requester is not the intended recipient of the VP. (It may return the aud value in the error response (tbd).) This stops some attacks, such as surreptitious forwarding. Note that if we add support for end to end encrypted VPs, then the (false) recipient will not be able to glean anything from the VP, as it will not know the private key of the real recipient.

The nonce provided by the RP to the wallet is there to stop replay attacks. It is mandatory to send this nonce if using the OIDC4VP protocol. The nonce should be in the returned JWT VP. This is already included in the VCDM v1.1, example 31. (Note. I am not sure why the nonce is also in example 28, a JWT VC. Perhaps this is an editorial error? Otherwise it would imply this is an ephemeral VC only to be used once.)

@mkhraisha
Copy link
Collaborator

mkhraisha commented Aug 23, 2022

Need for PR: Remove challenge, domain, and credentialStatus from the list of settable options for the /credentials/issue Issuer Service endpoint.

I understand removing challenge and domain, but why are we removing credentialStatus?
How does a person requesting a credential indicate the type of revocation mechanism they would like?

@msporny
Copy link
Contributor

msporny commented Aug 23, 2022

How does a person requesting a credential indicate the type of revocation mechanism they would like?

The expectation is that the issuer endpoint will be configured with an appropriate revocation mechanism, if one is needed. The discussion in the VC API group noted that there could be a combinatorial explosion in options and some of those options might not mix well (like, what happens if you ask for an Ed25519 signature on the VC, but a ECDSA signature on the status list?). The simpler option ended up being: Let the person setting up the issuer endpoint determine the proper combination of things and the caller just calls the endpoint to get a VC issued to them (with whatever configuration is active on the endpoint).

The spec doesn't prevent someone from providing other options that they can pass to the endpoint, but it is currently believed that the more options an issuer endpoint takes, the harder it will be to secure that endpoint from bad combinations of options. IOW, you can still provide setting credentialStatus as an option -- but it's believed to be a not so great practice right now and might become an anti-pattern in time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for PR Issue ready to be resolved via a Pull Request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants