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

Add support for JsonWebKey2020 and JsonWebSignature2020 #96

Merged
merged 4 commits into from
Aug 6, 2020

Conversation

OR13
Copy link
Contributor

@OR13 OR13 commented Jul 29, 2020

Copy link

@baha-ai baha-ai 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 fully supporting JsonWebKey2020 to represent JWKs.

@@ -11,6 +11,8 @@
"EcdsaSecp256k1VerificationKey2019": "sec:EcdsaSecp256k1VerificationKey2019",
"Ed25519Signature2018": "sec:Ed25519Signature2018",
"Ed25519VerificationKey2018": "sec:Ed25519VerificationKey2018",
"JsonWebKey2020": "sec:JsonWebKey2020",
Copy link
Member

@msporny msporny Jul 29, 2020

Choose a reason for hiding this comment

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

Is the purpose of JsonWebKey2020 to express private key material? Why does it deviate from other LD Security key formats by removing the "Verification" nomenclature? I'm asserting that doing so is dangerous because it may convey that this key type should be used for expressing private key material (I've heard this a number of times from those planning to use this key type). Can we change the name to this:

  • JwkVerificationKey2020

If not, are there other key names that would be acceptable to the JWK community that convey that this property should only be used for expressing public keys?

To be clear, there is no objection to adding a key and signature type for JWKs -- I personally want to see that. The objection is to the name, as it is misleading... let's pick a better name.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be open to renaming it to be JwkPublicKey2020, but last I heard your experience has taught you developers don't know what a public key is and verification key was more helpful. However, I'm opposed to defining the key usage within the key suite because it ends up coupling key types and LD cryptographic suites together in a way that makes in cumbersome to use, support, and define new suites.

The purpose in defining them this way is to allow this key suite which defines only a representation to be referenced by any cryptographic suite.

Additionally, if the intent behind verification key is to express key usage, why don't we add a key usage property the same way it's been done in web crypto and JOSE?

I'm not understanding how this coupling gets us anything. This is why I've advocated that we separate the keys from the cryptographic suites and explicitly document the dangers within the key suites and cryptographic suites around non best practice usage (auto conversion from Ed25519 to X25519) which has already been seen when using ld proof suites.

Furthermore, this coupling is actually more of a nice to have naming scheme rather than preventing the actual danger of key reuse.

As an example, nothing would stop me from taking a public key used in something like RSAVerificationKey2019 and reusing it in RSAEncryptionKey2020 which if the wrong algorithm choices are made then signatures can be forged via a Bleichenbacher’s attack.

This is all to say, I really don't think we're buying ourselves safe patterns here by combining the signature and the key usage into a single suite and I know from experience that implementing this is far more cumbersome because of it which is why I'd advocate for this to not include the key usage in the name of the cryptographic suite.

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

In general supportive of this. Concerns over the JsonWebKey2020 name, would prefer it be changed to something that makes its purpose clear (expressing public key (aka verification) material).

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 remain in support of renaming this.

@OR13
Copy link
Contributor Author

OR13 commented Jul 30, 2020

Thanks for a productive call, I think we are making progress towards resolving this issue. I'd like to summarize some of my arguments, and invite others to do the same.

Arguments for "type":"JsonWebKey2020"
  1. "VerificationKey" does not absolve software implementations from doing input validation (security reality)
  2. "VerificationKey" is not an existing standard, it is a W3C CCG convention (appeal to authority / with normative statement implications)
  3. "VerificationKey" imposes JSON-LD-isms on JOSE developers (developer experience)
  4. "VerificationKey" and "KeyAgreement" are redundant to verification relationships (developer experience)
  5. "VerificationKey" is not semantically equivalent to a Json Web Key public key. (controller intention)
  6. "KeyAgreement" is not semantically equivalent to a Json Web Key public key. (controller intention)
  7. "KeyAgreement" and "VerificationKey" both have members publicKeyJwk, its redundant (developer experience)
  8. "VerificationKey" and "KeyAgreement" require independent registrations (more work for developers / less work gets done)
Arguments for "@type":"@json"
  1. "@json" does not absolve software implementations from doing input validation (security reality)
  2. "@json" maps more directly to https://tools.ietf.org/html/rfc7517 (appeal to authority / developer experience)
  3. "@json" allows us to leverage JSON flexibility / interoperability inside JSON-LD (this is both a pro and a con IMO)
  4. "@json" is the simplest possible solution here (developer experience)

Arguing against myself

Arguments for "VerificationKey"
  1. "VerificationKey" is one more place to warn people about key use (repetition can be good)
  2. "VerificationKey" is a new standards convention for expressing key material (innovation can be good)
  3. "VerificationKey" works for key representations that are not JOSE friendly... (aesthetic similarity preference)
  4. "VerificationKey" and "KeyAgreement" require independent registrations (more work for developers / less work gets done)
