proposal: crypto/tls: synchronous processing of TLS handshake messages when using QUIC

[marten-seemann]

Proposal Details

crypto/tls currently starts a new Goroutine to perform the QUIC TLS 1.3 handshake. This is an implementation decision, since the TLS state machine could in principle be entirely driven by the QUIC stack: state transitions only occur in response to TLS handshake messages being received from the peer.

The current design has a few drawbacks:

1. Performance: A lot of context switching is happening between the two Goroutines involved (~one per handshake message). In my implementation of this proposal I measured a speedup of 8% for a benchmark that runs the QUIC handshake (incl. QUIC framing, QUIC packet encryption, UDP syscalls, etc.), meaning that the speedup of the TLS handshake is significantly higher than that.
2. Correctness: On the server side, the current API doesn't allow us to return errors that happen after processing the ClientHello, i.e. when generating the ServerHello and the EncryptedExtensions message. This is because the call to HandleMessage must return to allow the QUIC stack to process the QUICStoreSession and QUICTransportParametersRequired event, which are needed to ServerHello and the EncryptedExtensions message, respectively.

Ideally, an optimized server QUIC stack could run all QUIC handshakes using a fixed number of worker threads (having every state transition been driven by incoming packets from the client), and only spawn a new Goroutine after handshake completion.

Proposal Details

A small API change is required to make this change work. Since the QUIC stack has to act upon the QUICStoreSession (and in the case of a server, on the QUICTransportParametersRequired), it needs to tell crypto/tls once it has done so and the ClientHello (and the ServerHello, respectively) can be sent out.

// A QUICConfig configures a [QUICConn].
type QUICConfig struct {
	// ... exisiting struct
  
	// EnableSendFirstFlight may be set to true to enable the
	// [QUICFirstFlightReady] even.
	// The application should call [QUICConn.SendFirstFlight] to send the first flight.
	EnableSendFirstFlight bool
}

const (
	// QUICFirstFlightReady indicates that the first flight is ready to be sent, and the
	// application should call [QUICConn.SendFirstFlight] to send it.
	QUICFirstFlightReady QUICEventKind
)

// SendFirstFlight sends the first flight of the handshake.
// It must only be called once.
func (q *QUICConn) SendFirstFlight() error

Implementation

I implemented the proposed API in https://go-review.googlesource.com/c/go/+/693255, to be able to benchmark the performance impact:

name          old time/op    new time/op    delta
Handshake-16     464µs ± 2%     427µs ± 2%  -8.08%  (p=0.000 n=98+92)

As mentioned above, the benchmark is running an end-to-end QUIC handshake, i.e. it includes QUIC frame parsing, QUIC packet encryption, UDP syscalls, QUIC loss detection / recovery, etc, suggesting that the saving in the crypto/tls code path are quite significant.

The CL I linked changes the TLS 1.3 handshake logic towards a state machine, but only in the QUIC code path. We could reuse this state machine in TLS 1.3 / TCP code path. This would save quite a few LOC, and make the implementation more robust. I'd be happy to work on this if we decide to move forward with this proposal.

[gopherbot]

Change https://go.dev/cl/693255 mentions this issue: crypto/tls: implement sync processing of QUIC handshake

[gabyhelp]

Related Issues

• crypto/tls: support QUIC as a transport #44886 (closed)
• crypto/tls: improved 0-RTT QUIC APIs #63691 (closed)
• crypto/tls: QUIC 0-RTT APIs #60107 (closed)

Related Code Changes

• crypto/tls: support QUIC as a transport
• crypto/tls: support QUIC as a transport
• crypto/tls: QUIC: fix panics when processing post-handshake messages

_{(Emoji vote if this was helpful or unhelpful; more detailed feedback welcome in this discussion.)}

[neild]

What's the reason for QUICFirstFlightReady/SendFirstFlight? Why can't the TLS layer just provide the first flight of data in a QUICWriteData event when it's ready?

[marten-seemann]

Creating the ServerHello and the EncryptedExtensions message might result in an error, and there's no way to return this error: Neither does NextEvent have an error return value, nor do we have an Error field on the QUICEvent.

Just to be clear, the current API has the same deficiency, and any error will be ignored (the QUIC stack will run into an idle timeout eventually).

