Add JSON Web Key to specification #135

msporny · 2023-07-29T21:57:12Z

This PR addresses issue #130 and issue #73 by adding the JsonWebKey type to the specification and the vocabulary, and ensuring that the range of publicKeyJwk is JSON. This value was going to be defined in the JsonWebSignature2020 specification, but since that specification has been pulled, we have to define it somewhere else and the Data Integrity specification is probably the best place for it since it also defined controller documents and verification methods.

Preview | Diff

iherman

Just an editorial comment, possibly with an editorial note. It may be worth emphasizing that the publicKeyJwk having a JSON datatype means that all json terms within the target are not subject to the JSON-LD context processing. Ie, the terms like kid or alg are not mapped to any URL.

vocab/security/vocabulary.yml

index.html

vocab/security/vocabulary.yml

index.html

OR13 · 2023-07-31T19:59:34Z

index.html

+
+            <p>
+The example above is identical to the previous example, except that the
+information is stored in the `secretKeyJwk` property and its value encodes the


hmmm... privateKeyJwk vs secretKeyJwk... Is the intention to make this work with MAC Signatures? and thats why you choose secretKeyJwk ?

I agree that privateKeyJwk is a better name than secretKeyJwk.

Is the intention to make this work with MAC Signatures? and thats why you choose secretKeyJwk ?

Yes, that's one of the reasons.

The other reason is due to the English definition of the word "private" vs. the English definition of the word "secret":

private: intended for or restricted to the use of a particular person, group, or class
secret: kept from knowledge or view; something kept hidden or unexplained

IOW, the word "secret", that is -- "do not share this with others", is closer to the intended usage of the property than "private", which can be misinterpreted as "its ok to share this information with a group" (which is almost never the case with a symmetric or a private key).

OR13 · 2023-07-31T20:00:06Z

index.html

+            <p>
+The example above is identical to the previous example, except that the
+information is stored in the `secretKeyJwk` property and its value encodes the
+private key value in the `d` property.


This seems possibly not true for all secret keys.

this is definitely not true for all secret keys, and may yet be changing - why would we define any of this here rather than just point at rfc7517?

See https://datatracker.ietf.org/doc/draft-ietf-cose-key-thumbprint/

for concrete examples...

https://mailarchive.ietf.org/arch/msg/cose/T4Od25YoPmgOuALuuIGf1m8N4wo/

This seems possibly not true for all secret keys.

The language you are pointing to is providing a description of a specific example. It is not claiming that the example is true for all secret keys.

this is definitely not true for all secret keys, and may yet be changing - why would we define any of this here rather than just point at rfc7517?

Please re-read the /description of the example/. We are not defining anything here, merely explaining what an example is expressing.

vocab/security/vocabulary.yml

OR13

We should get more reviews on the secretKeyJwk part... will it be in the vocabulary? can it be used with HMAC?.

Concrete change request is to define secretKeyJwk in the vocabulary or remove the example.

I'm not attached to privateKeyJwk vs secretKeyJwk... perhapse it would be good to ask the experts on the JOSE and COSE mailing lists how they feel about this?

mprorock · 2023-07-31T20:24:11Z

+1 @OR13 - definitely want feedback in from the jose and cose lists

mprorock

lets get feedback requests out to appropriate experts - clean up some naming / guidance or just point to appropriate RFCs, otherwise i think this good

TallTed

Mostly small editorial stuff. One larger thing which I think is also editorial and hopefully not controversial.

index.html

OR13 · 2023-08-08T22:55:34Z

I agree with @iherman that secretKeyJwk or privateKeyJwk needs to be defined for this to be merged... and the same applies to publicKeyMultibase or secretKeyMultibase or privateKeyMultibase.

See also https://github.com/w3c/vc-data-integrity/pull/150/files#diff-af0ab1b57396186235708fdf500251dfc3f9b18aeeb992f927979d9630889c28R21

and https://github.com/w3c/vc-data-integrity/pull/149/files#diff-be8086ba9b3e09e91dd045c04051b6e1d0d2a10105db9d380835cb1151fd5470R29

Seems blocked pending #152 (review)

OR13

Pending merge / approval of #152 (review)

iherman

I have made some inline comments.

I must say I am lost among all the PRs that refer to various property names, anchors, ranges, etc. I am of the opinion at this point that all these PRs should be merged asap to allow for a comprehensive review of the vocabulary, domains, ranges, etc, possibly leading to new PRs. The current situation is simply too chaotic in my view.

