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

Specify how relative URLs should be processed #77

Closed
msporny opened this issue Jan 12, 2023 · 14 comments
Closed

Specify how relative URLs should be processed #77

msporny opened this issue Jan 12, 2023 · 14 comments
Assignees
Labels
before CR This issue needs to be resolved before the Candidate Recommendation phase. pr exists A pull request exists to address this issue.

Comments

@msporny
Copy link
Member

msporny commented Jan 12, 2023

It is possible to perform a key confusion attack if relative URLs are put together in the wrong way from a VC or a controller document. This issue is to track this concern and make sure there is language on how to process verification method URLs such that it is not possible to have a key confusion attack. This algorithm should go in at least the vc-data-integrity spec and possibly the vc-jwt specification. /cc @OR13 @tplooker

One way to address this concern is to specify what to do when an implementer uses both absolute and relative URLs (that is, we count on some implementers doing the wrong thing, and when they do the wrong thing, we make sure implementations eliminate the possibility of a key confusion attack).

So, we could say something to the effect of: "Implementations MUST use absolute URLs everywhere." and then "If you come across a relative URL, then you can construct an absolute URL using <ALGORITHM_A> and then ensure that the absolute URL resolves to a key in the controller document." I'll note that this eliminates a DID Document's ability to refer to a key that is external to a controller document, but that might be a agility/security trade-off that we're willing to make to ensure that there won't be key confusion attacks related to DID Documents.

@OR13
Copy link
Contributor

OR13 commented Jan 12, 2023

@OR13
Copy link
Contributor

OR13 commented Jan 12, 2023

There are data integrity equivalents to this to consider:

"proof": {
  "type": "Ed25519Signature2018",
  "created": "2022-05-07T15:30:56Z",
  "verificationMethod": "did:example:123#kid-123", // absolute and pretty much what we see today.
  "proofPurpose": "assertionMethod",
  "jws": "eyJhbGciOiJFZERTQ..."
}

vs

"proof": {
  "type": "Ed25519Signature2018",
  "created": "2022-05-07T15:30:56Z",
  "verificationMethod": "#kid-123", // relative to.... issuer / issuer.id ?
  "proofPurpose": "assertionMethod",
  "jws": "eyJhbGciOiJFZERTQ..."
}

@OR13
Copy link
Contributor

OR13 commented Jan 12, 2023

Related to DID Document side of this:

{
// didDocument.id
    "iss": "did:web:example.nz", 
    "nbf": 1516239022,
    "exp": 1516239922,
    "jti": "urn:uuid:cc599d04-0d51-4f7e-8ef5-d7b5f8461c5f",
    "vc": {
        "@context": [ "https://www.w3.org/2018/credentials/v1", "https://nzcp.covid19.health.nz/contexts/v1" ],
        "version": "1.0.0",
        "type": [ "VerifiableCredential", "PublicCovidPass" ],
        "credentialSubject": {
            "givenName": "John Andrew",
            "familyName": "Doe",
            "dob": "1979-04-14"
        }
    }
}
{
    "@context": ["https://www.w3.org/ns/did/v1", "https://w3id.org/security/suites/jws-2020/v1"],
    "iss": "did:web:example.nz"
    "verificationMethod": [{
// proof.verificationMethod || header.kid || header.iss + header.kid || payload.iss + header.kid
        "id": "did:web:example.nz#key1", 
        "type": "JsonWebKey2020",
        "controller": "did:web:example.com",
        "publicKeyJwk": {
          "kty":"EC",
          "crv":"P-256",
          "x":"MKBCTNIcKUSDii11ySs3526iDZ8AiTo7Tu6KPAqv7D4",
          "y":"4Etl6SRW2YiLUrN5vfvVHuhp7x8PxltmWWlbbM4IFyM",
        },
    }],
    "assertionMethod": ["did:web:example.com#key1"],
    "authentication": ["did:web:example.com#key1"],
}

@OR13
Copy link
Contributor

OR13 commented Jan 16, 2023

Safest path forward is to concretely describe how you get to a public key... all the ways you can get to one... better to define them all up front and then mark the "less desirable ones" not required to implement.

@tplooker
Copy link

tplooker commented Mar 8, 2023

IMO as a general rule, if the verification method is ever suppose to be checked against another property (e.g the issuer field in a VC) to resolve the key required to verify the VC AND establish the identity of the signer (e.g who is the issuer). Then the verification method should never be allowed to contain an absolute URL because doing so means the system fails open if the verifiers logic around comparing the verification method controller to the VC issuer is flawed. How popular JWT profiles work like id_tokens and how we designed NZCP as Orie referenced above uses an approach that makes it much much harder to ever fail like this because the key resolution process requires you to combine who the issuer is and the key id (conceptually similar to verificationMethod here) to get the required key to verify.

@OR13
Copy link
Contributor

OR13 commented Jun 8, 2023

We've updated iss and kid header parameters to have this property pretty much everywhere.

https://transmute-industries.github.io/vc-jwt-test-suite/#report

@msporny
Copy link
Member Author

msporny commented Jul 9, 2023

Safest path forward is to concretely describe how you get to a public key

For Data Integrity, this is now normatively defined here:

