Disable anti-amplification limit by address validation token #3326
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3326 +/- ##
=======================================
Coverage 85.37% 85.38%
=======================================
Files 135 135
Lines 9928 9931 +3
=======================================
+ Hits 8476 8479 +3
Misses 1067 1067
Partials 385 385
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
internal/ackhandler/ackhandler.go
Outdated
| @@ -15,7 +17,8 @@ func NewAckHandler( | |||
| tracer logging.ConnectionTracer, | |||
| logger utils.Logger, | |||
| version protocol.VersionNumber, | |||
| clientAddressValidated bool, | |||
There was a problem hiding this comment.
Nit: move this after the rttStats.
| func newSentPacketHandler( | ||
| initialPN protocol.PacketNumber, | ||
| initialMaxDatagramSize protocol.ByteCount, | ||
| rttStats *utils.RTTStats, | ||
| pers protocol.Perspective, | ||
| tracer logging.ConnectionTracer, | ||
| logger utils.Logger, | ||
| clientAddressValidated bool, |
session.go
Outdated
| @@ -249,6 +249,7 @@ var newSession = func( | |||
| tracingID uint64, | |||
| logger utils.Logger, | |||
| v protocol.VersionNumber, | |||
| clientAddressValidated bool, | |||
There was a problem hiding this comment.
Nit: move this after enable0RTT.
| // clientAddressValidated immediately disables the amplification limit. | ||
| // clientAddressValidated has no effect for a client. |
There was a problem hiding this comment.
| // clientAddressValidated immediately disables the amplification limit. | |
| // clientAddressValidated has no effect for a client. | |
| // If the address was validated, the amplification limit doesn't apply. It has no effect for a client. |
|
@marten-seemann please check again |
|
|
||
| It("do not limits the window", func() { | ||
| Expect(handler.SendMode()).To(Equal(SendAny)) | ||
| Expect(handler.peerAddressValidated).To(Equal(true)) |
There was a problem hiding this comment.
It would be better to test the expected behavior than to make assertion on implementation details.
|
@birneee Are you still working on this? Would love to get this merged! |
|
@marten-seemann i just worked on it :) |
There was a problem hiding this comment.
So, I messed up this review. Sorry for that!
I don't think it's correct to use the result of AcceptToken. Consider the following scenario: A server only wants to enable Retries when it's under load. Therefore, it will return true from the AcceptToken callback when it is not under load. However, that doesn't mean that we should trust the address and send an unlimited amount of data there.
|
@marten-seemann you are right, because the AcceptToken can be configured. The defaultAcceptToken function however checks the validity. So we could either use a function similar to defaultAcceptToken by calling it directly or add a new config ValidateToken, where by default AcceptToken = ValidateToken. But I am not sure if ValidateToken should be configurable. |
|
@marten-seemann please check |
|
@birneee We just completely rewrote the Retry API in #3501 (see #3494 for design considerations). As the token validation logic now lives inside quic-go, this should make it possible to disable the anti-amplification limit when we successfully validated a token. Do you want to rebase and modify your PR to take advantage of this? Fixing #3319 would be a super valuable contribution for everyone who actually wants to take full advantage of 0-RTT. |
birneee
force-pushed
the
3319-amplification
branch
from
862952d
to
3e2f97e
Compare
birneee
force-pushed
the
3319-amplification
branch
from
3e2f97e
to
47b4b1c
Compare
|
@marten-seemann it is rebased now |
Fixes #3319.