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 publicKeyMultibase. #94

Merged
merged 1 commit into from
May 16, 2021
Merged

Add support for publicKeyMultibase. #94

merged 1 commit into from
May 16, 2021

Conversation

msporny
Copy link
Member

@msporny msporny commented May 8, 2021

This PR is in support of addressing w3c/did-core#707 via PRs w3c-ccg/security-vocab#103, w3c/did-spec-registries#299, and w3c/did-core#731 based on the plan put forth in w3c/did-core#707 (comment).

It should be merged last, after all the other PRs have been merged.

@@ -141,16 +141,25 @@ const generateDidCorePropertiesTests = (
it('5.2.1 Verification Material - A verification method MUST NOT ' +
'contain multiple verification material properties for the same ' +
'material. For example, expressing key material in a verification method ' +
'using both publicKeyJwk and publicKeyBase58 at the same time is ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

related to w3c/did-core#731

@OR13
Copy link
Contributor

OR13 commented May 11, 2021

@msporny see also multiformats/multicodec#210

We can't support publicKeyMultibase for a key type until we can represent the private key component... currently Ed25519 is the only key type to have a private key representation...

I expect over developers will be bothered enough by this to delay upgrading, and would welcome PRs to fix it.

@msporny
Copy link
Member Author

msporny commented May 11, 2021

We can't support publicKeyMultibase for a key type until we can represent the private key component.

This is factually incorrect. One does not preclude the other.

You can support publicKeyMultibase without having a privateKeyMultibase; Digital Bazaar does this without issue today. You don't need a public vocabulary entry to express a private key.

That said. PRs on the security vocab for privateKeyMultibase are welcome... and PRs for DANGEROUS PRIVATE FOOTGUN CONTEXTS that are separate from the public contexts are welcome.

@OR13
Copy link
Contributor

OR13 commented May 12, 2021

@msporny

so you do this:

publicKeyMultibase: '...'
privateKeyBase58: '...' // no defined semantics

?

Of course this can be done... by point is that you cannot produce privateKeyMultibase if no encoding of a private key is possible for the key type due to no registry entry for it.

@msporny
Copy link
Member Author

msporny commented May 12, 2021

Of course this can be done... by point is that you cannot produce privateKeyMultibase if no encoding of a private key is possible for the key type due to no registry entry for it.

I feel like you could be trying to make a variety of different points, but it's not clear which one (or what you want us to do about it)... what point are you trying to make? What action would you like those reading this thread to take based on your point?

I suggest:

  • You register privateKeyMultibase in the Security Vocabulary (with a big giant warning that people shouldn't put this in contexts that are expected to be used for public data... like VC Extensions, DID Extensions, etc.)
  • You register whatever multicodec extensions you want for private keys in the multicodec table.
  • You publish a JSON-LD context as an example that is for private use only that expresses privateKeyMultibase with giant warnings to not use it in data published in public systems.

Would the above address your concerns?

NOTE: I continue to believe that all three items above are a terrible idea, and you're going to inflict a lot of pain on the industry by doing this, but also admit that if you don't do it, someone else will come along and do it (because JSON-LD has decentralized semantics, so no one can stop anyone from doing anything, no matter how dangerous the idea is -- FREEDOM! * screamed in william wallace *).

@peacekeeper
Copy link
Contributor

multicodec table

multibase table?

@OR13
Copy link
Contributor

OR13 commented May 12, 2021

@msporny you nailed all it, thats exactly what I intend to do.

But I was asking what you are planning, will you use privateKeyMultibase or only privateKeyBase58 ?

@msporny
Copy link
Member Author

msporny commented May 12, 2021

multicodec table

multibase table?

No, multicodec table:

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

@peacekeeper
Copy link
Contributor

peacekeeper commented May 12, 2021

Why does multicodec come into play here? My understanding is that the values of publicKeyMultibase and privateKeyMultibase use only multibase, not multicodec.

@msporny
Copy link
Member Author

msporny commented May 12, 2021

But I was asking what you are planning, will you use privateKeyMultibase or only privateKeyBase58 ?

We won't use privateKeyBase58 ... I'm going to lobby internally that we don't use privateKeyMultibase from a JSON-LD Context either (but DB makes many of it's important internal technical decisions by consensus, so I might lose that battle).

We don't export private keys to other systems as a general security principle. We may do it in the future if our customers request the feature, but we will try very hard to talk them out of it and to use key rotation in DID Documents instead.

We need to move the industry beyond this really broken concept of exporting private key material.

@msporny
Copy link
Member Author

msporny commented May 12, 2021

Why does multicodec come into play here? My understanding is that the values of publicKeyMultibase and privateKeyMultibase use only multibase, not multicodec.

The multibase-encoded byte string has a header for not only the base encoding, but the key type as well. So the format can be:

<MULTIBASE_HEADER><KEY_TYPE_HEADER><KEY_BYTES>

I say "can" because cryptosuites can choose to do something else... like not put the key type header on there.

@OR13
Copy link
Contributor

OR13 commented May 12, 2021

@msporny you do use privateKeyMultibase....

https://github.com/digitalbazaar/ed25519-verification-key-2020/blob/main/lib/Ed25519VerificationKey2020.js#L199

/**
   * Exports the serialized representation of the KeyPair
   * and other information that JSON-LD Signatures can use to form a proof.
   *
   * @param {object} [options={}] - Options hashmap.
   * @param {boolean} [options.publicKey] - Export public key material?
   * @param {boolean} [options.privateKey] - Export private key material?
   * @param {boolean} [options.includeContext] - Include JSON-LD context?
   *
   * @returns {object} A plain js object that's ready for serialization
   *   (to JSON, etc), for use in DIDs, Linked Data Proofs, etc.
   */
  export({publicKey = false, privateKey = false, includeContext = false} = {}) {
    if(!(publicKey || privateKey)) {
      throw new TypeError(
        'Export requires specifying either "publicKey" or "privateKey".');
    }
    const exportedKey = {
      id: this.id,
      type: this.type
    };
    if(includeContext) {
      exportedKey['@context'] = Ed25519VerificationKey2020.SUITE_CONTEXT;
    }
    if(this.controller) {
      exportedKey.controller = this.controller;
    }
    if(publicKey) {
      exportedKey.publicKeyMultibase = this.publicKeyMultibase;
    }
    if(privateKey) {
      exportedKey.privateKeyMultibase = this.privateKeyMultibase;
    }
    if(this.revoked) {
      exportedKey.revoked = this.revoked;
    }
    return exportedKey;
  }

@peacekeeper
Copy link
Contributor

peacekeeper commented May 12, 2021

@msporny I know that the format can be like that with multicodec, but the discussion around publicKeyMultibase so far has been about using multibase only, not multicodec, i.e. the format would be:

<MULTIBASE_HEADER><KEY_BYTES>

Not saying that I find multicodec good or bad, but to my knowledge it simply hasn't been part of the discussion so far.

See also comments here: w3c/did-core#707 (comment)

@msporny
Copy link
Member Author

msporny commented May 12, 2021

@msporny you do use privateKeyMultibase....

Note that I said:

I'm going to lobby internally that we don't use privateKeyMultibase * from a JSON-LD Context *

You'll note that we don't specify privateKeyMultibase in the JSON-LD Context:

https://github.com/digitalbazaar/ed25519-signature-2020-context/blob/master/contexts/ed25519-signature-2020-v1.jsonld

Our libraries also take other protections, like ensuring that if you JSON stringify that object, that you don't accidentally print the private key material to the outgoing string. Again, we're trying to balance being pragmatic (sometimes you need to generate a private key and convert the private key and thus need access to the private key material on the same system).

Exporting it from that system to some other system as privateKeyMultibase to elsewhere on the Internet in clear text, or serializing the private key to disk without encrypting it, are the sorts of foot guns we're trying to avoid. Ideally, these would all be keys in HSMs and there would be no option to export.

@peacekeeper
Copy link
Contributor

If the value of publicKeyMultibase includes multicodec in addition to multibase, then the question arises how the <KEY_TYPE_HEADER> in the property value relates to the verification method type. I guess we could define a generic verification method type that re-uses the multicodec table, e.g.:

{
  "@context": "https://www.w3.org/ns/did/v1",
  "id": "did:example:123456789abcdefghi",
  "authentication": [{
    "id": "did:example:123456789abcdefghi#keys-1",
    "type": "MulticodecKey2021",
    "controller": "did:example:123456789abcdefghi",
    "publicKeyMultibase": "<MULTIBASE_HEADER><KEY_TYPE_HEADER><KEY_BYTES>"
  }]
}

That might be useful, but could also lead to discussions about cryptographic agility, similar to publicKeyJwk.

@msporny
Copy link
Member Author

msporny commented May 12, 2021

MulticodecKey2021

We should not do this for the same reason that we shouldn't repeat JWKs mistakes. If you're going to express a key type, be specific about it. We don't want MulticodecKey2021 because it imports a gigantic attack surface (that is always getting larger).

@msporny
Copy link
Member Author

msporny commented May 12, 2021

but the discussion around publicKeyMultibase so far has been about using multibase only, not multicodec

Ultimately, this is a decision for each cryptosuite to make. It's an added 1-2 byte protection to make sure the developer hasn't accidentally shoved the wrong key type into the field.

@peacekeeper
Copy link
Contributor

peacekeeper commented May 12, 2021

We don't want MulticodecKey2021

Agreed.. Then why would someone ever want to include a value like <MULTIBASE_HEADER><KEY_TYPE_HEADER><KEY_BYTES> instead of just <MULTIBASE_HEADER><KEY_BYTES> in publicKeyMultibase / privateKeyMultibase ?

@msporny
Copy link
Member Author

msporny commented May 12, 2021

Agreed.. Then why would someone ever want to include a value like <MULTIBASE_HEADER><KEY_TYPE_HEADER><KEY_BYTES> instead of just <MULTIBASE_HEADER><KEY_BYTES> in publicKeyMultibase / privateKeyMultibase?

See #94 (comment).

@peacekeeper
Copy link
Contributor

Ultimately, this is a decision for each cryptosuite to make. It's an added 1-2 byte protection to make sure the developer hasn't accidentally shoved the wrong key type into the field.

Okay.. I feel like it might be a bit better to standardize that for the property itself rather than delegating it to cryptosuites, but can live with this as well :) Thanks for explanations.

@OR13
Copy link
Contributor

OR13 commented May 12, 2021

fyi, we will be / are doing MulticodecKey2021 as an alternate encoding for all IANA registered JWK kty, crv... this is one of those things that we won't agree on, but where each side as good points, and they should be communicated clearly.

Just because root accounts are bad to use for everything does not mean they should not exist...

for us, we see migration from JWK as only happening incrementally, and we think that adding 1-1 feature coverage is part of enabling that upgrade... we think not doing that is dooming publicKeyMultibase to essentially a slow death.

@peacekeeper
Copy link
Contributor

we will be / are doing MulticodecKey2021

I'm not really against this either, I just wonder if in this case it wouldn't be better to define both publicKeyMultibase and publicKeyMulticodec separately, so that the format of the property value is always consistent across verification method types.

@OR13
Copy link
Contributor

OR13 commented May 13, 2021

@peacekeeper I guess this is mostly building off work that started here:

https://github.com/w3c-ccg/did-method-key

https://w3c-ccg.github.io/lds-ed25519-2020/

Possible we named publicKeyMultibase incorrectly!

@msporny
Copy link
Member Author

msporny commented May 13, 2021

I'm not really against this either, I just wonder if in this case it wouldn't be better to define both publicKeyMultibase and publicKeyMulticodec separately, so that the format of the property value is always consistent across verification method types.

The real name could have been publicMultikey ... and we were waiting for that discussion to settle: ipfs/specs#58

... and that discussion hasn't settled yet. publicKeyMulticodec might be closer, but then it says nothing about the base encoding... so perhaps publicKeyMulticodecMultibase, which is ridiculously long... so we can't use that. There are arguments for/against all of this stuff. We settled on publicKeyMultibase because it is a public key that is encoded in multibase... and thus COULD be raw if people wanted it to be or COULD put a multicodec header on there... again, if the cryptosuite deems it to be the right thing to do.

In the end, we deferred to the cryptosuites on the final call here and the property makes no assertion to the inner format other than it being encoded as multibase.

@kdenhartog
Copy link
Member

But I was asking what you are planning, will you use privateKeyMultibase or only privateKeyBase58 ?

We won't use privateKeyBase58 ... I'm going to lobby internally that we don't use privateKeyMultibase from a JSON-LD Context either (but DB makes many of it's important internal technical decisions by consensus, so I might lose that battle).