Arguments for "@type":"JSON-LD unknown complexity"
  1. Maybe we can use JSON-LD contexts as the way to do input validation?
  2. Encourage more people to use JSON-LD by requiring them to consider why we restricted JWK in some novel way?
  3. Digital Bazaar volunteers to do this work, and maintain support for it forever?

In summary, my preference remains to merge the PR as is AND I plan to open similar PR for GPG, and COSE if we can sort out CBOR...

final thought... if we agree to remove the JSON only representation, I would be more willing to do complex JSON-LD stuff here (not use @json)... but keeping JSON only representation AND asking people to care this much about JSON-LD W3C CCG-isms, feels like a bigger community split, than having 2 conventions for LD Proof Verification and KeyAgreeement keys.

My preference would be to use @json and drop support for the "Pure JSON" data model, which nobody has contributed to.

Copy link

@selfissued selfissued left a comment

Choose a reason for hiding this comment

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

I agree with the names JsonWebKey2020 and JsonWebSignature2020.

@OR13
Copy link
Contributor Author

OR13 commented Jul 30, 2020

Another note about JwkVerificationKey2020 should we also go renaming all the existing key typesBase58BtcVerificationKey ?

@msporny
Copy link
Member

msporny commented Aug 3, 2020

Either way, the one thing I won't budge on is that this stuff needs to be documented in the spec

Sure, I agree that all of this needs to be documented. Your work to document the things you pointed out is appreciated -- really and truly.

Similarly, you should avoid discounting the work that others are doing in the group.

You need to internalize that the folks that wrote much of the work that you're building on top of (and simultaneously criticizing) are human beings as well. We have families -- significant others, kids, and aging parents and friends and can't spend all of our time serving the community and our companies. There are only so many hours in the day and we're constantly having to make hard choices on what to work on and what to leave to another day. I know you know that at times, but also seem to lose sight of it from time to time when you say things like:

It doesn't feel great to have people who have not done the work, demand that work continue to be done by others

You might not see it, but the work that you're building on top of has been going on for a long time. Much of it before you showed up. Now, you showed up and are doing great work, but pointing fingers at people and saying they haven't "done the work" is not helpful in general.

This is one of those cases. Explaining how things came to be is work. Attempting to find consensus is work. Writing multi-page responses on Github because you're attempting to engage in an honest debate is work. Folks engaged in this thread are working. It may not be the specific work you want to see right now, but it is effort expended and time we won't get back put into an issue that you are also putting your time into.

So, let's just all be cognizant that we're doing work here. Yes, it's frustrating at times, but it's necessary if we're going to come to consensus.

@msporny
Copy link
Member

msporny commented Aug 3, 2020

We describe a set of REQUIREMENTs for naming linked data terms, and decribe at length, the mental model, reasoning behind them, so that this question doesn't keep coming up.

Here is some spec text to place into the LD Proofs Specification:

Historically, cryptographers have used the term "public/private key cryptography" to refer to a variety of cryptographic mechanisms that enable secure communication. While the word "public" is intended to express that a particular piece of cryptographic material can be shared publicly, it does not clearly express what one can use the cryptographic material for. This often leads to confusion around whether a public key was intended for the purpose of authentication, signature creation, key agreement, or other crptographic protocol. For this reason, the Linked Data Security specifications attempt to more clearly express the intended usage of the cryptographic material in a way that is semantically concrete and simultaneously human readable.
Specification authors are strongly advised use Verification Method type names that are specific to their intended purpose. For example: JwkVerificationKey2020 instead of JsonWebKey2020. The former clearly expresses that the key is a JWK key intended for use in verification where the latter is semantically ambiguous.

I'm happy to write some text once we get to consensus. We're not there yet. The text should go in the Linked Data Proofs spec. I know what text I'd write, but it doesn't seem like others agree with that text in this thread, and until we hit consensus, writing that spec text is a waste of time. Would it have mattered if that spec text was written already? Based on the arguments I've heard from @kdenhartog and @tplooker, probably not, as they disagree in principle. So, we need to keep talking until we get to some sort of consensus.

Once we have some proposals down and have reached consensus on a few things, we can write some spec text that might survive the review process.

I prefer merging things, marking them unstable, opening issues, and incremental improvement.

Sure, we could do this. Just mark everything in this PR with an issue that says that the term is "unstable and shouldn't be implemented" and we can merge this PR. We'll only kick the can down the road, though... we'll be right back to this discussion in a few weeks/months... better to have it now.

you appear to want a number of things to happen all at once.

I wasn't aware that I was asking for a number of things to happen all at once.

Could you elaborate on what you think I want?

In the meantime, I'll write up a few proposals that we might all be able to agree to at some point tomorrow in preparation for the call on Tuesday.

Copy link
Member

@gjgd gjgd left a comment

Choose a reason for hiding this comment

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

