feat(identity): implement protobuf encoding for all key types

[drHuangMHT]

Description

Previously, we only supported encoding the ed25519 private key. To be fully compliant with the spec, we also need to support encoding the other 3 key types ECDSA, RSA and secp256k1.

Resolves #3630. Related #3623. Related #3350. Related #3649.

Notes & open questions

Breaking change. ~~Should we make the code panic when encountering missing feature?~~ Blocked by #540.

Change checklist

• [ ] I have performed a self-review of my own code
• [ ] I have made corresponding changes to the documentation
• [ ] I have added tests that prove my fix is effective or that my feature works
• [ ] A changelog entry has been made in the appropriate crates

[drHuangMHT]

Now I don't expect the branch to be merged with such aggressive changes......

[thomaseizinger]

I merged latest master because there was some diff of other PRs.

[drHuangMHT]

rsa::Keypair may be parsed from DER encoding, which cannot be transformed to pkcs#8 encoding, making it impossible to be deserialized from protobuf encoding. What to do?

[drHuangMHT]

I think I'm almost there. Ping @thomaseizinger for further review.

[thomaseizinger]

rsa::Keypair may be parsed from DER encoding, which cannot be transformed to pkcs#8 encoding, making it impossible to be deserialized from protobuf encoding. What to do?

This is a bit tricky. I see the following options:

1. Drop support for decoding from just DER and only support what is required in the spec. This is the easiest but also the most limiting.
2. Switch to a different RSA implementation that allows us to export the key: https://github.com/libp2p/rust-libp2p/issues/1133
3. Manually convert from DER to PKCS#8 as we import from DER so we can export it correctly afterwards.
4. Get ring to allow us to export key.

(2) is tricky because I don't know about the security implications. There has been an audit of the rsa crate but I am not sure it is sufficient. RSA is very tricky and prone to side-channels attacks. I trust ring more than I do the rsa crate so I'd rather not switch for now.

(4) might be possible but we'd have to lobby upstream, this takes time and will block this PR, which I wouldn't want.

(3) is possible but requires a lot of code in here only to allow importing a key from a different format. It is essentially the same as (1) where the external library has to convert their DER bytes to PKCS#8.

From the above, I would suggest we do (1) and require applications / other libraries that need to import from different crates to convert to our supported format. You can also open the issue with ring in the meantime and if we ever get support for exporting the key, we can drop our hack of storing the bytes and offer more decoding functions. Same applies if we ever switch to rsa.

Are you okay with this way of moving forward @drHuangMHT?

[thomaseizinger]

One final feature request: Can you generate a set of test vectors for each key type? In other words, generate a random key for each type and encode it to protobuf.

I want to submit them to the specs repository, see https://github.com/libp2p/specs/issues/533 and would like to check with the go-libp2p implementation before we merge this!

[drHuangMHT]

From the above, I would suggest we do (1) and require applications / other libraries that need to import from different crates to convert to our supported format. You can also open the issue with ring in the meantime and if we ever get support for exporting the key, we can drop our hack of storing the bytes and offer more decoding functions. Same applies if we ever switch to rsa.

Are you okay with this way of moving forward @drHuangMHT?

I'm OK with this, but I found #224 and #579 in ring. Seems that ring simply don't want to "leak" private keys in any way.

[drHuangMHT]

One final feature request: Can you generate a set of test vectors for each key type? In other words, generate a random key for each type and encode it to protobuf.

I found that crate ecdsa supports keys generated with secp256k1 curve, so I'm not sure why secp256k1 gets it's own implementation.

[thomaseizinger]

One final feature request: Can you generate a set of test vectors for each key type? In other words, generate a random key for each type and encode it to protobuf.

I found that crate ecdsa supports keys generated with secp256k1 curve, so I'm not sure why secp256k1 gets it's own implementation.

I think at the time of writing this initial implementation, the ecdsa crate didn't exist.

[thomaseizinger]

From the above, I would suggest we do (1) and require applications / other libraries that need to import from different crates to convert to our supported format. You can also open the issue with ring in the meantime and if we ever get support for exporting the key, we can drop our hack of storing the bytes and offer more decoding functions. Same applies if we ever switch to rsa. Are you okay with this way of moving forward @drHuangMHT?

I'm OK with this, but I found #224 and #579 in ring. Seems that ring simply don't want to "leak" private keys in any way.

Thanks for researching. That still leaves the path of eventually migrating to rsa so not all hope is lost. Great that we have a path forward!

[drHuangMHT]

I think at the time of writing this initial implementation, the ecdsa crate didn't exist.

Should we deprecate it in another PR?

[drHuangMHT]

I found that crate ecdsa supports keys generated with secp256k1 curve, so I'm not sure why secp256k1 gets it's own implementation.

I think at the time of writing this initial implementation, the ecdsa crate didn't exist.

Ah I made a mistake. libp2p-identity does not directly depend on ecdsa but p256. And the crate only supports secp256r1 keys. Should we move to directly depending on ecdsa?

[thomaseizinger]

I found that crate ecdsa supports keys generated with secp256k1 curve, so I'm not sure why secp256k1 gets it's own implementation.

I think at the time of writing this initial implementation, the ecdsa crate didn't exist.

Ah I made a mistake. libp2p-identity does not directly depend on ecdsa but p256. And the crate only supports secp256r1 keys. Should we move to directly depending on ecdsa?

I haven't fully thought through the implications. I guess our dependency tree would get smaller? Feel free to send a separate PR. I'd like to keep this one focused on the protobuf changes.

[thomaseizinger]

@drHuangMHT I've updated the PR description as well. As this PR is becoming fairly large, I was wondering whether you'd be up for splitting it into multiple. For example, some of the function renames like into_ed25519 to try_intoed25519` could easily be done in a separate PR.

That would allow us to focus on the actual encoding implementation in here.

Let me know what you think.

[drHuangMHT]

@drHuangMHT I've updated the PR description as well. As this PR is becoming fairly large, I was wondering whether you'd be up for splitting it into multiple. For example, some of the function renames like into_ed25519 to try_intoed25519` could easily be done in a separate PR.

That would allow us to focus on the actual encoding implementation in here.

Let me know what you think.

Sure. But I don't know how to "split" existing commits though, and I don't think it's possible on existing commits. However reverting some commits might be possible.
And for pkcs#8 encoding we need to directly depend on ecdsa to get access to provided trait implementation for encoding to and decoding from pkcs#8, so I'm afraid that this commit has to be blocked by that.

[drHuangMHT]

I can split the commits manually but that does need some time as a penalty for me being careless. So what about closing this PR later @thomaseizinger ?

[thomaseizinger]

I can split the commits manually but that does need some time as a penalty for me being careless. So what about closing this PR later @thomaseizinger ?

We don't need to close it later! :)

What we can do is create a 2nd PR with a subset of the current changes. Given that they are fairly isolated, what I would do is:

• Open the respective file in my editor
• Select all and copy
• Checkout a new branch based off master
• Paste
• Make minor adjustments manually
• Make a commit and push

Once that other PR is merged into master, we can just update this PR by merging master in. The changes are then simply going to disappear from this PR :)

Let me know if you need any help with that!

[thomaseizinger]

@drHuangMHT I've updated the PR description as well. As this PR is becoming fairly large, I was wondering whether you'd be up for splitting it into multiple. For example, some of the function renames like into_ed25519 to try_intoed25519` could easily be done in a separate PR. That would allow us to focus on the actual encoding implementation in here. Let me know what you think.

Sure. But I don't know how to "split" existing commits though, and I don't think it's possible on existing commits. However reverting some commits might be possible. And for pkcs#8 encoding we need to directly depend on ecdsa to get access to provided trait implementation for encoding to and decoding from pkcs#8, so I'm afraid that this commit has to be blocked by that.

Sure, depending a crate to get some of the work done is completely fine, we don't need to write this from scratch.

[drHuangMHT]

Sure, depending a crate to get some of the work done is completely fine, we don't need to write this from scratch.

Crate ecdsa provides ability to work with keys generated using secp256k1, r1 and secp384r1, should we remove secp256k1 module now, later or never? And for the spec, ECDSA seems to be generally considered to be including those curves, so maybe the spec need some changes?

[thomaseizinger]

Sure, depending a crate to get some of the work done is completely fine, we don't need to write this from scratch.

Crate ecdsa provides ability to work with keys generated using secp256k1, r1 and secp384r1, should we remove secp256k1 module now, later or never? And for the spec, ECDSA seems to be generally considered to be including those curves, so maybe the spec need some changes?

Dealing with secp256k1 is all we need for now. If you wanted to incorporate a new kind of key, you'd have to lobby over at https://github.com/libp2p/specs for that and also get the buy-in from the other implementations. Unless you have a concrete usecase for it, I'd advise against it :)

Overall, changing how we do secp256k1 keys should be a separate PR unless it is necessary for the implementation of the protobuf encoding. Even then, it would be better to ship that as a separate refactoring PR and build this one on top.

[drHuangMHT]

Dealing with secp256k1 is all we need for now. If you wanted to incorporate a new kind of key, you'd have to lobby over at https://github.com/libp2p/specs for that and also get the buy-in from the other implementations. Unless you have a concrete usecase for it, I'd advise against it :)

Overall, changing how we do secp256k1 keys should be a separate PR unless it is necessary for the implementation of the protobuf encoding. Even then, it would be better to ship that as a separate refactoring PR and build this one on top.

Filed an issue #540 in spec repository.

[drHuangMHT]

This is the main blocking comment of the PR, let me know if it is unclear for you how to proceed here :)

No. Seems that we need to switch to crate ecdsa for pkcs#8 encoding support, but we haven't decided whether or not to yet so it's blocked.

[thomaseizinger]

This is the main blocking comment of the PR, let me know if it is unclear for you how to proceed here :)

No. Seems that we need to switch to crate ecdsa for pkcs#8 encoding support, but we haven't decided whether or not to yet so it's blocked.

Do we necessarily need to switch or can we just convert our key to one from the ecdsa crate and encode it through that?

[drHuangMHT]

The exact encoding for secp256k1 is to be decided, see #190.
Checks failed for unmet minimum rustc version.

[thomaseizinger]

The exact encoding for secp256k1 is to be decided, see #190.

The encoding is decided (apparently it is the compressed form as described in the issue). #190 seems to be primarily a documentation bug.

[drHuangMHT]

The exact encoding for secp256k1 is to be decided, see #190.

The encoding is decided (apparently it is the compressed form as described in the issue). #190 seems to be primarily a documentation bug.

Should get that fixed as soon as possible!

[thomaseizinger]

Checks failed for unmet minimum rustc version.

That unfortunately makes this a breaking change, meaning it will have to wait a bit to merge. We are thinking of bumping the MSRV very soon though (~2 weeks).

[thomaseizinger]

The exact encoding for secp256k1 is to be decided, see #190.

The encoding is decided (apparently it is the compressed form as described in the issue). #190 seems to be primarily a documentation bug.

Should get that fixed as soon as possible!

I agree but unfortunately, I don't have the bandwidth for it at the moment. Happy to review a PR though if you are interested.

[drHuangMHT]

The exact encoding for secp256k1 is to be decided, see #190.

The encoding is decided (apparently it is the compressed form as described in the issue). #190 seems to be primarily a documentation bug.

The "compressed form" is for public keys. I still don't know what to do with private keys.

[drHuangMHT]

Currently we parse RSA key from pkcs#8 format, but the spec requires pkcs#1.