Add support for noise IX, XX, and IK handshakes. #888
Conversation
ghost
assigned 
| scalar: Scalar | ||
| } | ||
|
|
||
| impl SecretKey { |
There was a problem hiding this comment.
Would be nice to eventually move this type in the core or something.
cc #807
twittner
force-pushed
the
noise-handshakes
branch
from
44b45bd to
8a19c63
Compare
twittner
force-pushed
the
noise-handshakes
branch
from
8a19c63 to
5c1ceff
Compare
|
I brought this up recently in https://forum.web3.foundation/t/transport-layer-authentication-libp2ps-secio/69 We might eventually switch to https://noiseexplorer.com/ if it gets Rust support. |
|
Note that we need #882 before this is usable in practice. |
protocols/noise/src/xx.rs
Outdated
| self.0 = InboundState::RecvHandshake1(output) | ||
| } | ||
| InboundState::RecvHandshake1(mut io) => { | ||
| if io.poll_read(&mut [])?.is_ready() { |
There was a problem hiding this comment.
Every single call to poll_read or poll_write passes an empty slice.
What's the reason for going for AsyncRead and AsyncWrite if you're always passing empty slices?
There was a problem hiding this comment.
During the handshake phase there is no application data to send, so effectively only key material is exchanged. The trace log highlights what is happening. Here is an excerpt for an initial XX handshake write:
[TRACE libp2p_noise::io] write state: Init
[TRACE libp2p_noise::io] write state: BufferData { off: 0 }
[TRACE libp2p_noise::io] write: buffered 0 bytes
[TRACE libp2p_noise::io] flush: encrypting 0 bytes
[TRACE libp2p_noise::io] flush: cipher text len = 32 bytes
[TRACE libp2p_noise::io] flush: writing len (32)
[TRACE libp2p_noise::io] flush: wrote 32/32 bytes
[TRACE libp2p_noise::io] flush: finished writing 32 bytes
...
There was a problem hiding this comment.
Oh I see, the Read and Write are used post-handshake.
|
|
||
| fn dh(&self, pubkey: &[u8], out: &mut [u8]) -> Result<(), ()> { | ||
| let mut p = [0; 32]; | ||
| p.copy_from_slice(&pubkey[.. 32]); |
There was a problem hiding this comment.
Here you do &pubkey[..32] while in set you do just privkey. I'd prefer to add a verification that the lengths of pubkey and out are 32.
There was a problem hiding this comment.
The pubkey that is passed in is longer than 32 bytes, hence the need to restrict it. When called from HandshakeState::dh it uses its re or rs slices which are 56 bytes each.
Cargo.toml
Outdated
| libp2p-tcp = { version = "0.2.0", path = "./transports/tcp" } | ||
|
|
||
| [target.'cfg(any(target_os = "emscripten", target_os = "unknown"))'.dependencies] | ||
| stdweb = { version = "0.4", default-features = false } | ||
|
|
||
| [dev-dependencies] | ||
| env_logger = "0.6.0" | ||
| quicli = "0.4" |
There was a problem hiding this comment.
I suppose that this and structopt are no longer needed anymore, since the example is gone.
protocols/noise/src/io.rs
Outdated
| const_assert_eq!(G; 0, WRITE_CRYPTO_BEGIN - WRITE_END); // write crypto buffer follows write buffer | ||
|
|
||
| pub struct NoiseOutput<T> { | ||
| pub(super) io: T, |
There was a problem hiding this comment.
I'm not a fan of adding super everywhere. I think it's better to keep everything internal to the module. If you ever need to debug this, it's easier to know that all the modifications to the state are done within the module.
protocols/noise/src/io.rs
Outdated
| pub struct NoiseOutput<T> { | ||
| pub(super) io: T, | ||
| pub(super) session: snow::Session, | ||
| pub(super) buffer: Box<[u8; TOTAL_BUFFER_LEN]>, |
There was a problem hiding this comment.
An alternative way than splitting everywhere could do to create a separate type to reduce code duplication:
struct Buffer {
inner: Box<[u8; TOTAL_BUFFER_LEN]>
}
impl Buffer {
fn borrow(&mut self) -> BufferBorrow { /* call split_at_mut here */ }
}
struct BufferBorrow<'a> {
read: &'a mut [u8],
read_crypto: &'a mut [u8],
write: &'a mut [u8],
write_crypto: &'a mut [u8],
}| const_assert_eq!(F; 0, READ_CRYPTO_BEGIN - READ_END); // read crypto buffer follows read buffer | ||
| const_assert_eq!(G; 0, WRITE_CRYPTO_BEGIN - WRITE_END); // write crypto buffer follows write buffer | ||
|
|
||
| pub struct NoiseOutput<T> { |
There was a problem hiding this comment.
I'm not a fan of calling poll_read and poll_write with empty slices. It's a bit too weird.
I suggest we keep NoiseOutput as the "final" type (the one that the user receives), but add a Handshake type that wraps around NoiseOutput, performs the handshake, and has a into_transport_mode() method that turns it into a NoiseOutput. This way you can also make session private.
As per review suggestion.
Instead of having handshake futures per pattern, i.e. for XX, IX and IK, categorise the futures in roundtrips, i.e. rt1, rt2. Pattern IX and IK use one roundtrip whereas XX uses two.
No description provided.