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

Remove Ed25519 -> X25519 conversion from spec #39

Open
OR13 opened this issue Sep 23, 2021 · 20 comments
Open

Remove Ed25519 -> X25519 conversion from spec #39

OR13 opened this issue Sep 23, 2021 · 20 comments

Comments

@OR13
Copy link
Contributor

OR13 commented Sep 23, 2021

This would eliminate concern regarding the security of the operation.

and make ed25519 keys like every other key type.

@dmitrizagidulin
Copy link
Collaborator

@OR13 is the proposal to not have the keyAgreementKey purpose, in did:key DID docs?

@mprorock
Copy link

big fan of this - would simplify things and improve security

@dmitrizagidulin
Copy link
Collaborator

dmitrizagidulin commented Sep 23, 2021

@OR13 @mprorock Would making the keyAgreementKey generation part optional solve your concerns?

@dmitrizagidulin
Copy link
Collaborator

On further consideration, I'd be +1 to removing keyAgreement section from Ed25519-based did:key documents.
(In case people need ephemeral DID-based key agreement, they can generate an X25519 based did:key directly.)

@OR13
Copy link
Contributor Author

OR13 commented Sep 28, 2021

^ exactly... for example, here is how we generate X25519 keys today:

{
  "@context": [
    "https://www.w3.org/ns/did/v1",
    "https://w3id.org/security/suites/jws-2020/v1"
  ],
  "id": "did:key:z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7",
  "verificationMethod": [
    {
      "id": "did:key:z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7#z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7",
      "type": "JsonWebKey2020",
      "controller": "did:key:z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7",
      "publicKeyJwk": {
        "kty": "OKP",
        "crv": "X25519",
        "x": "xlONxp-pXBxR1vErZ9Mywv1GLXLzxOK5IsdloQkNckw"
      }
    }
  ],
  "keyAgreement": [
    "did:key:z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7#z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7"
  ]
}

Here would be the new structure for Ed25519:

{
  "@context": [
    "https://www.w3.org/ns/did/v1",
    "https://w3id.org/security/suites/jws-2020/v1"
  ],
  "id": "did:key:z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR",
  "verificationMethod": [
    {
      "id": "did:key:z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR#z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR",
      "type": "JsonWebKey2020",
      "controller": "did:key:z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR",
      "publicKeyJwk": {
        "kty": "OKP",
        "crv": "Ed25519",
        "x": "FmdIIZE4B0glkSA_Fd_SdAoaVSRSD8Js5BRMXmhDr-A"
      }
    }
  ],
  "assertionMethod": [
    "did:key:z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR#z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR"
  ],
  "authentication": [
    "did:key:z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR#z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR"
  ],
  "capabilityInvocation": [
    "did:key:z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR#z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR"
  ],
  "capabilityDelegation": [
    "did:key:z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR#z6MkfxiMhhrtPtWpAaCu2nmMY1wtUe2VYja3Y8hRAhZ4qHVR"
  ]
}

@OR13
Copy link
Contributor Author

OR13 commented Sep 28, 2021

imo how you derive a public key or private key is out of scope for the spec.... that includes curve conversion, mnemonics, or key generation functions that take passwords.

@dlongley
Copy link
Contributor

dlongley commented Sep 28, 2021

@mprorock,

big fan of this - would simplify things and improve security

For the use cases that have no need of an integrated key agreement key and for those use cases where its mere presence may cause some regulatory trouble (unknown), it would simplify things. For use cases where an integrated key agreement key is beneficial, it will complicate things.

Regarding "improve security", however, I have to disagree, if the current literature is to be believed (that sharing the same key for both ed25519 and x25519 operations has been proven secure). Sure, it may be the case that certain governments would not accept the same key to be used for both of these operations, but that is merely a matter of current policy.

@dmitrizagidulin,

(In case people need ephemeral DID-based key agreement, they can generate an X25519 based did:key directly.)

Being able to generate a key is not the same thing as having a verification relationship in a DID document authorizing its use for a specific purpose. Removing keyAgreement would be removing that statement and people should understand the consequences of that: it either requires did:key DIDs and DID documents to be treated differently from other methods that support keyAgreement (when keyAgreement is in use by an application) or it requires keyAgreement to be ignored in DID documents from all DID methods because some other method of authorization is put into place.

