Skip to content

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

Closed
drHuangMHT wants to merge 100 commits into
libp2p:masterfrom
drHuangMHT:identity-keypair
Closed

drHuangMHT wants to merge 100 commits into
libp2p:masterfrom
drHuangMHT:identity-keypair

Conversation

@drHuangMHT

@drHuangMHT drHuangMHT commented Mar 27, 2023 •

Copy link
Copy Markdown
Contributor

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 drHuangMHT changed the title feat(identity): implement protobuf encoding for RSA, ECDSA and secp256k1 keypairs feat(identity): implement protobuf encoding for libp2p-identity keypairs Mar 27, 2023
@drHuangMHT

Copy link
Copy Markdown
Contributor Author

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

@thomaseizinger thomaseizinger left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thank you for the tidy-up!

There are some good ideas in here. I've made some comments throughout. Most importantly, I think we can easily do this in a non-breaking way by just adding new functions and deprecating old ones.

Comment thread identity/src/keypair.rs Resolved
Comment thread identity/src/keypair.rs Resolved
Comment thread identity/src/keypair.rs Resolved
Comment thread identity/src/ecdsa.rs Resolved
Comment thread identity/src/keypair.rs Resolved
Comment thread identity/src/lib.rs Resolved
Comment thread identity/src/rsa.rs Resolved
Comment thread identity/src/rsa.rs Resolved
@thomaseizinger

Copy link
Copy Markdown
Contributor

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

Comment thread protocols/mdns/CHANGELOG.md Resolved
mergify Bot pushed a commit that referenced this pull request May 5, 2023
@mergify

mergify Bot commented May 5, 2023

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts. Could you please resolve them @drHuangMHT? 🙏

@drHuangMHT

Copy link
Copy Markdown
Contributor Author

Now I guess this commit is mostly about renaming.

@mergify

mergify Bot commented May 5, 2023

Copy link
Copy Markdown
Contributor

This pull request has merge conflicts. Could you please resolve them @drHuangMHT? 🙏

Comment thread identity/src/keypair.rs
/// Encode a private key as protobuf structure.
///
/// See <https://github.com/libp2p/specs/blob/master/peer-ids/peer-ids.md#key-types> for details on the encoding.
pub fn encode_protobuf(&self) -> Result<Vec<u8>, DecodingError> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It is a shame that we rename the function here and cannot do the signature change to infalliblity at the same time. For that we need to also implement RSA encoding but I think for that we need to switch to the rsa crate which I want to debate first.

I wonder:

  • should we delay the rename of this function until then so we can land the rest now
  • should we rename it with the others and send users through another rename (+ signature change) once we support RSA

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Delay the rename then.

@drHuangMHT

drHuangMHT commented May 22, 2023 •

Copy link
Copy Markdown
Contributor Author

Should we remove this from 0.52 milestone? We haven't decided to switch to rsa crate yet and I don't think we will do that very soon @thomaseizinger

@thomaseizinger thomaseizinger removed this from the v0.52.0 milestone May 22, 2023
@thomaseizinger

Copy link
Copy Markdown
Contributor

Should we remove this from 0.52 milestone? We haven't decided to switch to rsa crate yet and I don't think we will do that very soon @thomaseizinger

Thanks, I removed the milestone.

@drHuangMHT

Copy link
Copy Markdown
Contributor Author

Are we rsa yet? friendly ping @thomaseizinger

@thomaseizinger

Copy link
Copy Markdown
Contributor

Are we rsa yet? friendly ping @thomaseizinger

Has anything changed around the situation of the rsa crate? I have not followed its state.

@drHuangMHT

drHuangMHT commented Jan 2, 2024 •

Copy link
Copy Markdown
Contributor Author

Has anything changed around the situation of the rsa crate? I have not followed its state.

Well the crate has been audited RustCrypto/RSA#60, but the non constant-time issue RustCrypto/RSA#19 is still not solved yet.

@thomaseizinger

Copy link
Copy Markdown
Contributor

Has anything changed around the situation of the rsa crate? I have not followed its state.

Well the crate has been audited RustCrypto/RSA#60, but the non constant-time issue RustCrypto/RSA#19 is still not solved yet.

In that case, I'd say we keep things as is for now. I have very limited capacity this month and will step down from active maintenance after that so I'd like to focus the remaining time on other issues.

@drHuangMHT

Copy link
Copy Markdown
Contributor Author

Not planned in the foreseeable future, closing this to clean up work tree on my fork.

@drHuangMHT drHuangMHT closed this Mar 11, 2024
@drHuangMHT
drHuangMHT deleted the identity-keypair branch March 11, 2024 10:06
AdetsGithub pushed a commit to AdetsGithub/rust-libp2p that referenced this pull request Oct 16, 2026
Implement encoding to/decoding from DER-encoded secret key document for `ecdsa::SecretKey`.
Implement encoding to/decoding from protobuf format for ECDSA keys.
Bump dependency `p256` from 0.12 to 0.13.
Bump dependency `sec1` from 0.3.0 to 0.7

Related: libp2p#3681.

Pull-Request: libp2p#3863.
AdetsGithub pushed a commit to AdetsGithub/rust-libp2p that referenced this pull request Oct 16, 2026
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.

identity: Implement protobuf encoding for RSA, ECDSA and secp256k1 keypairs

2 participants