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

core/src/identity: Implement Keypair::from_protobuf_encoding for ed25519 #2090

Merged
merged 3 commits into from
Jun 7, 2021

Conversation

mxinden
Copy link
Member

@mxinden mxinden commented Jun 2, 2021

Implement Keypair decoding from Protobuf according to peer id
specification
. For now support ed25519 keys only. Future commits might
add RSA, secp256k1 and ecdsa.

Implement `Keypair` decoding from Protobuf according to [peer id
specification]. For now support ed25519 keys only. Future commits might
add RSA secp256k1 and ecdsa.

[peer id specification]: https://github.com/libp2p/specs/blob/master/peer-ids/peer-ids.md#keys
@mxinden
Copy link
Member Author

mxinden commented Jun 2, 2021

@marten-seemann do you have some time to give this pull request a review, especially from the libp2p cryptography perspective?

@vmx do you have some time to give this pull request a review, especially from the Rust perspective?

Copy link

@marten-seemann marten-seemann 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 not sure if I can do a meaningful review here, considering my (lack of) Rust skills.

Why are you only implementing ed25519 though?

@mxinden
Copy link
Member Author

mxinden commented Jun 2, 2021

Why are you only implementing ed25519 though?

Short story: Laziness.

Long story:

I have not found good support for parsing RSA private keys via PKCS1. Today we only support PKCS8 in rust-libp2p.

I have no easy way to test Secp256k1 parsing.

Implementations MUST support Ed25519. Implementations SHOULD support RSA if they wish to interoperate with the mainline IPFS DHT and the default IPFS bootstrap nodes. Implementations MAY support Secp256k1 and ECDSA, but nodes using those keys may not be able to connect to all other nodes.

https://github.com/libp2p/specs/blob/master/peer-ids/peer-ids.md

Given that the libp2p ecosystem mostly bets on Ed25519 I am reluctant to put in the work to implement support for RSA, which might never be used, and reluctant to implement support for Secp256k1 which might be incorrect.

Note, rust-libp2p does fully support RSA and Secp256k1 public keys, thus is able to communicate with nodes using RSA and Secp256k1 private keys.

@marten-seemann does the above make sense? Am I missing something?

@marten-seemann
Copy link

Implementations SHOULD support RSA if they wish to interoperate with the mainline IPFS DHT and the default IPFS bootstrap nodes

How does Rust bootstrap if it doesn't support RSA? Do we have ed25519 bootstrappers now? If so, should we update the docs?

@mxinden
Copy link
Member Author

mxinden commented Jun 2, 2021

How does Rust bootstrap if it doesn't support RSA?

rust-libp2p supports decoding RSA public keys from the protobuf definition defined in the peer-id specification. Thus it can communicate with RSA based nodes. Up until now rust-libp2p did not support decoding private keys from the protobuf definition defined in the peer-id specification. In order to load your private key you would have to pass the private key directly, e.g. PKCS8 encoded for an RSA private key and [private key bytes][public key bytes] "encoded" for an ed25519 private key.

This pull request adds support for decoding ed25519 private keys from the protobuf definition defined in the peer-id specification. The motivation for this patch is to be able to load a ed25519 private key from an IPFS config files.

Sorry for the confusion here. Does the above make sense @marten-seemann?

Do we have ed25519 bootstrappers now? If so, should we update the docs?

I don't know.

Copy link

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Thank you for the detailed explanation. That makes a lot of sense to me.

Copy link
Contributor

@vmx vmx left a comment

Choose a reason for hiding this comment

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

I complement @marten-seemann's review with lack of libp2p knowledge, but Rust knowlege :)

Just a minor comment, rest looks good.

core/src/identity.rs Outdated Show resolved Hide resolved
@mxinden mxinden merged commit a24e422 into libp2p:master Jun 7, 2021
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

3 participants