secret connection update by ValarDragon · Pull Request #2054 · tendermint/tendermint

ValarDragon · 2018-07-24T23:53:04Z

This PR updates secret connection to remove the ripemd primitive, and the salsa primitive. This gives us a resulting secret connection that looks alot more noise like.

This switches the sender and receiver to use different keys, similar to what noise does. We now use a single hkdf-sha2 invocation on the diffie hellman secret to generate the two keys and a challenge.

Closes #2039

Updated all relevant documentation in docs
Updated all code comments where relevant
Wrote tests
Updated CHANGELOG.md

zmanian · 2018-07-25T00:17:57Z

docs/spec/p2p/peer.md


 Tendermint implements the Station-to-Station protocol
-using ED25519 keys for Diffie-Helman key-exchange and NACL SecretBox for encryption.
+using Ed25519 keys for Diffie-Helman key-exchange and chacha20poly1305 for encryption.


The Montgomery keys are typically referred to as x25519

Good catch! Thanks

zmanian · 2018-07-25T00:18:05Z

docs/spec/p2p/peer.md

+using Ed25519 keys for Diffie-Helman key-exchange and chacha20poly1305 for encryption.
 It goes as follows:
- generate an emphemeral ED25519 keypair
+- generate an emphemeral Ed25519 keypair


codecov-io · 2018-07-25T00:18:57Z

Codecov Report

Merging #2054 into develop will increase coverage by 0.02%.
The diff coverage is 85.41%.

@@             Coverage Diff             @@
##           develop    #2054      +/-   ##
===========================================
+ Coverage    62.69%   62.72%   +0.02%     
===========================================
  Files          215      215              
  Lines        17377    17323      -54     
===========================================
- Hits         10895    10865      -30     
+ Misses        5586     5570      -16     
+ Partials       896      888       -8

Impacted Files	Coverage Δ
p2p/conn/secret_connection.go	`74.09% <85.41%> (-2.74%)`	⬇️
rpc/grpc/client_server.go	`66.66% <0%> (-4.77%)`	⬇️
types/priv_validator.go	`61.11% <0%> (-3.76%)`	⬇️
types/params.go	`96.61% <0%> (-0.96%)`	⬇️
consensus/reactor.go	`78.47% <0%> (+0.26%)`	⬆️
libs/common/random.go	`69.23% <0%> (+0.39%)`	⬆️
consensus/state.go	`77.58% <0%> (+0.49%)`	⬆️
p2p/peer.go	`61.36% <0%> (+1.13%)`	⬆️
libs/common/int.go	`17.85% <0%> (+1.19%)`	⬆️
libs/clist/clist.go	`69.06% <0%> (+1.65%)`	⬆️
... and 1 more

melekes · 2018-07-25T05:16:34Z

p2p/conn/secret_connection.go

 	recvBuffer []byte
-	recvNonce  *[24]byte
-	sendNonce  *[24]byte
+	recvNonce  *[chacha20poly1305.NonceSize]byte


aeadNonceSize

melekes · 2018-07-25T05:16:42Z

p2p/conn/secret_connection.go

-	sendNonce  *[24]byte
+	recvNonce  *[chacha20poly1305.NonceSize]byte
+	sendNonce  *[chacha20poly1305.NonceSize]byte
+	recvSecret *[chacha20poly1305.KeySize]byte


aeadKeySize

melekes · 2018-07-25T05:23:52Z

p2p/conn/secret_connection.go

+	io.ReadFull(hkdf, res[:])
+
+	challenge = new([32]byte)
+	recvSecret = new([32]byte)


aeadKeySize

My concern with aeadKeySize in this function was that it made something relatively simple suddenly seem fairly complicated. (I changed it here, and changed it back before committing)

I guess I could just add more comments to explain whats going on though, so it should hopefully be clear.

liamsi

LGTM. Left a few comments / nits.

liamsi · 2018-07-25T13:55:46Z

p2p/conn/secret_connection.go


-// Implements net.Conn
+// SecretConnection implements net.conn.
+// It is (meant to be) an implementation of the STS protocol.


Is it or is it really only meant to be?

liamsi · 2018-07-25T13:58:34Z

p2p/conn/secret_connection.go

 			if err2 != nil {
 				return nil, err2, true // abort
-			} else {
-				return _remEphPub, nil, false


It's a bit odd that the named returns are never used in this method. Do you mind removing these (or use them)?

I'd prefer to keep as is. The return values are needed for the interface which cmn.parallel requires. The code is much harder to read without named returns, and this isn't something which golint objects to.

Without both of these named returns, this gets pretty confusing I think.

liamsi · 2018-07-25T14:01:11Z

p2p/conn/secret_connection.go

+	hkdf := hkdf.New(hash, dhSecret[:], nil, []byte("TENDERMINT_SECRET_CONNECTION_KEY_AND_CHALLENGE_GEN"))
+	// get enough data for 2 aead keys, and a 32 byte challenge
+	res := new([2*aeadKeySize + 32]byte)
+	io.ReadFull(hkdf, res[:])


Do not skip the error here, or, explain why we can assume that no error can happen:
https://golang.org/pkg/io/#ReadFull

Generate keys with HKDF instead of hash functions, which provides better security properties. Add xchacha20poly1305 to secret connection. (Due to rebasing, this code has been removed)

This now uses one hkdf on the X25519 shared secret to create a key for the sender and receiver. The hkdf call is now just called upon the computed shared secret, since the shared secret is a function of the pubkeys. The nonces now start at 0, as we are using chacha as a stream cipher, and the sender and receiver now have different keys.

ValarDragon requested review from liamsi and zmanian July 24, 2018 23:53

ValarDragon requested review from ebuchman, jbibla, melekes, xla and zramsay as code owners July 24, 2018 23:53

ValarDragon force-pushed the dev/secret_connection branch 2 times, most recently from e5c61c3 to 88cec7a Compare July 24, 2018 23:58

ValarDragon self-assigned this Jul 24, 2018

zmanian reviewed Jul 25, 2018

View reviewed changes

melekes approved these changes Jul 25, 2018

View reviewed changes

liamsi approved these changes Jul 25, 2018

View reviewed changes

xla added the C:p2p Component: P2P pkg label Jul 25, 2018

xla added this to the launch milestone Jul 25, 2018

ValarDragon force-pushed the dev/secret_connection branch from 99c42e7 to 49bae51 Compare July 25, 2018 21:46

zmanian and others added 2 commits July 26, 2018 00:09

p2p: Remove RipeMd160.

1b04e4e

Generate keys with HKDF instead of hash functions, which provides better security properties. Add xchacha20poly1305 to secret connection. (Due to rebasing, this code has been removed)

xla force-pushed the dev/secret_connection branch from 0cbdfc9 to 7bf28af Compare July 25, 2018 22:16

xla merged commit bdab37a into develop Jul 25, 2018

xla deleted the dev/secret_connection branch July 25, 2018 22:29

zmanian mentioned this pull request Aug 8, 2018

Secret Connection Integration tendermint/tmkms#38

Merged

6 tasks

Conversation

ValarDragon commented Jul 24, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ValarDragon Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

liamsi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Jul 25, 2018 •

edited

Loading

ValarDragon Jul 25, 2018 •

edited

Loading