crypto/tls: add QUICEvent.Err

[neild]

Proposal Details

This is an offshoot of discussion on #74922.

crypto/tls.QUICConn can report errors occurring during the TLS handshake in several places:

1. QUICConn.Start returns an error if the handshake cannot be started.
2. QUICConn.HandleData returns an error if processing data from the peer causes a handshake error.
3. QUICConn.SendSessionTicket and QUICConn.StoreSession also return errors.

If errors can occur at points in the handshake other than during these functions, there is no way for QUICConn to report them.

The proposal is to add an additional way to report TLS errors. We add an Err field to QUICEvent, and produce an event with a type of QUICNoEvent and the Err field set when an error occurs:

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
)

An open question is what, if any, errors would be reported via this mechanism. It would be nice to have one concrete example of an error that we currently can't repor.

[aclements]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— aclements for the proposal review group

[marten-seemann]

An open question is what, if any, errors would be reported via this mechanism. It would be nice to have one concrete example of an error that we currently can't repor.

On the server side, the most important example is the recently introduced tls.Config.GetEncryptedClientHelloKeys callback, which has an error return value. It is called after requesting the QUIC transport parameters, hence the error return value can't be returned from HandleData.

However, it would be equally valid to call GetEncryptedClientHelloKeys before requesting the transport parameters (nothing passed to this callback depends on the QUIC transport parameters), so this particular instance could be resolved without the introduction of new API.

There are two more instances where errors could occur:

1. When signing with the private key fails:

   go/src/crypto/tls/handshake_server_tls13.go

   Lines 886 to 896 in d86ec92

   	sig, err := hs.cert.PrivateKey.(crypto.Signer).Sign(c.config.rand(), signed, signOpts)
   	if err != nil {
   	public := hs.cert.PrivateKey.(crypto.Signer).Public()
   	if rsaKey, ok := public.(*rsa.PublicKey); ok && sigType == signatureRSAPSS &&
   	rsaKey.N.BitLen()/8 < sigHash.Size()*2+2 { // key too small for RSA-PSS
   	c.sendAlert(alertHandshakeFailure)
   	} else {
   	c.sendAlert(alertInternalError)
   	}
   	return errors.New("tls: failed to sign handshake: " + err.Error())
   	}

2. When writing the key log:

   go/src/crypto/tls/handshake_server_tls13.go

   Lines 933 to 942 in d86ec92

   	err := c.config.writeKeyLog(keyLogLabelClientTraffic, hs.clientHello.random, hs.trafficSecret)
   	if err != nil {
   	c.sendAlert(alertInternalError)
   	return err
   	}
   	err = c.config.writeKeyLog(keyLogLabelServerTraffic, hs.clientHello.random, serverSecret)
   	if err != nil {
   	c.sendAlert(alertInternalError)
   	return err
   	}

[aclements]

It seems odd to add meaning to the existing QUICNoEvent kind. I'm not sure it would actually break anything, but it doesn't pass the sniff test. My understanding is that this is mostly about compatibility with @marten-seemann 's QUIC implementation.

What about instead adding a new QUICErrorEvent kind? If necessary, we could add a QUICConfig.EnableErrorEvents flag, though maybe that's not necessary.

[neild]

Proposal with a new kind:

type QUICEvent struct {
    // Set for QUICErrorEvent.
    // The error will wrap AlertError.
    Err error

    // existing fields as-is
}

const (
    // QUICErrorEvent indicates that a fatal error has occurred.
    // The handshake cannot proceed and the connection must be closed.
    // QUICEvent.Err is set.
    QUICErrEvent QUICEventKind
)

[gopherbot]

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

[marten-seemann]

I incorporated the QuicErrEvent as described in #75108 (comment) into my CL: https://go-review.googlesource.com/c/go/+/695515/4..5.

My understanding is that this is mostly about compatibility with @marten-seemann 's QUIC implementation.

The fact that errors at certain stages of the TLS handshake cannot be returned to the calling QUIC stack affects all QUIC stacks building on top of crypto/tls.

It also turns out that properly returning the error makes it easier to handle TLS handshake messages synchronously, as I proposed in #74922, allowing a significant speedup of the QUIC handshake.

[aclements]

My understanding is that this is mostly about compatibility with @marten-seemann 's QUIC implementation.
The fact that errors at certain stages of the TLS handshake cannot be returned to the calling QUIC stack affects all QUIC stacks building on top of crypto/tls.

To clarify, I meant that the specific approach of using QUICNoEvent for errors as opposed to introducing a new QUICEventKind was for compatibility. Does the QUICErrEvent approach cause problems with using an older version of your QUIC package that doesn't understand QUICErrEvent and a newer version of crypto/tls that produces QUICErrEvent?

Thanks for pointing out #74922. I see this current issue grew out of that. Do I understand correctly that addressing the issue with errors with this current proposal would mean the performance gains of #74922 could be realized without (further) API changes?

[aclements]

Based on the discussion above, this proposal seems like a likely accept.
— aclements for the proposal review group

The proposal details are in #75108 (comment)