-
Notifications
You must be signed in to change notification settings - Fork 112
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
Sync and fix bugs in v2 context. #1259
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -5,6 +5,7 @@ | |||
|
||||
"id": "@id", | ||||
"type": "@type", | ||||
|
||||
"kid": { | ||||
"@id": "https://www.iana.org/assignments/jose#kid", | ||||
"@type": "@id" | ||||
|
@@ -43,17 +44,20 @@ | |||
}, | ||||
"cnf": { | ||||
"@id": "https://www.iana.org/assignments/jwt#cnf", | ||||
"@context": { | ||||
"@protected": true, | ||||
"kid": { | ||||
"@id": "https://www.iana.org/assignments/jwt#kid", | ||||
"@type": "@id" | ||||
}, | ||||
"jwk": { | ||||
"@id": "https://www.iana.org/assignments/jwt#jwk", | ||||
"@type": "@json" | ||||
"@context": [ | ||||
null, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you explain what this is "fixing" ? (in more detail than the PR description has). Specifically what cases would this prevent that might be valuable to RDF processing community who would be impacted by this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
You defined If you didn't mean to do that, we wouldn't need to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So what this is fixing is: Using the more specific registry to define the terms underneath This seems like an excellent improvement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If anything else needs to be defined under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dlongley that seems like breaking changes, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you want to keep all of the other terms we can just remove There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I suggest we let the default context apply, unless there is agreement to replace it with a different value, for all terms under There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm happy to just drop the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not easy to suggest this in github right now, but want it tracked, so don't apply directly:
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in c44da90 |
||||
{ | ||||
"@protected": true, | ||||
OR13 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
"kid": { | ||||
"@id": "https://www.iana.org/assignments/jwt#kid", | ||||
"@type": "@id" | ||||
}, | ||||
"jwk": { | ||||
"@id": "https://www.iana.org/assignments/jwt#jwk", | ||||
"@type": "@json" | ||||
} | ||||
} | ||||
} | ||||
] | ||||
}, | ||||
"_sd_alg": { | ||||
"@id": "https://www.iana.org/assignments/jwt#_sd_alg" | ||||
|
@@ -146,7 +150,8 @@ | |||
"verifiableCredential": { | ||||
"@id": "https://www.w3.org/2018/credentials#verifiableCredential", | ||||
"@type": "@id", | ||||
"@container": "@graph" | ||||
"@container": "@graph", | ||||
"@context": null | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just trying to test my own understanding, @dlongley @msporny. My JSON-LD knowledge is rusty. Setting Is this indeed the goal of this extra setting? If so, shouldn't there be a similar setting in the security There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like it would make some of these examples illegal: https://w3c.github.io/vc-jose-cose/#presentations {
"@context": ["https://www.w3.org/ns/credentials/v2"],
"type": ["VerifiablePresentation"],
"holder": "urn:ietf:params:oauth:jwk-thumbprint:sha-256:_Fpfe27AuGmEljZE9s2lw2UH-qrZLRFNrWbJrWIe4SI",
"verifiableCredential": [{
"@context": [
"https://www.w3.org/ns/credentials/v2"
],
"type": [
"VerifiableCredential"
],
"issuer": "https://issuer.example/issuers/68",
"validFrom": "2023-06-07T21:14:14.148Z",
"credentialSubject": {
"id": "https://subject.vendor.example"
}
},
"https://vendor.example/credentials/42", "did:example:123",
"urn:uuid:01ec9426-c175-4e39-a006-d30050e28214",
"urn:ietf:params:oauth:jwk-thumbprint:sha-256:_Fpfe27AuGmEljZE9s2lw2UH-qrZLRFNrWbJrWIe4SI",
"data:application/vc+ld+json;base64,eyJAY29udGV4dCI6WyJodHRwczovL3d3dy53My5vcmcvbnMvY3JlZGVudGlhbHMvdjIiXSwidHlwZSI6WyJWZXJpZmlhYmxlQ3JlZGVudGlhbCJdLCJpc3N1ZXIiOiJkaWQ6andrOmV5SnJhV1FpT2lKMWNtNDZhV1YwWmpwd1lYSmhiWE02YjJGMWRHZzZhbmRyTFhSb2RXMWljSEpwYm5RNmMyaGhMVEkxTmpwdlFtUm1kbVpET1hoNk1GOUJVWFpSTjNZMU1YbERXbDl6ZUdwNU56VkNUSEpJZWsxT1Jqa3lPV1U0SWl3aWEzUjVJam9pVDB0UUlpd2lZM0oySWpvaVJXUXlOVFV4T1NJc0ltRnNaeUk2SWtWa1JGTkJJaXdpZUNJNklqTmljbU5zYjBJNGFEUk5XbFZJYms5UVVHbGtTbXd0U2pkdVVsRkpXSFJUYUZwM1oyNW1jbHAxVDI4aWZRIiwidmFsaWRGcm9tIjoiMjAyMy0wNi0wN1QyMToxNDoxNC4xNDhaIiwiY3JlZGVudGlhbFN1YmplY3QiOnsiaWQiOiJodHRwczovL3N1YmplY3QudmVuZG9yLmV4YW1wbGUifX0="
]
} Or are these ok, as long as they "dereference" to a compact JSON-LD object representing a conforming document for a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The processing rules don't allow it as the content is expected to be present (it will not be auto-dereferenced and even then, there are no content hashes).
Those examples were never legal in v1.0, v1.1, and v2.0... none of them are conforming documents as defined by the specification. The addition is the null'ing of the context, which is required to address issue #1150. So, this change is purely additive and doesn't impact the examples you provided (the answers below would be the same w/o this PR). That said: If you pre-processed this URL and put this object into the array, it might be a conforming document. By itself, it's not a conforming document.
A DID Document isn't a VC, so, no, that wouldn't work, AFAICT. It's not a conforming document.
Presuming this thing is an identifier for a graph, you could potentially express it, but then it's not clear how you'd dereference it or guarantee the security around the object unless you were to provide a content hash. This couldn't work unless one were to presume that there was some sort of "graph dereferencing" function in play. Again, not a conforming document.
Same answer as the above.
If you pre-processed this URL and put the resulting object into the array, it might be a conforming document. By itself, it's not a conforming document. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I'd like to see spec text addressing this, its not obvious that what you are saying is true, and these n-quads look fine to me:
This can't be true, the only normative requirement for a DID Document is that it have an This is one step off of a signed did resolution result... we should not be designing incompatibility here, we don't want to preclude use cases we see developing in the wild.
Similar to DID Documents, you can't presume that DIDs can't be dereferenced, even though it's not defined normatively. I didn't share the resolved document, but if I shared one that had an
Same answer as above.
Its clear we don't have agreement on the domain / range of The v1 context seems to support this assertion. "issuer": {
"@id": "cred:issuer",
"@type": "@id"
}, "proof": {
"@id": "sec:proof",
"@type": "@id",
"@container": "@graph"
}, "verifiableCredential": {
"@id": "cred:verifiableCredential",
"@type": "@id",
"@container": "@graph"
} This is the definition in the v2 context today: "verifiableCredential": {
"@id": "https://www.w3.org/2018/credentials#verifiableCredential",
"@type": "@id",
"@container": "@graph"
}, For comparison, here are the context definitions for verification relationships: "assertionMethod": {
"@id": "https://w3id.org/security#assertionMethod",
"@type": "@id",
"@container": "@set"
}, As you can see the only difference is It does mean that the n-quads produce blank nodes, as you can see from the example above. There are lots of cases in RDF / JSON-LD where you may have previously dereferenced an identifier to a graph node / object... and you don't need to send a "full copy" of the same object over and over again... this is especially common in supply chain use cases cc @mprorock @mkhraisha ... we use this pattern when communicating references to previous "credentials" and "presentations" and "workflows" which are just another large JSON object that is referred to be "id". TLDR, I am opposed to breaking changes to this experience in the v2 context. I am fine leaving the behavior as it is above (where you can always replace an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There aren't breaking changes here -- If you just put a string value in that field, then all that happens is an empty graph container (with a blank node identifying it) is created and there's a relationship established between the VP and this empty graph. A string value (ID / URL) for a subject does not establish any claims or additional relationships so the new graph is just empty. The only relationship added is between the VP and the blank node of this empty graph via the But! There is a property that can do what you want to do in VCDM 2.0. I would recommend using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've also mentioned that we need to make sure to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This AFAIK, never actually resulted in implementations.
Yes, unless we remove the
When you say
This seems to be confusing "VerifiableCredential" with "VerifiableCredentialGraph" cc @iherman & @msporny , we discussed confusion related to this on #1240 It would help to understand the normative / english language value of the "VerifiableCredentialGraph" since that is what is "empty".
I like the suggestion, lets not let it stop this PR, I will file a separate issue for follow up: #1265 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Staying on topic, I am opposed to adding the "null" context here: https://github.com/w3c/vc-data-model/pull/1259/files#diff-267b7e47789daf9a1d312b42826aa937a1bb5af4312ee90df6b8a64cd6426f4fR154 Until we achieve clarity / consensus on the topic above. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes, if you wanted to strike that part ("or data derived from verifiable credentials"), I'd be ok with it and would favor just requiring embedded VCs.
Well, what the
The
If I understand you, the value is to put a wrapper around a set of statements to isolate them from other statements made: to help keep track of associations. The statements made in a VC are "wrapped" by a separate graph (put "into" that graph) to keep them separate from statements made in any other VC embedded in the VP. A VP can carry N-many VCs this way, potentially coming from many different issuers, whilst tracking the association of the statements in the graph model.
+1 |
||||
}, | ||||
"termsOfUse": { | ||||
"@id": "https://www.w3.org/2018/credentials#termsOfUse", | ||||
|
@@ -273,7 +278,10 @@ | |||
} | ||||
} | ||||
}, | ||||
"cryptosuite": "https://w3id.org/security#cryptosuite", | ||||
"cryptosuite": { | ||||
"@id": "https://w3id.org/security#cryptosuite", | ||||
"@type": "https://w3id.org/security#cryptosuiteString" | ||||
OR13 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
}, | ||||
"proofValue": { | ||||
"@id": "https://w3id.org/security#proofValue", | ||||
"@type": "https://w3id.org/security#multibase" | ||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13 Just to be clear, all values you've declared as
"@type": "@id"
MUST be URIs... you cannot have non-URL values in there... JOSE defines many of these values as strings, IIRC... are you sure you want to do this? Have you thought through all of the use cases?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
JSON-LD also says that some text fields are datetime strings... my experience is that eventually you process a string that is supposed to be a URL or a datetime, and it turns out to be an emoji.... Also, google uses
@id
values that are not URLs in their enterprise knowledge graph search API, so i'm not sure I would assume that those values are URLs.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13,
What's being said here is that processors will raise errors if you use
"@type": "@id"
in the term's context definition if something other than a URL or graph node is used in the data. This is not the same case as using a datetime string that is malformed, etc., as those will not be processed by default. So, if you want to allow any kind of string in the field, you should remove"@type": "@id"
, otherwise default processing will try to enforce the use of URLs or graph nodes by raising errors when something else is present.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A JOSE
kid
is a string and need not be a URI. The typing should reflect that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@selfissued not sure it matters since URIs are strings, and only an RDF processor will see these term definitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's possible for the
@type: @id
and String to both be legal, that would be preferred... but in absence of that, we prefer the processing of these fields with DIDs or other URLs or URNs to build useful graphs.We also see cases where
@id
is not a URL from google:https://cloud.google.com/enterprise-knowledge-graph/docs/mid
https://github.com/GoogleCloudPlatform/python-docs-samples/blob/aaf4a56ce412d300134fae8359b874359c8f3330/enterpriseknowledgegraph/search/lookup_sample_test.py#L22
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
URIs have a structure, they're not just arbitrary strings (which, IIUC, @selfissued is saying
kid
can be any arbitrary string).If you make
kid
andiss
antype: @id
, a JSON-LD processor will throw an error when processingkid
andiss
if either value isn't a URI... I don't think we want that behavior, it'll lead to interop issues.That said, if you want to enforce that
kid
andiss
be a URI when used in VCs, then usingtype: @id
is one way to do that... but again, we'd be narrowing the scope of JWT in that case, which I didn't think was the intention of the WG.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As shown in the example above, the industry does not agree that
@id
must be a URI... we don't need to do anything here, here is digital bazaar's json-ld processor handling the above example without erroring:https://v.jsld.org/FwjjEErfzWY5ogNK7HM6ZFCRsfCBtfGxfpp64361qdd2bu7zFeykxD3e4gTXmcpNrztDGpfaa4zn5BNd8Uy5znajBTBBbFpwZK8QEGKSAgaU3aF7rq3DsLmuvJdGi2FFqgmQkvPiwzULpji68V6WNcHCvKmSB136ngyXwizmqP6BY8hB3zWCKAeNpt9Y5o45rdMNyUxNsdUa9JMfoecVqB94VKAHJ2z92U5KfbaDEHdAkH56JnwjVQf1EA7r2HEDVTefgiTiz81Q1KzbSrv636XupufPGKS6CwTBTZLT5paZjcQkjiiYBMRZQWxYxc8Ji4ZzUHzLngspxERKcUheDaKvemoRGrTxktsR7bwDFm6dTkStg8baTymnbPCYiX1GbHTv7mK6aJUUeykESpwedtzGRMZTmZuhdRJ2eCyZvZTyWRe8NmNMZXeek5drfFjjpCavDuHDQiZP76iuo2xEMkuL99hgrNJbGaKC6ULpYTVtXbEC6ep9oCXqebF97jibSuEkHkZMWjEKL1hF5PXKr5dReptbxExcLeUqNVbMtASXYqrQvhR9UikDTabmX19HPnua6S3KPanmMm9Mg58VGSAqiZKw1u9S8WJ1anzBy4UpS7ANWkAnTDAAZoyxKWEQrapoSgppqzEYHDqHqg1kzXJW59cQEXn1yPYW8pwNX2XUL1Kf6EjwHLjXLR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@OR13,
That value only works when there's a base URL (like there always is with Web pages during search crawling and all that). With VCs, there is no base URL, it's presumed to be
null
, so it would result in a relative URL being used and an error as shown here: https://tinyurl.com/pys6xa4kSo I recommend we remove
"@type": "@id"
from all of the JOSE term context definitions that can take something other than a URL.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This concern is now tracked in #1275.