We don't export private keys to other systems as a general security principle. We may do it in the future if our customers request the feature, but we will try very hard to talk them out of it and to use key rotation in DID Documents instead.

We need to move the industry beyond this really broken concept of exporting private key material.

If it makes any difference in your argument to them because of the way the JS engine garbage collects and shallow copies strings, it's much harder to manage how long a private key remains in memory when compared with a Buffer which can be overwritten with all 0s once you've finished use of the key within the scope of the function where you create the variable.

@OR13
Copy link
Contributor

OR13 commented May 14, 2021

If only there was a standard for encrypted a private key before exporting it as a string.... oh wait... https://datatracker.ietf.org/doc/html/rfc2440#section-3.6

Our PGP suite supports this: https://github.com/OR13/lds-pgp2021/blob/main/src/PgpKeyPair2021.ts#L23

It introduces another set of challenges... now you need to remember passwords somewhere...

This feels a bit like arguing that gasoline is dangerous, but also used to drive cars, so the solution is to not use cars that run on gas... or banning knives because they can be used to kill people in addition to cutting up food so you don't choke.... how exactly are you going to cold storage your crypto without exporting a private key or its generator to paper?

There are many legitimate reasons for being able to export a private key, and yes, encrypting before exporting is a best practice... perhaps we should register a multcodec for "encrypted" private key?... under what scheme? with what cipher?

