noise: make it possible for the server to send early data by marten-seemann · Pull Request #1750 · libp2p/go-libp2p

marten-seemann · 2022-09-13T18:56:26Z

So far, only the client could send early data in its first handshake message, which the server would then receive.

This PR extends the logic, such that the server can now also send early data in its first handshake message.

Needed for libp2p/specs#404 (comment). Also needed for libp2p/specs#446, if I understand correctly.

julian88110

Hi Marten, Thanks for making this API available!

julian88110 · 2022-09-13T22:32:22Z

p2p/security/noise/handshake.go

 			return err
 		}
+		if s.earlyDataHandler != nil {
+			if err := s.earlyDataHandler.Received(ctx, s.insecureConn, rcvdEd); err != nil {


The early data handler may need to know the handshake state to drive its internal state machine, for example if this is the server or client side, at which stage is the early data received. Do you think we should pass the handshake state info to the earlyDataHandler too? or maybe that is available in other ways already and I missed it?

For both client and server, the data is sent with their respective first flights.

I think the nicest way to resolve this is to pass separate client and server EarlyDataHandlers to the EarlyData option. I applied that change in 0b36832. What do you think?

That looks great, thanks for the changes which make the Noise based muxer selection and other early data utilization smoother.

julian88110 · 2022-09-13T22:34:52Z

p2p/security/noise/handshake.go

 	// Also, since we prefix every noise handshake message with its length, we need to account for
 	// it when we fetch the buffer from the pool
-	maxMsgSize := 2*noise.DH25519.DHLen() + len(payload) + 2*chacha20poly1305.Overhead
+	maxMsgSize := 2*noise.DH25519.DHLen() + 2*chacha20poly1305.Overhead + 1024 /* payload */


Just wondering if there is a more efficient way of setting length rather than a hard limit here, I don't have a better way readily available, just want to throw some thoughts here to explore the options.

Changed to a fixed value of 2048 in 24459c5.

It's not even a hard limit, it's just the size we use to get a buffer from the buffer pool. If we get a buffer that's too small, the worst thing that happens is that append would allocate a larger buffer.

We can just use a large enough buffer here. What exactly "large enough" means depends on the amount of early data we send, but 2048 should be enough for practically all use cases. And if not, we can live with that one allocation.

Thanks for the clarification, that makes sense. Sorry I was not aware that we can do extra allocation if the limit is exceeded.

julian88110

LGTM

p2p/security/noise/handshake.go

p2p/security/noise/session.go

p2p/security/noise/session_transport.go

p2p/security/noise/transport_test.go

marten-seemann · 2022-09-15T05:24:02Z

@MarcoPolo Good catch, this will make the code easier to understand. I updated the variable names to match the spec.

MarcoPolo

One small thing

p2p/security/noise/session_transport.go

MarcoPolo

Approved with the comments I added to EarlyDataHandler. Feel free to tweak, but the gist should be the same (a user should understand the caveats around this and what "early data" means in go-libp2p since this isn't a standard).

marten-seemann · 2022-09-16T05:44:59Z

I updated the PR, such that early data is sent with the 2nd and 3rd handshake message. This means that early data is always sent encrypted.

The slight annoyance here is that if the Received function returns an error, we can't fail the handshake any more more, as the client already completed its handshake. Instead, it will just encounter a closed connection. Note that this is similar to what happens in TLS 1.3 when the server rejects the client certificate, so I'm not too worried about this.

MarcoPolo

One fix for the comment, but otherwise looks good. Feel free to ping when the change is in and I’ll approve

MarcoPolo · 2022-09-17T23:06:08Z

p2p/security/noise/session_transport.go

-// EarlyDataHandler allows attaching an (unencrypted) application payload to the first handshake message.
-// While unencrypted, the integrity of this early data is retroactively authenticated on completion of the handshake.
+// EarlyDataHandler defines what the application payload is for either the second
+// (if initiator) or third (if responder) handshake message, and defines the


Suggested change

// (if initiator) or third (if responder) handshake message, and defines the

// (if responder) or third (if initiator) handshake message, and defines the

Right?

I’m going to commit this and approve.

noise: make it possible for the server to send early data

65270f1

marten-seemann requested review from MarcoPolo and julian88110 September 13, 2022 18:56

julian88110 reviewed Sep 13, 2022

View reviewed changes

marten-seemann requested a review from julian88110 September 14, 2022 18:04

julian88110 approved these changes Sep 14, 2022

View reviewed changes

MarcoPolo requested changes Sep 14, 2022

View reviewed changes

p2p/security/noise/handshake.go Outdated Show resolved Hide resolved

p2p/security/noise/session.go Show resolved Hide resolved

p2p/security/noise/session_transport.go Outdated Show resolved Hide resolved

p2p/security/noise/transport_test.go Show resolved Hide resolved

marten-seemann added 2 commits September 15, 2022 08:21

noise: use separate early data handlers for initiator and responder

d27a209

noise: use a 2 kb buffer for all handshake operations

4f8a466

marten-seemann force-pushed the noise-server-send-early-data branch from 24459c5 to 4f8a466 Compare September 15, 2022 05:21

marten-seemann requested a review from MarcoPolo September 15, 2022 05:24

MarcoPolo requested changes Sep 15, 2022

View reviewed changes

p2p/security/noise/session_transport.go Show resolved Hide resolved

Add comments around EarlyData(Handler)

0fe5519

MarcoPolo approved these changes Sep 15, 2022

View reviewed changes

marten-seemann requested review from MarcoPolo and julian88110 September 16, 2022 05:42

noise: send early data with 2nd and 3rd handshake message

e6c29ed

marten-seemann force-pushed the noise-server-send-early-data branch from 78ede96 to e6c29ed Compare September 16, 2022 05:48

marten-seemann mentioned this pull request Sep 16, 2022

webtransport: move certhash verification to the client #1757

Merged

MarcoPolo reviewed Sep 17, 2022

View reviewed changes

Update p2p/security/noise/session_transport.go

d2ffc90

MarcoPolo approved these changes Sep 17, 2022

View reviewed changes

marten-seemann merged commit c1bdab4 into master Sep 19, 2022

	// (if initiator) or third (if responder) handshake message, and defines the
	// (if responder) or third (if initiator) handshake message, and defines the

Conversation

marten-seemann commented Sep 13, 2022

Uh oh!

julian88110 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julian88110 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

marten-seemann commented Sep 15, 2022

Uh oh!

MarcoPolo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MarcoPolo left a comment

Choose a reason for hiding this comment

Uh oh!

marten-seemann commented Sep 16, 2022

Uh oh!

MarcoPolo 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants