remove Config.MaxRetryTokenAge, set it to the handshake timeout (#4064)

There is no good reason to manually set the validity period for Retry
tokens. Retry tokens are only valid on a single connection during the
handshake, so it makes sense to limit their validity to the configured
handshake timeout.
This commit is contained in:
Marten Seemann
2023-09-10 13:53:12 +07:00
committed by GitHub
parent e1fcac3e46
commit abfe1ef548
7 changed files with 25 additions and 16 deletions

View File

@@ -19,6 +19,10 @@ func (c *Config) handshakeTimeout() time.Duration {
return 2 * c.HandshakeIdleTimeout
}
func (c *Config) maxRetryTokenAge() time.Duration {
return c.handshakeTimeout()
}
func validateConfig(config *Config) error {
if config == nil {
return nil
@@ -52,9 +56,6 @@ func populateServerConfig(config *Config) *Config {
if config.MaxTokenAge == 0 {
config.MaxTokenAge = protocol.TokenValidity
}
if config.MaxRetryTokenAge == 0 {
config.MaxRetryTokenAge = protocol.RetryTokenValidity
}
if config.RequireAddressValidation == nil {
config.RequireAddressValidation = func(net.Addr) bool { return false }
}
@@ -114,7 +115,6 @@ func populateConfig(config *Config) *Config {
HandshakeIdleTimeout: handshakeIdleTimeout,
MaxIdleTimeout: idleTimeout,
MaxTokenAge: config.MaxTokenAge,
MaxRetryTokenAge: config.MaxRetryTokenAge,
RequireAddressValidation: config.RequireAddressValidation,
KeepAlivePeriod: config.KeepAlivePeriod,
InitialStreamReceiveWindow: initialStreamReceiveWindow,

View File

@@ -80,8 +80,6 @@ var _ = Describe("Config", func() {
f.Set(reflect.ValueOf(time.Hour))
case "MaxTokenAge":
f.Set(reflect.ValueOf(2 * time.Hour))
case "MaxRetryTokenAge":
f.Set(reflect.ValueOf(2 * time.Minute))
case "TokenStore":
f.Set(reflect.ValueOf(NewLRUTokenStore(2, 3)))
case "InitialStreamReceiveWindow":

View File

@@ -10,6 +10,7 @@ import (
"time"
"github.com/quic-go/quic-go"
quicproxy "github.com/quic-go/quic-go/integrationtests/tools/proxy"
"github.com/quic-go/quic-go/internal/protocol"
"github.com/quic-go/quic-go/internal/qerr"
"github.com/quic-go/quic-go/internal/qtls"
@@ -454,16 +455,31 @@ var _ = Describe("Handshake tests", func() {
})
It("rejects invalid Retry token with the INVALID_TOKEN error", func() {
const rtt = 10 * time.Millisecond
serverConfig.RequireAddressValidation = func(net.Addr) bool { return true }
serverConfig.MaxRetryTokenAge = -time.Second
// The validity period of the retry token is the handshake timeout,
// which is twice the handshake idle timeout.
// By setting the handshake timeout shorter than the RTT, the token will have expired by the time
// it reaches the server.
serverConfig.HandshakeIdleTimeout = rtt / 5
server, err := quic.ListenAddr("localhost:0", getTLSConfig(), serverConfig)
Expect(err).ToNot(HaveOccurred())
defer server.Close()
serverPort := server.Addr().(*net.UDPAddr).Port
proxy, err := quicproxy.NewQuicProxy("localhost:0", &quicproxy.Opts{
RemoteAddr: fmt.Sprintf("localhost:%d", serverPort),
DelayPacket: func(quicproxy.Direction, []byte) time.Duration {
return rtt / 2
},
})
Expect(err).ToNot(HaveOccurred())
defer proxy.Close()
_, err = quic.DialAddr(
context.Background(),
fmt.Sprintf("localhost:%d", server.Addr().(*net.UDPAddr).Port),
fmt.Sprintf("localhost:%d", proxy.LocalPort()),
getTLSClientConfig(),
nil,
)

View File

@@ -265,9 +265,6 @@ type Config struct {
// See https://datatracker.ietf.org/doc/html/rfc9000#section-8 for details.
// If not set, every client is forced to prove its remote address.
RequireAddressValidation func(net.Addr) bool
// MaxRetryTokenAge is the maximum age of a Retry token.
// If not set, it defaults to 5 seconds. Only valid for a server.
MaxRetryTokenAge time.Duration
// MaxTokenAge is the maximum age of the token presented during the handshake,
// for tokens that were issued on a previous connection.
// If not set, it defaults to 24 hours. Only valid for a server.

View File

@@ -65,9 +65,6 @@ const MaxAcceptQueueSize = 32
// TokenValidity is the duration that a (non-retry) token is considered valid
const TokenValidity = 24 * time.Hour
// RetryTokenValidity is the duration that a retry token is considered valid
const RetryTokenValidity = 10 * time.Second
// MaxOutstandingSentPackets is maximum number of packets saved for retransmission.
// When reached, it imposes a soft limit on sending new packets:
// Sending ACKs and retransmission is still allowed, but now new regular packets can be sent.

View File

@@ -531,7 +531,7 @@ func (s *baseServer) validateToken(token *handshake.Token, addr net.Addr) bool {
if !token.IsRetryToken && time.Since(token.SentTime) > s.config.MaxTokenAge {
return false
}
if token.IsRetryToken && time.Since(token.SentTime) > s.config.MaxRetryTokenAge {
if token.IsRetryToken && time.Since(token.SentTime) > s.config.maxRetryTokenAge() {
return false
}
return true

View File

@@ -833,7 +833,8 @@ var _ = Describe("Server", func() {
It("sends an INVALID_TOKEN error, if an expired retry token is received", func() {
serv.config.RequireAddressValidation = func(net.Addr) bool { return true }
serv.config.MaxRetryTokenAge = time.Millisecond
serv.config.HandshakeIdleTimeout = time.Millisecond / 2 // the maximum retry token age is equivalent to the handshake timeout
Expect(serv.config.maxRetryTokenAge()).To(Equal(time.Millisecond))
raddr := &net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: 1337}
token, err := serv.tokenGenerator.NewRetryToken(raddr, protocol.ConnectionID{}, protocol.ConnectionID{})
Expect(err).ToNot(HaveOccurred())