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

implement a new API to let servers control client address verification #3501

Merged
merged 3 commits into from Aug 13, 2022

Conversation

marten-seemann
Copy link
Member

Fixes #3494.

This PR makes 2 changes:

  1. It implements the new API suggested in redesign the Retry Token API #3494.
  2. It disables address verification by default. This means that, unless configured otherwise, the QUIC handshake will take a single RTT. This is in line with providing safe defaults, as we implement the 3x amplification protection.

@mholt, would be happy about your feedback on this PR!

@marten-seemann marten-seemann force-pushed the new-address-validation-api branch 2 times, most recently from 8bc1532 to 5da735a Compare August 12, 2022 08:22
@mholt
Copy link
Contributor

mholt commented Aug 12, 2022

This looks better!

So basically, if I understand right, I would implement RequireAddressValidation(net.Addr) bool to return true if the server is under load, correct? And I don't need to call any sort of defaultAcceptToken() function in addition?

@marten-seemann
Copy link
Member Author

So basically, if I understand right, I would implement RequireAddressValidation(net.Addr) bool to return true if the server is under load, correct? And I don't need to call any sort of defaultAcceptToken() function in addition?

Exactly! All the token validation logic is now handled under the hood (tunable using the MaxTokenAge and MaxRetryTokenAge). All you have to do is say if you want to perform address validation in the first place (at the cost of 1 additional RTT during the handshake).

Copy link
Contributor

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Sounds like just what I was hoping for then. Thank you!

@marten-seemann marten-seemann force-pushed the new-address-validation-api branch 2 times, most recently from df8d9a2 to 2ee3bcc Compare August 13, 2022 13:31
We should provide safe defaults. Since we implement the 3x amplification
limit, disabling address validation is not unsafe, and will save 1 RTT
for every handshake for applications that don't explicitely configure
Retries.
@codecov
Copy link

codecov bot commented Aug 13, 2022

Codecov Report

Merging #3501 (bbfb7bd) into master (556a6e2) will decrease coverage by 0.00%.
The diff coverage is 94.74%.

@@            Coverage Diff             @@
##           master    #3501      +/-   ##
==========================================
- Coverage   85.69%   85.69%   -0.00%     
==========================================
  Files         136      136              
  Lines        9995     9992       -3     
==========================================
- Hits         8565     8562       -3     
  Misses       1052     1052              
  Partials      378      378              
Impacted Files Coverage Δ
server.go 80.60% <88.89%> (-0.56%) ⬇️
config.go 100.00% <100.00%> (ø)
connection.go 77.66% <100.00%> (+0.10%) ⬆️
internal/handshake/token_generator.go 76.60% <100.00%> (-1.84%) ⬇️

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

@mholt
Copy link
Contributor

mholt commented Aug 13, 2022

@marten-seemann Excellent! I updated caddyserver/caddy#4707 and it seems to be working (haven't tested the "highLoad" scenario but HTTP/3 works by default).

@marten-seemann marten-seemann deleted the new-address-validation-api branch August 15, 2022 09:02
storjBuildBot pushed a commit to storj/common that referenced this pull request Feb 2, 2023
I need this upgraded to avoid dependency conflict in
storj.io/ipfs-go-ds-storj

Note that AcceptToken has been removed in recent guic-go releases, and
address verification is disabled by default. So we don't need to set it
anyway.

Details here: quic-go/quic-go#3501

Change-Id: I6682ac45e4de8d36002630fff75f6d6072c5010d
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.

redesign the Retry Token API
3 participants