-
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
Discarding an invalid Initial is allowed #4359
Conversation
But we managed to hide that in the security considerations. Closes #4350.
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.
LGTM. Regarding the MUST clause for the two bits, adding "unless stated otherwise" works for me too.
This PR is fine, but I agree with @huitema above. I think we should add the exception and a reference to this text in 17.2. |
How many other rules do we need to create exemptions for? What if the Initial contains a bad frame? What if the CRYPTO frame is too long? |
If you don't want to have the discussion about exceptions, then write SHOULD instead of MUST. |
Rather than enumerate all the ways in which processing rules might have exceptions for Initial packets, just include a blanket override here.
I don't want to go to SHOULD. Because for other packet types, MUST is what we want. And finding and creating exceptions is impossible. So I have added a "this permission overrides rules that mandate a connection error". |
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.
I think what @martinthomson says makes sense. This is a "MUST unless" however we cannot have that stated at every point. Having a catch-all statement makes sense.
I tend to wonder if the catch-all statement should be in the Protected Packets section or in the Error Handling section, but the PR looks good regardless.
That's a good suggestion. I moved it. Then it is more likely to be seen. |
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.
Thanks for the new text, @martinthomson -- this is good.
But we managed to hide that in the security considerations.
Closes #4350.