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.

Sign up for GitHub

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

Jump to bottom

core/: Introduce rsa feature flag to avoid ring dependency #2860

Merged
merged 11 commits into from
Sep 7, 2022
Merged

core/: Introduce rsa feature flag to avoid ring dependency #2860

merged 11 commits into from
Sep 7, 2022

Conversation

GamePad64
Copy link
Contributor

Description

I've tried to build libp2p for router, running mips cpu, but the build has failed because of ring dependency. Ring is used mainly for RSA in libp2p-code, so I've made rsa as an optional feature

Links to any relevant issues

#1396
#1975

Open Questions

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

@GamePad64 GamePad64 marked this pull request as ready for review August 31, 2022 11:15
thomaseizinger
thomaseizinger reviewed Aug 31, 2022
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Generally, I am happy to land this change. Please see inline comments :)

core/CHANGELOG.md Outdated Show resolved Hide resolved
thomaseizinger
thomaseizinger approved these changes Sep 1, 2022
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger 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 doing all the version updates and changelog entries!

One more question and I'd like to have @mxinden's review on this as well before we merge it.

core/src/identity/error.rs Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Sep 1, 2022 •
edited
Loading

I've tried to build libp2p for router, running mips cpu, but the build has failed because of ring dependency. Ring is used mainly for RSA in libp2p-code, so I've made rsa as an optional feature

One more note on this, I am pretty sure libp2p-quic - once merged - will also bring in a dependency on ring again. Using QUIC is obviously optional but I just wanted to mention that RSA keys are not the only thing that ties us to ring.

@GamePad64
Copy link
Contributor Author

libp2p-core is a central part of libp2p, so removing this problematic dependency from it would be much better. libp2p-quic is not a required transport, so we can just continue to use libp2p-tcp 😉
The biggest problem with ring, that it supports only x86 and arm in 32- and 64-bit variants. Using it denies building your apps for embedded targets, supported in Rust (like routers and such).

@thomaseizinger thomaseizinger changed the title Allow disabling rsa identity, so ring could be disabled core: Introduce rsa feature flag to avoid ring dependency Sep 2, 2022
thomaseizinger
thomaseizinger approved these changes Sep 4, 2022
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Thanks!

One more note regarding using the new dep syntax.

core/Cargo.toml Outdated Show resolved Hide resolved
core/Cargo.toml Outdated Show resolved Hide resolved
transports/noise/src/protocol/x25519.rs
@@ -120,6 +120,7 @@ impl Protocol<X25519> for X25519 {
Ok(PublicKey(X25519(pk)))
}

#[allow(irrefutable_let_patterns)]
Copy link
Contributor

Choose a reason for hiding this comment

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

This is unfortunate :/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but I don't know how to change this without a massive rewrite :(

@thomaseizinger
Copy link
Contributor

Thanks for adopting the suggestions so quickly. Just waiting for @mxinden now :)

@mxinden mxinden changed the title core: Introduce rsa feature flag to avoid ring dependency core/: Introduce rsa feature flag to avoid ring dependency Sep 5, 2022
mxinden
mxinden reviewed Sep 5, 2022
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the patch and thanks for following this all the way through, updating the many versions 🙏.

This looks good to me apart from one thing.

The libp2p-core feature secp256k1 is passed through to libp2p. In addition the secp256k1 is part of the libp2p default feature.

According to the libp2p specification:

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#key-types

I read this as rsa taking a higher priority as secp256k1. Thus I suggest passing the rsa feature through libp2p and adding rsa to the default feature on libp2p. That would as well remove the breaking change, i.e. the removal of default support for decoding rsa keys.

@GamePad64 @thomaseizinger does the above reasoning make sense?

@mxinden mxinden mentioned this pull request Sep 5, 2022
Merged
5 tasks
@GamePad64
Copy link
Contributor Author

Yes, this makes sense to me :) Added rsa feature to base libp2p crate and added it to default.

thomaseizinger
thomaseizinger approved these changes Sep 5, 2022
View reviewed changes
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

@mxinden That sounds good to me.

It is a breaking change regardless though. A user depending on the crate with disabled default features will not get RSA support whereas previously, they did.

mxinden
mxinden approved these changes Sep 7, 2022
View reviewed changes
Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

Thanks for the follow-ups!

@mxinden
Copy link
Member

mxinden commented Sep 7, 2022

It is a breaking change regardless though. A user depending on the crate with disabled default features will not get RSA support whereas previously, they did.

👍 I will point this out in the release notes on #2869.

@mxinden
Copy link
Member

mxinden commented Sep 7, 2022

🎉 first pull request running the interop tests #2835 //CC @laurentsenta.

@mxinden mxinden merged commit 8644c65 into libp2p:master Sep 7, 2022
@mxinden mxinden mentioned this pull request Sep 25, 2022
Merged
@dariusc93 dariusc93 mentioned this pull request Oct 17, 2022
Merged
umgefahren pushed a commit to umgefahren/rust-libp2p that referenced this pull request Mar 8, 2024
@GamePad64
core/: Introduce rsa feature flag to avoid ring dependency (libp2…
8473046 
…p#2860)

- Introduce `rsa` feature flag to `libp2p-core`.
- Expose `rsa` feature in `libp2p`.
- Add `rsa` feature to `libp2p` `default`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thomaseizinger thomaseizinger thomaseizinger approved these changes

@mxinden mxinden mxinden approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@GamePad64 @thomaseizinger @mxinden