iherman · 2023-08-10T06:33:04Z

index.html

+  "@context": ["https://www.w3.org/ns/security/jwk/v1"],
+  "id": "did:example:123456789abcdefghi#key-1",
+  "type": "JsonWebKey",
+  "controller": "did:example:123456789abcdefghi",
+  "publicKeyJwk": {
+    "kty": "OKP",
+    "alg": "EdDSA"
+    "crv": "Ed25519",
+    "kid": "key-1",
+    "x": "_1EiHquO2aUx9JARSu0P8jdYT_OVneYxYOnOMAmUcFI",
+  }
+}


Why do we need yet another context here? The definition of publicKeyJwk stipulates that its object is of datatype JSON, meaning that the properties within that object are ignored by any JSON-LD processing. If so, then I do not see any reason using a different context (which I have not seen defined anywhere).

Proposal: just use the usual security context here.

We don't need another context here.

I will object if one is required... however, I think its fine to ship many smaller contexts from w3id.org (which is not the W3C)... IMO W3C examples should use W3C context URLs.

Also, seems we are following a different style here than the core context, where we seem to prefer bundling, that inconsistency, feels... not good to me.

@OR13,

The @vocab in the core v2 context is what is driving the need to bundle. Whenever that isn't used, bundling should be avoided (unless there's some other very good reason to do it). That's the consistent rule we're applying.

Raised #164 to track @OR13's concern, which seems to be more of a misunderstanding than a misalignment.

iherman · 2023-08-10T06:36:20Z

index.html

+  "@context": ["https://www.w3.org/ns/security/jwk/v1"],
+  "id": "did:example:123456789abcdefghi#key-1",
+  "type": "JsonWebKey",
+  "controller": "did:example:123456789abcdefghi",
+  "secretKeyJwk": {
+    "kty": "OKP",
+    "alg": "EdDSA"
+    "crv": "Ed25519",
+    "kid": "key-1",
+    "d": "Q6JwjCUdThSnoxfXHSFt5C1nVFycY_ZpW7qVzK644_g",
+    "x": "_1EiHquO2aUx9JARSu0P8jdYT_OVneYxYOnOMAmUcFI",
+  }


Same comment as above. Except that we use here secretKeyJwk, but I presume the range of that property is also JSON

Same response as above, yes, range is JSON.

msporny · 2023-08-12T19:22:58Z

PR #152 has been merged into this PR. It has been two weeks since we asked for feedback from the COSE mailing list (and no feedback has been given). If we get feedback in the future, we can always raise an issue and modify this via a future PR. In the meantime, this PR needs to go in so we have the appropriate anchors to finish up the vocabulary and JSON Web Key sections of the specification.

Co-authored-by: Ivan Herman <[email protected]>

Co-authored-by: Ted Thibodeau Jr <[email protected]>

Co-authored-by: Orie Steele <[email protected]>

Co-authored-by: Dmitri Zagidulin <[email protected]>

Co-authored-by: Dave Longley <[email protected]>

Co-authored-by: Ted Thibodeau Jr <[email protected]>

msporny · 2023-08-12T19:28:47Z

Normative, multiple reviews, changes requested and made, no objections, merging.

msporny requested review from Wind4Greg and seabass-labrax as code owners July 29, 2023 21:57

msporny requested review from dlongley, iherman, dmitrizagidulin, TallTed, mprorock, David-Chadwick, brianorwhatever, decentralgabe, OR13, brentzundel, mkhraisha and awoie July 29, 2023 21:58

This was referenced Jul 29, 2023

Add JsonWebKey definition in Data Integrity #130

Closed

domain of publicKeyJWK should be rdf:JSON #73

Closed

mkhraisha approved these changes Jul 29, 2023

View reviewed changes

Wind4Greg approved these changes Jul 29, 2023

View reviewed changes

dlongley approved these changes Jul 29, 2023

View reviewed changes

iherman approved these changes Jul 30, 2023

View reviewed changes

vocab/security/vocabulary.yml Outdated Show resolved Hide resolved

iherman requested changes Jul 30, 2023

View reviewed changes

index.html Outdated Show resolved Hide resolved

vocab/security/vocabulary.yml Show resolved Hide resolved

decentralgabe approved these changes Jul 31, 2023

View reviewed changes

OR13 reviewed Jul 31, 2023

View reviewed changes

index.html Outdated Show resolved Hide resolved

OR13 reviewed Jul 31, 2023

View reviewed changes