@msporny
Copy link
Member Author

msporny commented May 14, 2021

If only there was a standard for encrypted a private key before exporting it as a string.... oh wait... https://datatracker.ietf.org/doc/html/rfc2440#section-3.6

It's telling that you link to a security specification from 1998 to make a point :) -- that's exactly the sort of thinking we need to stamp out. I can point further back to ASN.1 expression of private keys... that we /can/ do something doesn't mean we should. Remember, 1998 was a time where there really weren't the sorts of practices we have around private key management (like FIPS certified HSMs) that we have today.

Our PGP suite supports this: https://github.com/OR13/lds-pgp2021/blob/main/src/PgpKeyPair2021.ts#L23

Passphrase is optional? That's a nice footgun you have there... I can't wait until our engineers pick up your library and blow our leg off with it. :)

It introduces another set of challenges... now you need to remember passwords somewhere...

Yeah... the whole "let's export private keys" is riddled with all sorts of problems.

This feels a bit like arguing that gasoline is dangerous, but also used to drive cars, so the solution is to not use cars that run on gas...

The solution is exactly not to use cars that run on gas... that's why the world is moving to electric, because gasoline-powered cars are not sustainable. In the same vein, exporting private keys are not sustainable... we can do much, much better.

or banning knives because they can be used to kill people in addition to cutting up food so you don't choke.... how exactly are you going to cold storage your crypto without exporting a private key or its generator to paper?