A large part of the interoperability story around DIDs is that applications can treat the DID documents the same, regardless of the method used. This provides a separation of concerns: use a trusted DID resolver and once you've got a DID document, you can rely on its verification relationships and methods from there and ignore the DID method.

@dlongley
Copy link
Contributor

To be clear on my position here:

If the use cases just aren't there for key agreement keys for ed25519 did:key DIDs, then removing keyAgreement makes sense -- provided that x25519 did:key DIDs are created to cover the key agreement use cases. It does necessarily mean that any binding between an ed25519 did:key and an x25519 did:key will have to be done separately -- which would not be necessary for any other DID method that supports keyAgreement (my interop point above stands and it should be considered carefully, IMO).

@dlongley
Copy link
Contributor

dlongley commented Sep 28, 2021

A use case that needs to be considered here (and whether or not did:key will be an acceptable DID method for it) is:

You are required to use a single DID for which you can both authenticate (or take an action, i.e., invoke a capability) and do encrypted communications/data.

The fact that did:key only supports one key is incidental here, the application doesn't really care. It only cares about there being a single DID. It's just that, if you want to use did:key in this use case, it will be the same underlying key. Another DID method may not have that constraint.

So -- are there use cases such as the one above where did:key will no longer fit in with other DID method options? My understanding was that the whole DIDComm approach was such a use case and, if so, it would further constrain one's choice of DID method when using DIDComm. I could be wrong about this, however.

@dlongley
Copy link
Contributor

Another option to consider, would be to register another public key type in the multicodec table that means: "an ed25519 public key that can be transformed using a birational map for use as an x25519 key". So there would be a different did:key option for those that need to use keyAgreement with their DID in a way that interoperates with other DID methods. The codec choices for creating a 25519-style did:key would then be ed25519, x25519, and ed25519+x25519.

@OR13
Copy link
Contributor Author

OR13 commented Sep 28, 2021

yep, if the multicodec says "ed25519 public key that will be converted to x25519 public key" thats fine... but still likely to trigger security folks who don't want to see that happening.

worth noting that RSA / P-256 and other key types already support both key agreement and signatures... using "snowflake" did key formats should be optional imo...

another option is to do the conversion before encoding the did, and handle this like we handle bls12381 g1 + g2.

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

@dmitrizagidulin
Copy link
Collaborator

@OR13

worth noting that RSA / P-256 and other key types already support both key agreement and signatures

Waiiit, the whole point here is that this conversation got started because of the security "best practice" that one should not use the same key for both purposes. (Or derive the key, either.)
So if we shouldn't do that with Ed25519 keys, then same applies to RSA and P-256.

@OR13
Copy link
Contributor Author

OR13 commented Oct 4, 2021

@dmitrizagidulin

So if we shouldn't do that with Ed25519 keys, then same applies to RSA and P-256.

No, because we are not using Ed25519 for keyAgreement today, we are converting to X25519 and then using that key...

You can't have it both ways...

Either you think it's a good idea to have a key for a single purpose, or you think it's a good idea to be honest about what purposes a key can support.

I think we should expose what is supported by a SINGLE KEY TYPE, and nothing more....

I would also be ok making did key look like this:

{
  "@context": [
    "https://www.w3.org/ns/did/v1",
    "https://w3id.org/security/suites/jws-2020/v1"
  ],
  "id": "did:key:z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7",
  "verificationMethod": [
    {
      "id": "did:key:z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7#z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7",
      "type": "JsonWebKey2020",
      "controller": "did:key:z6LSq2MnTYfm1MqSV6uy9d2DTLLAvNqU1Bwn5u6SrhXGePQ7",
      "publicKeyJwk": {
        "kty": "OKP",
        "crv": "X25519",
        "x": "xlONxp-pXBxR1vErZ9Mywv1GLXLzxOK5IsdloQkNckw"
      }
    }
  ],
}