I support the name "JsonWebKey2020" or "JsonWebPublicKey2020" but not "JwkVerificationKey2020".

As others have previously stated, public keys will not necessarily be used for verifying digital signatures and having several names like JwkVerificationKey2020, JwkEncryptionKey2020, JwkKeyAgreementKey2020, etc... for the same key material would be more confusing than helpful.

@msporny
Copy link
Member

msporny commented Aug 3, 2020

having several names like JwkVerificationKey2020, JwkEncryptionKey2020, JwkKeyAgreementKey2020, etc... for the same key material would be more confusing than helpful

Why is it more confusing to have specific names for the intended type and purpose of the key? To put it another way, do you support the use parameter in JWK? If so, how is that mechanism different from this mechanism? Please be specific as you're stating an opinion w/o providing the reasoning that you used to get to that opinion and so it's difficult for others to understand your thought process.

@OR13
Copy link
Contributor Author

OR13 commented Aug 3, 2020

@msporny I opened this issue here: #101

to track the fact that all the terms in did-v1.jsonld are unstable, and note that JsonWebKey2020 might get renamed if / when we get sufficient WG consensus on naming requirements for verification methods in did core.

@msporny
Copy link
Member

msporny commented Aug 3, 2020

In an attempt to propose a few things that might gain consensus, here goes:

PROPOSAL: Existing documentation in the DID Core specification should be refined to point out that Verification Relationships are arcs in a graph of information and Verification Methods are nodes in a graph of information. That is, they provide different types of information. The first (Verification Relationship) is the expression of a relationship between a DID Subject and cryptographic information used for the purposes of verifying a proof. The second (Verification Method) is the expression of cryptographic material. An image should be provided depicting the explanation.

PROPOSAL: Documentation should be written in the Linked Data Security specifications that clarifies how public / private key terminology has been used in the past, why it is vague and leads to security issues (like key reuse attacks), and why the LDS specs strongly advise specification authors to be more specific about key type and usage when naming key types.

PROPOSAL: Documentation should be written in a DID specification (DID Core or DID Spec Registries) pointing to the naming convention as something that is strongly advised (but not required).

PROPOSAL: The JsonWebKey2020 parameter should at least be renamed to JwkPublicKey2020 with a warning in the spec noting that the name is still under debate. The reasoning for the change is that the key type is not intended to hold private key information. The Jwk prefix is used to align with the publicKeyJwk property suffix.

PROPOSAL: The values associated with publicKeyJwk property MUST be expressed as pure JSON. This is being done to align with the value space of RFC7517. The JSON-LD Context MUST align the value space of publicKeyJwk by specifying "@type": "@json".

Let's see where we get with those proposals... I expect at least the last two to achieve consensus, which would unblock this PR.

@gjgd
Copy link
Member

gjgd commented Aug 3, 2020

Why is it more confusing to have specific names for the intended type and purpose of the key? To put it another way, do you support the use parameter in JWK? If so, how is that mechanism different from this mechanism? Please be specific as you're stating an opinion w/o providing the reasoning that you used to get to that opinion and so it's difficult for others to understand your thought process.

@msporny It is confusing because you can already express that distinction with top level properties in the did document. I would put my JsonWebKey2020 (or JsonWebPublicKey2020) key meant for verification in the authentication array, my JsonWebKey2020 meant for key agreement in the keyAgreement array, etc...

I understand looking at the current registry that most other types have "VerificationKey" in their name. Not sure if it's a convention or a rule, but I'm saying this allows you to have a did document like this, with a VerificationKey in a keyAgreement array, or an EncryptionKey in an authentication array, and that is confusing

{
  ...
  "keyAgreement": [
    {
      "id": "did:example:123#_TKzHv2jFIyvdTGF1Dsgwngfdg3SH6TpDv0Ta1aOEkw",
      "type": "JwkVerificationKey2020",
      "controller": "did:example:123",
      "publicKeyJwk": {
        "crv": "P-256",
        "x": "38M1FDts7Oea7urmseiugGW7tWc3mLpJh6rKe7xINZ8",
        "y": "nDQW6XZ7b_u2Sy9slofYLlG03sOEoug3I0aAPQ0exs4",
        "kty": "EC",
        "kid": "_TKzHv2jFIyvdTGF1Dsgwngfdg3SH6TpDv0Ta1aOEkw"
      }
    }
  ]
}

@OR13
Copy link
Contributor Author

OR13 commented Aug 3, 2020

PROPOSAL: The values associated with publicKeyJwk property MUST be expressed as pure JSON. This is being done to align with the value space of RFC7517. The JSON-LD Context MUST align the value space of publicKeyJwk by specifying "@type": "@json".

+1

