From ca8061e7a0567cfacd55e032aeb8d113533c301e Mon Sep 17 00:00:00 2001 From: Guy Arbitman Date: Fri, 17 Nov 2023 13:54:31 +0200 Subject: [PATCH 1/3] usm: http2: Change type from u8 to u64, to allow indexes bigger than 255 --- pkg/network/ebpf/c/protocols/http2/decoding.h | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/network/ebpf/c/protocols/http2/decoding.h b/pkg/network/ebpf/c/protocols/http2/decoding.h index 8b5555bcc043a..847dd65a07616 100644 --- a/pkg/network/ebpf/c/protocols/http2/decoding.h +++ b/pkg/network/ebpf/c/protocols/http2/decoding.h @@ -43,7 +43,7 @@ static __always_inline http2_stream_t *http2_fetch_stream(const http2_stream_key } // Similar to read_var_int, but with a small optimization of getting the current character as input argument. -static __always_inline bool read_var_int_with_given_current_char(struct __sk_buff *skb, skb_info_t *skb_info, __u8 current_char_as_number, __u8 max_number_for_bits, __u8 *out) { +static __always_inline bool read_var_int_with_given_current_char(struct __sk_buff *skb, skb_info_t *skb_info, __u64 current_char_as_number, __u64 max_number_for_bits, __u64 *out) { current_char_as_number &= max_number_for_bits; if (current_char_as_number < max_number_for_bits) { @@ -51,8 +51,8 @@ static __always_inline bool read_var_int_with_given_current_char(struct __sk_buf return true; } - __u8 next_char = 0; - if (bpf_skb_load_bytes(skb, skb_info->data_off, &next_char, sizeof(next_char)) >=0 && (next_char & 128) == 0) { + __u64 next_char = 0; + if (bpf_skb_load_bytes(skb, skb_info->data_off, &next_char, 1) >=0 && (next_char & 128) == 0) { skb_info->data_off++; *out = current_char_as_number + next_char & 127; return true; @@ -68,9 +68,9 @@ static __always_inline bool read_var_int_with_given_current_char(struct __sk_buf // n must always be between 1 and 8. // // The returned remain buffer is either a smaller suffix of p, or err != nil. -static __always_inline bool read_var_int(struct __sk_buff *skb, skb_info_t *skb_info, __u8 max_number_for_bits, __u8 *out) { - __u8 current_char_as_number = 0; - if (bpf_skb_load_bytes(skb, skb_info->data_off, ¤t_char_as_number, sizeof(current_char_as_number)) < 0) { +static __always_inline bool read_var_int(struct __sk_buff *skb, skb_info_t *skb_info, __u64 max_number_for_bits, __u64 *out) { + __u64 current_char_as_number = 0; + if (bpf_skb_load_bytes(skb, skb_info->data_off, ¤t_char_as_number, 1) < 0) { return false; } skb_info->data_off++; @@ -119,8 +119,8 @@ READ_INTO_BUFFER(path, HTTP2_MAX_PATH_LEN, BLK_SIZE) // parse_field_literal handling the case when the key is part of the static table and the value is a dynamic string // which will be stored in the dynamic table. -static __always_inline bool parse_field_literal(struct __sk_buff *skb, skb_info_t *skb_info, http2_header_t *headers_to_process, __u8 index, __u64 global_dynamic_counter, __u8 *interesting_headers_counter) { - __u8 str_len = 0; +static __always_inline bool parse_field_literal(struct __sk_buff *skb, skb_info_t *skb_info, http2_header_t *headers_to_process, __u64 index, __u64 global_dynamic_counter, __u8 *interesting_headers_counter) { + __u64 str_len = 0; if (!read_var_int(skb, skb_info, MAX_6_BITS, &str_len)) { return false; } @@ -162,8 +162,8 @@ static __always_inline __u8 filter_relevant_headers(struct __sk_buff *skb, skb_i const __u32 end = frame_end < skb_info->data_end + 1 ? frame_end : skb_info->data_end + 1; bool is_literal = false; bool is_indexed = false; - __u8 max_bits = 0; - __u8 index = 0; + __u64 max_bits = 0; + __u64 index = 0; __u64 *global_dynamic_counter = get_dynamic_counter(tup); if (global_dynamic_counter == NULL) { From f5545b35494c597c9dfd30ff5d814be934f6b3cf Mon Sep 17 00:00:00 2001 From: Guy Arbitman Date: Fri, 17 Nov 2023 13:56:31 +0200 Subject: [PATCH 2/3] usm: http2: Add missing parenthesis That bug cause wrong order or execution and lead into truncation --- pkg/network/ebpf/c/protocols/http2/decoding.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/network/ebpf/c/protocols/http2/decoding.h b/pkg/network/ebpf/c/protocols/http2/decoding.h index 847dd65a07616..89ae937da2b6d 100644 --- a/pkg/network/ebpf/c/protocols/http2/decoding.h +++ b/pkg/network/ebpf/c/protocols/http2/decoding.h @@ -54,7 +54,7 @@ static __always_inline bool read_var_int_with_given_current_char(struct __sk_buf __u64 next_char = 0; if (bpf_skb_load_bytes(skb, skb_info->data_off, &next_char, 1) >=0 && (next_char & 128) == 0) { skb_info->data_off++; - *out = current_char_as_number + next_char & 127; + *out = current_char_as_number + (next_char & 127); return true; } From b0564109b7435fa4348ca94595697555c1c8320d Mon Sep 17 00:00:00 2001 From: Guy Arbitman Date: Fri, 17 Nov 2023 14:02:43 +0200 Subject: [PATCH 3/3] usm: http2: Added test --- pkg/network/usm/monitor_test.go | 53 +++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/pkg/network/usm/monitor_test.go b/pkg/network/usm/monitor_test.go index 3424ab387d447..d5ae71eb7534b 100644 --- a/pkg/network/usm/monitor_test.go +++ b/pkg/network/usm/monitor_test.go @@ -539,6 +539,59 @@ func TestHTTP2(t *testing.T) { }) } +func (s *USMHTTP2Suite) TestHTTP2ManyDifferentPaths() { + t := s.T() + cfg := networkconfig.New() + cfg.EnableHTTP2Monitoring = true + + startH2CServer(t) + + monitor, err := NewMonitor(cfg, nil, nil, nil) + require.NoError(t, err) + require.NoError(t, monitor.Start()) + defer monitor.Stop() + + // Should be bigger than the length of the http2_dynamic_table which is 1024 + numberOfRequests := 1500 + clients := getClientsArray(t, 1) + for i := 0; i < numberOfRequests; i++ { + for j := 0; j < 2; j++ { + req, err := clients[0].Post(fmt.Sprintf("%s/test-%d", http2SrvAddr, i+1), "application/json", bytes.NewReader([]byte("test"))) + require.NoError(t, err, "could not make request") + req.Body.Close() + } + } + + matches := PrintableInt(0) + + seenRequests := map[string]int{} + assert.Eventuallyf(t, func() bool { + stats := monitor.GetProtocolStats() + http2Stats, ok := stats[protocols.HTTP2] + if !ok { + return false + } + http2StatsTyped := http2Stats.(map[http.Key]*http.RequestStats) + for key, stat := range http2StatsTyped { + if (key.DstPort == http2SrvPort || key.SrcPort == http2SrvPort) && key.Method == http.MethodPost && strings.HasPrefix(key.Path.Content.Get(), "/test") { + if _, ok := seenRequests[key.Path.Content.Get()]; !ok { + seenRequests[key.Path.Content.Get()] = 0 + } + seenRequests[key.Path.Content.Get()] += stat.Data[200].Count + matches.Add(stat.Data[200].Count) + } + } + + return matches.Load() == 2*numberOfRequests + }, time.Second*10, time.Millisecond*100, "%v != %v", &matches, 2*numberOfRequests) + + for i := 0; i < numberOfRequests; i++ { + if v, ok := seenRequests[fmt.Sprintf("/test-%d", i+1)]; !ok || v != 2 { + t.Logf("path: /test-%d should have 2 occurrences but instead has %d", i+1, v) + } + } +} + func (s *USMHTTP2Suite) TestSimpleHTTP2() { t := s.T() cfg := networkconfig.New()