Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable anti-amplification limit by address validation token #3326

Merged
merged 1 commit into from
Aug 20, 2022

Conversation

birneee
Copy link
Contributor

@birneee birneee commented Jan 28, 2022

Fixes #3319.

@codecov
Copy link

codecov bot commented Jan 28, 2022

Codecov Report

Merging #3326 (3e2f97e) into master (60bbe92) will increase coverage by 0.00%.
The diff coverage is 83.33%.

❗ Current head 3e2f97e differs from pull request most recent head 862952d. Consider uploading reports for the commit 862952d to get more accurate results

@@           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           
Impacted Files Coverage Δ
internal/ackhandler/ackhandler.go 0.00% <0.00%> (ø)
connection.go 77.40% <100.00%> (+0.04%) ⬆️
internal/ackhandler/sent_packet_handler.go 78.01% <100.00%> (ø)
server.go 81.21% <100.00%> (+0.05%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good already. Can you add a unit test in the ackhandler package?

internal/ackhandler/ackhandler.go Outdated Show resolved Hide resolved
@@ -15,7 +17,8 @@ func NewAckHandler(
tracer logging.ConnectionTracer,
logger utils.Logger,
version protocol.VersionNumber,
clientAddressValidated bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here too.

session.go Outdated
@@ -249,6 +249,7 @@ var newSession = func(
tracingID uint64,
logger utils.Logger,
v protocol.VersionNumber,
clientAddressValidated bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: move this after enable0RTT.

Comment on lines 105 to 106
// clientAddressValidated immediately disables the amplification limit.
// clientAddressValidated has no effect for a client.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// 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.

@birneee
Copy link
Contributor Author

birneee commented Feb 1, 2022

@marten-seemann please check again

internal/ackhandler/sent_packet_handler.go Outdated Show resolved Hide resolved

It("do not limits the window", func() {
Expect(handler.SendMode()).To(Equal(SendAny))
Expect(handler.peerAddressValidated).To(Equal(true))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be better to test the expected behavior than to make assertion on implementation details.

@marten-seemann
Copy link
Member

@birneee Are you still working on this? Would love to get this merged!

@birneee
Copy link
Contributor Author

birneee commented Feb 20, 2022

@marten-seemann i just worked on it :)

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@birneee
Copy link
Contributor Author

birneee commented Feb 21, 2022

@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.

@birneee
Copy link
Contributor Author

birneee commented May 13, 2022

@marten-seemann please check
I think, configuring token validity is another issue.

@marten-seemann
Copy link
Member

@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
Copy link
Contributor Author

birneee commented Aug 16, 2022

@marten-seemann it is rebased now

Copy link
Member

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very clean! Thank you @birneee!

@marten-seemann marten-seemann merged commit 7da024d into quic-go:master Aug 20, 2022
@birneee birneee deleted the 3319-amplification branch December 1, 2022 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Address Validation Token does not disable Anti-Amplification Limit
2 participants