PROPOSAL: Existing documentation in the DID Core specification should be refined to point out that Verification Relationships are arcs in a graph of information and Verification Methods are nodes in a graph of information. That is, they provide different types of information. The first (Verification Relationship) is the expression of a relationship between a DID Subject and cryptographic information used for the purposes of verifying a proof. The second (Verification Method) is the expression of cryptographic material. An image should be provided depicting the explanation.

+1

PROPOSAL: Documentation should be written in the Linked Data Security specifications that clarifies how public / private key terminology has been used in the past, why it is vague and leads to security issues (like key reuse attacks), and why the LDS specs strongly advise specification authors to be more specific about key type and usage when naming key types.

+0 ...not this WGs job... but still a good idea.

PROPOSAL: Documentation should be written in a DID specification (DID Core or DID Spec Registries) pointing to the naming convention as something that is strongly advised (but not required).

-1 when coupled with the request for the name to change.

COUNTER PROPOSAL: Documentation will be written in the DID core specification describing a required naming convention for verification method types.

PROPOSAL: The JsonWebKey2020 parameter should at least be renamed to JwkPublicKey2020 with a warning in the spec noting that the name is still under debate. The reasoning for the change is that the key type is not intended to hold private key information. The Jwk prefix is used to align with the publicKeyJwk property suffix.

-1 when coupled with a "recommendation" that is not a requirement of W3C DID Core.

COUNTER PROPOSAL: The JsonWebKey2020 will be renamed to SOMETHING matching the required naming convention described in DID Core WHEN..... DID Core has a required naming convention for verification method types AND...DID Core states clearly that no private information of any type is to be included in did documents, including but not limited to verification relationships and services. DID Core states that fragments SHOULD NOT contain private or sensitive information. DID Core warns that all key representation SHOULD NOT contain private or sensitive information. DID Core warns that encryption SHOULD NOT be used to protect sensitive information (counter to the JOSE spec guidance).

@msporny
Copy link
Member

msporny commented Aug 3, 2020

Forgot to mention, if you -1 something, please provide an alternate proposal that you think will achieve consensus.

@msporny
Copy link
Member

msporny commented Aug 3, 2020

@msporny It is confusing because you can already express that distinction with top level properties in the did document.

Ah! Now I understand what you are saying, thank you for taking the time to elaborate.

You seem to be assuming that the only place that an Linked Data Key exists is in a DID Document, but there are other places it can exist where that top-level relationship doesn't exist. For example, an array of keys. What happens when you lose the top-level relationship? You can no longer tell what the intended purpose of the key is! "But what about JWK use!", I hear someone bemoan from the back of the room". That assumes that everyone is using publicKeyJwk to express the cryptographic material... and not everyone is doing that. Having multiple uses per key is also a dangerous thing to do as has been discovered over the years:

There are many other papers, but those should get you started on why using the same key to do different sorts of cryptographic operations is, in general, a dangerous thing to do.

In addition, you'll note that we used to have publicKey, which has been renamed to verificationMethod, which is basically a big bag of cryptographic material in a DID Document. Now, tell me what I intended this key below to do:

{
  ...
  "verificationMethod": [{
      "id": "did:example:123#_TKzHv2jFIyvdTGF1Dsgwngfdg3SH6TpDv0Ta1aOEkw",
      "type": "JsonWebKey2020",
      "controller": "did:example:123",
      "publicKeyJwk": {
        "crv": "P-256",
        "x": "38M1FDts7Oea7urmseiugGW7tWc3mLpJh6rKe7xINZ8",
        "y": "nDQW6XZ7b_u2Sy9slofYLlG03sOEoug3I0aAPQ0exs4",
        "kty": "EC",
        "kid": "_TKzHv2jFIyvdTGF1Dsgwngfdg3SH6TpDv0Ta1aOEkw"
      }
  }]
}

I understand looking at the current registry that most other types have "VerificationKey" in their name. Not sure if it's a convention or a rule

It's currently a loose convention and the topic of this particular debate is if it holds and should be made stronger / documented more.

but I'm saying this allows you to have a did document like this, with a VerificationKey in a keyAgreement array, or an EncryptionKey in an authentication array, and that is confusing

Ah! No, it doesn't. In fact, those are illegal constructs and they're immediately detectable if we type the verification methods with something more than just stating whether they're a public key or not. If we just do something like JsonWebKey2020, we have no idea if the intent behind the key was to use it as an authentication method or an encryption key. So, your example is a case in point. If we have type and key use in there, we can detect an error. If we don't, we can't detect key misuse / accidental reuse.

@OR13
Copy link
Contributor Author

OR13 commented Aug 3, 2020

Ah! No, it doesn't. In fact, those are illegal constructs and they're immediately detectable if we type the verification methods with something more than just stating whether they're a public key or not. If we just do something like JsonWebKey2020, we have no idea if the intent behind the key was to use it as an authentication method or an encryption key. So, your example is a case in point. If we have type and key use in there, we can detect an error. If we don't, we can't detect key misuse / accidental reuse.

This is not true.... again... software that does the "input validation" will be written in many languages, and will not rely on JSON-LD 1.1 as the only form of input validation.

We will have as much knowledge about what is behind the key as developers have skill.

Relying on a string property for a "type" is one of the least secure things we could possibly recommend doing :)

@OR13
Copy link
Contributor Author

OR13 commented Aug 3, 2020

Related comment on the thread attempting to register BLS12381 keys... w3c-ccg/ld-cryptosuite-registry#35 (comment)

The correct thing to do from a semantic and security perspective would be to define 3 types...

  • Ed25519KeyPair2018
  • Ed25519PublicKey2018
  • Ed25519PrivateKey2018

where publicKeyBase58 is required in Ed25519KeyPair2018 and Ed25519PublicKey2018 and Ed25519PrivateKey2018.

and where privateKeyBase58 is required in Ed25519KeyPair2018 and Ed25519PrivateKey2018 and FORBIDDEN in Ed25519PublicKey2018....

Relying on a string for types is terrible security design.

BTW, a type in JSON is any set of properties that are required, and their required values, and no additional properties.

See also:

@msporny
Copy link
Member

msporny commented Aug 3, 2020

software that does the "input validation" will be written in many languages, and will not rely on JSON-LD 1.1 as the only form of input validation.

Hmm, this may be where we've been talking past each other. What I'm describing does not require JSON-LD processing at all.

I mean this, literally:

if(verificationMethod.type === 'JwkVerificationKey2020') {
  // at this point, we know that this is meant to be a JWK-encoded key used 
  // for the purposes of checking digital signatures.
}

Now, compare that against the following:

if(verificationMethod.type === 'JsonWebKey2020') {
  // at this point, we have no idea what the developer intended the key to be
  // used for
}

That does not depend on JSON-LD processing in any way.

Relying on a string property for a "type" is one of the least secure things we could possibly recommend doing :)

Do you think that this is the only protection the specification will have? Do you find any value in specifying type, if so, what?

@msporny
Copy link
Member

msporny commented Aug 3, 2020

COUNTER PROPOSAL: Documentation will be written in the DID core specification describing a required naming convention for verification method types.

This is effectively: "The DID Core specification will describe required naming conventions for the JOSE stack".

I'm fine w/ putting language like that in the DID Core spec, but it really feels like a layering violation that folks will complain about. So, +0 -- if this is all we can get to in a compromise, I won't block it, but it feels like a layering violation. How is it not a layering violation?

COUNTER PROPOSAL: The JsonWebKey2020 will be renamed to SOMETHING matching the required naming convention described in DID Core WHEN..... DID Core has a required naming convention for verification method types AND...DID Core states clearly that no private information of any type is to be included in did documents, including but not limited to verification relationships and services. DID Core states that fragments SHOULD NOT contain private or sensitive information. DID Core warns that all key representation SHOULD NOT contain private or sensitive information. DID Core warns that encryption SHOULD NOT be used to protect sensitive information (counter to the JOSE spec guidance).

I'm going to break this counter proposal apart and +1/-1 individual items. This is the challenge with writing too much in a proposal, they tend to fail because of small subclauses that cause complications.

The JsonWebKey2020 will be renamed to SOMETHING matching the required naming convention described in DID Core

-1 to vague and doesn't warn readers that the topic is under discussion. The counter-proposal is to put a warning into the spec until this is settled.

DID Core has a required naming convention for verification method types

+0 belongs in LD Security specs, just like any sort of naming conventions for JOSE are expressed in those specs. If the group goes here, which I hope they don't, I won't stand in the way as having the language /somewhere/ would be better than nowhere.

DID Core states clearly that no private information of any type is to be included in did documents, including but not limited to verification relationships and services.

+1, although we already have a statement to that effect in the specification:

https://w3c.github.io/did-core/#keep-personally-identifiable-information-pii-private

DID Core states that fragments SHOULD NOT contain private or sensitive information.

+1, but again, feel like it's already covered:

https://w3c.github.io/did-core/#keep-personally-identifiable-information-pii-private

DID Core warns that all key representation SHOULD NOT contain private or sensitive information.

+1, and again, covered by the following?

https://w3c.github.io/did-core/#keep-personally-identifiable-information-pii-private

DID Core warns that encryption SHOULD NOT be used to protect sensitive information (counter to the JOSE spec guidance).

Wait, what? Why would it do that. You mean on ledger? If so, we already say that, so +1 if that's what you meant:

https://w3c.github.io/did-core/#encrypted-data-in-did-documents

@tplooker
Copy link
Contributor

tplooker commented Aug 3, 2020

The correct thing to do from a semantic and security perspective would be to define 3 types...

+1 to this proposal, that is what I think we should be doing.

@peacekeeper
Copy link
Contributor

where privateKeyBase58 is required in Ed25519KeyPair2018 and Ed25519PrivateKey2018

then what's the difference between Ed25519KeyPair2018 and Ed25519PrivateKey2018?

@tplooker
Copy link
Contributor

tplooker commented Aug 3, 2020

You seem to be assuming that the only place that an Linked Data Key exists is in a DID Document, but there are other places it can exist where that top-level relationship doesn't exist. For example, an array of keys. What happens when you lose the top-level relationship? You can no longer tell what the intended purpose of the key is! "But what about JWK use!", I hear someone bemoan from the back of the room". That assumes that everyone is using publicKeyJwk to express the cryptographic material... and not everyone is doing that. Having multiple uses per key is also a dangerous thing to do as has been discovered over the years:

@msporny I agree with your point about how we cannot always rely on the layer of verification methods being present around LD key pairs in order to infer their usage, but I still really don't think we should be attempting to convey this in the name. I accept that in many cases it is considered bad practise from a security perspective to use a key for more than one purpose however there are still valid usages that we would be preventing, say a Nistp256Key based did:key method would look like this under your proposal.

{
  "@context": ["https://www.w3.org/ns/did/v1", "https://w3id.org/security/v1"],
  "id": "did:key:z123456789abcdefghi",
  "publicKey": [{
    "id": "did:key:z123456789abcdefghi#keys-1",
    "type": "NistP256VerificationKey2020",
    "controller": "did:key:z123456789abcdefghi",
    "publicKeyBase58": "H3C2AVvLMv6gmMNam3uVAjZpfkcJCwDwnZn6z3wXmqPV"
  }, {
    "id": "did:key:z123456789abcdefghi#keys-2",
    "type": "NistP256KeyAgreementKey2020",
    "controller": "did:key:z123456789abcdefghi",
    "publicKeyBase58": "H3C2AVvLMv6gmMNam3uVAjZpfkcJCwDwnZn6z3wXmqPV"
  }],
  "keyAgreement": [ "did:example:z123456789abcdefghi#keys-2" ],
  "assertionMethod" [ "did:example:z123456789abcdefghi#keys-1" ]
}

Instead of

{
  "@context": ["https://www.w3.org/ns/did/v1", "https://w3id.org/security/v1"],
  "id": "did:key:z123456789abcdefghi",
  "publicKey": [{
    "id": "did:key:z123456789abcdefghi#keys-1",
    "type": "NistP256PublicKey2020",
    "controller": "did:key:z123456789abcdefghi",
    "publicKeyBase58": "H3C2AVvLMv6gmMNam3uVAjZpfkcJCwDwnZn6z3wXmqPV"
  }],
  "keyAgreement": [ "did:example:z123456789abcdefghi#keys-1" ],
  "assertionMethod" [ "did:example:z123456789abcdefghi#keys-1" ]
}

If we want a way for a key to explicitly express whats its valid usage is then I think we should be doing something similar to JOSE with a convention like

{
    "id": "did:key:z123456789abcdefghi#keys-1",
    "type": "NistP256PublicKey2020",
    "controller": "did:key:z123456789abcdefghi",
    "publicKeyBase58": "H3C2AVvLMv6gmMNam3uVAjZpfkcJCwDwnZn6z3wXmqPV",
    "usage": [ "Verification" ]
}

IMO a convention like this

  1. makes it far easier to develop against with evolving list of suites (e.g if I want to check if a key is valid for verification) rather than maintaining branching logic for all the suites that happen to have Verification in their name.
if (key.usage === "Verification") {
 // I know the key is valid for verifying digital signatures
}
  1. Allows for a key to have multiple valid usages

@tplooker
Copy link
Contributor

tplooker commented Aug 3, 2020

then what's the difference between Ed25519KeyPair2018 and Ed25519PrivateKey2018?

This is a good point I think these could be collapsed together

@OR13
Copy link
Contributor Author

OR13 commented Aug 3, 2020

where privateKeyBase58 is required in Ed25519KeyPair2018 and Ed25519PrivateKey2018

then what's the difference between Ed25519KeyPair2018 and Ed25519PrivateKey2018?

@peacekeeper it would probably suck... but you can derive the keypair from the private key... and technically, an object with 2 properties is different type than an object with 1 property....

If you never actually need the private key by itself... you could probably get away with only 2 types....

or we can relax all of this and just keep doing what the folks at digital bazaar started us off with.... labeling private keys as verification methods :)

This is what you get when you do JSON.stringify(Ed25519KeyPair.generate()):

