multi: refactor and dynamic auth data #27

ellemouton · 2022-01-11T12:03:57Z

This PR does the following:

add a test for the current noise handshake logic
refactor's the noise handshakestate logic to be more generic. This makes any changes a bit easier and also paves the way for us to add the second _KK pattern handshake when the time comes (a test for this is added too).
Handshake versioning logic.
Handshake version 1 which uses a dynamic, length prefixed payload in act 2.

mailbox/noise.go

Roasbeef · 2022-01-22T00:41:52Z

mailbox/noise.go

@@ -685,7 +713,10 @@ func (b *Machine) GenActTwo() ([ActTwoSize]byte, error) {

 	authPayload := b.EncryptAndHash(payload[:])

-	actTwo[0] = HandshakeVersion
+	// The responder will always send its preferred handshake version in
+	// the hopes that the initiator will then upgrade to this version in


Upgrade to the version in act 2? I think this assumes a lot w.r.t how new versions will be rolled out all together.

The reasoning behind this flow is that in our case, the initiator (TW) will always be updated before the responder (Litd) . And since the initiator is also the one that sends the first message (act 1), it makes this negotiation a bit tricky. So we went with this approach:

Act 1: client sends its lowest acceptable version in act 1 (client cant send its max version here since then every new handshake version would be a breaking change and everyone would need to upgrade their Lit nodes)

Act 2: server sends its max version. Client will get this and will know which version the server will be happy with and will adapt to that version.

Act 3: client sends back what ever the server sent in act 2.

Let me know if there is a better way 👍

mailbox/noise.go

lightninglabs-deploy · 2022-01-31T00:55:09Z

@ellemouton, remember to re-request review from reviewers when ready

ellemouton · 2022-01-31T15:00:44Z

Cool, I have changed this PR up quite a lot. Pls see the updated description 👍
basically first did a big refactor of the Noise logic so that the handshake patterns are executed in series. This also paves the way for us to easily use the KK_ pattern for the second handshake when the time comes. The refactor also made it easier to add the logic for handshake 1.
keen to hear some feedback on this approach.

guggero

Did a first round to familiarize myself with the refactor.
Amazing work on this, it really looks great! And I think I learned a thing or two about the noise handshake 💯

Will need a second round to catch all the details. But in the meantime I'll submit my comments.
The linter also has a few complaints by the way.

mailbox/noise_test.go

mailbox/grpc_noise_conn.go

mailbox/noise_patterns.go

mailbox/noise.go

mailbox/noise_test.go

mailbox/grpc_noise_conn.go

mailbox/noise.go

ellemouton

Thanks for taking a look @guggero 🚀 updated 👍

mailbox/grpc_noise_conn.go

guggero

Great refactor, LGTM 🎉

mailbox/grpc_noise_conn.go

This commit adds TestXXHandshake test which just ensures that the current handshake logic works correctly. The reason for adding this test now is so that we can use it to test a refactor too.

GBN reads an entire packet at once and so does not really care about the number of bytes that the caller is expecting to read. To allow the caller to read a payload byte by byte, we can use a bytes buffer which we will populate with the full gbn payload and then subsequent read calls will read from the buffer until it is empty.

Roasbeef · 2022-02-10T01:24:19Z

Failing the linter rn:

Error: mailbox/noise_test.go:395:2: S1023: redundant `return` statement (gosimple)
level=info msg="Memory: 199 samples, avg is 1597.2MB, max is 3621.1MB"
level=info msg="Execution took 19.805837[26](https://github.com/lightninglabs/lightning-node-connect/runs/5121684068?check_suite_focus=true#step:6:26)3s"
	return
	^
Error: mailbox/noise_test.go:399:2: S1023: redundant `return` statement (gosimple)
	return
	^
make: *** [Makefile:83: lint] Error 1
Error: Process completed with exit code 2.

Roasbeef

Excellent work!

Have always wanted to do such a refactor, but back then we didn't have any other handshakes we were using on the LN layer. Completed an initial pass, and have another queue'd up for tomorrow. Solid tests coverage, just have a few other things I want to examine more closely.

Roasbeef · 2022-02-10T01:25:15Z

mailbox/noise_test.go

+	pk2, err := btcec.NewPrivateKey(btcec.S256())
+	require.NoError(t, err)
+
+	// Create a password that will be used to mask the first ephemeral key.


Seems to have a lot of overlap with these tests? https://github.com/lightninglabs/lightning-node-connect/blob/master/mailbox/tcp_noise_test.go#L108

I guess this is from a slightly higher layer of abstraction though, the gRPC level.

yeah i wanted to write tests that actually use the ServerHandshake and ClientHandshake functions.
afaict, we dont use the code in tcp_noise_conn.go and tcp_noise_listener.go anywhere?

afaict, we dont use the code in tcp_noise_conn.go and tcp_noise_listener.go anywhere?

Correct, but we can use them when we eventually add the pure TCP version of LNC, for nodes that don't really need the NAT traversal component.

Roasbeef · 2022-02-10T01:28:05Z

mailbox/noise.go

@@ -298,19 +303,38 @@ type handshakeState struct {

 	remoteStatic    *btcec.PublicKey // nolint
 	remoteEphemeral *btcec.PublicKey // nolint
+
+	password        []byte


Perhaps this should live in something more PAKE specific?

Same comment re the payloads below.

meaning embed a new struct here? I would say only the password is PAKE specific? the payloads are part of any Noise handshake.

Roasbeef · 2022-02-10T01:28:35Z

mailbox/noise.go

 	}

+	protocolName := fmt.Sprintf(
+		"Noise_%s_%s_%s_%s", pattern.Name, dhFn, cipherFn, hashFn,


Need to make sure we're not making a backwards incompatible change here, as this is used to generate the very first handshake digest.

yeah def. I have tested running a new client (which runs off the refactor) agains a server with the previous code and that works 👍

Roasbeef · 2022-02-10T01:30:46Z

mailbox/noise.go

+			return fmt.Errorf("unknown act number: %d", mp.ActNum)
+		}
+
+		_, err := buff.Write(h.EncryptAndHash(payload))


Getting through the PR still, but marking here to ensure that the other party is actually able to read this information as is. As right now, I don't see the existing encrypted length prefix that we use (that includes a MAC) as normal.

for version 0, currently there is a fixed 500 bytes for the Act 2 payload that must be written/read by both sides (see the ActTwoPayloadSize variable on L395) and then we do write the encrypted length prefix on L404

Roasbeef · 2022-02-10T01:31:01Z

mailbox/noise.go

+// the MessagePattern Tokens are processed and then any extra cipher text is
+// decrypted as defined by the handshake version.
+func (h *handshakeState) readMsgPattern(r io.Reader, mp MessagePattern) error {
+


Stray new line.

Roasbeef · 2022-02-10T01:34:44Z

mailbox/noise.go

+	for _, token := range tokens {
+		switch token {
+		case e:
+			e, err := btcec.NewPrivateKey(btcec.S256())


We still want that ephemeralGen function here, so we can do things like test vectors eventually and also make the tests more determinstic.

ok cool yeah that makes sense. Will add it back 👍

Roasbeef · 2022-02-10T01:35:39Z

mailbox/noise_patterns.go

+		},
+	}
+
+	// KKPattern represents the KK Noise pattern in which both initiator


Noice, also this works as both sides already know which mailbox ID to be expecting, so they know which keys to expect.

Roasbeef · 2022-02-10T01:37:34Z

mailbox/noise_patterns.go

+	me Token = "me"
+)
+
+// ActNum is the


Incomplete sentence.

haha will fix 👍

btw, what are your thoughts on having this ActNum variable added to the message patterns in the first place? cause technically we can just get the number from the index of the message pattern in the handshake pattern. So this isnt strictly necessary... But just added it to make the code a bit more readable

mailbox/noise.go

Refactor the noise handshakestate code. This commit makes the code more reusable and takes advantage of the fact that the message pattern token writing and reading are completely separate from the payload writing and reading. With this commit, we also have all the cryto code necessary for when we add the `KK` pattern for the second handshake. A test is added for the KK pattern here too.

In this commit, the idea of a min supported and max supported handshake version is introduced so that client and server can negotiate which version they will use. The client will always send the minimum version they support in act 1. The server will then send its maximum supported version back in act 2. This is done due to the fact that for TW, the client will always be the first to upgrade versions. What ever the server sends back in act 2 is the version they will continue with. This commit also adds a unit test to test if a handshake is successfull given different version congigurations.

With handshake version 1, the auth data is sent in a dynamic field in act 2 to allow for large authdata.

ellemouton

Thanks for the review @Roasbeef 🚀 updated 👍

ellemouton · 2022-02-10T08:07:14Z

mailbox/noise_test.go

+	pk2, err := btcec.NewPrivateKey(btcec.S256())
+	require.NoError(t, err)
+
+	// Create a password that will be used to mask the first ephemeral key.


yeah i wanted to write tests that actually use the ServerHandshake and ClientHandshake functions.
afaict, we dont use the code in tcp_noise_conn.go and tcp_noise_listener.go anywhere?

ellemouton · 2022-02-10T08:15:54Z

mailbox/noise.go

+			return fmt.Errorf("unknown act number: %d", mp.ActNum)
+		}
+
+		_, err := buff.Write(h.EncryptAndHash(payload))


