Skip to content

Require hash of JSON-LD context #146

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

Closed
wants to merge 3 commits into from
Closed

Conversation

rhiaro
Copy link
Member

@rhiaro rhiaro commented Oct 25, 2020

For a while there has been language in the DID Core spec about the JSON-LD context in the Registries being associated with a hash. Currently it's in JSON-LD consumption but in w3c/did#436 its moved to production:

URLs registered in the DID Specification Registries [DID-SPEC-REGISTRIES] referring to JSON-LD Contexts SHOULD be associated with a cryptographic hash of the content of the JSON-LD Context.

In w3c/did#403 Ivan points out that the DID Core spec can't make this requirement of the Registries. So I have added it as a requirement of the registration process in this PR. The DID Core spec can then add language about checking against the hash in JSON-LD consumption if necessary.

I think this then means the Registries needs to display the hash somewhere? It's gonna get messy for every one but maybe we can find some nice css/js to collapse them or something.


Preview | Diff

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this requirement may encourage less contribution, and the format of the hash is not defined sufficiently.

Would need to see a link to the standard used for hashing to consider, support for multiple hashing algorithms seems like it would make everyones life a lot worse.

@rhiaro
Copy link
Member Author

rhiaro commented Oct 26, 2020

Would definitely love to be able to specify how to do the hashing. But if we're not going to add this to the Registries, we need to take the text out of the DID Core spec.

@OR13
Copy link
Contributor

OR13 commented Oct 29, 2020

@rhiaro to be clear, I won't approve "a cryptographic hash"... but I would approved "sha256 base64url encoded"... saying a hash must be included is not helpful unless a single format for the hash is described.

@OR13
Copy link
Contributor

OR13 commented Oct 29, 2020

The same guidance should be applied in did core.

Copy link
Member

@kdenhartog kdenhartog left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be a useful addition to what I'm looking to do in #143 as well as relates to what we wanted to do in #17. I'm in favor of this.

@OR13
Copy link
Contributor

OR13 commented Nov 30, 2020

blocked by w3c/did#464

@OR13
Copy link
Contributor

OR13 commented Dec 12, 2020

PR is up in did core for w3c/did#485

@OR13
Copy link
Contributor

OR13 commented Jan 5, 2021

https://github.com/w3c/did-core/pull/485/files was merged.

@rhiaro can you please review my requested changes and align with did core as needed?

@rhiaro rhiaro force-pushed the feature/context-hash branch from 721e72a to d8f42c8 Compare January 10, 2021 15:44
@rhiaro rhiaro requested a review from OR13 January 10, 2021 15:44
@rhiaro
Copy link
Member Author

rhiaro commented Jan 10, 2021

@OR13 I've added the bit about how to compute the hash that you put in DID Core. I think we can probably take this out of DID Core now (made a separate PR).

@OR13
Copy link
Contributor

OR13 commented Jan 14, 2021

@rhiaro your PR in DID Core, does not match the integrity format proposed here: https://github.com/w3c/did-core/pull/534/files

Specifically you propose IPFS or Hashlink as solutions which is not described here.

I see a couple paths forward:

  1. Remove all references to JSON-LD integrity checking, since this implementation specific.
  2. Describe how JSON-LD integrity is protected in 1 place, and point all other places to it, and provide examples.

@rhiaro
Copy link
Member Author

rhiaro commented Jan 17, 2021

The PR to DID core was modified (and then merged), so it no longer mentions IPFS or Hashlink. (It wasn't a requirement though, just a security suggestion - and applied to any external links, not just JSON-LD, and I had just paraphrased that out of the VC spec, by the way, which has a similar problem.. but I guess times have changed :)

Remove all references to JSON-LD integrity checking, since this implementation specific.

So, there was at some point a desire to suggest JSON-LD contexts are integrity protected, as a MITM on the context being fetched over the network is an attack vector. So language was added about hashing contexts and checking against them. But that wasn't specific enough, and you added very specific language about how to do it.

I'm a bit concerned about the options:

  1. Don't mention it at all (but leave a security vulnerability some people might not think to solve).
  2. Mention it without any specifics on how (you argued for needing the specifics).
  3. Mention it with the specifics (this is out of scope / implementation specific).

Is there a 4. I'm missing? Is one of these not as bad as the others?

  1. might not be so bad, as the maintenance WG will be able to update the Registries text as technologies around this change in the future. So we could add a note that this guidance may be updated? Or would that make things worse?

@OR13
Copy link
Contributor

OR13 commented Jan 20, 2021

@rhiaro my main issue with the language is that when telling people to register a URL with integrity protection, if we don't tell them 1 way to do it, we will get the following, which is undoubtably the worst option:

  1. https://example.com/123#sha-256-hex
  2. https://example.com/123?hash=sha-256-base64url
  3. ipfs:Qm.....
  4. https://example.com/123?hl=z6....

the problem is that this is a registry.... so we should either have 1 way of doing things nicely.... or we shouldn't take registrations with integrity checking....

Copy link
Contributor

@OR13 OR13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rhiaro can you either provide an example using this, or remove it.

I think all we are needing to approve this, is a single example that actually uses it.

@rhiaro
Copy link
Member Author

rhiaro commented Jan 22, 2021

Hesitant to compute the hash for the /v1/ jsonld context, as we may still be changing that. Is there another one I should add? Wouldn't it be the remit of the extension submitters do that themselves really, once the requirements are updated?

If we remove the requirement altogether, are we acknowledging there is no security vulnerability issue here, or that there is a security vulnerability issue that we're not telling people how to address?

@OR13
Copy link
Contributor

OR13 commented Jan 25, 2021

@rhiaro we are not acknowledging prototype pollution, MITM, DNS poisoning, etc... all of which are relevant to loading ANY JSON over a network... for that reason I would suggest not including this section.... because its implying that we are addressing security concerns when in fact, we are not really.

@OR13
Copy link
Contributor

OR13 commented Jan 25, 2021

I would be ok with a paragraph stating:

Each representation has its own security concerns. Implementers should be aware of common attacks on each representation before attempting an implementation.

  • its infeasible to include a complete list of security concerns for JSON, JSON-LD and CBOR... and even if we could, there might be other representations registered later.

@rhiaro
Copy link
Member Author

rhiaro commented Jan 25, 2021

That's fine. There is already a bit in the Core spec about implementations should cache contexts locally, and this is best/common practice, so I'm convinced we don't need to mention the hashing stuff in the Registries. Happy to close this PR without further action.

@OR13
Copy link
Contributor

OR13 commented Jan 28, 2021

yes, suggest closing.

@OR13
Copy link
Contributor

OR13 commented Jan 28, 2021

closing feel free to reopen, i expect we may need to do some editorial cleanup after #183

@OR13 OR13 closed this Jan 28, 2021
@msporny msporny deleted the feature/context-hash branch November 14, 2021 21:31
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 this pull request may close these issues.

4 participants