{
    "type": "Ed25519VerificationKey2018",
    "id": "#z6MkhKuTPc4mqF3E4oQLLWZE41vmSPhqeEbooVPHFYkkhcc1",
    "controller": "did:key:z6MkhKuTPc4mqF3E4oQLLWZE41vmSPhqeEbooVPHFYkkhcc1",
    "publicKeyBase58": "3seQoMpLVhYkxJZdewbPCvNmcpRzEMMT7UUMRGnjnPpd",
    "privateKeyBase58": "Y5ukCnYMFJfeJyRM2HEHnKjUbSRRzkR2WgwwiZoTuj9FXG6jkbXC1vndVwRgKPeNYpnqYMzKsU6DeYMTBNt4kzm"
  }

Shouldn't this really be:

{
    "type": "Ed25519KeyPair2018",
    "id": "#z6MkhKuTPc4mqF3E4oQLLWZE41vmSPhqeEbooVPHFYkkhcc1",
    "controller": "did:key:z6MkhKuTPc4mqF3E4oQLLWZE41vmSPhqeEbooVPHFYkkhcc1",
    "publicKeyBase58": "3seQoMpLVhYkxJZdewbPCvNmcpRzEMMT7UUMRGnjnPpd",
    "privateKeyBase58": "Y5ukCnYMFJfeJyRM2HEHnKjUbSRRzkR2WgwwiZoTuj9FXG6jkbXC1vndVwRgKPeNYpnqYMzKsU6DeYMTBNt4kzm"
  }

With an option to export a verification method that looks like:

{
    "type": "Ed25519VerificationKey2018",
    "id": "#z6MkhKuTPc4mqF3E4oQLLWZE41vmSPhqeEbooVPHFYkkhcc1",
    "controller": "did:key:z6MkhKuTPc4mqF3E4oQLLWZE41vmSPhqeEbooVPHFYkkhcc1",
    "publicKeyBase58": "3seQoMpLVhYkxJZdewbPCvNmcpRzEMMT7UUMRGnjnPpd"
  }

@OR13
Copy link
Contributor Author

OR13 commented Aug 3, 2020

As this also related to encryption and keyAgreement, cross linking this: w3c/did-core#185

@selfissued if you can take a look at this, it would be helpful to know if we are walking into a problem wrt JWE and interop via keyAgreement

@OR13
Copy link
Contributor Author

OR13 commented Aug 3, 2020

I think what this really boils down to, is a desire to have nice looking RDF Triples...

DID Subject -> keyAgreement -> X25519KeyAgreementKey2019
DID Subject -> keyAgreement -> X25519PublicKey2019
DID Subject -> keyAgreement -> X25519Key2019

... end of the day... private keys are used for key agreement... so I am not sure the existing name is actually helpful... at all.... and private keys can be used to verify signatures... you just need to derive the public key first.... and well, the use case for that is you want to make sure nobody has tampered with your statements... and you only preserve some entropy... like a mnemonic... and you constantly regenerate key pairs only as needed.... this is really common on public blockchains like bitcoin and ethereum... https://en.bitcoin.it/wiki/Deterministic_wallet_tools

personally I would prefer to define JsonWebKey2020 with the following typescript interfaces...

export type P384PublicKeyJwk = {
  kty: 'EC';
  crv: 'P-384';
  x: string;
  y: string;
  kid?: string;
};

export type P384PrivateKeyJwk = {
  kty: 'EC';
  crv: 'P-384';
  x: string;
  y: string;
  d: string;
  kid?: string;
};

export type X25519PublicKeyJwk = {
  kty: 'OKP';
  crv: 'X25519';
  x: string;
  y: string;
  kid?: string;
};

export type JsonWebKey2020PrivateKeyJwk = P384PrivateKeyJwk;
export type JsonWebKey2020PublicKeyJwk = P384PublicKeyJwk | X25519PublicKeyJwk;

export interface JsonWebKey2020 {
  id: string;
  controller: string;
  type: 'JsonWebKey2020';
  publicKeyJwk: P384PublicKeyJwk;
  privateKeyJwk?: JsonWebKey2020PrivateKeyJwk;
}

because its the most similar to what digital bazaar is doing today here: https://github.com/digitalbazaar/crypto-ld/tree/master/lib ... (so it would be easy to keep using their existing code / libraries)... but does not rely on object oriented inheritance / javascript which lacks an actual type system...

When working with typescript modules that define types like this, developers get IDE warnings and compile time errors when they confuse public and private keys...

@OR13
Copy link
Contributor Author

OR13 commented Aug 4, 2020

Do you think that this is the only protection the specification will have? Do you find any value in specifying type, if so, what?

@msporny I missed this, but think I covered it in my responses... TL;DR;... I don't consider a type to be matched until all its members have been compared to expected values and no additional properties are present... checking type alone... is not a solution for me.

@dlongley
Copy link
Contributor

dlongley commented Aug 4, 2020

I don't consider a type to be matched until all its members have been compared to expected values and no additional properties are present... checking type alone... is not a solution for me.

This seems reasonable to me. But, just to more fully register my opinion here on naming considerations and the utility of "verification methods" / cryptographic suites/profiles more fully:

