-
Notifications
You must be signed in to change notification settings - Fork 204
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
handle reordered NEW_CONNECTION_ID frames #3202
handle reordered NEW_CONNECTION_ID frames #3202
Conversation
Co-Authored-By: Martin Thomson <mt@lowentropy.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small editorial suggestion, but LG
It might be cleaner to say that when you receive RPT, you need to retire the indicated sequence numbers even if you haven't received them yet. Then you can just discard the NCID frames upon arrival. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small tweak to @ianswett's wording suggestion, but otherwise looks good!
@MikeBishop That brings with it a request to replace them, which could get a little messy if I sent RPT of 10000, it seems like it would cut down on useless traffic if we didn't generate the RCID frames until someone actually tries to use them? |
The situation is a bit perverse to begin with. RPT isn't permitted to be >= the sequence number of the new CID, which means they've already issued you 10k CIDs for that situation to be real. If the peer revokes a block of 10k CIDs via RPT, they're expecting to get 10k RCID frames, minus any that might already have been sent. Yes, that means they might then issue a similarly large block of CIDs, which you will then forget silently and the insanity will play itself out again the next time they revoke a large batch. This glut is best managed by using the existing TP and not permitting the peer to send you an unlimited number of CIDs. This also argues, perhaps, for making the default something more sensible than "infinite" in #3197. |
@MikeBishop There's a DoS attack here. If the attacker doesn't comply with the rule that new CIDs have to be issued in order (which is one of the non-enforceable MUSTs in the spec) and issues you CID 1000000 while with the same value for Retire Prior To, the endpoint would have to send a million RETIRE_CONNECTION_ID frames. I think the only way to reliable avoid this DoS is to only send RETIRE_CONNECTION_ID for CIDs that you actually received. |
Co-Authored-By: Martin Thomson <mt@lowentropy.net>
Larger change, but one solution to that might be permitting a RCID frame to retire ranges instead of a single CID. |
@MikeBishop what about RETIRE_CONNECTION_ID meaning that all IDs equal or less than that one have been retired? That would force them to be used in order, which seems like a feature in my opinion. |
We had that at one point, didn't we? It is a feature until you start doing probes on other paths. If you use a CID on a different path, then want to retire it, that also retires a CID you might still be using on the main path. And it's no problem if you have another CID to roll forward to, but it forces you to burn more CIDs than you otherwise would. I believe there was also an objection to that model that the gratuitous CID rolls leaked signals about use of other paths. |
So I think if we were to do that, it would have to be a flag that indicates whether it's a point retirement or an "everything up to" retirement. That would require renumbering some frames, which would be a pain. 🤔 |
@MikeBishop I'm not sure if there's a problem here that we need to solve. The DoS that I described only works if an endpoint sends RETIRE_CONNECTION_ID frames for CIDs that it hasn't received / doesn't remember any more. As long as you only retire CIDs that you're actually keeping track of (as the spec currently says), the DoS doesn't work. |
Fixes #3194.