h2: Rapid reset mitigations (7.3) #4011
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This adds parameters h2_rst_allowance and h2_rst_allowance_period, which govern the rate of which we allow clients to reset h/2 streams. If the limit is exceeded the connection is closed. Mitigates: varnishcache#3996
Only RST frames received earlier than this duration will be considered rapid.
It was particularly hard to follow once we reach client c3.
The goal is for top-level transports to report whether the client is still present or not.
Once a client is reportedly gone, processing its VCL task(s) is just a waste of resources. The execution of client-facing VCL is intercepted and an artificial return(fail) is returned in that scenario. Thanks to the introduction of the universal return(fail) proper error handling and resource tear down is already in place, which makes this change safe modulus unknown bugs. This adds a circuit breaker anywhere in the client state machine where there is VCL execution. A new Reset time stamp is logged to convey when a task does not complete because the client is gone. This is a good complement to the walk away feature and its original circuit breaker for the waiting list, but this has not been integrated yet. While the request is technically failed, it won't increase the vcl_fail counter, and a new req_reset counter is incremented. This new behavior is guarded by a new vcl_req_reset feature flag, enabled by default. Refs varnishcache#3835 Refs 61a15cb Refs e5efc2c Refs ba54dc9 Refs 6f50a00 Refs b881699 Conflicts: include/tbl/feature_bits.h
The error check is not performed in a critical section to avoid contention, at the risk of not seeing the error until the next transport poll.
With varnishcache#3998 we need to ensure streams are not going to skip vcl_recv if reset faster than reaching this step for the request task. The alternative to prevent the vcl_req_reset feature from interfering is to simply disable it.
Noticed while porting varnishcache#3998 to the 6.0 branch with a varnishtest more sensitive to timing.
This will allow per-session adjustments and also significantly lower the risk of inconsistent calculations in the rate limit code during parameter changes. Ref varnishcache#3996
Conflicts: vmod/automake_boilerplate_h2.am vmod/vmod_h2.vcc
as agreed on IRC.
we can not make the parameter const because API.
Conflicts: vmod/vmod_h2.c
nigoroll
approved these changes
Oct 23, 2023
Signed-off-by: Dridi Boukelmoune <[email protected]>
Signed-off-by: Dridi Boukelmoune <[email protected]>
Signed-off-by: Dridi Boukelmoune <[email protected]>
Control characters will be caught by vct_ishdrval() anyways, but this condition would also reject allowed obs-text non-ASCII characters. Signed-off-by: Dridi Boukelmoune <[email protected]>
Adds coverage for tab characters at start/end of field value. Regarding the "fo o" " bar" header, it cumulates an error in the name and another in the value, but only one of them will trigger the expected PROTOCOL_ERROR. Only the invalid "fo o" is checked now, and the other error is part of the new coverage. Signed-off-by: Dridi Boukelmoune <[email protected]>
|
Added patch series containing the patch requested in #3996 (comment). |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Port of #3997, #3998, #3999 and adjacent commits to the 7.3 branch.
One trivial conflict, and a more consequential conflict for the lack of
$Restrictin this branch.