As we have discussed in many issues/calls, there are a lot of cryptographic primitives that people could put together for a number of different reasons.

I think we have consensus to support having different "profiles" of parameters for keys and various protocols
to help reduce potentially unsafe combinations and enforce good validation rules. In the DID core spec, we can express these "profiles" as verification methods.

Each verification method is defined by its type, which refers to a set of key and protocol parameters that are expected to be expressed and used when employing that method.

Therefore, this type information can be used by implementers to know how to validate the rest of the information expressed in the verification method -- and DID methods should absolutely perform validation on this information, particularly when it comes to ensuring only valid data gets onto a verifiable data registry. Again, I think we have agreement here.

The type information can also serve as a simple way for users to describe the type of signature or key agreement scheme they support as consumers or producers. As we've spoken about many times in this group, it would perhaps be ideal to even name these methods things like ExpertRecommendedSignature2020 or NistApprovedKeyAgreement2020 and then tuck the expected details about which curve is used or and other expected parameters behind those types.

Now, while it is possible to enable lots of options for a given profile, a major reason for creating these profiles in the first place is to limit optionality. Again, this is to reduce the likelihood that users may create dangerous combinations -- and to remove the
burden from the user of having to make choices they may not want to fully understand. By calling out specific limited profiles it is easier for implementations to look up their validation rules and apply them rather than having to perform combinatorial checks against a wide variety of parameters. In this latter case, it simplifies implementations and enables them to be more easily secured.

So, I think these are all good reasons to be more restrictive with what a verification method type maps to. This does mean, however, that spec authors and extenders need to put in more effort to create new profiles vs. choosing an easier path of creating just one profile with lots of optionality -- because while that is easier for spec authors and extenders, it creates more challenges further on down the line for users that now have to figure out how to create safe configurations themselves.

I'd be happy to adopt a simple "registries rule" that a verification method type maps to a single combination of primitives -- and that, as a community, we should offer a very limited set of "best configurations" for a given calendar year. But in listening to others in the group and particularly on this issue, I can also be flexible to allow for slightly more optionality if there are trade offs that the group really feels are worth it. But I want to make sure that we understand that we are, in fact, making an important security trade off, as "more" isn't always "better".

I finally want to say that existing standards aren't perfect; they were made by groups of people just like us in the DID WG with all kinds of constraints like time and resources. And we, as a standards group, have an opportunity and a responsibility to make improvements where we can.

@msporny
Copy link
Member

msporny commented Aug 5, 2020

I don't consider a type to be matched until all its members have been compared to expected values and no additional properties are present... checking type alone... is not a solution for me.

Agreed, none of the proposals put forward assert that type alone is a solution. It is a part of a larger solution, namely what you outline. Check type, then check all of the expected values. This doesn't meant that type is not important... it should be as specific as we can make it. Rather than elaborate, @dlongley has done a good job of highlighting a reasonable path forward here: #96 (comment) (the comment above this one).

Copy link
Member

@msporny msporny left a comment

Choose a reason for hiding this comment

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

The following proposal was put forward and passed in the DID Special Topic call today:

RESOLUTION: Add one issue in the Verification Methods section with a warning stating that there is an ongoing discussion around the naming of verification methods that may impact the final names used in the specification.

As long as that holds, and we get to some concrete, documented guidance around the naming of LD Security types, I'm fine w/ pulling this PR in (because we're making it clear in a PR to come that these names could change and my expectation is that they will).

What that means is that we can pull this PR in, but we're not going to be able to go to CR until we resolve this issue, so let's get a group of folks that care about it into another call and decide what to do. It was also decided on the call today that the DID WG will not be taking a position on the naming conventions that the LD Security specs use. That work will be done in the W3C CCG, who are in charge of the LD Security specs at present. @OR13 and I volunteered to liaise with the CCG on the issue.

I'll raise a separate PR for the resolution on the call today, assuming it holds in 7 days.

This PR is clear to be merged in 24 hours, unless we hear objections. There is still a possibility that someone will object to the resolution above in the next 7 days, and if so, I'll submit a request to back out this PR as this PR going in is contingent on the resolution we made on the DID Special Topic call today.

Copy link
Contributor

@dlongley dlongley left a comment

Choose a reason for hiding this comment

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

Approving given the resolution on the call and my comments here in the thread.

@msporny
Copy link
Member

msporny commented Aug 6, 2020

Normative, multiple reviews, changes requested but not made, resolution to warn about potential name change to content in this PR (waiting for objections before merging a separate PR for that), this PR staying in the registry is dependent on the aforementioned warning being placed into the registry, no current objections, merging.

@msporny msporny merged commit 76c3d3a into master Aug 6, 2020
@msporny msporny deleted the feat/add-support-for-json-web-key-2020 branch November 14, 2021 21:33
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.