There are well known security industry standards related to doing this that don't require either of the solutions you've put forward.

Raw private keys don't need to leave devices... systems that allow private keys to leave devices are fundamentally less secure than ones that don't. We have created a mechanism, in DIDs, that allow us to rotate ourselves out of this problem instead of carrying it forward to the future. It doesn't work in every case, that's true, but we really need to start treating this as a dangerous exception instead of standard practice.

There are many legitimate reasons for being able to export a private key, and yes, encrypting before exporting is a best practice...

There are very few legitimate reasons... those reasons just happen to be that HSMs are expensive and out of reach for most people... I'd rather we fix that problem than keep focusing on creating footguns.

perhaps we should register a multcodec for "encrypted" private key?... under what scheme? with what cipher?

Sounds like a job for standards... :)

@msporny
Copy link
Member Author

msporny commented May 16, 2021

All multibase PRs (w3c/did-core#707 via PRs w3c-ccg/security-vocab#103, w3c/did-spec-registries#299, and w3c/did-core#731) have been merged. Merging this to reflect the updates to the spec.

@msporny msporny merged commit 9ddb948 into main May 16, 2021
@OR13
Copy link
Contributor

OR13 commented May 17, 2021

It's telling that you link to a security specification from 1998 to make a point :)

just because its old doesn't mean we need to reinvent it every 2 years... crypto standards are not js front end frameworks :)

There are well known security industry standards related to doing this that don't require either of the solutions you've put forward.

Sorry, what standard approaches to exporting private keys are there other than exporting them as plaintext or cipher text?

Raw private keys don't need to leave devices...

But what if this assumption is wrong? and what do you mean by "leave" you generated on a device from a seed, but let the seed move?

systems that allow private keys to leave devices are fundamentally less secure than ones that don't.

100% agree

@msporny msporny deleted the msporny-add-multibase branch June 18, 2021 15:17
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.

None yet

4 participants