From b9074c6842497e5e7ad2f5475213f67b84a267f3 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Mon, 16 Oct 2023 19:14:56 +0200 Subject: [PATCH 01/29] Bump cli_limit to fit param.show -j with more parameters coming --- include/tbl/params.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/tbl/params.h b/include/tbl/params.h index cf46b165846..35266275f3a 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -316,7 +316,7 @@ PARAM_SIMPLE( /* type */ bytes_u, /* min */ "128b", /* max */ "99999999b", - /* def */ "48k", + /* def */ "64k", /* units */ "bytes", /* descr */ "Maximum size of CLI response. If the response exceeds this " From 5cdffa18fff34e14f2dee99f2ee50b6fa9a3b606 Mon Sep 17 00:00:00 2001 From: Dag Haavi Finstad Date: Thu, 12 Oct 2023 10:50:55 +0200 Subject: [PATCH 02/29] h2: Add a rate limit facility for h/2 RST handling 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: #3996 --- bin/varnishd/http2/cache_http2.h | 2 + bin/varnishd/http2/cache_http2_proto.c | 35 ++++++++++++++++- bin/varnishd/http2/cache_http2_session.c | 3 ++ bin/varnishtest/tests/r03996.vtc | 49 ++++++++++++++++++++++++ include/tbl/params.h | 28 ++++++++++++++ 5 files changed, 116 insertions(+), 1 deletion(-) create mode 100644 bin/varnishtest/tests/r03996.vtc diff --git a/bin/varnishd/http2/cache_http2.h b/bin/varnishd/http2/cache_http2.h index eb6e8f6a9f3..9457c99f2dd 100644 --- a/bin/varnishd/http2/cache_http2.h +++ b/bin/varnishd/http2/cache_http2.h @@ -193,6 +193,8 @@ struct h2_sess { VTAILQ_HEAD(,h2_req) txqueue; h2_error error; + double rst_budget; + vtim_real last_rst; }; #define ASSERT_RXTHR(h2) do {assert(h2->rxthr == pthread_self());} while(0) diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c index c1ced04b680..3f900b0a7ce 100644 --- a/bin/varnishd/http2/cache_http2_proto.c +++ b/bin/varnishd/http2/cache_http2_proto.c @@ -317,9 +317,41 @@ h2_rx_push_promise(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2) /********************************************************************** */ +static h2_error +h2_rapid_reset(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2) +{ + vtim_real now; + vtim_dur d; + + CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); + ASSERT_RXTHR(h2); + CHECK_OBJ_NOTNULL(r2, H2_REQ_MAGIC); + + if (cache_param->h2_rapid_reset_limit == 0) + return (0); + + now = VTIM_real(); + d = now - h2->last_rst; + h2->rst_budget += cache_param->h2_rapid_reset_limit * d / + cache_param->h2_rapid_reset_period; + h2->rst_budget = vmin_t(double, h2->rst_budget, + cache_param->h2_rapid_reset_limit); + h2->last_rst = now; + + if (h2->rst_budget < 1.0) { + Lck_Lock(&h2->sess->mtx); + VSLb(h2->vsl, SLT_Error, "H2: Hit RST limit. Closing session."); + Lck_Unlock(&h2->sess->mtx); + return (H2CE_ENHANCE_YOUR_CALM); + } + h2->rst_budget -= 1.0; + return (0); +} + static h2_error v_matchproto_(h2_rxframe_f) h2_rx_rst_stream(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2) { + h2_error h2e; CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); ASSERT_RXTHR(h2); @@ -329,8 +361,9 @@ h2_rx_rst_stream(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2) return (H2CE_FRAME_SIZE_ERROR); if (r2 == NULL) return (0); + h2e = h2_rapid_reset(wrk, h2, r2); h2_kill_req(wrk, h2, r2, h2_streamerror(vbe32dec(h2->rxf_data))); - return (0); + return (h2e); } /********************************************************************** diff --git a/bin/varnishd/http2/cache_http2_session.c b/bin/varnishd/http2/cache_http2_session.c index 846b319f183..b37dd105c89 100644 --- a/bin/varnishd/http2/cache_http2_session.c +++ b/bin/varnishd/http2/cache_http2_session.c @@ -127,6 +127,9 @@ h2_init_sess(struct sess *sp, h2_local_settings(&h2->local_settings); h2->remote_settings = H2_proto_settings; h2->decode = decode; + h2->rst_budget = cache_param->h2_rapid_reset_limit; + h2->last_rst = sp->t_open; + AZ(isnan(h2->last_rst)); AZ(VHT_Init(h2->dectbl, h2->local_settings.header_table_size)); diff --git a/bin/varnishtest/tests/r03996.vtc b/bin/varnishtest/tests/r03996.vtc new file mode 100644 index 00000000000..5864c837f00 --- /dev/null +++ b/bin/varnishtest/tests/r03996.vtc @@ -0,0 +1,49 @@ +varnishtest "h2 rapid reset" + +barrier b1 sock 5 + +server s1 { + rxreq + txresp +} -start + +varnish v1 -cliok "param.set feature +http2" +varnish v1 -cliok "param.set debug +syncvsl" +varnish v1 -cliok "param.set h2_rapid_reset_limit 3" + +varnish v1 -vcl+backend { + import vtc; + + sub vcl_recv { + vtc.barrier_sync("${b1_sock}"); + } + +} -start + +client c1 { + stream 0 { + rxgoaway + expect goaway.err == ENHANCE_YOUR_CALM + } -start + + stream 1 { + txreq + txrst + } -run + stream 3 { + txreq + txrst + } -run + stream 5 { + txreq + txrst + } -run + stream 7 { + txreq + txrst + } -run + + barrier b1 sync + stream 0 -wait +} -run + diff --git a/include/tbl/params.h b/include/tbl/params.h index 35266275f3a..214fd21ffcc 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1243,6 +1243,34 @@ PARAM_SIMPLE( "HTTP2 maximum size of an uncompressed header list." ) +PARAM_SIMPLE( + /* name */ h2_rapid_reset_limit, + /* typ */ uint, + /* min */ "0", + /* max */ NULL, + /* def */ "0", + /* units */ NULL, + /* descr */ + "HTTP2 RST Allowance.\n" + "Specifies the maximum number of allowed stream resets issued by\n" + "a client over a time period before the connection is closed.\n" + "Setting this parameter to 0 disables the limit.", + /* flags */ EXPERIMENTAL, +) + +PARAM_SIMPLE( + /* name */ h2_rapid_reset_period, + /* typ */ timeout, + /* min */ "1.000", + /* max */ NULL, + /* def */ "60.000", + /* units */ "seconds", + /* descr */ + "HTTP2 sliding window duration for h2_rapid_reset_limit.", + /* flags */ EXPERIMENTAL|WIZARD, +) + + /*-------------------------------------------------------------------- * Memory pool parameters */ From 54146bc2a0a94ba402bfb23c65ede2825c72cfb2 Mon Sep 17 00:00:00 2001 From: Dag Haavi Finstad Date: Tue, 17 Oct 2023 11:20:11 +0200 Subject: [PATCH 03/29] Introduce RAPID_RESET as a sess_close reason --- bin/varnishd/http2/cache_http2_proto.c | 3 ++- bin/varnishtest/tests/r03996.vtc | 1 + include/tbl/h2_error.h | 12 ++++++++++++ include/tbl/sess_close.h | 1 + lib/libvsc/VSC_main.vsc | 8 ++++++++ 5 files changed, 24 insertions(+), 1 deletion(-) diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c index 3f900b0a7ce..ea4429569ad 100644 --- a/bin/varnishd/http2/cache_http2_proto.c +++ b/bin/varnishd/http2/cache_http2_proto.c @@ -47,6 +47,7 @@ #include "vtcp.h" #include "vtim.h" +#define H2_CUSTOM_ERRORS #define H2EC1(U,v,r,d) const struct h2_error_s H2CE_##U[1] = {{#U,d,v,0,1,r}}; #define H2EC2(U,v,r,d) const struct h2_error_s H2SE_##U[1] = {{#U,d,v,1,0,r}}; #define H2EC3(U,v,r,d) H2EC1(U,v,r,d) H2EC2(U,v,r,d) @@ -342,7 +343,7 @@ h2_rapid_reset(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2) Lck_Lock(&h2->sess->mtx); VSLb(h2->vsl, SLT_Error, "H2: Hit RST limit. Closing session."); Lck_Unlock(&h2->sess->mtx); - return (H2CE_ENHANCE_YOUR_CALM); + return (H2CE_RAPID_RESET); } h2->rst_budget -= 1.0; return (0); diff --git a/bin/varnishtest/tests/r03996.vtc b/bin/varnishtest/tests/r03996.vtc index 5864c837f00..6feef496303 100644 --- a/bin/varnishtest/tests/r03996.vtc +++ b/bin/varnishtest/tests/r03996.vtc @@ -47,3 +47,4 @@ client c1 { stream 0 -wait } -run +varnish v1 -expect sc_rapid_reset == 1 diff --git a/include/tbl/h2_error.h b/include/tbl/h2_error.h index 2cd06f01107..c6c6bed8918 100644 --- a/include/tbl/h2_error.h +++ b/include/tbl/h2_error.h @@ -147,5 +147,17 @@ H2_ERROR( /* descr */ "Use HTTP/1.1 for the request" ) +#ifdef H2_CUSTOM_ERRORS +H2_ERROR( + /* name */ RAPID_RESET, + /* val */ 11, /* ENHANCE_YOUR_CALM */ + /* types */ 1, + /* reason */ SC_RAPID_RESET, + /* descr */ "http/2 rapid reset detected" +) + +# undef H2_CUSTOM_ERRORS +#endif + #undef H2_ERROR /*lint -restore */ diff --git a/include/tbl/sess_close.h b/include/tbl/sess_close.h index b91c93951c9..f15d34a64aa 100644 --- a/include/tbl/sess_close.h +++ b/include/tbl/sess_close.h @@ -50,6 +50,7 @@ SESS_CLOSE(PIPE_OVERFLOW, pipe_overflow,1, "Session pipe overflow") SESS_CLOSE(RANGE_SHORT, range_short, 1, "Insufficient data for range") SESS_CLOSE(REQ_HTTP20, req_http20, 1, "HTTP2 not accepted") SESS_CLOSE(VCL_FAILURE, vcl_failure, 1, "VCL failure") +SESS_CLOSE(RAPID_RESET, rapid_reset, 1, "HTTP2 rapid reset") #undef SESS_CLOSE /*lint -restore */ diff --git a/lib/libvsc/VSC_main.vsc b/lib/libvsc/VSC_main.vsc index 047218ad88a..a935a569a34 100644 --- a/lib/libvsc/VSC_main.vsc +++ b/lib/libvsc/VSC_main.vsc @@ -631,6 +631,14 @@ Number of session closes with Error VCL_FAILURE (VCL failure) +.. varnish_vsc:: sc_rapid_reset + :level: diag + :oneliner: Session Err RAPID_RESET + + Number of times we failed an http/2 session because it hit its + configured limits for the number of permitted rapid stream + resets. + .. varnish_vsc:: client_resp_500 :level: diag :group: wrk From 74bbb482b0e6c66fb714dfd55d953217bce32f65 Mon Sep 17 00:00:00 2001 From: Dag Haavi Finstad Date: Tue, 17 Oct 2023 11:27:44 +0200 Subject: [PATCH 04/29] Add param h2_rapid_reset Only RST frames received earlier than this duration will be considered rapid. --- bin/varnishd/http2/cache_http2_proto.c | 5 +++++ bin/varnishtest/tests/r03996.vtc | 1 + include/tbl/params.h | 15 ++++++++++++++- 3 files changed, 20 insertions(+), 1 deletion(-) diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c index ea4429569ad..a006d708f9d 100644 --- a/bin/varnishd/http2/cache_http2_proto.c +++ b/bin/varnishd/http2/cache_http2_proto.c @@ -332,6 +332,11 @@ h2_rapid_reset(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2) return (0); now = VTIM_real(); + CHECK_OBJ_NOTNULL(r2->req, REQ_MAGIC); + AN(r2->req->t_first); + if (now - r2->req->t_first > cache_param->h2_rapid_reset) + return (0); + d = now - h2->last_rst; h2->rst_budget += cache_param->h2_rapid_reset_limit * d / cache_param->h2_rapid_reset_period; diff --git a/bin/varnishtest/tests/r03996.vtc b/bin/varnishtest/tests/r03996.vtc index 6feef496303..3fee3706c35 100644 --- a/bin/varnishtest/tests/r03996.vtc +++ b/bin/varnishtest/tests/r03996.vtc @@ -10,6 +10,7 @@ server s1 { varnish v1 -cliok "param.set feature +http2" varnish v1 -cliok "param.set debug +syncvsl" varnish v1 -cliok "param.set h2_rapid_reset_limit 3" +varnish v1 -cliok "param.set h2_rapid_reset 5" varnish v1 -vcl+backend { import vtc; diff --git a/include/tbl/params.h b/include/tbl/params.h index 214fd21ffcc..fb3b95131c0 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1243,6 +1243,20 @@ PARAM_SIMPLE( "HTTP2 maximum size of an uncompressed header list." ) +PARAM_SIMPLE( + /* name */ h2_rapid_reset, + /* typ */ timeout, + /* min */ "0.000", + /* max */ NULL, + /* def */ "1.000", + /* units */ "seconds", + /* descr */ + "The upper threshold for how rapid an http/2 RST has to come for " + "it to be treated as suspect and subjected to the rate limits " + "specified by h2_rapid_reset_limit and h2_rapid_reset_period.", + /* flags */ EXPERIMENTAL, +) + PARAM_SIMPLE( /* name */ h2_rapid_reset_limit, /* typ */ uint, @@ -1270,7 +1284,6 @@ PARAM_SIMPLE( /* flags */ EXPERIMENTAL|WIZARD, ) - /*-------------------------------------------------------------------- * Memory pool parameters */ From de3a875d74264c3b8874662b51574a702cfbb7bb Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Tue, 17 Oct 2023 13:30:01 +0200 Subject: [PATCH 05/29] Polish h2_rapid_reset docs --- include/tbl/params.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/include/tbl/params.h b/include/tbl/params.h index fb3b95131c0..03363327327 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1251,9 +1251,10 @@ PARAM_SIMPLE( /* def */ "1.000", /* units */ "seconds", /* descr */ - "The upper threshold for how rapid an http/2 RST has to come for " - "it to be treated as suspect and subjected to the rate limits " - "specified by h2_rapid_reset_limit and h2_rapid_reset_period.", + "The upper threshold for how rapid an http/2 RST has to come after " + "a HEADERS frame for it to be treated as suspect and subjected to " + "the rate limits specified by h2_rapid_reset_limit and " + "h2_rapid_reset_period.", /* flags */ EXPERIMENTAL, ) From d241f134972214cf0d59f5e88fccf13f75e2ec62 Mon Sep 17 00:00:00 2001 From: Poul-Henning Kamp Date: Tue, 17 Oct 2023 14:35:22 +0000 Subject: [PATCH 06/29] Flexelinting --- bin/varnishd/http2/cache_http2.h | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/varnishd/http2/cache_http2.h b/bin/varnishd/http2/cache_http2.h index 9457c99f2dd..65ea1d2df8d 100644 --- a/bin/varnishd/http2/cache_http2.h +++ b/bin/varnishd/http2/cache_http2.h @@ -49,6 +49,7 @@ struct h2_error_s { typedef const struct h2_error_s *h2_error; +#define H2_CUSTOM_ERRORS #define H2EC1(U,v,r,d) extern const struct h2_error_s H2CE_##U[1]; #define H2EC2(U,v,r,d) extern const struct h2_error_s H2SE_##U[1]; #define H2EC3(U,v,r,d) H2EC1(U,v,r,d) H2EC2(U,v,r,d) From 874a7cca06c0f138e44fe5db68da061af1f0e168 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Tue, 17 Oct 2023 18:29:43 +0200 Subject: [PATCH 07/29] slinkified dridi-polish --- include/tbl/params.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/include/tbl/params.h b/include/tbl/params.h index 03363327327..5bee92256c4 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1251,10 +1251,10 @@ PARAM_SIMPLE( /* def */ "1.000", /* units */ "seconds", /* descr */ - "The upper threshold for how rapid an http/2 RST has to come after " - "a HEADERS frame for it to be treated as suspect and subjected to " - "the rate limits specified by h2_rapid_reset_limit and " - "h2_rapid_reset_period.", + "The upper threshold for how soon an http/2 RST_STREAM frame has " + "to be parsed after a HEADERS frame for it to be treated as " + "suspect and subjected to the rate limits specified by " + "h2_rapid_reset_limit and h2_rapid_reset_period.", /* flags */ EXPERIMENTAL, ) From 2eac5912cda882d8b9718ec5746ac93750d33639 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Thu, 12 Oct 2023 11:17:09 +0200 Subject: [PATCH 08/29] vtc: Avoid cycling the barrier in t02014 It was particularly hard to follow once we reach client c3. --- bin/varnishtest/tests/t02014.vtc | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-) diff --git a/bin/varnishtest/tests/t02014.vtc b/bin/varnishtest/tests/t02014.vtc index f47eed65767..8add78a0d37 100644 --- a/bin/varnishtest/tests/t02014.vtc +++ b/bin/varnishtest/tests/t02014.vtc @@ -1,6 +1,9 @@ varnishtest "Exercise h/2 sender flow control code" -barrier b1 sock 3 -cyclic +barrier b1 sock 3 +barrier b2 sock 3 +barrier b3 sock 3 +barrier b4 sock 3 server s1 { rxreq @@ -23,7 +26,9 @@ varnish v1 -vcl+backend { } sub vcl_deliver { - vtc.barrier_sync("${b1_sock}"); + if (req.http.barrier) { + vtc.barrier_sync(req.http.barrier); + } } } -start @@ -43,7 +48,7 @@ client c1 { } -start stream 1 { - txreq + txreq -hdr barrier ${b1_sock} barrier b1 sync delay .5 txwinup -size 256 @@ -61,15 +66,15 @@ client c1 { client c2 { stream 0 { - barrier b1 sync + barrier b2 sync } -start stream 1 { - txreq + txreq -hdr barrier ${b2_sock} txdata -data "fail" rxrst expect rst.err == STREAM_CLOSED - barrier b1 sync + barrier b2 sync } -run stream 0 -wait @@ -77,8 +82,8 @@ client c2 { client c3 { stream 0 { - barrier b1 sync - barrier b1 sync + barrier b3 sync + barrier b4 sync delay .5 txwinup -size 256 delay .5 @@ -89,17 +94,17 @@ client c3 { } -start stream 1 { - txreq -req "POST" -nostrend + txreq -req "POST" -hdr barrier ${b3_sock} -nostrend txdata -data "ok" txdata -data "fail" rxrst expect rst.err == STREAM_CLOSED - barrier b1 sync + barrier b3 sync } -run stream 3 { - txreq - barrier b1 sync + txreq -hdr barrier ${b4_sock} + barrier b4 sync delay .5 txwinup -size 256 delay .5 From 467a4db0bbb2955eb3837a34607460287c2160b3 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Wed, 11 Oct 2023 11:57:48 +0200 Subject: [PATCH 09/29] transport: New poll method The goal is for top-level transports to report whether the client is still present or not. --- bin/varnishd/cache/cache_transport.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bin/varnishd/cache/cache_transport.h b/bin/varnishd/cache/cache_transport.h index 0e5f03c65ef..79f0a7b2d5b 100644 --- a/bin/varnishd/cache/cache_transport.h +++ b/bin/varnishd/cache/cache_transport.h @@ -44,6 +44,7 @@ typedef void vtr_sess_panic_f (struct vsb *, const struct sess *); typedef void vtr_req_panic_f (struct vsb *, const struct req *); typedef void vtr_req_fail_f (struct req *, stream_close_t); typedef void vtr_reembark_f (struct worker *, struct req *); +typedef int vtr_poll_f (struct req *); typedef int vtr_minimal_response_f (struct req *, uint16_t status); struct transport { @@ -64,6 +65,7 @@ struct transport { vtr_sess_panic_f *sess_panic; vtr_req_panic_f *req_panic; vtr_reembark_f *reembark; + vtr_poll_f *poll; vtr_minimal_response_f *minimal_response; VTAILQ_ENTRY(transport) list; From fa83fee2d5e123720b4cf1b254bbec51a9174201 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Wed, 11 Oct 2023 19:51:58 +0200 Subject: [PATCH 10/29] vcl_vrt: Skip VCL execution if the client is gone 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 #3835 Refs 61a15cbffe1141c13b87e30d48ce1402f84433bf Refs e5efc2c8dc0d003e5f0fa1a30b598f5949112897 Refs ba54dc919076b1ddb85434d886ce82a6553d926b Refs 6f50a00f80c7f74b2a8b18bb80593f58b74816fd Refs b8816994cfab58261d8000ea8e6941cb5de640fa Conflicts: include/tbl/feature_bits.h --- bin/varnishd/cache/cache_vrt_vcl.c | 37 ++++++++++++++++++++++++++++++ doc/sphinx/reference/vsl.rst | 4 ++++ include/tbl/feature_bits.h | 5 ++++ include/tbl/params.h | 4 +++- include/tbl/req_flags.h | 1 + lib/libvsc/VSC_main.vsc | 7 ++++++ 6 files changed, 57 insertions(+), 1 deletion(-) diff --git a/bin/varnishd/cache/cache_vrt_vcl.c b/bin/varnishd/cache/cache_vrt_vcl.c index c98274e9d57..85b594b7d86 100644 --- a/bin/varnishd/cache/cache_vrt_vcl.c +++ b/bin/varnishd/cache/cache_vrt_vcl.c @@ -42,6 +42,7 @@ #include "vbm.h" #include "cache_director.h" +#include "cache_transport.h" #include "cache_vcl.h" #include "vcc_interface.h" @@ -521,6 +522,40 @@ VRT_VCL_Allow_Discard(struct vclref **refp) FREE_OBJ(ref); } +/*-------------------------------------------------------------------- + */ + +static int +req_poll(struct worker *wrk, struct req *req) +{ + struct req *top; + + /* NB: Since a fail transition leads to vcl_synth, the request may be + * short-circuited twice. + */ + if (req->req_reset) { + wrk->vpi->handling = VCL_RET_FAIL; + return (-1); + } + + top = req->top->topreq; + CHECK_OBJ_NOTNULL(top, REQ_MAGIC); + CHECK_OBJ_NOTNULL(top->transport, TRANSPORT_MAGIC); + + if (!FEATURE(FEATURE_VCL_REQ_RESET)) + return (0); + if (top->transport->poll == NULL) + return (0); + if (top->transport->poll(top) >= 0) + return (0); + + VSLb_ts_req(req, "Reset", W_TIM_real(wrk)); + wrk->stats->req_reset++; + wrk->vpi->handling = VCL_RET_FAIL; + req->req_reset = 1; + return (-1); +} + /*-------------------------------------------------------------------- * Method functions to call into VCL programs. * @@ -552,6 +587,8 @@ vcl_call_method(struct worker *wrk, struct req *req, struct busyobj *bo, CHECK_OBJ_NOTNULL(req->sp, SESS_MAGIC); CHECK_OBJ_NOTNULL(req->vcl, VCL_MAGIC); CHECK_OBJ_NOTNULL(req->top, REQTOP_MAGIC); + if (req_poll(wrk, req)) + return; VCL_Req2Ctx(&ctx, req); } assert(ctx.now != 0); diff --git a/doc/sphinx/reference/vsl.rst b/doc/sphinx/reference/vsl.rst index e8ea7a36d89..d8b440a088b 100644 --- a/doc/sphinx/reference/vsl.rst +++ b/doc/sphinx/reference/vsl.rst @@ -76,6 +76,10 @@ Resp Restart Client request is being restarted. +Reset + The client closed its connection or reset its stream. Request + processing is interrupted and considered failed. + Pipe handling timestamps ~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/include/tbl/feature_bits.h b/include/tbl/feature_bits.h index 68ae398393c..a3df4a03b35 100644 --- a/include/tbl/feature_bits.h +++ b/include/tbl/feature_bits.h @@ -86,6 +86,11 @@ FEATURE_BIT(BUSY_STATS_RATE, busy_stats_rate, "Make busy workers comply with thread_stats_rate." ) +FEATURE_BIT(VCL_REQ_RESET, vcl_req_reset, + "Stop processing client VCL once the client is gone. " + "When this happens MAIN.req_reset is incremented." +) + #undef FEATURE_BIT /*lint -restore */ diff --git a/include/tbl/params.h b/include/tbl/params.h index 5bee92256c4..d5e080758f3 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1810,7 +1810,9 @@ PARAM_PRE PARAM_BITS( /* name */ feature, /* fld */ feature_bits, - /* def */ "+validate_headers", + /* def */ + "+validate_headers," + "+vcl_req_reset", /* descr */ "Enable/Disable various minor features.\n" "\tdefault\tSet default value\n" diff --git a/include/tbl/req_flags.h b/include/tbl/req_flags.h index 88eaff6391c..6a9e6acbb18 100644 --- a/include/tbl/req_flags.h +++ b/include/tbl/req_flags.h @@ -40,6 +40,7 @@ REQ_FLAG(is_hit, 0, 0, "") REQ_FLAG(waitinglist, 0, 0, "") REQ_FLAG(want100cont, 0, 0, "") REQ_FLAG(late100cont, 0, 0, "") +REQ_FLAG(req_reset, 0, 0, "") #define REQ_BEREQ_FLAG(lower, vcl_r, vcl_w, doc) \ REQ_FLAG(lower, vcl_r, vcl_w, doc) #include "tbl/req_bereq_flags.h" diff --git a/lib/libvsc/VSC_main.vsc b/lib/libvsc/VSC_main.vsc index a935a569a34..1bb5ad22e78 100644 --- a/lib/libvsc/VSC_main.vsc +++ b/lib/libvsc/VSC_main.vsc @@ -342,6 +342,13 @@ Number of times an HTTP/2 stream was refused because the queue was too long already. See also parameter thread_queue_limit. +.. varnish_vsc:: req_reset + :group: wrk + :oneliner: Requests reset + + Number of times a client left before the VCL processing of its + requests completed. + .. varnish_vsc:: n_object :type: gauge :group: wrk From a1501a6e8d530aa83b3b727dd8741e9886d8e2e9 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Wed, 11 Oct 2023 12:38:21 +0200 Subject: [PATCH 11/29] http2_session: Implement transport polling 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. --- bin/varnishd/http2/cache_http2_session.c | 11 ++++++ bin/varnishtest/tests/t02014.vtc | 31 ++++++++++++++- bin/varnishtest/tests/t02025.vtc | 49 ++++++++++++++++++++++++ doc/sphinx/reference/vsl.rst | 3 +- lib/libvsc/VSC_main.vsc | 4 +- 5 files changed, 94 insertions(+), 4 deletions(-) create mode 100644 bin/varnishtest/tests/t02025.vtc diff --git a/bin/varnishd/http2/cache_http2_session.c b/bin/varnishd/http2/cache_http2_session.c index b37dd105c89..29af53093ee 100644 --- a/bin/varnishd/http2/cache_http2_session.c +++ b/bin/varnishd/http2/cache_http2_session.c @@ -441,6 +441,16 @@ h2_new_session(struct worker *wrk, void *arg) wrk->vsl = NULL; } +static int v_matchproto_(vtr_poll_f) +h2_poll(struct req *req) +{ + struct h2_req *r2; + + CHECK_OBJ_NOTNULL(req, REQ_MAGIC); + CAST_OBJ_NOTNULL(r2, req->transport_priv, H2_REQ_MAGIC); + return (r2->error ? -1 : 1); +} + struct transport HTTP2_transport = { .name = "HTTP/2", .magic = TRANSPORT_MAGIC, @@ -450,4 +460,5 @@ struct transport HTTP2_transport = { .req_body = h2_req_body, .req_fail = h2_req_fail, .sess_panic = h2_sess_panic, + .poll = h2_poll, }; diff --git a/bin/varnishtest/tests/t02014.vtc b/bin/varnishtest/tests/t02014.vtc index 8add78a0d37..f4083c89077 100644 --- a/bin/varnishtest/tests/t02014.vtc +++ b/bin/varnishtest/tests/t02014.vtc @@ -5,6 +5,9 @@ barrier b2 sock 3 barrier b3 sock 3 barrier b4 sock 3 +barrier b2_err cond 2 +barrier b3_err cond 2 + server s1 { rxreq txresp -bodylen 66300 @@ -64,6 +67,13 @@ client c1 { stream 0 -wait } -run +varnish v1 -vsl_catchup + +logexpect l2 -v v1 -g raw { + expect * * ReqMethod GET + expect * = VCL_call DELIVER +} -start + client c2 { stream 0 { barrier b2 sync @@ -71,6 +81,7 @@ client c2 { stream 1 { txreq -hdr barrier ${b2_sock} + barrier b2_err sync txdata -data "fail" rxrst expect rst.err == STREAM_CLOSED @@ -78,7 +89,17 @@ client c2 { } -run stream 0 -wait -} -run +} -start + +logexpect l2 -wait +barrier b2_err sync + +client c2 -wait + +logexpect l3 -v v1 -g raw { + expect * * ReqMethod POST + expect * = VCL_call DELIVER +} -start client c3 { stream 0 { @@ -96,6 +117,7 @@ client c3 { stream 1 { txreq -req "POST" -hdr barrier ${b3_sock} -nostrend txdata -data "ok" + barrier b3_err sync txdata -data "fail" rxrst expect rst.err == STREAM_CLOSED @@ -117,4 +139,9 @@ client c3 { } -run stream 0 -wait -} -run +} -start + +logexpect l3 -wait +barrier b3_err sync + +client c3 -wait diff --git a/bin/varnishtest/tests/t02025.vtc b/bin/varnishtest/tests/t02025.vtc new file mode 100644 index 00000000000..3b7e90e8aae --- /dev/null +++ b/bin/varnishtest/tests/t02025.vtc @@ -0,0 +1,49 @@ +varnishtest "h2 reset interrupt" + +barrier b1 sock 2 +barrier b2 sock 2 + +varnish v1 -cliok "param.set feature +http2" +varnish v1 -cliok "param.set debug +syncvsl" +varnish v1 -vcl { + import vtc; + + backend be none; + + sub vcl_recv { + vtc.barrier_sync("${b1_sock}"); + vtc.barrier_sync("${b2_sock}"); + } + + sub vcl_miss { + vtc.panic("unreachable"); + } +} -start + +logexpect l1 -v v1 -g raw -i Debug { + expect * * Debug "^H2RXF RST_STREAM" +} -start + +client c1 { + stream 1 { + txreq + barrier b1 sync + txrst + } -run +} -start + +logexpect l1 -wait +barrier b2 sync + +varnish v1 -vsl_catchup +varnish v1 -expect req_reset == 1 + +# NB: The varnishncsa command below shows a minimal pattern to collect +# "rapid reset" suspects per session, with the IP address. Here rapid +# is interpreted as before a second elapsed. Session VXIDs showing up +# numerous times become increasingly more suspicious. The format can of +# course be extended to add anything else useful for data mining. +shell -expect "1000 ${localhost}" { + varnishncsa -n ${v1_name} -d \ + -q 'Timestamp:Reset[2] < 1.0' -F '%{VSL:Begin[2]}x %h' +} diff --git a/doc/sphinx/reference/vsl.rst b/doc/sphinx/reference/vsl.rst index d8b440a088b..98c126d2d1d 100644 --- a/doc/sphinx/reference/vsl.rst +++ b/doc/sphinx/reference/vsl.rst @@ -77,7 +77,8 @@ Restart Client request is being restarted. Reset - The client closed its connection or reset its stream. Request + The client closed its connection, reset its stream or caused + a stream error that forced Varnish to reset the stream. Request processing is interrupted and considered failed. Pipe handling timestamps diff --git a/lib/libvsc/VSC_main.vsc b/lib/libvsc/VSC_main.vsc index 1bb5ad22e78..aa2e3674482 100644 --- a/lib/libvsc/VSC_main.vsc +++ b/lib/libvsc/VSC_main.vsc @@ -347,7 +347,9 @@ :oneliner: Requests reset Number of times a client left before the VCL processing of its - requests completed. + requests completed. For HTTP/2 sessions, either the stream was + reset by an RST_STREAM frame from the client, or a stream or + connection error occurred. .. varnish_vsc:: n_object :type: gauge From 84b67dccdfd5c66a8fb999a219322d3092ad7302 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Wed, 18 Oct 2023 11:02:48 +0200 Subject: [PATCH 12/29] vtc: Stabilize r3996 and increase coverage With #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. --- bin/varnishtest/tests/r03996.vtc | 47 +++++++++++++++++++++++++++++--- 1 file changed, 43 insertions(+), 4 deletions(-) diff --git a/bin/varnishtest/tests/r03996.vtc b/bin/varnishtest/tests/r03996.vtc index 3fee3706c35..7faf78316d5 100644 --- a/bin/varnishtest/tests/r03996.vtc +++ b/bin/varnishtest/tests/r03996.vtc @@ -1,6 +1,7 @@ varnishtest "h2 rapid reset" -barrier b1 sock 5 +barrier b1 sock 2 -cyclic +barrier b2 sock 5 -cyclic server s1 { rxreq @@ -16,7 +17,10 @@ varnish v1 -vcl+backend { import vtc; sub vcl_recv { - vtc.barrier_sync("${b1_sock}"); + if (req.http.barrier) { + vtc.barrier_sync(req.http.barrier); + } + vtc.barrier_sync("${b2_sock}"); } } -start @@ -27,6 +31,41 @@ client c1 { expect goaway.err == ENHANCE_YOUR_CALM } -start + stream 1 { + txreq -hdr barrier ${b1_sock} + barrier b1 sync + txrst + } -run + stream 3 { + txreq -hdr barrier ${b1_sock} + barrier b1 sync + txrst + } -run + stream 5 { + txreq -hdr barrier ${b1_sock} + barrier b1 sync + txrst + } -run + stream 7 { + txreq -hdr barrier ${b1_sock} + barrier b1 sync + txrst + } -run + + barrier b2 sync + stream 0 -wait +} -run + +varnish v1 -expect sc_rapid_reset == 1 + +varnish v1 -cliok "param.set feature -vcl_req_reset" + +client c2 { + stream 0 { + rxgoaway + expect goaway.err == ENHANCE_YOUR_CALM + } -start + stream 1 { txreq txrst @@ -44,8 +83,8 @@ client c1 { txrst } -run - barrier b1 sync + barrier b2 sync stream 0 -wait } -run -varnish v1 -expect sc_rapid_reset == 1 +varnish v1 -expect sc_rapid_reset == 2 From ae7b0b544061688aa510e3a4c68aa5e6f5603d40 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Wed, 18 Oct 2023 13:23:15 +0200 Subject: [PATCH 13/29] vtc: Missing synchronization in t02025 Noticed while porting #3998 to the 6.0 branch with a varnishtest more sensitive to timing. --- bin/varnishtest/tests/t02025.vtc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bin/varnishtest/tests/t02025.vtc b/bin/varnishtest/tests/t02025.vtc index 3b7e90e8aae..578dbf5c84b 100644 --- a/bin/varnishtest/tests/t02025.vtc +++ b/bin/varnishtest/tests/t02025.vtc @@ -30,11 +30,14 @@ client c1 { barrier b1 sync txrst } -run + expect_close } -start logexpect l1 -wait barrier b2 sync +client c1 -wait + varnish v1 -vsl_catchup varnish v1 -expect req_reset == 1 From f5f8f271a32fb4ded20631e3b9761bc58fc0c813 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Tue, 17 Oct 2023 13:47:39 +0200 Subject: [PATCH 14/29] Copy rapid reset parameters to the h2 session This will allow per-session adjustments and also significantly lower the risk of inconsistent calculations in the rate limit code during parameter changes. Ref #3996 --- bin/varnishd/http2/cache_http2.h | 7 +++++++ bin/varnishd/http2/cache_http2_proto.c | 10 +++++----- bin/varnishd/http2/cache_http2_session.c | 7 ++++++- include/tbl/params.h | 14 ++++++++------ 4 files changed, 26 insertions(+), 12 deletions(-) diff --git a/bin/varnishd/http2/cache_http2.h b/bin/varnishd/http2/cache_http2.h index 65ea1d2df8d..ca2e6599301 100644 --- a/bin/varnishd/http2/cache_http2.h +++ b/bin/varnishd/http2/cache_http2.h @@ -194,6 +194,13 @@ struct h2_sess { VTAILQ_HEAD(,h2_req) txqueue; h2_error error; + + // rst rate limit parameters, copied from h2_* parameters + vtim_dur rapid_reset; + int64_t rapid_reset_limit; + vtim_dur rapid_reset_period; + + // rst rate limit state double rst_budget; vtim_real last_rst; }; diff --git a/bin/varnishd/http2/cache_http2_proto.c b/bin/varnishd/http2/cache_http2_proto.c index a006d708f9d..dc3d4a14476 100644 --- a/bin/varnishd/http2/cache_http2_proto.c +++ b/bin/varnishd/http2/cache_http2_proto.c @@ -328,20 +328,20 @@ h2_rapid_reset(struct worker *wrk, struct h2_sess *h2, struct h2_req *r2) ASSERT_RXTHR(h2); CHECK_OBJ_NOTNULL(r2, H2_REQ_MAGIC); - if (cache_param->h2_rapid_reset_limit == 0) + if (h2->rapid_reset_limit == 0) return (0); now = VTIM_real(); CHECK_OBJ_NOTNULL(r2->req, REQ_MAGIC); AN(r2->req->t_first); - if (now - r2->req->t_first > cache_param->h2_rapid_reset) + if (now - r2->req->t_first > h2->rapid_reset) return (0); d = now - h2->last_rst; - h2->rst_budget += cache_param->h2_rapid_reset_limit * d / - cache_param->h2_rapid_reset_period; + h2->rst_budget += h2->rapid_reset_limit * d / + h2->rapid_reset_period; h2->rst_budget = vmin_t(double, h2->rst_budget, - cache_param->h2_rapid_reset_limit); + h2->rapid_reset_limit); h2->last_rst = now; if (h2->rst_budget < 1.0) { diff --git a/bin/varnishd/http2/cache_http2_session.c b/bin/varnishd/http2/cache_http2_session.c index 29af53093ee..cdd99d3dfb4 100644 --- a/bin/varnishd/http2/cache_http2_session.c +++ b/bin/varnishd/http2/cache_http2_session.c @@ -127,7 +127,12 @@ h2_init_sess(struct sess *sp, h2_local_settings(&h2->local_settings); h2->remote_settings = H2_proto_settings; h2->decode = decode; - h2->rst_budget = cache_param->h2_rapid_reset_limit; + + h2->rapid_reset = cache_param->h2_rapid_reset; + h2->rapid_reset_limit = cache_param->h2_rapid_reset_limit; + h2->rapid_reset_period = cache_param->h2_rapid_reset_period; + + h2->rst_budget = h2->rapid_reset_limit; h2->last_rst = sp->t_open; AZ(isnan(h2->last_rst)); diff --git a/include/tbl/params.h b/include/tbl/params.h index d5e080758f3..679d29894c6 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1243,6 +1243,8 @@ PARAM_SIMPLE( "HTTP2 maximum size of an uncompressed header list." ) +#define H2_RR_INFO \ + "Changes to this parameter affect the default for new HTTP2 sessions" PARAM_SIMPLE( /* name */ h2_rapid_reset, /* typ */ timeout, @@ -1254,8 +1256,8 @@ PARAM_SIMPLE( "The upper threshold for how soon an http/2 RST_STREAM frame has " "to be parsed after a HEADERS frame for it to be treated as " "suspect and subjected to the rate limits specified by " - "h2_rapid_reset_limit and h2_rapid_reset_period.", - /* flags */ EXPERIMENTAL, + "h2_rapid_reset_limit and h2_rapid_reset_period.\n" H2_RR_INFO, + /* flags */ EXPERIMENTAL|DELAYED_EFFECT, ) PARAM_SIMPLE( @@ -1269,8 +1271,8 @@ PARAM_SIMPLE( "HTTP2 RST Allowance.\n" "Specifies the maximum number of allowed stream resets issued by\n" "a client over a time period before the connection is closed.\n" - "Setting this parameter to 0 disables the limit.", - /* flags */ EXPERIMENTAL, + "Setting this parameter to 0 disables the limit.\n" H2_RR_INFO, + /* flags */ EXPERIMENTAL|DELAYED_EFFECT, ) PARAM_SIMPLE( @@ -1281,8 +1283,8 @@ PARAM_SIMPLE( /* def */ "60.000", /* units */ "seconds", /* descr */ - "HTTP2 sliding window duration for h2_rapid_reset_limit.", - /* flags */ EXPERIMENTAL|WIZARD, + "HTTP2 sliding window duration for h2_rapid_reset_limit.\n" H2_RR_INFO, + /* flags */ EXPERIMENTAL|DELAYED_EFFECT|WIZARD, ) /*-------------------------------------------------------------------- From c21f9b2a885ae6196d9e585027baa19c99686209 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Mon, 16 Oct 2023 16:07:05 +0200 Subject: [PATCH 15/29] Add vmod_h2 to control rapid_reset parameters per session Conflicts: vmod/automake_boilerplate_h2.am vmod/vmod_h2.vcc --- include/tbl/params.h | 4 +- vmod/Makefile.am | 1 + vmod/automake_boilerplate_h2.am | 35 +++++++++++ vmod/tests/h2_b00000.vtc | 90 ++++++++++++++++++++++++++++ vmod/vmod_h2.c | 100 ++++++++++++++++++++++++++++++++ vmod/vmod_h2.vcc | 88 ++++++++++++++++++++++++++++ 6 files changed, 317 insertions(+), 1 deletion(-) create mode 100644 vmod/automake_boilerplate_h2.am create mode 100644 vmod/tests/h2_b00000.vtc create mode 100644 vmod/vmod_h2.c create mode 100644 vmod/vmod_h2.vcc diff --git a/include/tbl/params.h b/include/tbl/params.h index 679d29894c6..1a9fe78ad2e 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1244,7 +1244,9 @@ PARAM_SIMPLE( ) #define H2_RR_INFO \ - "Changes to this parameter affect the default for new HTTP2 sessions" + "Changes to this parameter affect the default for new HTTP2 " \ + "sessions. vmod_h2(3) can be used to adjust it from VCL." + PARAM_SIMPLE( /* name */ h2_rapid_reset, /* typ */ timeout, diff --git a/vmod/Makefile.am b/vmod/Makefile.am index 29ffcf6bd11..da6eaaeffbf 100644 --- a/vmod/Makefile.am +++ b/vmod/Makefile.am @@ -20,6 +20,7 @@ AM_CPPFLAGS = \ vmod_LTLIBRARIES = include $(srcdir)/automake_boilerplate_blob.am +include $(srcdir)/automake_boilerplate_h2.am include $(srcdir)/automake_boilerplate_cookie.am include $(srcdir)/automake_boilerplate_debug.am include $(srcdir)/automake_boilerplate_directors.am diff --git a/vmod/automake_boilerplate_h2.am b/vmod/automake_boilerplate_h2.am new file mode 100644 index 00000000000..905014e239b --- /dev/null +++ b/vmod/automake_boilerplate_h2.am @@ -0,0 +1,35 @@ +# Generated by vmodtool.py --boilerplate. + +vmod_LTLIBRARIES += libvmod_h2.la + +libvmod_h2_la_SOURCES = \ + vmod_h2.c + +libvmod_h2_la_CFLAGS = + +vmodtoolargs_h2 ?= --strict --boilerplate -o vcc_h2_if +vmod_h2_symbols_regex ?= Vmod_h2_Data + +libvmod_h2_la_LDFLAGS = \ + -export-symbols-regex $(vmod_h2_symbols_regex) \ + $(AM_LDFLAGS) \ + $(VMOD_LDFLAGS) + +nodist_libvmod_h2_la_SOURCES = vcc_h2_if.c vcc_h2_if.h + +EXTRA_libvmod_h2_la_DEPENDENCIES = $(nodist_libvmod_h2_la_SOURCES) + +EXTRA_DIST += $(srcdir)/vmod_h2.vcc automake_boilerplate_h2.am + +$(libvmod_h2_la_OBJECTS): vcc_h2_if.h + +vcc_h2_if.h vmod_h2.rst vmod_h2.man.rst: vcc_h2_if.c + +vcc_h2_if.c: $(vmodtool) $(srcdir)/vmod_h2.vcc + @PYTHON@ $(vmodtool) $(vmodtoolargs_h2) $(srcdir)/vmod_h2.vcc + +clean-local: clean-vmod-h2 + +clean-vmod-h2: + rm -f $(nodist_libvmod_h2_la_SOURCES) + rm -f vmod_h2.rst vmod_h2.man.rst diff --git a/vmod/tests/h2_b00000.vtc b/vmod/tests/h2_b00000.vtc new file mode 100644 index 00000000000..e6754fc84dd --- /dev/null +++ b/vmod/tests/h2_b00000.vtc @@ -0,0 +1,90 @@ +varnishtest "VMOD h2 basics" + +varnish v1 -arg "-p feature=+http2" -vcl { + import h2; + + backend proforma none; + + sub vcl_recv { + return(synth(200)); + } + + sub vcl_synth { + set resp.http.http2-is = h2.is(); + set resp.body = ""; + return (deliver); + } +} -start + +client c1 { + txreq + rxresp + expect resp.status == 200 + expect resp.http.http2-is == false +} -start + +client c2 { + stream 7 { + txreq + rxresp + expect resp.status == 200 + expect resp.http.http2-is == true + } -run +} -start + +client c1 -wait +client c2 -wait + +# coverage +varnish v1 -vcl { + import h2; + + backend proforma none; + + sub vcl_recv { + return(synth(200)); + } + + sub vcl_synth { + set resp.http.rapid-reset-o = h2.rapid_reset(10ms); + set resp.http.rapid-reset-n = h2.rapid_reset(); + set resp.http.rapid-reset-limit-o = h2.rapid_reset_limit(100); + set resp.http.rapid-reset-limit-n = h2.rapid_reset_limit(); + set resp.http.rapid-reset-period-o = h2.rapid_reset_period(10s); + set resp.http.rapid-reset-period-n = h2.rapid_reset_period(); + set resp.http.rapid-reset-budget = h2.rapid_reset_budget(); + set resp.body = ""; + return (deliver); + } +} + +client c1 { + txreq + rxresp + expect resp.status == 200 + expect resp.http.rapid-reset-o == -1.000 + expect resp.http.rapid-reset-n == -1.000 + expect resp.http.rapid-reset-limit-o == -1 + expect resp.http.rapid-reset-limit-n == -1 + expect resp.http.rapid-reset-period-o == -1.000 + expect resp.http.rapid-reset-period-n == -1.000 + expect resp.http.rapid-reset-budget == -1.000 +} -start + +client c2 { + stream 7 { + txreq + rxresp + expect resp.status == 200 + expect resp.http.rapid-reset-o == 1.000 + expect resp.http.rapid-reset-n == 0.010 + expect resp.http.rapid-reset-limit-o == 0 + expect resp.http.rapid-reset-limit-n == 100 + expect resp.http.rapid-reset-period-o == 60.000 + expect resp.http.rapid-reset-period-n == 10.000 + expect resp.http.rapid-reset-budget == 100.000 + } -run +} -start + +client c1 -wait +client c2 -wait diff --git a/vmod/vmod_h2.c b/vmod/vmod_h2.c new file mode 100644 index 00000000000..bded788f45d --- /dev/null +++ b/vmod/vmod_h2.c @@ -0,0 +1,100 @@ +/*- + * Copyright 2023 UPLEX - Nils Goroll Systemoptimierung + * All rights reserved. + * + * Author: Nils Goroll + * + * SPDX-License-Identifier: BSD-2-Clause + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are met: + * 1. Redistributions of source code must retain the above copyright notice, + * this list of conditions and the following disclaimer. + * 2. Redistributions in binary form must reproduce the above copyright notice, + * this list of conditions and the following disclaimer in the documentation + * and/or other materials provided with the distribution. + * + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND ANY + * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED + * WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE + * DISCLAIMED. IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE FOR ANY + * DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; + * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS + * SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#include "config.h" + +#include "cache/cache_varnishd.h" + +#include "vcc_h2_if.h" + +#include "cache/cache_transport.h" +#include "http2/cache_http2.h" + +static struct h2_sess * +h2get(VRT_CTX) +{ + struct h2_sess *h2; + uintptr_t *up; + + CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC); // $Restrict client + if (ctx->req->transport != &HTTP2_transport) + return (NULL); + AZ(SES_Get_proto_priv(ctx->req->sp, &up)); + CAST_OBJ_NOTNULL(h2, (void *)*up, H2_SESS_MAGIC); + return (h2); +} +VCL_BOOL +vmod_is(VRT_CTX) +{ + struct h2_sess *h2 = h2get(ctx); + + return (h2 != NULL); +} + +#define GETSET(type, name, argname) \ +type \ +vmod_ ## name(VRT_CTX, struct VARGS(name) *args) \ +{ \ + struct h2_sess *h2 = h2get(ctx); \ + type r; \ + \ + if (h2 == NULL) \ + return (-1); \ + \ + if (! args->valid_ ## argname) \ + return (h2->name); \ + if (h2->name == args->argname) \ + return (h2->name); \ + \ + Lck_Lock(&h2->sess->mtx); \ + r = h2->name; \ + if (h2->name != args->argname) { \ + h2->name = args->argname; \ + h2->rst_budget = h2->rapid_reset_limit; \ + h2->last_rst = ctx->now; \ + } \ + Lck_Unlock(&h2->sess->mtx); \ + return (r); \ +} + +GETSET(VCL_DURATION, rapid_reset, threshold) +GETSET(VCL_INT, rapid_reset_limit, number) +GETSET(VCL_DURATION, rapid_reset_period, duration) + +VCL_REAL +vmod_rapid_reset_budget(VRT_CTX) +{ + struct h2_sess *h2 = h2get(ctx); + + if (h2 == NULL) + return (-1); + + return (h2->rst_budget); +} diff --git a/vmod/vmod_h2.vcc b/vmod/vmod_h2.vcc new file mode 100644 index 00000000000..cd7529c9f77 --- /dev/null +++ b/vmod/vmod_h2.vcc @@ -0,0 +1,88 @@ +#- +# Copyright 2023 UPLEX - Nils Goroll Systemoptimierung +# All rights reserved. +# +# Author: Nils Goroll +# +# SPDX-License-Identifier: BSD-2-Clause +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions +# are met: +# 1. Redistributions of source code must retain the above copyright +# notice, this list of conditions and the following disclaimer. +# 2. Redistributions in binary form must reproduce the above copyright +# notice, this list of conditions and the following disclaimer in the +# documentation and/or other materials provided with the distribution. +# +# THIS SOFTWARE IS PROVIDED BY THE AUTHOR AND CONTRIBUTORS ``AS IS'' AND +# ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE +# IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE +# ARE DISCLAIMED. IN NO EVENT SHALL AUTHOR OR CONTRIBUTORS BE LIABLE +# FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL +# DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS +# OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) +# HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT +# LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY +# OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF +# SUCH DAMAGE. + +$ABI strict +$Module h2 3 "Module to control the built-in HTTP2 transport" + +DESCRIPTION +=========== + +This VMOD contains functions to control the HTTP2 transport built into +Varnish-Cache. + +$Function BOOL is() + +Returns true when called on a session handled by the built-in HTTP2 transport. + +$Function DURATION rapid_reset([DURATION threshold]) + +Get and optionally set the ``h2_rapid_reset`` parameter (See +:ref:`varnishd(1)`) for this HTTP2 session only. + +Returns -1 when used outside the HTTP2 transport. Otherwise returns +the previous value. + +If the call leads to a change in the rate limit parameters, the +current budget as retuned by +`h2.rapid_reset_budget()`_ is reset. + +$Function INT rapid_reset_limit([INT number]) + +Get and optionally set the ``h2_rapid_reset_limit`` parameter (See +:ref:`varnishd(1)`) for this HTTP2 session only. + +Returns -1 when used outside the HTTP2 transport. Otherwise returns +the previous value. + +If the call leads to a change in the rate limit parameters, the +current budget as retuned by +`h2.rapid_reset_budget()`_ is reset. + +$Function DURATION rapid_reset_period([DURATION duration]) + +Get and optionally set the ``h2_rapid_reset_period`` parameter (See +:ref:`varnishd(1)`) for this HTTP2 session only. + +Returns -1 when used outside the HTTP2 transport. Otherwise returns +the previous value. + +If the call leads to a change in the rate limit parameters, the +current budget as retuned by +`h2.rapid_reset_budget()`_ is reset. + +$Function REAL rapid_reset_budget() + +Return how many RST frames classified as "rapid" the client is still +allowed to send before the session is going to be closed. + +SEE ALSO +======== + +* :ref:`varnishd(1)` +* :ref:`vsl(7)` From 53a1cd5e80641c3798879598d22a4bc61ba1a51f Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Wed, 18 Oct 2023 13:53:53 +0200 Subject: [PATCH 16/29] Start with a reasonable default for h2_rapid_reset_limit as agreed on IRC. --- include/tbl/params.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/tbl/params.h b/include/tbl/params.h index 1a9fe78ad2e..4ee8d01e131 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -1267,7 +1267,7 @@ PARAM_SIMPLE( /* typ */ uint, /* min */ "0", /* max */ NULL, - /* def */ "0", + /* def */ "100", /* units */ NULL, /* descr */ "HTTP2 RST Allowance.\n" From f064f6406e6d39d4da29a9e417dfdf98f36fb9f5 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Wed, 18 Oct 2023 14:01:33 +0200 Subject: [PATCH 17/29] Adjust test case to previous commit (sorry) --- vmod/tests/h2_b00000.vtc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/vmod/tests/h2_b00000.vtc b/vmod/tests/h2_b00000.vtc index e6754fc84dd..aedb6aad8c5 100644 --- a/vmod/tests/h2_b00000.vtc +++ b/vmod/tests/h2_b00000.vtc @@ -48,7 +48,7 @@ varnish v1 -vcl { sub vcl_synth { set resp.http.rapid-reset-o = h2.rapid_reset(10ms); set resp.http.rapid-reset-n = h2.rapid_reset(); - set resp.http.rapid-reset-limit-o = h2.rapid_reset_limit(100); + set resp.http.rapid-reset-limit-o = h2.rapid_reset_limit(10); set resp.http.rapid-reset-limit-n = h2.rapid_reset_limit(); set resp.http.rapid-reset-period-o = h2.rapid_reset_period(10s); set resp.http.rapid-reset-period-n = h2.rapid_reset_period(); @@ -78,11 +78,11 @@ client c2 { expect resp.status == 200 expect resp.http.rapid-reset-o == 1.000 expect resp.http.rapid-reset-n == 0.010 - expect resp.http.rapid-reset-limit-o == 0 - expect resp.http.rapid-reset-limit-n == 100 + expect resp.http.rapid-reset-limit-o == 100 + expect resp.http.rapid-reset-limit-n == 10 expect resp.http.rapid-reset-period-o == 60.000 expect resp.http.rapid-reset-period-n == 10.000 - expect resp.http.rapid-reset-budget == 100.000 + expect resp.http.rapid-reset-budget == 10.000 } -run } -start From f28e5da8c9477f6713553a9b075141f3aab16a0b Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Wed, 18 Oct 2023 15:17:11 +0200 Subject: [PATCH 18/29] Flexelinting we can not make the parameter const because API. --- vmod/vmod_h2.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/vmod/vmod_h2.c b/vmod/vmod_h2.c index bded788f45d..45642f14023 100644 --- a/vmod/vmod_h2.c +++ b/vmod/vmod_h2.c @@ -65,6 +65,8 @@ vmod_ ## name(VRT_CTX, struct VARGS(name) *args) \ struct h2_sess *h2 = h2get(ctx); \ type r; \ \ + (void)args; \ + \ if (h2 == NULL) \ return (-1); \ \ From 941d3fda6ece9d63943093164c3e8bbc1ab43bd4 Mon Sep 17 00:00:00 2001 From: Dag Haavi Finstad Date: Wed, 18 Oct 2023 15:32:32 +0200 Subject: [PATCH 19/29] vmod_h2: VRT_fail if called outside of client context Conflicts: vmod/vmod_h2.c --- vmod/vmod_h2.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/vmod/vmod_h2.c b/vmod/vmod_h2.c index 45642f14023..f62105384dd 100644 --- a/vmod/vmod_h2.c +++ b/vmod/vmod_h2.c @@ -43,7 +43,12 @@ h2get(VRT_CTX) uintptr_t *up; CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); - CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC); // $Restrict client + if (ctx->req == NULL) { + VRT_fail(ctx, + "vmod_h2 can only be called from client-side VCL."); + return (NULL); + } + CHECK_OBJ_NOTNULL(ctx->req, REQ_MAGIC); if (ctx->req->transport != &HTTP2_transport) return (NULL); AZ(SES_Get_proto_priv(ctx->req->sp, &up)); From 2e361dea7362737d0f218e35c9819c3146f6378f Mon Sep 17 00:00:00 2001 From: Poul-Henning Kamp Date: Tue, 22 Aug 2023 08:41:21 +0000 Subject: [PATCH 20/29] Redo H2 field validation Fixes #3952 --- bin/varnishd/http2/cache_http2_hpack.c | 86 ++++++++++++++++++-------- bin/varnishtest/tests/t02023.vtc | 44 +++++++++++++ 2 files changed, 105 insertions(+), 25 deletions(-) diff --git a/bin/varnishd/http2/cache_http2_hpack.c b/bin/varnishd/http2/cache_http2_hpack.c index 36570a751a2..26be1b4d1ef 100644 --- a/bin/varnishd/http2/cache_http2_hpack.c +++ b/bin/varnishd/http2/cache_http2_hpack.c @@ -39,6 +39,7 @@ #include "http2/cache_http2.h" #include "vct.h" +// rfc9113,l,2493,2528 static h2_error h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) { @@ -48,46 +49,80 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) AN(b); assert(namelen >= 2); /* 2 chars from the ': ' that we added */ assert(namelen <= len); + assert(b[namelen - 2] == ':'); + assert(b[namelen - 1] == ' '); if (namelen == 2) { VSLb(hp->vsl, SLT_BogoHeader, "Empty name"); return (H2SE_PROTOCOL_ERROR); } - for (p = b; p < b + len; p++) { - if (p < b + (namelen - 2)) { - /* Check valid name characters */ - if (p == b && *p == ':') - continue; /* pseudo-header */ + // VSLb(hp->vsl, SLT_Debug, "CHDR [%.*s] [%.*s]", + // (int)namelen, b, (int)(len - namelen), b + namelen); + + int state = 0; + for (p = b; p < b + namelen - 2; p++) { + switch(state) { + case 0: /* First char of field */ + state = 1; + if (*p == ':') + break; + /* FALL_THROUGH */ + case 1: /* field name */ + if (*p <= 0x20 || *p >= 0x7f) { + VSLb(hp->vsl, SLT_BogoHeader, + "Illegal field header name (control): %.*s", + (int)(len > 20 ? 20 : len), b); + return (H2SE_PROTOCOL_ERROR); + } if (isupper(*p)) { VSLb(hp->vsl, SLT_BogoHeader, - "Illegal header name (upper-case): %.*s", + "Illegal field header name (upper-case): %.*s", (int)(len > 20 ? 20 : len), b); return (H2SE_PROTOCOL_ERROR); } - if (vct_istchar(*p)) { - /* XXX: vct should have a proper class for - this avoiding two checks */ - continue; + if (!vct_istchar(*p) || *p == ':') { + VSLb(hp->vsl, SLT_BogoHeader, + "Illegal field header name (non-token): %.*s", + (int)(len > 20 ? 20 : len), b); + return (H2SE_PROTOCOL_ERROR); } - VSLb(hp->vsl, SLT_BogoHeader, - "Illegal header name: %.*s", - (int)(len > 20 ? 20 : len), b); - return (H2SE_PROTOCOL_ERROR); - } else if (p < b + namelen) { - /* ': ' added by us */ - assert(*p == ':' || *p == ' '); - } else { - /* Check valid value characters */ - if (!vct_isctl(*p) || vct_issp(*p)) - continue; - VSLb(hp->vsl, SLT_BogoHeader, - "Illegal header value: %.*s", - (int)(len > 20 ? 20 : len), b); - return (H2SE_PROTOCOL_ERROR); + break; + default: + WRONG("http2 field name validation state"); } } + state = 2; + for (p = b + namelen; p < b + len; p++) { + switch(state) { + case 2: /* First char of field */ + if (*p == ' ' || *p == 0x09) { + VSLb(hp->vsl, SLT_BogoHeader, + "Illegal field value start %.*s", + (int)(len > 20 ? 20 : len), b); + return (H2SE_PROTOCOL_ERROR); + } + state = 3; + /* FALL_THROUGH */ + case 3: /* field value character */ + if (*p != 0x09 && (*p < 0x20 || *p == 0x7f)) { + VSLb(hp->vsl, SLT_BogoHeader, + "Illegal field value (control) %.*s", + (int)(len > 20 ? 20 : len), b); + return (H2SE_PROTOCOL_ERROR); + } + break; + default: + WRONG("http2 field value validation state"); + } + } + if (state == 3 && b[len - 1] <= 0x20) { + VSLb(hp->vsl, SLT_BogoHeader, + "Illegal val (end) %.*s", + (int)(len > 20 ? 20 : len), b); + return (H2SE_PROTOCOL_ERROR); + } return (0); } @@ -271,6 +306,7 @@ h2h_decode_fini(const struct h2_sess *h2) * block. This is a connection level error. * * H2E_PROTOCOL_ERROR: Malformed header or duplicate pseudo-header. + * Violation of field name/value charsets */ h2_error h2h_decode_bytes(struct h2_sess *h2, const uint8_t *in, size_t in_l) diff --git a/bin/varnishtest/tests/t02023.vtc b/bin/varnishtest/tests/t02023.vtc index cfd843da3ec..59c7fe5d79d 100644 --- a/bin/varnishtest/tests/t02023.vtc +++ b/bin/varnishtest/tests/t02023.vtc @@ -46,3 +46,47 @@ client c1 { rxrst } -run } -run + +varnish v1 -vsl_catchup + +client c1 { + stream 1 { + txreq -hdr "fo o" " bar" + rxrst + } -run +} -run + +client c1 { + stream 1 { + txreq -hdr "foo" " " + rxrst + } -run +} -run + +client c1 { + stream 1 { + txreq -hdr ":foo" "bar" + rxrst + } -run +} -run + +client c1 { + stream 1 { + txreq -hdr "foo" "b\x0car" + rxrst + } -run +} -run + +client c1 { + stream 1 { + txreq -hdr "f o" "bar" + rxrst + } -run +} -run + +client c1 { + stream 1 { + txreq -hdr "f: o" "bar" + rxrst + } -run +} -run From fec46c8d04537d6b9777d4d6efaeced4e7ed1979 Mon Sep 17 00:00:00 2001 From: Poul-Henning Kamp Date: Tue, 22 Aug 2023 10:58:30 +0000 Subject: [PATCH 21/29] Slightly more coverage & consistency --- bin/varnishd/http2/cache_http2_hpack.c | 2 +- bin/varnishtest/tests/t02023.vtc | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/bin/varnishd/http2/cache_http2_hpack.c b/bin/varnishd/http2/cache_http2_hpack.c index 26be1b4d1ef..b05ab31e096 100644 --- a/bin/varnishd/http2/cache_http2_hpack.c +++ b/bin/varnishd/http2/cache_http2_hpack.c @@ -119,7 +119,7 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) } if (state == 3 && b[len - 1] <= 0x20) { VSLb(hp->vsl, SLT_BogoHeader, - "Illegal val (end) %.*s", + "Illegal field value (end) %.*s", (int)(len > 20 ? 20 : len), b); return (H2SE_PROTOCOL_ERROR); } diff --git a/bin/varnishtest/tests/t02023.vtc b/bin/varnishtest/tests/t02023.vtc index 59c7fe5d79d..5f5f434bf55 100644 --- a/bin/varnishtest/tests/t02023.vtc +++ b/bin/varnishtest/tests/t02023.vtc @@ -90,3 +90,10 @@ client c1 { rxrst } -run } -run + +client c1 { + stream 1 { + txreq -hdr "foo" "bar " + rxrst + } -run +} -run From 5e5a8cc5e15d69dc0fe1812727dec2d02bc13fd7 Mon Sep 17 00:00:00 2001 From: Walid Boudebouda Date: Fri, 8 Sep 2023 16:55:18 +0200 Subject: [PATCH 22/29] hpack: Turn header validation state into an enum Signed-off-by: Dridi Boukelmoune --- bin/varnishd/http2/cache_http2_hpack.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/bin/varnishd/http2/cache_http2_hpack.c b/bin/varnishd/http2/cache_http2_hpack.c index b05ab31e096..7d9625b41ab 100644 --- a/bin/varnishd/http2/cache_http2_hpack.c +++ b/bin/varnishd/http2/cache_http2_hpack.c @@ -44,6 +44,12 @@ static h2_error h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) { const char *p; + enum { + FLD_NAME_FIRST, + FLD_NAME, + FLD_VALUE_FIRST, + FLD_VALUE + } state; CHECK_OBJ_NOTNULL(hp, HTTP_MAGIC); AN(b); @@ -60,15 +66,15 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) // VSLb(hp->vsl, SLT_Debug, "CHDR [%.*s] [%.*s]", // (int)namelen, b, (int)(len - namelen), b + namelen); - int state = 0; + state = FLD_NAME_FIRST; for (p = b; p < b + namelen - 2; p++) { switch(state) { - case 0: /* First char of field */ - state = 1; + case FLD_NAME_FIRST: + state = FLD_NAME; if (*p == ':') break; /* FALL_THROUGH */ - case 1: /* field name */ + case FLD_NAME: if (*p <= 0x20 || *p >= 0x7f) { VSLb(hp->vsl, SLT_BogoHeader, "Illegal field header name (control): %.*s", @@ -93,19 +99,19 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) } } - state = 2; + state = FLD_VALUE_FIRST; for (p = b + namelen; p < b + len; p++) { switch(state) { - case 2: /* First char of field */ + case FLD_VALUE_FIRST: if (*p == ' ' || *p == 0x09) { VSLb(hp->vsl, SLT_BogoHeader, "Illegal field value start %.*s", (int)(len > 20 ? 20 : len), b); return (H2SE_PROTOCOL_ERROR); } - state = 3; + state = FLD_VALUE; /* FALL_THROUGH */ - case 3: /* field value character */ + case FLD_VALUE: if (*p != 0x09 && (*p < 0x20 || *p == 0x7f)) { VSLb(hp->vsl, SLT_BogoHeader, "Illegal field value (control) %.*s", @@ -117,7 +123,7 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) WRONG("http2 field value validation state"); } } - if (state == 3 && b[len - 1] <= 0x20) { + if (state == FLD_VALUE && b[len - 1] <= 0x20) { VSLb(hp->vsl, SLT_BogoHeader, "Illegal field value (end) %.*s", (int)(len > 20 ? 20 : len), b); From 34dde811b7465f49b1d1525facce03928f5704aa Mon Sep 17 00:00:00 2001 From: Walid Boudebouda Date: Fri, 8 Sep 2023 17:25:06 +0200 Subject: [PATCH 23/29] hpack: Check illegal header blanks with vct_issp() Signed-off-by: Dridi Boukelmoune --- bin/varnishd/http2/cache_http2_hpack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/varnishd/http2/cache_http2_hpack.c b/bin/varnishd/http2/cache_http2_hpack.c index 7d9625b41ab..b57fda19124 100644 --- a/bin/varnishd/http2/cache_http2_hpack.c +++ b/bin/varnishd/http2/cache_http2_hpack.c @@ -103,7 +103,7 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) for (p = b + namelen; p < b + len; p++) { switch(state) { case FLD_VALUE_FIRST: - if (*p == ' ' || *p == 0x09) { + if (vct_issp(*p)) { VSLb(hp->vsl, SLT_BogoHeader, "Illegal field value start %.*s", (int)(len > 20 ? 20 : len), b); @@ -123,7 +123,7 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) WRONG("http2 field value validation state"); } } - if (state == FLD_VALUE && b[len - 1] <= 0x20) { + if (state == FLD_VALUE && vct_issp(b[len - 1])) { VSLb(hp->vsl, SLT_BogoHeader, "Illegal field value (end) %.*s", (int)(len > 20 ? 20 : len), b); From 5db4965a5b921980fd858cb770004753f4088037 Mon Sep 17 00:00:00 2001 From: Walid Boudebouda Date: Fri, 8 Sep 2023 17:37:26 +0200 Subject: [PATCH 24/29] hpack: Validate header values with vct_ishdrval() Signed-off-by: Dridi Boukelmoune --- bin/varnishd/http2/cache_http2_hpack.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/varnishd/http2/cache_http2_hpack.c b/bin/varnishd/http2/cache_http2_hpack.c index b57fda19124..74e1d8520b7 100644 --- a/bin/varnishd/http2/cache_http2_hpack.c +++ b/bin/varnishd/http2/cache_http2_hpack.c @@ -112,9 +112,9 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) state = FLD_VALUE; /* FALL_THROUGH */ case FLD_VALUE: - if (*p != 0x09 && (*p < 0x20 || *p == 0x7f)) { + if (!vct_ishdrval(*p)) { VSLb(hp->vsl, SLT_BogoHeader, - "Illegal field value (control) %.*s", + "Illegal field value %.*s", (int)(len > 20 ? 20 : len), b); return (H2SE_PROTOCOL_ERROR); } From 01059ea7ea49dca22cc3bad8526a61941e6ad221 Mon Sep 17 00:00:00 2001 From: Walid Boudebouda Date: Fri, 8 Sep 2023 17:13:19 +0200 Subject: [PATCH 25/29] hpack: Remove redundant/incorrect header validation 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 --- bin/varnishd/http2/cache_http2_hpack.c | 6 ------ 1 file changed, 6 deletions(-) diff --git a/bin/varnishd/http2/cache_http2_hpack.c b/bin/varnishd/http2/cache_http2_hpack.c index 74e1d8520b7..9c179064ba5 100644 --- a/bin/varnishd/http2/cache_http2_hpack.c +++ b/bin/varnishd/http2/cache_http2_hpack.c @@ -75,12 +75,6 @@ h2h_checkhdr(const struct http *hp, const char *b, size_t namelen, size_t len) break; /* FALL_THROUGH */ case FLD_NAME: - if (*p <= 0x20 || *p >= 0x7f) { - VSLb(hp->vsl, SLT_BogoHeader, - "Illegal field header name (control): %.*s", - (int)(len > 20 ? 20 : len), b); - return (H2SE_PROTOCOL_ERROR); - } if (isupper(*p)) { VSLb(hp->vsl, SLT_BogoHeader, "Illegal field header name (upper-case): %.*s", From 247826303555cc911dcd0bb8043914861c5651dc Mon Sep 17 00:00:00 2001 From: Walid Boudebouda Date: Fri, 8 Sep 2023 17:54:43 +0200 Subject: [PATCH 26/29] vtc: More HPACK header validation coverage 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 --- bin/varnishtest/tests/t02023.vtc | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/bin/varnishtest/tests/t02023.vtc b/bin/varnishtest/tests/t02023.vtc index 5f5f434bf55..388f3a70ad1 100644 --- a/bin/varnishtest/tests/t02023.vtc +++ b/bin/varnishtest/tests/t02023.vtc @@ -30,6 +30,7 @@ client c1 { stream 1 { txreq -url "" rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -37,6 +38,7 @@ client c1 { stream 1 { txreq -scheme "" rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -44,6 +46,7 @@ client c1 { stream 1 { txreq -req "" rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -51,8 +54,9 @@ varnish v1 -vsl_catchup client c1 { stream 1 { - txreq -hdr "fo o" " bar" + txreq -hdr "foo" " bar" rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -60,6 +64,7 @@ client c1 { stream 1 { txreq -hdr "foo" " " rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -67,6 +72,7 @@ client c1 { stream 1 { txreq -hdr ":foo" "bar" rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -74,6 +80,7 @@ client c1 { stream 1 { txreq -hdr "foo" "b\x0car" rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -81,6 +88,7 @@ client c1 { stream 1 { txreq -hdr "f o" "bar" rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -88,6 +96,7 @@ client c1 { stream 1 { txreq -hdr "f: o" "bar" rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run @@ -95,5 +104,22 @@ client c1 { stream 1 { txreq -hdr "foo" "bar " rxrst + expect rst.err == PROTOCOL_ERROR + } -run +} -run + +client c1 { + stream 1 { + txreq -hdr "foo" " bar" + rxrst + expect rst.err == PROTOCOL_ERROR + } -run +} -run + +client c1 { + stream 1 { + txreq -hdr "foo" "bar " + rxrst + expect rst.err == PROTOCOL_ERROR } -run } -run From 05123d431d2e91ef9c924af3400ab085e469ca8e Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Thu, 14 Sep 2023 12:59:21 +0200 Subject: [PATCH 27/29] vtc: Coverage for h2 empty header in t02023 --- bin/varnishtest/tests/t02023.vtc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/bin/varnishtest/tests/t02023.vtc b/bin/varnishtest/tests/t02023.vtc index 388f3a70ad1..13c4cb4459f 100644 --- a/bin/varnishtest/tests/t02023.vtc +++ b/bin/varnishtest/tests/t02023.vtc @@ -1,4 +1,4 @@ -varnishtest "Empty pseudo-headers" +varnishtest "Empty and invalid headers" server s1 { rxreq @@ -50,6 +50,14 @@ client c1 { } -run } -run +client c1 { + stream 1 { + txreq -hdr "empty" "" + rxresp + expect resp.status == 200 + } -run +} -run + varnish v1 -vsl_catchup client c1 { From 2df7705d8928417849e1890121cb858afaa122dd Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 24 Oct 2023 14:13:39 +0200 Subject: [PATCH 28/29] vmod_h2: Polish manual --- vmod/vmod_h2.vcc | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/vmod/vmod_h2.vcc b/vmod/vmod_h2.vcc index cd7529c9f77..b96ac5a8bf4 100644 --- a/vmod/vmod_h2.vcc +++ b/vmod/vmod_h2.vcc @@ -34,7 +34,8 @@ DESCRIPTION =========== This VMOD contains functions to control the HTTP2 transport built into -Varnish-Cache. +Varnish-Cache. Using VMOD outside of a client context will immediately +fail the VCL transaction. $Function BOOL is() @@ -49,8 +50,7 @@ Returns -1 when used outside the HTTP2 transport. Otherwise returns the previous value. If the call leads to a change in the rate limit parameters, the -current budget as retuned by -`h2.rapid_reset_budget()`_ is reset. +current budget as retuned by `h2.rapid_reset_budget()`_ is reset. $Function INT rapid_reset_limit([INT number]) @@ -61,8 +61,7 @@ Returns -1 when used outside the HTTP2 transport. Otherwise returns the previous value. If the call leads to a change in the rate limit parameters, the -current budget as retuned by -`h2.rapid_reset_budget()`_ is reset. +current budget as retuned by `h2.rapid_reset_budget()`_ is reset. $Function DURATION rapid_reset_period([DURATION duration]) @@ -73,8 +72,7 @@ Returns -1 when used outside the HTTP2 transport. Otherwise returns the previous value. If the call leads to a change in the rate limit parameters, the -current budget as retuned by -`h2.rapid_reset_budget()`_ is reset. +current budget as retuned by `h2.rapid_reset_budget()`_ is reset. $Function REAL rapid_reset_budget() From 990bf8147283fab1bcc6e41fb02ee7247ccb508f Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 24 Oct 2023 14:20:47 +0200 Subject: [PATCH 29/29] build: Generate and install man/vmod_h2.3 --- man/Makefile.am | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/man/Makefile.am b/man/Makefile.am index b78dc7cfe92..a1f4896c110 100644 --- a/man/Makefile.am +++ b/man/Makefile.am @@ -25,7 +25,8 @@ dist_man_MANS = \ vmod_vtc.3 \ vmod_blob.3 \ vmod_unix.3 \ - vmod_proxy.3 + vmod_proxy.3 \ + vmod_h2.3 CLEANFILES = $(dist_man_MANS) @@ -136,4 +137,7 @@ vmod_unix.3: $(top_builddir)/vmod/vmod_unix.man.rst vmod_proxy.3: $(top_builddir)/vmod/vmod_proxy.man.rst $(BUILD_MAN) $? $@ +vmod_h2.3: $(top_builddir)/vmod/vmod_h2.man.rst + $(BUILD_MAN) $? $@ + .NOPATH: $(dist_man_MANS)