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

Update to Candidate Recommendation JSON-LD Context. #277

Merged
merged 4 commits into from
Apr 7, 2021

Conversation

msporny
Copy link
Member

@msporny msporny commented Mar 28, 2021

The WG forgot to update the Candidate Recommendation JSON-LD Context to something that didn't have gaping security holes before we entered CR (the current context allows redefinition, which let's you switch things like verification relationships). This was a fairly major oversight, this PR attempts to fix that. :)

This PR assumes that the JSON-LD Context ONLY contains context values that are specified in the DID Core specification. It doesn't include the cryptosuites because different DID Methods will make different choices there (some will chose to only use pure Ed25519 or Koblitz, while others will support the JOSE version of Koblitz, while others will pull in the kitchen sink). Same with service types. The expectation here is that DID Methods will specify at least ONE extra Method-specific context, and that's where they will specify cryptosuites and services supported by the DID Method.

DB has done some light testing on this JSON-LD Context. We need more eyes on it to make sure it's what we want.

"@type": "@id",
"@container": "@set"
},
"publicKey": {
Copy link
Contributor

Choose a reason for hiding this comment

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

publicKey not being retained for backwards compatibility?

Copy link
Member Author

@msporny msporny Mar 29, 2021

Choose a reason for hiding this comment

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

No I don't think we should, publicKey has been deprecated for a while now. It's usually a bad practice to keep deprecated things around, especially since we have the opportunity to do the right thing still (in CR) and because it would lead to two code paths in crypto libraries (and we really want to try and reduce things to a few code paths as possible to reduce attack surface).

vocabs/v1/context.jsonld Show resolved Hide resolved
vocabs/v1/context.jsonld Show resolved Hide resolved
vocabs/v1/context.jsonld Show resolved Hide resolved
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.

I'm good with approving now irregardless of decision about metadata properties

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

This PR assumes that the JSON-LD Context ONLY contains context values that are specified in the DID Core specification. It doesn't include the cryptosuites because different DID Methods will make different choices there

+1 to this.

The expectation here is that DID Methods will specify at least ONE extra Method-specific context, and that's where they will specify cryptosuites and services supported by the DID Method.

I think this needs more discussion on a call.. My understanding is that putting this into method-specific contexts would imply that all the cryptosuite property names and values would have to be removed from the DID spec registries (otherwise, which JSON-LD context will the registry show e.g. for Ed25519VerificationKey2018?). This in turn will make lossless conversion between representations harder (= the original purpose of the registry).

"@type": "@id"
"@id": "https://w3id.org/security#verificationMethod",
"@type": "@id",
"@container": "@set"
Copy link
Contributor

@peacekeeper peacekeeper Mar 30, 2021

Choose a reason for hiding this comment

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

This change in abda671 breaks compatibility with both security-v2.jsonld and the current security-v3-unstable.jsonld, i.e. this leads to "protected term redefinition" JSON-LD errors.

Is the expectation that verificationMethod will be changed in security-v3-unstable.jsonld as well? Couldn't this lead to problems with existing LD proof and VC implementations?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is the assumption that the DID context and the security contexts will never be used together (I hope not).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point @peacekeeper... we'll have to think hard about what we want here.

I think what we're (slowly) learning is that having giant context files for security purposes is a double-edged sword. It's nice for testing and running experiments, but when you get to production, you really do want to protect the security terms and to be very specific about the security primitives you're pulling in.

So, the current proposal is to do stuff like this for DID Documents (did:key example using 2020 suites -- note that we're pulling in a specific security suite, not the entire security context):

  "@context": [
    "https://www.w3.org/ns/did/v1",
    "https://w3id.org/security/suites/ed25519-2020/v1"
  ],

VCs would do more or less the same thing:

  "@context": [
    "https://www.w3.org/2018/credentials/v1",
    "https://w3id.org/security/suites/ed25519-2020/v1"

Example of the item above working:

https://tinyurl.com/yzp6alkg

Thoughts?

Comment on lines 57 to 58
"@type": "@id",
"@container": "@set"
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree that this should not be a @set -- it wasn't in an earlier revision of this PR.

Suggested change
"@type": "@id",
"@container": "@set"
"@type": "@id"

Copy link
Contributor

Choose a reason for hiding this comment

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

assuming we accept this change, won't this context allow you to embed an object? seems like thats actually incompatible with the did core spec..... I have never once seen a did document that allowed a verification relationship to be anything but an array... and I object to implying that its a good idea outside of the actual spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

The context doesn't stop anyone from embedding objects anywhere. But if you don't follow the DID core spec, you won't be compliant.

Copy link
Contributor

Choose a reason for hiding this comment

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

strong -1 to did context allowing non conformant data structures... feel free to do that in another context.... not sure I am reading this correctly, but it sounds like you are saying the did core context should be more flexible than the did core spec... I don't agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OR13, No, I'm saying that the did core context does not work the way you seem to be implying. No context does. It's not a schema.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also aggregate signature schemes generally.

Yeah, we need to discuss that as a community. I have my concerns based on some proposed structures (and verificationMethod ever being allowed to be an array in a proof -- there may be use cases there, but one use case we definitely have today is the "not an array" use case):

w3c-ccg/community#188 (comment)

We should make did core the same as security vocab.

Agreed.

I thought you were arguing against this, seems like you aren't anymore?

Consistency is worth more here IMO, if we are in a world where you need to make sure 1 thing is an array, why not make it so that all relationships are treated the same.

I'm fine w/ doing that. That would mean none of those would be associated with "@container": "@set", and instead, JSON-LD Producers have to ensure that they're sets when they produce the DID Document (which is the normative requirement in the spec today).

Copy link
Contributor

Choose a reason for hiding this comment

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

Please be aware that today all of the verification relationships that aren't the "catch all" verificationMethod are defined as @sets in contexts across the ecosystem. Deciding to make those now NOT @sets in the DID context "just for consistency" is ... not consistent. This latter inconsistency is more negatively impactful to the existing ecosystem.

Copy link
Member Author

@msporny msporny Apr 3, 2021

Choose a reason for hiding this comment

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

Deciding to make those now NOT @sets in the DID context "just for consistency"

It's not being done "just for consistency"... I'm starting to think that "@container": "@set" is an anti-pattern when used with protected contexts... it creates more problems than it's worth.

Copy link
Contributor

Choose a reason for hiding this comment

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

it creates more problems than it's worth.

I agree with that.

@msporny I adjusted my argument here:

#277 (review)

Essentially DID Core should go all in on sec-v2 or all in on sec-v3 (really this is sec-v3 following did core instead of sec-v2 on this issue).

I don't thing not breaking LD Signatures on a DID Document is a compelling reason to avoid consistency on this matter.

sec-v2 cannot be changed, did core and sec-v3 can... whatever decision we make should be present in both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Reminder that the VC context uses @set in its proof purpose definitions -- so that should also be considered as a potential hazard.

I'm starting to think that a future revision to JSON-LD's protected term feature should consider allowing @set to be ignored since it doesn't change the meaning of the term, only the preferred shape when compaction is used -- which doesn't really matter when you're intentionally not compacting anything and simply validating your inputs using JSON schema, etc. (@iherman, this is something to consider in the future / or perhaps an "evergreen" thing).

@msporny
Copy link
Member Author

msporny commented Mar 30, 2021

@peacekeeper wrote:

I think this needs more discussion on a call.

Agreed. We need to come to consensus as a community on this.

My understanding is that putting this into method-specific contexts would imply that all the cryptosuite property names and values would have to be removed from the DID spec registries

That's not the same conclusion that I came to.

otherwise, which JSON-LD context will the registry show e.g. for Ed25519VerificationKey2018?

The Registry could show multiple contexts (we would ideally have some sort of script that automatically runs to auto-generate those fields in the registry). We would feed in every JSON-LD Context that we know, and for each registered property we would (in some not horrible way) list all the contexts that use the property.

This in turn will make lossless conversion between representations harder (= the original purpose of the registry).

You might be realizing why a subset of us always wanted @context to be preserved during a round-trip. :)

This is why I've been insisting that you can't re-create the @context value from the registry... because you can't. There is not just /one/ JSON-LD Context file that might include a particular cryptosuite.

@peacekeeper
Copy link
Contributor

peacekeeper commented Mar 30, 2021

@msporny I like your example in #277 (comment).

When you said "DID Methods will specify at least ONE extra Method-specific context", I thought you meant that each DID method would have to define their own context, and then every cryptosuite would be replicated into a new context for each new DID method that needs it. This would be a bit "too granular" I think.

Maybe we could call your approach "cryptosuite-specific contexts" rather than "method-specific-contexts"?

@msporny
Copy link
Member Author

msporny commented Mar 30, 2021

Maybe we could call your approach "cryptosuite-specific contexts" rather than "method-specific-contexts"?

It feels like the "cryptosuite-specific contexts" is the right way to go, but I didn't want to rule out the "method-specific-contexts" -- they're both valid. The former is probably safer, but might lead to larger @context arrays depending on what a DID Document / VC contains. For example, some DID Methods might support 12 cryptosuites and so putting all of them at the top might cause some developers to think "it's ugly" (it's not a good reason, but some developers do make choices based on ergonomics vs. security). The latter can keep the @context array to 2 items... but at the cost of dumping everything that's not in the did/v1 context into the example-method/v1 context.

We'd prefer folks do the former, but expect some subset of the 85 DID Methods to do the latter...

@peacekeeper
Copy link
Contributor

The Registry could show multiple contexts

You might be realizing why a subset of us always wanted @context to be preserved during a round-trip. :)

@msporny I guess this becomes even more interesting when considering the verificationMethod example in this thread. Could the registry contain multiple JSON-LD contexts for a property, if the term definitions in those contexts are DIFFERENT?

Then you would say, hey, even more reason for preserving @context!

But my counterargument would be that such a property would be impossible to use in representations other than JSON-LD (or CBOR-LD or other *-LD), and that therefore there wouldn't be any point in putting it into a registry in the first place...

@msporny
Copy link
Member Author

msporny commented Apr 1, 2021

@peacekeeper wrote:

@msporny I guess this becomes even more interesting when considering the verificationMethod example in this thread. Could the registry contain multiple JSON-LD contexts for a property, if the term definitions in those contexts are DIFFERENT?

Then you would say, hey, even more reason for preserving @context!

But my counterargument would be that such a property would be impossible to use in representations other than JSON-LD (or CBOR-LD or other *-LD), and that therefore there wouldn't be any point in putting it into a registry in the first place...

Yes, that's an excellent point... while it's a corner-case, and we hope that people wouldn't redefine things of that sort, we know it's going to happen. That's the downside with decentralized systems... people can choose to do things that you don't want them to do. :)

I wouldn't say that the property would be impossible to use in representations other than JSON-LD... JSON and CBOR representations do stuff like that all the time, people mean different things when they use 'homepage' in non-semantically-rich payloads. The point of the registry is to give developers a heads-up on what's available to them. We could even go to great lengths to warn developers that "this property is not defined in exactly the same way across contexts!" -- and even go further and say "every context defines the term in the same way, except for contexts 6 and 8".

Not putting something in the registry just because the ecosystem defines the term in slightly different ways would probably harm interoperability, not help it.

We should err on the side of giving developers more information so that they can make a well-informed decision. Does that seem like a legitimate strategy to you, @peacekeeper?

@peacekeeper
Copy link
Contributor

The point of the registry is to give developers a heads-up on what's available to them. We could even go to great lengths to warn developers that "this property is not defined in exactly the same way across contexts!"

Well, at the Amsterdam F2F when we invented the registry, I was told that its main purpose would not be extensibility, but interoperability and lossless conversion between representations. With this understanding in mind, every time I see anything in the registry that doesn't help with lossless conversion, I think "this shouldn't be there". So if e.g. in a plain JSON DID document without @context I see a property which has two different contexts in the registry, with different definitions, then lossless conversion to the JSON-LD representation is impossible (and this isn't always a bad thing).

But I understand the purpose of the registry has shifted a bit since then, it now also serves as documentation of what's available, with additional information that can be used in various ways.

Thinking about this even more, the verificationMethod case in this thread is especially interesting, since the difference between the two definitions is not of a semantic nature, but only about its data model.

We should err on the side of giving developers more information so that they can make a well-informed decision. Does that seem like a legitimate strategy to you, @peacekeeper?

Yes, no objection, even though personally I still want to drop and re-create @context :)

