Secret connection plan

[ValarDragon]

This issue is meant to summarize / consolidate the secret connection conversation. (Its currently across 3 different locations) This is per discussion in the tendermint meeting today.

The current issue in secret connection is that we are relying on the salsa primitive, which has basically been abandoned in favor of chacha in many applications, such as TLS. We're also relying on the RipeMD160 primitive as a sub-component of nacl secret box.

The proposed solution is the following:

In the handshake, use a single HKDF-Sha2 invocation on the shared DH secret, local eph pubkey and remote eph pubkey to generate the secret key, send nonce, recv nonce, and challenges.

Use chacha20poly1305 as a stream cipher, via incrementing the send and recv nonces.

The only remaining question I have is that is there a need for send nonce or recv nonce to be derived from hkdf? Could it instead be that one of the two participants has their recv nonce initially as 1, and the other has their send nonce initially as 2**96 -1 and decrements their nonce. The reason I'm wondering that is that that would minimize the chance of collisions (since we're using the nonce as a stream here, and not one of the randomized nonce variants)

/cc @zmanian @tarcieri

[adrianbrink]

I came across https://github.com/libp2p/go-libp2p-secio yesterday and was wondering if we can learn from how they implemented encryption for a tcp connection.

[zmanian]

I see no benefit to using HKDF derived nonces and I think we should just start nonces at 1 and increment.

[zmanian]

What libp2p does is reuse the ciphersuite strings and ciphersuites from > = TLS 1.2.

[tarcieri]

This all sounds good to me, except for this:

The only remaining question I have is that is there a need for send nonce or recv nonce to be derived from hkdf? Could it instead be that one of the two participants has their recv nonce initially as 1, and the other has their send nonce initially as 2**96 -1 and decrements their nonce. The reason I'm wondering that is that that would minimize the chance of collisions (since we're using the nonce as a stream here, and not one of the randomized nonce variants)

An alternative to trying to divvy up the nonce space when using a construction like HKDF is to request enough bytes of output for two keys instead of just one, i.e. deriving separate keys for the connector and for the listener, and then both sides can start with the same initial counter (e.g. 0), but only encrypt under their own key, and always decrypt with the other.

I think this is best hygiene and the approach used by both TLS and Noise. Have a look at the Split() function in section 5.2 of the Noise Protocol Specification.

[ValarDragon]

That sounds like a great plan to me. Thanks tony!

[hendrikhofstadt]

Currently the basic implementation of the tcp socket is broken:

tendermint/privval/socket_tcp.go

Line 43 in 60378fd

err := ln.SetDeadline(time.Now().Add(ln.acceptDeadline))

The Deadline is only set once and not updated when a packet is received. That means that after the deadline expires the connection will time out even though packets were sent and the connection is healthy.

This bug exists both on the client and server implementation.

That basically makes the socket_pv unusable at the moment.

We have fixed this on our validator deployments but since it looks like you are going to change most of the crypto implementation it is probably better for you to rewrite it rather than me submitting a PR with our changes.

Our fix also includes a sync.Mutex to allow only one routine/request to be active and writing to the socket at a time.

[ValarDragon]

Thanks for debugging that @SLAMPER! Could you write that up in a seperate issue / PR? That should be done in a seperate PR from the crypto changes in secret connection (which doesn't change any of the lines regarding setting deadline)

[hendrikhofstadt]

@ValarDragon I see you are not rewriting pv but just replacing the crypto.

I will take some time this evening to clean up and pr our fixes 👍