[neild]

What's an example of a case where we generate an error which isn't returned?

It's been a while since we designed the crypto/tls QUIC API, but I believe the intent was that errors should be returned from QUICConn.Start if the configuration prevents initiating a handshake, or QUICConn.HandleData if the handshake cannot proceed after receiving data from the peer. If there are errors that we could surface from one of those methods but aren't, that's a bug. If there are errors that can't be surfaced from either, that's a design bug.

[marten-seemann]

It's been a while since we designed the crypto/tls QUIC API, but I believe the intent was that errors should be returned from QUICConn.Start if the configuration prevents initiating a handshake, or QUICConn.HandleData if the handshake cannot proceed after receiving data from the peer.

Yes, that was the intention.

My argument is that we're dealing with a design bug. Consider the following sequence of event:

• The server calls QUICConn.Start. The configuration is fine, no error is returned.
• The server receives the ClientHello and calls QUICConn.HandleData. HandleData blocks until the ClientHello has been processed, and it's valid, so no error is returned.
• After HandleData has returned, the following events are read by the QUIC stack (in this order):
  • QUICTransportParameters, containing the transport parameters received from the client.
  • QUICResumeSession: The QUIC stack can reject 0-RTT by unsetting SessionState.EarlyData, and this is reflected in the EncryptedExtensions message.
  • QUICWriteData, containing the serialized ServerHello
  • QUICSetReadSecret and QUICSetWriteSecret, containing Handshake keys
  • QUICTransportParametersRequired: Only after the QUIC stack has provided the transport parameters, we can generate the EncryptedExtensions.
  • QUICWriteData, containing the EncryptedExtension to be sent to the client
  • QUICSetWriteSecret, containing the 1-RTT keys
  • QUICNoEvent

Note that all these events are returned after HandleData has already returned, so we are not able to return any error that occurs there.

For example, generating the EncryptedExtension message could fail here, or when serializing the message:

go/src/crypto/tls/handshake_server_tls13.go

Lines 819 to 825 in 00a7bdc

	if len(echKeys) > 0 && len(hs.clientHello.encryptedClientHello) > 0 && hs.echContext == nil {
	encryptedExtensions.echRetryConfigs, err = buildRetryConfigList(echKeys)
	if err != nil {
	c.sendAlert(alertInternalError)
	return err
	}
	}

I haven't seen this in practice (I haven't spent any time trying to trigger an error in this code path either), but I believe this is sufficient to show that there are in fact code paths from which it is not possible to return an error to the QUIC stack, hence we're dealing with a design bug.

[neild]

If we do have a case where an error can occur that we currently can't report, then how about something like this?

We add an Err field to QUICEvent. When an error occurs, we generate a QUICNoEvent error with Err set.

type QUICEvent struct {
    // Set for QUICNoEvent
    Err error

    // existing fields as-is
}

const (
    // QUICNoEvent indicates that there are no events available.
    // QUICEvent.Err is set if the connection has encountered a fatal error.
    QUICNoEvent QUICEventKind = iota
)

Alternatively, we could add a new event (QUICError? QUICAlert?) indicating that a fatal error has occurred, but extending the existing event seems like the smallest and least disruptive API change.

I don't see the use for QUICConn.SendFirstFlight, but perhaps I'm missing something.

If we do need to add additional error reporting, it would be good to have an actual example of an error that can be reported though. (So we can write a test for it, if nothing else.)

This all seems orthogonal to synchronous processing of TLS messages, though, which seems like something we can do with no API changes. Whether we should do it is a question for @rolandshoemaker and @FiloSottile, who are the crypto/tls experts.

[gopherbot]

Change https://go.dev/cl/695515 mentions this issue: crypto/tls: implement sync processing of QUIC handshake, with error on QUICEvent

[marten-seemann]

@neild I like the idea, it simplifies the code quite a bit! I submitted a new CL in https://go-review.googlesource.com/c/go/+/695515.

In particular, we don't need to spawn an additional Goroutine (as in my previous CL) for QUIC stacks that don't enable this option.

[neild]

I filed #75108 for QUICEvent.Err to separate the API change from the proposed implementation change.