Copy link
Contributor

@peacekeeper peacekeeper left a comment

Choose a reason for hiding this comment

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

Approve, but would prefer to discuss this on a call before merging.

@peacekeeper
Copy link
Contributor

FYI since this also came up in comments above, I'm working on a PR to the did-resolution context that adds metadata properties:

w3c/did-resolution#63

@OR13 OR13 self-requested a review April 3, 2021 14:56
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.

@msporny This PR is large, and I think we should merge it ASAP.

However, I think we need to treat @container, @set consistently in places where it may be an array or object, and have the producer for JSON-LD be responsible for maintaining conformance, NOT "out of the box JSON-LD tooling"....

The same will be true of every representation... sadly, thanks to the WGs terrible decision to support an unbounded number of representations.

 "assertionMethod": {"@id": "sec:assertionMethod", "@type": "@id", "@container": "@set"},

Its true that assuming w3id/security/v2 ... you will see conflicts...

I see only 2 paths forward for did core:

  1. hard couple did core v1 to sec v2
  "verificationMethod": {"@id": "sec:verificationMethod", "@type": "@id"},
  "assertionMethod": {"@id": "sec:assertionMethod", "@type": "@id", "@container": "@set"},
  1. hard couple did core v1 to sec v3
  "verificationMethod": {"@id": "sec:verificationMethod", "@type": "@id"},
  "assertionMethod": {"@id": "sec:assertionMethod", "@type": "@id"}, // corrected

or

  "verificationMethod": {"@id": "sec:verificationMethod", "@type": "@id", "@container": "@set"}, // corrected
   "assertionMethod": {"@id": "sec:assertionMethod", "@type": "@id", "@container": "@set"},

Path 1 feels like letting a broken bone heal without being set, and celebrating that you can still walk.

Path 2 feels out of scope for the WG, and blocked by the painfully slow contribution to sec v3.

If I had to choose, I would break all signatures over did documents with sec v2... because we already know sec v2 is on its way to the grave... and breaking signatures on did documents might be a great way to have folks actually fix sec v3 so this never happens again.

@msporny
Copy link
Member Author

msporny commented Apr 3, 2021

However, I think we need to treat @container, @set consistently in places where it may be an array or object, and have the producer for JSON-LD be responsible for maintaining conformance

Ok, I can do that... I'm going to strip all "@container": "@set" declarations from the JSON-LD Context. I don't think we need normative changes to the spec (because the spec already states they need to be sets). We could add non-normative language to the spec targeted at implementers to make sure they do this when producing JSON-LD DID Documents (same goes for JSON producers).

@msporny
Copy link
Member Author

msporny commented Apr 7, 2021

Ok, this is what I think we settled on (thanks to everyone for providing input and being flexible).

We have an existing ecosystem, specifically the vc-data-model, ActivityPub, and signature vocabs. They define things in ways that are @protected such that if we were to redefine things in the DID JSON-LD Context, errors would be thrown (redefinition errors). If we want to make sure all these contexts can continue to be be used together, we can't redefine existing protected declarations.