for version 0, currently there is a fixed 500 bytes for the Act 2 payload that must be written/read by both sides (see the ActTwoPayloadSize variable on L395) and then we do write the encrypted length prefix on L404

ellemouton · 2022-02-10T08:18:21Z

mailbox/noise.go

+	for _, token := range tokens {
+		switch token {
+		case e:
+			e, err := btcec.NewPrivateKey(btcec.S256())


ok cool yeah that makes sense. Will add it back 👍

ellemouton · 2022-02-10T08:22:38Z

mailbox/noise_patterns.go

+	me Token = "me"
+)
+
+// ActNum is the


haha will fix 👍

btw, what are your thoughts on having this ActNum variable added to the message patterns in the first place? cause technically we can just get the number from the index of the message pattern in the handshake pattern. So this isnt strictly necessary... But just added it to make the code a bit more readable

ellemouton · 2022-02-10T08:51:24Z

mailbox/noise.go

@@ -298,19 +303,38 @@ type handshakeState struct {

 	remoteStatic    *btcec.PublicKey // nolint
 	remoteEphemeral *btcec.PublicKey // nolint
+
+	password        []byte


meaning embed a new struct here? I would say only the password is PAKE specific? the payloads are part of any Noise handshake.

ellemouton · 2022-02-10T08:52:11Z

mailbox/noise.go

 	}

+	protocolName := fmt.Sprintf(
+		"Noise_%s_%s_%s_%s", pattern.Name, dhFn, cipherFn, hashFn,


yeah def. I have tested running a new client (which runs off the refactor) agains a server with the previous code and that works 👍

Roasbeef

LGTM 🍭

Roasbeef · 2022-02-14T22:34:11Z

With the new handshake (Noise_KK), we'll need to do a later update to abstract away storage/retrieval for long term public key, and also to have the client detect when it no longer needs to use the pass phrase for pairing.

ellemouton mentioned this pull request Jan 11, 2022

mailbox: dynamic auth data size #26

Closed

ellemouton force-pushed the dynamicActTwo branch 3 times, most recently from be578c6 to 157afdd Compare January 12, 2022 07:35

Roasbeef requested changes Jan 22, 2022

View reviewed changes

ellemouton marked this pull request as draft January 27, 2022 16:24

ellemouton changed the title ~~[WIP] Dynamic auth data method 2~~ [WIP] Dynamic auth data Jan 27, 2022

ellemouton mentioned this pull request Jan 31, 2022

multi: readonly and admin macaroon session lightninglabs/lightning-terminal#292

Merged

ellemouton force-pushed the dynamicActTwo branch from 157afdd to d17e479 Compare January 31, 2022 14:47

ellemouton marked this pull request as ready for review January 31, 2022 14:48

ellemouton changed the title ~~[WIP] Dynamic auth data~~ multi: refactor and dynamic auth data Jan 31, 2022

ellemouton requested a review from Roasbeef January 31, 2022 14:57

ellemouton force-pushed the dynamicActTwo branch from d17e479 to dd3efba Compare February 1, 2022 11:45

ellemouton requested a review from guggero February 1, 2022 11:47

guggero reviewed Feb 3, 2022

View reviewed changes

ellemouton force-pushed the dynamicActTwo branch from dd3efba to 5d11038 Compare February 4, 2022 14:24

ellemouton commented Feb 4, 2022

View reviewed changes

mailbox/grpc_noise_conn.go Outdated Show resolved Hide resolved

mailbox/grpc_noise_conn.go Outdated Show resolved Hide resolved

ellemouton force-pushed the dynamicActTwo branch from 5d11038 to 833972e Compare February 4, 2022 14:51

guggero approved these changes Feb 7, 2022

View reviewed changes

mailbox/grpc_noise_conn.go Outdated Show resolved Hide resolved

ellemouton force-pushed the dynamicActTwo branch 3 times, most recently from ffa3dab to 17d4d01 Compare February 9, 2022 07:47

ellemouton added 2 commits February 9, 2022 09:50

mailbox: add Noise handshake test

0311292

This commit adds TestXXHandshake test which just ensures that the current handshake logic works correctly. The reason for adding this test now is so that we can use it to test a refactor too.

ellemouton force-pushed the dynamicActTwo branch from 17d4d01 to 4e6d8d8 Compare February 9, 2022 07:50

Roasbeef requested changes Feb 10, 2022

View reviewed changes

ellemouton added 2 commits February 10, 2022 11:26

mailbox: add handskake version 1

4553a9b

With handshake version 1, the auth data is sent in a dynamic field in act 2 to allow for large authdata.

ellemouton commented Feb 10, 2022

View reviewed changes

ellemouton force-pushed the dynamicActTwo branch from 4e6d8d8 to 4553a9b Compare February 10, 2022 09:29

ellemouton requested a review from Roasbeef February 10, 2022 10:51

Roasbeef approved these changes Feb 14, 2022

View reviewed changes

ellemouton merged commit 44032fe into lightninglabs:master Feb 15, 2022

ellemouton deleted the dynamicActTwo branch February 15, 2022 09:41

ellemouton mentioned this pull request Mar 21, 2022

noise layer: allow authData payload size to be dynamic #16

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multi: refactor and dynamic auth data #27

multi: refactor and dynamic auth data #27

ellemouton commented Jan 11, 2022 •

edited

Loading

Roasbeef Jan 22, 2022

ellemouton Jan 31, 2022

lightninglabs-deploy commented Jan 31, 2022

ellemouton commented Jan 31, 2022

guggero left a comment

ellemouton left a comment

guggero left a comment

Roasbeef commented Feb 10, 2022

Roasbeef left a comment •

edited

Loading

Roasbeef Feb 10, 2022

Roasbeef Feb 10, 2022

ellemouton Feb 10, 2022

Roasbeef Feb 14, 2022

Roasbeef Feb 10, 2022

ellemouton Feb 10, 2022

Roasbeef Feb 10, 2022

ellemouton Feb 10, 2022

Roasbeef Feb 10, 2022

ellemouton Feb 10, 2022

Roasbeef Feb 10, 2022

Roasbeef Feb 10, 2022

ellemouton Feb 10, 2022

Roasbeef Feb 10, 2022

Roasbeef Feb 10, 2022

ellemouton Feb 10, 2022

ellemouton left a comment

ellemouton Feb 10, 2022

ellemouton Feb 10, 2022

ellemouton Feb 10, 2022

ellemouton Feb 10, 2022

ellemouton Feb 10, 2022

ellemouton Feb 10, 2022

Roasbeef left a comment

Roasbeef commented Feb 14, 2022

multi: refactor and dynamic auth data #27

multi: refactor and dynamic auth data #27

Conversation

ellemouton commented Jan 11, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lightninglabs-deploy commented Jan 31, 2022

ellemouton commented Jan 31, 2022

guggero left a comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

Roasbeef commented Feb 10, 2022

Roasbeef left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellemouton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Roasbeef left a comment

Choose a reason for hiding this comment

Roasbeef commented Feb 14, 2022

ellemouton commented Jan 11, 2022 •

edited

Loading

Roasbeef left a comment •

edited

Loading