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

Implement secio #33

Merged
merged 6 commits into from
Nov 20, 2017
Merged

Implement secio #33

merged 6 commits into from
Nov 20, 2017

Conversation

tomaka
Copy link
Member

@tomaka tomaka commented Nov 10, 2017

Ready for review!

Successfully tested against both the Go and JS implementations.

Two contentious points that still exist:

  • The ring library doesn't like some stuff inside DER public keys. Therefore I have to remove the 24 first bytes from public keys generated by OpenSSL and sent by remote implementations in order to make it work. This is obviously not a great solution, but writing a DER parser is not a trivial task.

  • Right now this only accepts RSA keys, because the Go and JS implementations only use RSA, despite the fact that the protocol also accepts Ed25519 and Secp256k1.


// Custom algorithm translated from reference implementations. Needs to be the same algorithm
// amongst all implementations.
fn stretch_key(key: &SigningKey, result: &mut [u8]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

seems like something that should be tested (just skimming)

@tomaka
Copy link
Member Author

tomaka commented Nov 11, 2017

Just realized I had committed some non-secio-related changes. I force-pushed to remove them.

protoc --rust_out . keys.proto"

mv -f structs.rs ./src/structs_proto.rs
mv -f keys.rs ./src/keys_proto.rs
Copy link
Contributor

Choose a reason for hiding this comment

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

🎉 we should use the same snippet in parity repo!

Copy link
Contributor

@debris debris left a comment

Choose a reason for hiding this comment

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

neat!

let mut out_buffer = BytesMut::with_capacity(capacity);
// Note: Alternatively to `extend`, we could also call `advance_mut()`, which will add
// uninitialized bytes to the buffer. But that's unsafe.
out_buffer.extend((0..item.len()).map(|_| 0));
Copy link
Contributor

Choose a reason for hiding this comment

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

imo, repeat take looks better ;)

out_buffer.extend(repeat(0).take(item.len());

Choose a reason for hiding this comment

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

According to my (very simplistic) bench, extend_from_slice is 3× faster than extending from iterator (373±93 ns/iter vs 1,045±18 ns/iter).

Maybe we can use it here?

Copy link
Contributor

Choose a reason for hiding this comment

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

requires an allocation in this case though

Copy link

@kirushik kirushik Nov 14, 2017

Choose a reason for hiding this comment

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

@rphmeier I've updated the bench with a third variant — allocate the whole structure as a Vec of zeroes, then use split_at_mut to get two mutable slices into it.
It's an additional 3× improvement, and no extra memory copying, as far as I can tell.


let future = future::ok::<_, SecioError>(context)
// Generate our nonce.
.and_then(|mut context| {
Copy link
Contributor

Choose a reason for hiding this comment

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

bad indentation, use tabs

stretch_key(&key1, &mut output);
assert_eq!(
&output,
&[
Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry for the very bad formatting, but I think the option to fix that in rustfmt doesn't work.

rustfmt also likes to convert tabs into spaces for some reason, but only at some specific places.

@folsen folsen merged commit 14bb64f into libp2p:master Nov 20, 2017
@tomaka tomaka deleted the secio branch November 20, 2017 11:29
dkuehr pushed a commit to openmina/rust-libp2p that referenced this pull request Oct 24, 2023
dkuehr pushed a commit to openmina/rust-libp2p that referenced this pull request Oct 24, 2023
dkuehr pushed a commit to openmina/rust-libp2p that referenced this pull request Oct 24, 2023
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

5 participants