vocab/security/vocabulary.yml Show resolved Hide resolved

OR13 suggested changes Jul 31, 2023

View reviewed changes

mprorock suggested changes Jul 31, 2023

View reviewed changes

TallTed suggested changes Aug 1, 2023

View reviewed changes

msporny mentioned this pull request Aug 6, 2023

Add definition sections for secretKeyJwk and secretKeyMultibase #151

Closed

TallTed approved these changes Aug 7, 2023

View reviewed changes

OR13 approved these changes Aug 8, 2023

View reviewed changes

msporny requested a review from iherman August 10, 2023 00:56

iherman approved these changes Aug 10, 2023

View reviewed changes

msporny mentioned this pull request Aug 12, 2023

Is another context required to express JsonWebKey or Multikey? #164

Closed

msporny and others added 18 commits August 12, 2023 15:24

Add JsonWebKey definition and usage guidance.

2aca662

Add JsonWebKey to vocabulary.

e4070ba

Ensure range of publicKeyJwk is JSON.

f2d37ea

Fix up JWK context URL and kid values.

2c71aa0

Fix syntax and unnecessary definition in JSON Web Key section.

8f6077b

Co-authored-by: Ivan Herman <[email protected]>

Fix grammar and punctuation in JSON Web Key section.

0c68dfc

Co-authored-by: Ted Thibodeau Jr <[email protected]>

Use more generic URL for JOSE registry.

fe58b01

Co-authored-by: Orie Steele <[email protected]>

Add secretKeyJwk to normative statement about JWK usage.

1432380

Set domain for publicKeyMultibase and publicKeyJwk.

7d61be3

Move definition of publicKeyJwk to JsonWebKey section.

d5cc1c0

Add definition of secretKeyJwk.

3f6ebbc

Move definition of publicKeyMultibase to Multikey section.

f4d2c4c

Add definition of secretKeyMultibase.

93d1efd

Add definition anchors for JsonWebKey.

f452e42

Fix grammar and links for Verification Material section.

a558186

Fix grammar in publicKeyJwk description.

ffa1049

Co-authored-by: Dmitri Zagidulin <[email protected]>

Fix minor issues w/ secret key definitions.

f54b5d4

Co-authored-by: Dave Longley <[email protected]>

Fix punctuation and grammar for secreKey* sections.

1852514

Co-authored-by: Ted Thibodeau Jr <[email protected]>

msporny force-pushed the msporny-json-web-key branch from cf9f708 to 1852514 Compare August 12, 2023 19:26

msporny merged commit 8d5a7cf into main Aug 12, 2023

msporny deleted the msporny-json-web-key branch August 12, 2023 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add JSON Web Key to specification #135

Add JSON Web Key to specification #135

msporny commented Jul 29, 2023 •

edited by pr-preview bot

Loading

iherman left a comment

OR13 Jul 31, 2023

selfissued Aug 4, 2023

msporny Aug 5, 2023

OR13 Jul 31, 2023

mprorock Jul 31, 2023

OR13 Aug 4, 2023

msporny Aug 5, 2023

OR13 left a comment

mprorock commented Jul 31, 2023

mprorock left a comment

TallTed left a comment

OR13 commented Aug 8, 2023 •

edited

Loading

OR13 left a comment

iherman left a comment •

edited

Loading

iherman Aug 10, 2023

OR13 Aug 11, 2023 •

edited

Loading

OR13 Aug 11, 2023

dlongley Aug 11, 2023

msporny Aug 12, 2023

iherman Aug 10, 2023

msporny Aug 12, 2023

msporny commented Aug 12, 2023

msporny commented Aug 12, 2023

Add JSON Web Key to specification #135

Add JSON Web Key to specification #135

Conversation

msporny commented Jul 29, 2023 • edited by pr-preview bot Loading

iherman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OR13 left a comment

Choose a reason for hiding this comment

mprorock commented Jul 31, 2023

mprorock left a comment

Choose a reason for hiding this comment

TallTed left a comment

Choose a reason for hiding this comment

OR13 commented Aug 8, 2023 • edited Loading

OR13 left a comment

Choose a reason for hiding this comment

iherman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OR13 Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msporny commented Aug 12, 2023

msporny commented Aug 12, 2023

msporny commented Jul 29, 2023 •

edited by pr-preview bot

Loading

OR13 commented Aug 8, 2023 •

edited

Loading

iherman left a comment •

edited

Loading

OR13 Aug 11, 2023 •

edited

Loading