decentralized-identity / JWS-Test-Suite

JsonWebSignature2020 Test Suite
https://identity.foundation/JWS-Test-Suite
Apache License 2.0
9 stars 11 forks source link

RSA Implementation Should Not Check For "Holder" Property in VPs #52

Closed decentralgabe closed 2 years ago

decentralgabe commented 2 years ago

https://github.com/decentralized-identity/JWS-Test-Suite/pull/49#issuecomment-1063050687

All implementations that do not include the holder property in VPs seem to fail. This should be dropped from verification correctness.

cc: @clehner @OR13

clehner commented 2 years ago

I'm trying to understand what is the meaning or use case for a verifiable presentation without a holder property, what is the convention here, and how to apply that. The ssi library in Spruce's implementation currently requires a VP to have a holder property to verify, unless the verificationMethod verify option is used (i.e. the option removed from VC API in https://github.com/w3c-ccg/vc-api/pull/253). So one of the following must be done:

It's true that the holder property is optional. I guess I thought that without it, one would have a "presentation", rather than a "verifiable presentation". Non-normative sections in vc-data-model refer to the holder as the entity that generates verifiable presentation; and verification as involving authenticity, i.e. that the holder generated the VP. I suppose it may still be considered verifiable with the proof, even though the holder relationship is not asserted. Does the holder not want to identify as such, for some reason? Or are there roles and relationships to express that do not fit the semantics of "holder"?

Continuing from https://github.com/decentralized-identity/JWS-Test-Suite/pull/49#issuecomment-1063223049, https://github.com/decentralized-identity/JWS-Test-Suite/pull/49#issuecomment-1063223049...

[...] Should rather any signature/proof from the test suite's example keypair be allowed, regardless of the holder property?

Your last sentence is my understanding. [...]

The example keypair and DID are not provided as arguments to the verify command/function. Should these be hard-coded in the verifier? Is it an anti-pattern to have an additional parameter like that, outside the credential/presentation, for verification? My intuition is that a verifiable credential/presentation should be self-contained, such that it contains, as an integrity-protected document, all information needed to verify it - other than perhaps the verification material obtained via a document loader (e.g. DID URL dereferencer), and the revocation state. Maybe this intuition is not totally correct? Or maybe it is up to VP sub-types to specify this behavior (i.e. checking the holder property or some another mechanism)?

Alternatively to checking for a signature/proof from the example DID/keypair (which would need to be passed in to the verify function, or hard-coded within the verify function), maybe it would be equivalent for this test suite to simply allow any proof to verify (i.e. any JsonWebSignature2020 proof). i.e. if the holder property is not present, that could act as a "wildcard" for the holder property -- an escape hatch from the holder binding. If the holder property is present, the holder-VM relationship could be checked -- or not, as this test suite is just for the signature suite, not the VC Data Model. Does that make sense?

decentralgabe commented 2 years ago

The ssi library could be updated to support VPs without holder property in a general way. (More about that below). this makes sense to me

It's true that the holder property is optional. I guess I thought that without it, one would have a "presentation", rather than a "verifiable presentation".

why? what if the proof is signed by a verificationMethod which contains a did that matches the did and/or verificationMethod of the listed credentials? wouldn't that be a verifiable presentation?

The example keypair and DID are not provided as arguments to the verify command/function. Should these be hard-coded in the verifier?

the key used to verify needs to match the provided verificationMethod in that respect, perhaps it should be an argument. Our impl parses out the signing key from the test file name, and loads that independently. We do take the parameter.

maybe it would be equivalent for this test suite to simply allow any proof to verify

I guess I'd back up and ask what the point of the test suite is. are we verifying signing implementations, or the combination of signing implementations against a standard which may have its own nuances? I think there's a middle ground -- i.e. the Transmute impl fails if a challenge property is not present on VPs, and this makes sense. That's not exactly in the spec either.

I find it weird to add properties to data before signing, which is why our impl does not add the holder property that's not present.

So...I guess to summarize:

wdyt? @OR13 ?

OR13 commented 2 years ago

Per the spec, a VP only requires @context and type... any additional requirements enforced by an implementation are "bugs" imo... adding required fields that the spec does not list as required causes problems with interoperability.

There are a few issues with VPs, that should probably be tracked separately, but I will list them all here first.

  1. holder is optional
  2. in JSON-LD, proof.domain is optional and chosen by the "holder" even if they are not present.
  3. in JSON-LD, proof.challenge is required and chosen by the "verifier"
  4. if present, domain is mapped to aud and challenge is mapped to nonce in the JWT variant of a VC.
  5. sub is optional in a VP-JWT, because of point 1.
  6. in JSON-LD, proofPurpose for a VP is authentication
  7. in JSON-LD, verificationMethod for a VP is expected to be in the authentication section of the did document
  8. in VP-JWT, its strongly recommended that kid look like verificationMethod and that it be in the authentication section of the did document

If Transmute's implementation violates any of these things we consider that a bug... we know we have at least a problem with the holder being treated as required.

decentralgabe commented 2 years ago

Fixed in https://github.com/decentralized-identity/JWS-Test-Suite/pull/55