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

Address Validation Token does not disable Anti-Amplification Limit #3319

Closed
birneee opened this issue Jan 19, 2022 · 6 comments · Fixed by #3326
Closed

Address Validation Token does not disable Anti-Amplification Limit #3319

birneee opened this issue Jan 19, 2022 · 6 comments · Fixed by #3326

Comments

@birneee
Copy link
Contributor

birneee commented Jan 19, 2022

Currently ackhandler.sentPacketHandler always waits for a handshake packet from the client to disable the anti-amplification limit.
But the anti-amplification limit could be disabled when an address validation token is provide by the client.
This would be beneficial for servers responding to 0-RTT requests.
See https://datatracker.ietf.org/doc/html/rfc9000#section-8.1

@marten-seemann
Copy link
Member

That sounds reasonable. Do you want to submit a PR?

@birneee
Copy link
Contributor Author

birneee commented Jan 28, 2022

Yes, I will do that

@birneee
Copy link
Contributor Author

birneee commented Jan 28, 2022

To verify that this is a problem, I made a 0-RTT connection on a 1 s RTT path.

with anti-amplification limit:

second 1: 29.49 kbit/s, bytes received: 3.689 kB, packets received: 15
second 2: 321.46 kbit/s, bytes received: 40.204 kB, packets received: 34
second 3: 644.13 kbit/s, bytes received: 80.539 kB, packets received: 66
second 4: 1.28 Mbit/s, bytes received: 161.025 kB, packets received: 133
second 5: 2.53 Mbit/s, bytes received: 316.637 kB, packets received: 261

without anti-amplification limit:

second 1: 304.23 kbit/s, bytes received: 38.067 kB, packets received: 43
second 2: 552.94 kbit/s, bytes received: 69.187 kB, packets received: 58
second 3: 1.11 Mbit/s, bytes received: 139.661 kB, packets received: 115
second 4: 2.23 Mbit/s, bytes received: 279.827 kB, packets received: 231
second 5: 3.28 Mbit/s, bytes received: 410.387 kB, packets received: 341

@birneee
Copy link
Contributor Author

birneee commented Jan 28, 2022

#3326

@marten-seemann
Copy link
Member

Copying from my response on the PR:

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.

We probably need to improve the AcceptToken callback. One option would be to have a ternary return value.

@birneee
Copy link
Contributor Author

birneee commented Feb 23, 2022

We probably need to improve the AcceptToken callback. One option would be to have a ternary return value.

@marten-seemann I would rather not combine AcceptToken and the address token validation, as one might want to configure one but not the other.
I would rather make the TokenValidity and RetryTokenValidity configurable, e.g. by adding a new IsTokenValid callback.
But I think this should be an extra issue.
For this issue i think it is ok to stick with the default values.
I have updated #3326 by f82e447.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants