w3c / did-core

W3C Decentralized Identifier Specification v1.0
https://www.w3.org/TR/did-core/
Other
395 stars 93 forks source link

Add warning about publicKeyMultibase #772

Closed OR13 closed 2 years ago

OR13 commented 3 years ago

This PR:

  1. changes the order of verification material so that the "non-normative" feature is defined after the normative one.
  2. adds a warning about privateKeyMultibase representations potentially not existing.

https://pr-preview.s3.amazonaws.com/w3c/did-core/pull/772.html#dfn-publickeymultibase


Preview | Diff

peacekeeper commented 3 years ago

Agree with the re-ordering, but don't really understand the warning..

privateKeyMultibase is not in the DID Spec Registries, and it's not in the Security Vocab. And why even talk about private key representations at all in DID Core?

oed commented 3 years ago

Why would you ever want to represent the private key in the DID document?

OR13 commented 3 years ago

@peacekeeper

And why even talk about private key representations at all in DID Core?

see the comments on d in the normative feature, publicKeyJwk.

TallTed commented 3 years ago

[@OR13 said] see the comments on d in the normative feature, publicKeyJwk.

Could you please provide a link to those comments, or at least to their neighborhood?

OR13 commented 3 years ago

@oed

You would not want to ever represent a private key in a did document.

You probably would want to be able to represent a private key in the same way you represent the public key in a did document... for publicKeyJwk, we comment on this by noting that d should be excluded.

For publicKeyMultibase can you provide an example of a privateKeyMultibase for P-256 or Bls12381G2 : )

Thats what the advisement is about... as of right now, its not possible and its not documented anywhere... here are some PRs that made this possible for:

https://github.com/multiformats/multicodec/pulls?q=is%3Apr+is%3Aclosed+private

Noting that those are not all the key types that we see in did documents... in particular, there is not a single FIPS/NIST approved key that can have its private key component represented by privateKeyMultibase, unless you consider Ed25519 to be the only key type in use, the warning would appear to be required.

OR13 commented 3 years ago

@TallTed

https://w3c.github.io/did-core/#dfn-publickeyjwk

The map MUST NOT contain "d", or any other members of the private information class as described in Registration Template.

OR13 commented 3 years ago

A related comment regarding the lack of clarity on privateKeyBase58 and privateKeyMultibase:

oed commented 3 years ago

@OR13 Even if it would be nice publicKeyMultibase doesn't use multicodec at all, so if they are part of the multicodec table is not really relevant here.

OR13 commented 3 years ago

@selfissued thats fair, one example of that potential change would be requiring or forbidding EC point compression, although I have yet to see a publicKeyMultibase or publicKeyBase58 where an uncompressed public key was represented.

OR13 commented 3 years ago

@oed https://github.com/w3c/did-test-suite/search?q=publicKeyMultibase

z6Mkh1... -> z = base58, 6Mk -> base58 of uvarint of:

https://github.com/multiformats/multicodec/blob/master/table.csv#L86

and as the table notes, its status is "draft".

Are you using publicKeyMultibase differently?

oed commented 3 years ago

@OR13 According to the did-core spec:

If present, the value MUST be a string representation of a [MULTIBASE] encoded public key.

It says nothing about using the multicodec varint so if those implementations use that it seems incorrect?

Seems like if you use multicodec it should be named something like publicKeyMulticodec.

OR13 commented 3 years ago

@oed

publicKeyMulticodec has been discussed before.

oed commented 3 years ago

@OR13 you are missing my point. What's in did-core right now clearly does not mandate the use of multicodec.

OR13 commented 3 years ago

@oed I get your point, show me an example of what is in did core today, in the wild or in the test suite...

I might even suggest that did core is wrong today, given the only example in the test suite appears to use multicodec.

Also, would you mind commenting on this PR: https://github.com/w3c-ccg/lds-ed25519-2020/pull/11

ping @clehner and @msporny

oed commented 3 years ago

I might even suggest that did core is wrong today, given the only example in the test suite appears to use multicodec.

I would be fine with that since I think using multicodec makes sense :)

OR13 commented 3 years ago

Folks may find this helpful in understanding why the reference to MULTIBASE - https://datatracker.ietf.org/doc/html/draft-multiformats-multibase-03 is currently included, but there is no reference to MULTICODEC:

https://github.com/multiformats/multicodec#faq

OR13 commented 3 years ago

@jonnycrunch you may also have feelings on this subject :)

msporny commented 3 years ago

the only example in the test suite appears to use multicodec.

While this is true, we shouldn't assume everyone that uses publicKeyMultibase will want to use multicodec or not. Some may prefer raw keys, others may prefer multicodec. The only commonality between those two approaches is multibase (not multicodec), which is why the property is named the way it is.

The assumption here is that your type value will tell you whether or not the publicKeyMultibase value is multicodec-encoded or not. It's also the most general property we could use that would result in across-the-board ideal compression in things like CBOR-LD.

I expect there would be a large number of -1s for changing this to publicKeyMulticodec for those reasons (not that I think anyone is suggesting that at this point).

oed commented 3 years ago

The assumption here is that your type value will tell you whether or not the publicKeyMultibase value is multicodec-encoded or not.

It would make sense to just have a type that's Multicodec, since you can always find the information about which type of public key it is from the multicodec varint.

msporny commented 3 years ago

It would make sense to just have a type that's Multicodec, since you can always find the information about which type of public key it is from the multicodec varint.

The danger there is that you take something that has two failsafes and move it to only having one.

One of the goals with the Linked Data Security work is to make cryptography safer to use for most developers. That means putting in multiple fail-safes to reduce the number of foot guns in the ecosystem. For example -- being able to semantically express private key values should either not be defined (and be application-specific) OR should not be defined in cryptosuites that are used in DID Documents and other public-facing data structures (this is counter to what JWKs do, which is globally define private key values across all applications -- which has led to disastrous publication of private key material in public spaces). While we can't prevent that sort of thing from happening 100% of the time, there are things we can do to reduce the possibility of that happening.

Another fail safe is double-checking the key type against the multicodec header. For example, "type": "Ed25519VerificationKey2020" is expected to have a publicKeyMultibase value that starts with a ed25519-pub multicodec header. So, there are two failsafes there (to make sure you're not accidentally dealing with private key material) ... the type, and the multicodec header. While some will argue that this is overkill, the counter-argument is that it's warranted give the mistakes that developers have made with JWK. Creating a Multicodec keytype repeats some of those same mistakes that have been made over the years in the same way that the publicKeyJwk property does. I know there are people in the community that are bound and determined to provide "key agility" -- and one of the features of Linked Data Security (decentralized innovation) is that we can't stop those people from proceeding down a dangerous road... but we should all be aware of the philosophies that are going into these key designs and what's driving each design.

Hope that helps provide some background, @oed. :)

oed commented 3 years ago

@msporny Hm, I fail to see why having two if statements would be better than one. I understand the concern about private keys, but that could easily be mitigated by not allowing private key multicodecs?

msporny commented 3 years ago

@msporny Hm, I fail to see why having two if statements would be better than one.

For the same reason that having seatbelts paired with airbags is a good idea. Having two failsafes are typically better than one, especially if there is no big cost to adding the failsafe. Two barriers to get through to prevent catastrophic failure. :)

I understand the concern about private keys, but that could easily be mitigated by not allowing private key multicodecs?

Private key multicodecs are already a thing -- that ship sailed when ed25519-priv was placed into the table 10 months ago:

https://github.com/multiformats/multicodec/commit/28f7ad5fbed9180d60b59e02d787858e81146e87

The table now contains multiple private key multicodecs, making it possible to accidentally expose (and index) private keys encoded in multicodec.

https://github.com/multiformats/multicodec/blob/master/table.csv#L131

oed commented 3 years ago

For the same reason that having seatbelts paired with airbags is a good idea. Having two failsafes are typically better than one, especially if there is no big cost to adding the failsafe. Two barriers to get through to prevent catastrophic failure. :)

It's more like having two seat belts in this case :P My implementations are likely going to end up generating the type from the multicode (they already do).

Private key multicodecs are already a thing

Sure but you could easily put private keys into a DID document even if multicodes for them were not a thing. I fail to see the difference.

msporny commented 3 years ago

It's more like having two seat belts in this case :P My implementations are likely going to end up generating the type from the multicode (they already do).

Well, then your implementations will be more dangerous than others and I wouldn't be surprised if you get issues raised against your library doing dangerous things (like presuming the type from the multicodec value and not the other way around) or developers choosing to avoid your implementations. :)

Sure but you could easily put private keys into a DID document even if multicodes for them were not a thing. I fail to see the difference.

Yes, and IF you were to do that, your library should throw an error if the type and multicodec value don't match. That would be the point of having two failsafes. That's an example of your first multicodec failsafe failing, which is the whole reason you have two failsafes.

OR13 commented 3 years ago

related, vocabulary for. "multicodec based keys"... that "behave like JWK"...

My personal opinion having done a LOT of key conversion between these forms recently is that JWK is a much better solution for JSON based key representations, and the ambiguity regarding non JWK based key representation is harmful for interop.

here is an example:

P-256 "Key Pair":

(ignoring that private key compression is not a thing... or is it?)

You can see how this leads to a lot of "potentially legal" interpretations of did core, that are not directly interoperable without some key conversion.

We recently did some upgrades regarding this, and when we started testing for compatibility with:

Our developers were surprised that:

const edKeyPair = await Ed25519VerificationKey2020.generate();

generated a private key, that is used for signing in the suite.

simply put, we don't share the same design aesthetic / semantic preferences as others.