since interpreting 'verification relationships as purposes' and then allowing everything that is possible to be expressed seems to be a security pattern failure... or a reflection of practical reality... (ED25519 for assertion and authentication).

since did key is deterministic, and multiformats don't encode purposes in the key type (hmm i wonder why?).... we would appear to have 2 options:

  1. a did key representation will encode all possible registered purposes.
  2. a did key representation will not encode purposes.

since 2 would make did:key useless, we are left with 1.

Ed25519 should be allowed for keyAgreement IF and ONLY IF, it's possible to use it for that purpose.

NEW_KEY_TYPE should be allowed for PURPOSE IF and ONLY IF, it's possible to use it for that purpose.

Converting between types for specific keys should not be a legal way to circumvent 1... its complexity, attack surface, and implementation burden.

@dlongley
Copy link
Contributor

dlongley commented Oct 4, 2021

@OR13,

No, because we are not using Ed25519 for keyAgreement today, we are converting to X25519 and then using that key...

You can't have it both ways...

Hmm, to me, calling it a different key and saying that there's a security-related concern because it's the same key is having it both ways.

Factually, it is the same key, just represented differently (a point on one curve vs. a point on another birationally equivalent one). It is precisely the fact that it is the same key that is driving the security-related concern from some people (though there's a security proof suggesting the concern is unwarranted). If it were actually a different key, then it would be the "snowflake" (as you describe) but the security-related concern would be dropped.

Perhaps the argument is that it's a snowflake because the representation changes. Maybe that's what you were going for. But I don't think we should say that having a different representation for key agreement makes something a "snowflake" that should be discouraged. We don't know what kind of curves/other math primitives might be of use in the future to make the same key material be used more efficiently in different protocols.

@dlongley
Copy link
Contributor

dlongley commented Oct 4, 2021

@OR13,

Ed25519 should be allowed for keyAgreement IF and ONLY IF, it's possible to use it for that purpose.

NEW_KEY_TYPE should be allowed for PURPOSE IF and ONLY IF, it's possible to use it for that purpose.

Converting between types for specific keys should not be a legal way to circumvent 1... its complexity, attack surface, and implementation burden.

If we can get an ed25519+x25519 key type into multiformats, then I think that's the path forward here as it would work with the above and give us the consistency we want.

@OR13
Copy link
Contributor Author

OR13 commented Oct 4, 2021

Hmm, to me, calling it a different key and saying that there's a security-related concern because it's the same key is having it both ways.

Sure, but thats an argument to have with DJB and the folks who decided Ed25519 != Curve25519... they are different keys, their bytes are different.... they have different id values... we need them to, in order to use them properly...

@dlongley

do you think ed25519+x25519 should be encoded as ed25519 with allowed derivations?

or should it be encoded as ed25519 pub + x25519 pub...

this matters if resolvers refuse to implement derivation, the latter representation would still allow them to resolve... and... it would also support:

seed 1 - > Ed25519 k1
seed 2 -> X25519 k2

did:key:(ed25519+x25519) + k1 + k2

Whereas today:

seed 1 - > Ed25519 k1 -> X25519 k2 is happening.... this would probably be valuable when working with HSMs.

@dlongley
Copy link
Contributor

dlongley commented Oct 5, 2021

@OR13,

Hmm, we may end up needing to support both approaches but perhaps we should only start out with the first one. We should probably use a different multicodec value for each one (or include some value that indicates whether there's more than one set of key bytes). The latter approach would probably have the benefits you suggest, but I'd be concerned about having to think through all of the implications of people starting to use N many keys in did:key DIDs.

@tmarkovski
Copy link

The ephemeral nature of this method favors convenience features like this, when security measures allow. In that sense, I do support the idea of having key agreement in ed25519.

@OR13
Copy link
Contributor Author

OR13 commented Feb 1, 2022

But the method is weakened by "snowflake edge cases"... you still have the ability to use X25519 keys with did:key... and you still have the ability to do key agreement and signature with P-384.

Security reviews of the method will object to these edge cases... as they should.

@OR13
Copy link
Contributor Author

OR13 commented Sep 19, 2022

We will be removing key conversion from our libraries shortly.

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

No branches or pull requests

5 participants