This is really only an issue for verificationMethod and alsoKnownAs -- all the verification relationships are being defined for the first time in DID Core, so they don't have any legacy issues and we can do @container: @set on those items.

The real solution here is to relax the JSON-LD implementations so that they don't throw an error if @container: @set is used (it doesn't change the semantics, just the structures returned... and @protected really was intended to protect semantics and not structure).

The general rule going forward is: Do not use @container: @set until JSON-LD 1.2 is released... you'll just cause yourself and the rest of the ecosystem pain. We are taking a special exception for verification relationships because they can always refer to a set of relationships.

Seeing a bunch of +1s, no objections, and that the conversation has settled, merging.

@msporny msporny merged commit d96e8f5 into main Apr 7, 2021
@OR13
Copy link
Contributor

OR13 commented Apr 7, 2021

it doesn't change the semantics, just the structures returned...

but I thought it wasn't a schema language...

resize

@troyronda
Copy link
Contributor

troyronda commented Apr 7, 2021

I'm late to commenting on this PR, but I am curious about the context suggestion. @msporny

The expectation here is that DID Methods will specify at least ONE extra Method-specific context

I'm trying to understand this impact a bit better - with this solution, I now need to pre-load contexts for each supported DID method into my software. I have been curious if there should be a DID method context or if the DID method should specify individual crypto suite contexts for the keys contained in the DID document (and then we just need to pre-load the common crypto suite contexts). (or are there any plans for a registry approach of LD suites?)

@msporny
Copy link
Member Author

msporny commented Apr 7, 2021

I'm trying to understand this impact a bit better - with this solution, I now need to pre-load contexts for each supported DID method into my software.

Well, you always needed to do that... this change just makes it more evident.

The challenge here is that different people in these ecosystems want different things -- some want a mega context (like schema.org), others want more tightly defined contexts (like #include / import statements in programming languages).

I'm trying my best to read the group and it seems (given all the +1s above) that folks are leaning toward the latter (this year, at least) :)

I have been curious if there should be a DID method context or if the DID method should specify individual crypto suite contexts for the keys contained in the DID document (and then we just need to pre-load the common crypto suite contexts). (or are there any plans for a registry approach of LD suites?)

There are plans for a registry approach for LD Suites... which might cause some people to do stuff like this:

https://github.com/w3c/did-test-suite/pull/54/files#diff-c81ddb975aa4caf0733a08c11a8b2f8bde67286e56b24223abd77cc27fa587d4R13-R17

Others might rail at that and choose to do it some other way (kitchen sink DID Method JSON-LD Context). I think we're trying to nudge folks towards the former (tightly defined context files for common stuff -- like cryptosuites) and then a DID Context for everything else (if the DID Method has anything to add).

@msporny
Copy link
Member Author

msporny commented Apr 8, 2021

but I thought it wasn't a schema language...

har har :)

If it is a schema language... it's really bad at its job and should be fired... and JSON Schema should be hired in its place.

To be more pedantic (even though I know you were joking, others might benefit from the clarification), a schema language imposes constraints on inputs, the @set feature in a JSON-LD context is an imperative instruction to produce a certain structure regardless of the input when performing compaction.

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.

8 participants