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

Retry service handling of non-Initial #96

Closed
martinduke opened this issue Mar 4, 2021 · 4 comments · Fixed by #97
Closed

Retry service handling of non-Initial #96

martinduke opened this issue Mar 4, 2021 · 4 comments · Fixed by #97

Comments

@martinduke
Copy link
Contributor

In Section 7:
"Retry services MUST forward all QUIC packets that are not of type Initial or 0-RTT. Other packet types might involve changed IP addresses or connection IDs, so it is not practical for Retry Services to identify such packets as valid or invalid."

MUST is too strong. If it keeps any state (i.e. tracking 4-tuples) it can drop non-initial packets. (However, this would make migration not work).

If the Retry Service is in front of a QUIC-LB load balancer, the LB will drop random 1-RTT packets but not Handshake and 0-RTT unless it is version-aware, so passing 1-RTT is "safe."

If the Retry Service is behind the load balancer, which is probably better because LBs are often NATs, then random 1-RTT is already dropped and it is safe to forward 1-RTT to preserve migration.

If there's a non QUIC-LB load balancers, migration doesn't work anyway; might as well drop it.

If it's a single server and the CIDs are random, admitting 1-RTT is weakening the DoS defense.

And then there is the issue with QUIC versions and admitting/dropping them, which is hard to adjudicate with short headers.

@nibanks
Copy link
Member

nibanks commented Mar 5, 2021

I don't know. Do you have a case where Retry services should not forward those packets? I don't want this to be a source of ossification because of a possibly incorrect assumption about placement of the retry service in the pipeline.

@martinduke
Copy link
Contributor Author

Well, the reason to not forward them is to block DDoS vectors. A garbage Handshake or 1-RTT packets is at least easily discarded by the server without much in the way of processing. I would actually appreciate some input from your Azure contacts about this; is filtering non-SYN packets important today?

@nibanks
Copy link
Member

nibanks commented Mar 5, 2021

Here's the data I can provide: As I understand it, the DDoS would be in front of the LB. It's all in hardware so it's update cycle could be very long, so any logic added there could effectively be written in stone for many years. So, I really want to be careful about any assumptions here. If we get it wrong for a future version, either all of Azure might end up breaking/preventing future versions/features or we'd have to completely disable and lose the protection.

On the topic of dropping garbage/invalid packets, I'd be Ok with that. Perhaps we could change the text to the following?

Retry services MUST forward all valid QUIC packets that are not of type Initial or 0-RTT. Other packet types might involve changed IP addresses or connection IDs, so it is not practical for Retry Services to identify such packets as valid or invalid.

This adds some wiggle room for what is considered "valid" or not. Obviously if the header is garbage it can be tossed. If a device knows a particular CID is invalid, then it could drop it. I'd just strongly caution any "smarts" here because of the possibility of changes in the future.

@martinduke
Copy link
Contributor Author

If "in front of the LB" is cancnical use case, I can shade the language towards that.

However, this makes me wonder about the shared-state case. My perception is that combining a NAT with an LB is quite common; will shared token verification work if the server and retry service are seeing different client IP addresses? This calls into question the entire shared-state concept.

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 a pull request may close this issue.

2 participants