https://www.w3.org/TR/vc-data-integrity/#retrieve-verification-method

The normative algorithm for retrieving a public key is defined such that relative URLs are not allowed in Data Integrity proofs in the verificationMethod property. If a relative URL is attempted to be used, the dereferencing operation in the Retrieve Verification Method algorithm will fail, and the algorithm will error out (failing closed).

VC-JWT can have different rules, which can be defined in that specification.

The original question asked in this issue has been answered (usage of a relative URL will result in an error). Marking as pending 7 day close. If anyone believes that the issue has not been addressed, please let us know.

@msporny msporny added pending close (7 days) This issue will be closed after 7 days. before CR This issue needs to be resolved before the Candidate Recommendation phase. pr exists A pull request exists to address this issue. and removed pr exists A pull request exists to address this issue. labels Jul 9, 2023
@OR13
Copy link
Contributor

OR13 commented Jul 12, 2023

The normative algorithm for retrieving a public key is defined such that relative URLs are not allowed in Data Integrity proofs in the verificationMethod property.

I wonder if we should do the same for kid in vc-jwt?

If a relative URL is attempted to be used, the dereferencing operation in the Retrieve Verification Method algorithm will fail, and the algorithm will error out (failing closed).

What about if an absolute URL that is different from issuer is used?

From a quick glance, this would succeed:

issuer: did:good.guy.example:123
proof: {
 verificationMethod: did:bag.guy.example:456#key-42

@OR13
Copy link
Contributor

OR13 commented Jul 12, 2023

Please remove the pending close until the binding to issuer is addressed.

@msporny
Copy link
Member Author

msporny commented Jul 15, 2023

What about if an absolute URL that is different from issuer is used?

That is a business rule that is outside the base Data Integrity specification. Remember, there is no such thing as an issuer in the Data Integrity specification; it's supposed to be oblivious to VC data structures.

That rule probably belongs in the vc-data-model, given that it's a part of validation, not verification... and it's not clear that you'd always want to enforce it (such as if there is an M-of-N signatures requirement associated with a particular issuer, like a Board of Directors)... or if you're dealing with a Notary use case, where you have a chained signature where the signatory and the notary are clearly not going to be the same issuer.

All that said, I'm not entirely averse to adding something in the Data Integrity specification about it, but then we're going to have to repeat the language across all the securing specifications. We should probably say something about it, without making it a MUST (given the two use cases above that demonstrate that ALWAYS doing that sort of binding would not allow for identified use cases).

Let me know how you want to proceed. I propose we add something to vc-data-model about cross-referencing the signing key with the issuer (in some way), and put that text in the validation section.

@OR13
Copy link
Contributor

OR13 commented Jul 17, 2023

@msporny I agree with what you wrote.

But I don't think we are chartered to apply data integrity proofs to data models that are different from w3c verifiable credentials.

It sounds to me like the clarification that is needed is something like this:

verificationMethod is always absolute URL that resolves to a controller document (what we have today) and in the context of "verifying" a data integrity proof... no additional information is needed (I think we have this already).

In the context of "validating" a document secured by one or more data integrity proofs, additional processing might occur. (what is needed in this spec).

It won't surprise you to hear that I object to filling the core data model with DataIntegrityProof specific validation logic or examples.

That text should be in this work item, not the core data model... but it's possible that it's not relevant to implementers, of either spec, in which case, it can be omitted from both.

@msporny
Copy link
Member Author

msporny commented Jul 17, 2023

But I don't think we are chartered to apply data integrity proofs to data models that are different from w3c verifiable credentials.

We are chartered to make Data Integrity generally useful, but our scope is not to extend beyond Verifiable Credentials. For example, if someone wants to work on how DI is applicable to graphs of arbitrary sizes, or streamed-signing of RDF Quads that are coming out of a quadstore, those things are out of scope (this is why the work was placed into this WG, to give us a set of limited use cases, and then if successful, expand the scope of DI in future WGs that might not be the VCWG). Hard coding DI to VCs was never a goal... all that said...

That text should be in this work item, not the core data model... but it's possible that it's not relevant to implementers, of either spec, in which case, it can be omitted from both.

I can add some text to the Data Integrity spec that talks about usage of Data Integrity with VCs and validation and issuer (and use that as an example of "further processing" that some applications might do), since we're chartered to speak to that. The guidance might not be useful for everyone using DataIntegrityProofs, but that's fine... if it's not useful to an implementer, they don't have to pay attention to the section.

@msporny msporny added the ready for pr This issue is ready to be resolved via a pull request label Jul 17, 2023
@msporny
Copy link
Member Author

msporny commented Jul 17, 2023

PR #119 has been raised to address this issue. This issue will be closed once PR #119 has been merged.

@msporny msporny added pr exists A pull request exists to address this issue. and removed ready for pr This issue is ready to be resolved via a pull request labels Jul 17, 2023
@msporny
Copy link
Member Author

msporny commented Jul 28, 2023

PR #119 has been merged to address this issue, closing.

@msporny msporny closed this as completed Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
before CR This issue needs to be resolved before the Candidate Recommendation phase. pr exists A pull request exists to address this issue.
Projects
None yet
Development

No branches or pull requests

3 participants