We think a new key representation needs to do the following:

  1. be capable of representing the full key (both public and private)
  2. make it clear when looking at the key material if its public or private (multicodec does this, multibase does not)
  3. be capable of key conversion to legacy formats (JWK/PEM)... (can't do this if there is no privateKeyMultibase, or if its domain is undefined )

We agree that type can help play a role here... we don't agree that its the only place, and neither do others who use multicodec in publicKeyMultibase since they key type ends up getting encoded there as well, in a way that is a helpful redundancy.

We decided to put this out of scope for did core, and I think that is the right move still..

But I am worried that publicKeyMultibase may not be defined well enough to protect users, and given its a "non-normative feature".... we seem to be stuck... if we are unwilling to define it fully in a way that supports interoperability, we should at least warn folks that we couldn't do that.

Here are some things I believe would help:

  1. Add language that requires multicodec to be used with publicKeyMultibase, when present in a did document.
  2. Accept the proposed warnings regarding publicKeyMultibase... add explicit note that it might NOT have multicodec in it.
  3. Remove the property from did core

I expect 3 is dead of arrival, but I note that we already rely on external registries to cover 100% of the verificationMethod.type space.

It wouldn't be the end of the world, and if there really is not consensus on whether it should contain multicodec or not... I would be in favor of 3, and I think the property is truly harmful to interoperability in the same way that this stop light is harmful to understanding what to do:

image

oed commented 3 years ago

@OR13 Fyi, multicodec specifies that public keys should be compressed: https://github.com/multiformats/multicodec/blob/master/table.csv#L126

msporny commented 3 years ago

@OR wrote:

Add language that requires multicodec to be used with publicKeyMultibase, when present in a did document.

-1 because the DID WG would be overstepping into definition of cryptosuites, which is not in our charter.

Accept the proposed warnings regarding publicKeyMultibase... add explicit note that it might NOT have multicodec in it.

+1 to this approach, which I believe this PR does. To be clear, I'm supportive of this PR.

Remove the property from did core

-1 because doing so will give the wrong impression, that publicKeyJwk is the ONLY way to express key material when we already have examples in the ecosystem (Ed25519, BBS+, EthereumEip712Signature2021) where that's not the case.

Our developers were surprised that: const edKeyPair = await Ed25519VerificationKey2020.generate(); generated a private key, that is used for signing in the suite.

Also, to be clear -- I think it's tragic that our libraries do that... it's a footgun, and unfortunately, it's going to take minor surgery to undo that footgun at this point... but to be clear, that's our mistake for doing something so confusing. What we really should have done was something to the effect of Ed25519KeyPair.generate({version: "2020"}) and then perhaps a .toVerificationKey() for inclusion into a DID Document. Expect this change in a future breaking release, our apologies for the footgun'ing developer confusion this is causing.

OR13 commented 3 years ago

@oed

@OR13 Fyi, multicodec specifies that public keys should be compressed:

but multibase does not!?!?... this is making the point even clearer.

OR13 commented 3 years ago

-1 because doing so will give the wrong impression, that publicKeyJwk is the ONLY way to express key material when we already have examples in the ecosystem (Ed25519, BBS+, EthereumEip712Signature2021) where that's not the case.

It might give the impression that publicKeyJwk is the only STANDARD way to express verification material, and that any other way relies on did-spec-registries for interoperability.... If type is so important, and we already rely on the registry for them... I don't think this is as bad as it might look.

+1 to this approach, which I believe this PR does. To be clear, I'm supportive of this PR.

Regarding multicodec warning changes, here is some revised language based on the thread, @msporny would you mind helping me smith it out:

publicKeyMultibase might rely on multicodec or might rely on a pairing with type defined externally to determine the specific format of the key material. publicKeyMultibase might be paired with privateKeyMultibase or might be paired with privateKeyBase58 and type to determine the private representation of the public material present in the did document. The suite that defines type should define which of these optional approaches should be used for a given type.

oed commented 3 years ago

Well, then your implementations will be more dangerous than others and I wouldn't be surprised if you get issues raised against your library doing dangerous things (like presuming the type from the multicodec value and not the other way around) or developers choosing to avoid your implementations. :)

I mean in the implementations of my DID methods the underlying data structure just uses multicodec to represent public keys. Then uses the information in there to generate the DID document deterministically.

but multibase does not!?!?... this is making the point even clearer.

Multibase doesn't say anything about what the data itself is!

OR13 commented 2 years ago

@msporny @peacekeeper can you please review?

OR13 commented 2 years ago

@oed do you think we are making a terrible mistake by using publicKeyMultibase instead of publicKeyMulticodec?

Do we believe this will result in 2 non interoperable representations of secp256k1 for example?

oed commented 2 years ago

@OR13 if the intention is to specify what the actual representation is in the "type" then I guess that's ok. I think it's redundant, but w/e.

msporny commented 2 years ago

@OR13 if the intention is to specify what the actual representation is in the "type" then I guess that's ok. I think it's redundant, but w/e.

Yes, that's the intention. The encoding beyond the multibase encoding is signalled via type. We've already implemented this across the board for our Ed25519*2020 keys/signatures/etc... and if people hate this, we can always change it to something else in future cryptosuites (like we did for publicKeyBase58).

OR13 commented 2 years ago

@msporny accepted changes, pinged for re-review.

kdenhartog commented 2 years ago

related side note, are we planning to update json-ld suite contexts to support publicKeyMultibase (and publicKeyJwk)?

msporny commented 2 years ago

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