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

remove rustc-serialize (#359) #386

Merged
merged 13 commits into from
Aug 10, 2018
Merged

remove rustc-serialize (#359) #386

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
66193eb
Replace rust-crypto dependency by RustCrypto project crate aes-ctr
cheme Aug 2, 2018
c4a4a86
Support for x86 build supporting both aesni and aessoft at runtime.
cheme Aug 2, 2018
8537e96
Change CI to build with aes ni on all-features
cheme Aug 6, 2018
bc9dd63
Switch aes-all logic (is_x86 runtime test did not work as I thought when
cheme Aug 7, 2018
25f91c0
Use latest aesni from crates.io (with pr to disable compile time
cheme Aug 9, 2018
f8da503
Merge branch 'master' into RustCrypto_switch
tomaka Aug 10, 2018
cfb9990
Replace rust-crypto dependency by RustCrypto project crate aes-ctr
cheme Aug 2, 2018
d6b1a3e
Support for x86 build supporting both aesni and aessoft at runtime.
cheme Aug 2, 2018
7c8f52e
Change CI to build with aes ni on all-features
cheme Aug 6, 2018
6fb5fd1
Switch aes-all logic (is_x86 runtime test did not work as I thought when
cheme Aug 7, 2018
2d0f8d6
Use latest aesni from crates.io (with pr to disable compile time
cheme Aug 9, 2018
edf1d69
Merge branch 'RustCrypto_switch' of github.com:cheme/rust-libp2p into…
cheme Aug 10, 2018
eb2a9d5
Merge branch 'master' into RustCrypto_switch
gnunicorn Aug 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ jobs:
command: docker run --rm -v "/cache/cargo/registry:/usr/local/cargo/registry" -v "/cache/target:/app/target" -it rust-libp2p cargo test --no-default-features
- run:
name: Run tests, inside a docker image, with all features
command: docker run --rm -v "/cache/cargo/registry:/usr/local/cargo/registry" -v "/cache/target:/app/target" -it rust-libp2p sh -c 'RUSTFLAGS="-C target-feature=+aes,+ssse3,+sse2" RUSTDOCFLAGS="-C target-feature=+aes,+ssse3,+sse2" cargo test --all-features'
command: docker run --rm -v "/cache/cargo/registry:/usr/local/cargo/registry" -v "/cache/target:/app/target" -it rust-libp2p cargo test --all-features
- save_cache:
key: test-cache
paths:
Expand Down
4 changes: 2 additions & 2 deletions secio/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ protobuf = "2.0.2"
rand = "0.3.17"
ring = { version = "0.12.1", features = ["rsa_signing"] }
aes-ctr = "0.1.0"
aes-soft = { version = "0.2", optional = true }
aesni = { git="https://github.com/cheme/block-ciphers.git", features = ["nocheck"], optional = true }
Copy link
Contributor

Choose a reason for hiding this comment

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

@cheme is there anything in the git-repo that isn't in the release on crates.io? Or why can't we use that?

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 there was a little change, but RustCrypto guys merge it: RustCrypto/block-ciphers#22 , so we can now use the crates.io dependancy (I change it).

ctr = { version = "0.1", optional = true }
lazy_static = { version = "0.2.11", optional = true }
rw-stream-sink = { path = "../rw-stream-sink" }
Expand All @@ -25,7 +25,7 @@ untrusted = "0.5.1"
[features]
default = ["secp256k1"]
secp256k1 = ["eth-secp256k1"]
aes-all = ["ctr","aes-soft","lazy_static"]
aes-all = ["ctr","aesni","lazy_static"]

[dev-dependencies]
libp2p-tcp-transport = { path = "../tcp-transport" }
Expand Down
1 change: 0 additions & 1 deletion secio/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,6 @@ mod handshake;
mod structs_proto;
mod stream_cipher;


/// Implementation of the `ConnectionUpgrade` trait of `libp2p_core`. Automatically applies
/// secio on any connection.
#[derive(Clone)]
Expand Down
13 changes: 7 additions & 6 deletions secio/src/stream_cipher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,19 @@ pub(crate) fn ctr(key_size: KeySize, key: &[u8], iv: &[u8]) -> Box<StreamCipherC
#[cfg(all(feature = "aes-all", any(target_arch = "x86_64", target_arch = "x86")))]
pub(crate) fn ctr(key_size: KeySize, key: &[u8], iv: &[u8]) -> Box<StreamCipherCore + 'static> {
if *aes_alt::AES_NI {
Copy link
Member

Choose a reason for hiding this comment

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

I haven't really digged into the crate, but from what I understand the aes_ctr crate already does that, no?
It would really really really be great if we didn't have platform-specific code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aes_ctr run compile time switch between NI and soft implementation (to activate NI only crate flags from original PR msg should be use). A runtime check is not in aes_ctr crate, that is what eas-all feature does, but honestly I doubt that there is a lot of x86 cpu that could not run NI (I did my tests with qemu).
The best thing to do would be to try to PR runtime NI check to RustCrypto (not directly this code as dynamic type usage does not fit RustCrypto crates design), not sure if it would be easily accepted.
Another option would be to simply remove the 'aes-all' feature as I do not know if it is wanted (usecase would be single x86 build supporting old x86 cpu, it seems reasonable to me to defaults to NI activated build (binary distribution) and ask users to build from source if they are using x86 cpu that does not support aes NI and sse3). For info in #371 Sergei low end config could run aes-ni.

ctr_int(key_size, key, iv)
} else {
aes_alt::ctr_alt(key_size, key, iv)
} else {
ctr_int(key_size, key, iv)
}
}


#[cfg(all(feature = "aes-all", any(target_arch = "x86_64", target_arch = "x86")))]
mod aes_alt {
extern crate ctr;
extern crate aes_soft;
extern crate aesni;
use self::ctr::Ctr128;
use self::aes_soft::{Aes128, Aes256};
use self::aesni::{Aes128, Aes256};
use self::ctr::stream_cipher::{NewFixStreamCipher, StreamCipherCore};
use self::ctr::stream_cipher::generic_array::GenericArray;
use super::KeySize;
Expand Down Expand Up @@ -99,10 +99,11 @@ fn assert_non_native_run() {
#[cfg(all(
feature = "aes-all",
any(target_arch = "x86_64", target_arch = "x86"),
not(all(target_feature = "aes", target_feature = "ssse3")),
any(target_feature = "aes", target_feature = "ssse3"),
))]
compile_error!(
"enable aes and ssse3 target features if using aes-all to build, e.g. with \
"aes-all must be compile without aes and sse3 flags : currently \
is_x86_feature_detected macro will not detect feature correctly otherwhise. \
RUSTFLAGS=\"-C target-feature=+aes,+ssse3\" enviromental variable. \
For x86 target arch additionally enable sse2 target feature."
);