Skip to content

add Transport config options to limit the number of handshakes#4248

Merged
marten-seemann merged 3 commits intomasterfrom
retry-config
Jan 23, 2024
Merged

add Transport config options to limit the number of handshakes#4248
marten-seemann merged 3 commits intomasterfrom
retry-config

Conversation

@marten-seemann
Copy link
Copy Markdown
Member

@marten-seemann marten-seemann commented Jan 18, 2024

Fixes #3549.

@marten-seemann marten-seemann added this to the v0.42 milestone Jan 18, 2024
@marten-seemann marten-seemann force-pushed the server-accept-closed-connections branch from 908a1ae to 28c730d Compare January 19, 2024 06:29
@marten-seemann marten-seemann changed the base branch from server-accept-closed-connections to master January 19, 2024 06:49
@codecov
Copy link
Copy Markdown

codecov Bot commented Jan 19, 2024 •
edited
Loading

Codecov Report

Attention: 8 lines in your changes are missing coverage. Please review.

Comparison is base (594440b) 84.09% compared to head (ded0673) 84.08%.
Report is 1 commits behind head on master.

Files Patch % Lines
server.go 88.57% 6 Missing and 2 partials ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #4248      +/-   ##
==========================================
- Coverage   84.09%   84.08%   -0.00%     
==========================================
  Files         150      150              
  Lines       15401    15431      +30     
==========================================
+ Hits        12950    12975      +25     
- Misses       1950     1953       +3     
- Partials      501      503       +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@marten-seemann marten-seemann mentioned this pull request Jan 19, 2024
Merged
marten-seemann
marten-seemann commented Jan 19, 2024
View reviewed changes
Comment thread transport.go Outdated

const (
defaultMaxNumUnvalidatedHandshakes = 32
defaultMaxNumHandshakes = 64
Copy link
Copy Markdown
Member Author

@marten-seemann marten-seemann Jan 19, 2024

Choose a reason for hiding this comment

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

These values seem unreasonably low.

Copy link
Copy Markdown
Member Author

@marten-seemann marten-seemann Jan 21, 2024

Choose a reason for hiding this comment

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

Maybe we should set defaultMaxNumHandshakes to unlimited? It's really hard to pick a limit that fits for all.

Copy link
Copy Markdown
Collaborator

@sukunrt sukunrt Jan 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

64 feels too low for defaultMaxNumHandshakes. This number is way too easy to DoS for even very small machines. I think a high value > 1000 is better. I think unlimited is also better than this being < 1000.

Copy link
Copy Markdown
Member Author

@marten-seemann marten-seemann Jan 23, 2024

Choose a reason for hiding this comment

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

Changed to unlimited in 77ede93. Not entirely happy with that, but we can always change it later once we come up with a better value.

@marten-seemann marten-seemann mentioned this pull request Jan 21, 2024
Open
sukunrt
sukunrt requested changes Jan 22, 2024
View reviewed changes
Comment thread server.go Outdated
@marten-seemann marten-seemann merged commit 892851e into master Jan 23, 2024
@marten-seemann marten-seemann deleted the retry-config branch January 23, 2024 05:06
@marten-seemann marten-seemann mentioned this pull request Jan 23, 2024
Closed
mgjeong pushed a commit to mgjeong/quic-go that referenced this pull request Feb 13, 2024
@marten-seemann @mgjeong
add Transport config options to limit the number of handshakes (quic-…
09adfa6 
…go#4248)

* add Transport config options to limit the number of handshakes

* fix accounting for failed handshakes

* increase handshake limits, improve documentation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sukunrt sukunrt sukunrt approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v0.42

Development

Successfully merging this pull request may close these issues.

How to best implement address validation callback

2 participants

@marten-seemann @sukunrt