Skip to content

Commit

Permalink
[USM] Fix HTTP/2 verifier issue (#25076)
Browse files Browse the repository at this point in the history 
* try to fix verifier issue

* removed fragmented telemetry

* fix print

* fix issue
  • Loading branch information
@amitslavin
amitslavin committed May 8, 2024
1 parent afdb477 commit 1b556e5
Show file tree
Hide file tree
Showing 8 changed files with 17 additions and 175 deletions.
19 changes: 0 additions & 19 deletions pkg/network/ebpf/c/protocols/http2/decoding-common.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,23 +166,4 @@ static __always_inline void reset_frame(http2_frame_t *out) {
*out = (http2_frame_t){ 0 };
}

// check_frame_split checks if the frame is split into multiple tcp payloads, if so, it increments the split counter.
static __always_inline void check_frame_split(http2_telemetry_t *http2_tel, __u32 data_off, __u32 data_end, http2_frame_t current_frame){
bool is_headers = current_frame.type == kHeadersFrame;
bool is_rst_frame = current_frame.type == kRSTStreamFrame;
bool is_data_end_of_stream = ((current_frame.flags & HTTP2_END_OF_STREAM) == HTTP2_END_OF_STREAM) && (current_frame.type == kDataFrame);
bool is_headers_end_of_stream = ((current_frame.flags & HTTP2_END_OF_STREAM) == HTTP2_END_OF_STREAM) && (is_headers);
if (data_off + current_frame.length > data_end) {
if (is_headers_end_of_stream) {
__sync_fetch_and_add(&http2_tel->fragmented_frame_count_headers_eos, 1);
} else if (is_data_end_of_stream) {
__sync_fetch_and_add(&http2_tel->fragmented_frame_count_data_eos, 1);
} else if (is_rst_frame) {
__sync_fetch_and_add(&http2_tel->fragmented_frame_count_rst, 1);
} else if (is_headers) {
__sync_fetch_and_add(&http2_tel->fragmented_frame_count_headers, 1);
}
}
}

#endif
4 changes: 0 additions & 4 deletions pkg/network/ebpf/c/protocols/http2/decoding-defs.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -244,10 +244,6 @@ typedef struct {
__u64 exceeding_max_interesting_frames;
__u64 exceeding_max_frames_to_filter;
__u64 path_size_bucket[HTTP2_TELEMETRY_PATH_BUCKETS+1];
__u64 fragmented_frame_count_headers;
__u64 fragmented_frame_count_rst;
__u64 fragmented_frame_count_data_eos;
__u64 fragmented_frame_count_headers_eos;
} http2_telemetry_t;

typedef struct {
Expand Down
3 changes: 0 additions & 3 deletions pkg/network/ebpf/c/protocols/http2/decoding-tls.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -548,8 +548,6 @@ static __always_inline void tls_find_relevant_frames(tls_dispatcher_arguments_t
break;
}

check_frame_split(http2_tel, info->data_off, info->data_end, current_frame);

// END_STREAM can appear only in Headers and Data frames.
// Check out https://datatracker.ietf.org/doc/html/rfc7540#section-6.1 for data frame, and
// https://datatracker.ietf.org/doc/html/rfc7540#section-6.2 for headers frame.
Expand Down Expand Up @@ -652,7 +650,6 @@ int uprobe__http2_tls_handle_first_frame(struct pt_regs *ctx) {
return 0;
}

check_frame_split(http2_tel, dispatcher_args_copy.data_off, dispatcher_args_copy.data_end, current_frame);
bool is_headers_or_rst_frame = current_frame.type == kHeadersFrame || current_frame.type == kRSTStreamFrame;
bool is_data_end_of_stream = ((current_frame.flags & HTTP2_END_OF_STREAM) == HTTP2_END_OF_STREAM) && (current_frame.type == kDataFrame);
if (is_headers_or_rst_frame || is_data_end_of_stream) {
Expand Down
4 changes: 0 additions & 4 deletions pkg/network/ebpf/c/protocols/http2/decoding.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -444,9 +444,6 @@ static __always_inline bool find_relevant_frames(struct __sk_buff *skb, skb_info
break;
}

// We are not checking for frame splits in the previous condition due to a verifier issue.
check_frame_split(http2_tel, skb_info->data_off,skb_info->data_end, current_frame);

// END_STREAM can appear only in Headers and Data frames.
// Check out https://datatracker.ietf.org/doc/html/rfc7540#section-6.1 for data frame, and
// https://datatracker.ietf.org/doc/html/rfc7540#section-6.2 for headers frame.
Expand Down Expand Up @@ -557,7 +554,6 @@ int socket__http2_handle_first_frame(struct __sk_buff *skb) {
return 0;
}

check_frame_split(http2_tel, dispatcher_args_copy.skb_info.data_off, dispatcher_args_copy.skb_info.data_end, current_frame);
bool is_headers_or_rst_frame = current_frame.type == kHeadersFrame || current_frame.type == kRSTStreamFrame;
bool is_data_end_of_stream = ((current_frame.flags & HTTP2_END_OF_STREAM) == HTTP2_END_OF_STREAM) && (current_frame.type == kDataFrame);
if (is_headers_or_rst_frame || is_data_end_of_stream) {
Expand Down
6 changes: 1 addition & 5 deletions pkg/network/protocols/http2/model_linux.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -428,10 +428,6 @@ HTTP2Telemetry{
"literal values exceed message count": %d,
"messages with more frames than we can filter": %d,
"messages with more interesting frames than we can process": %d,
"fragmented headers frame count": %d,
"fragmented headers frame count end of stream": %d,
"fragmented data frame count end of stream": %d,
"fragmented rst frame count": %d,
"path headers length distribution": {
"in range [0, 120)": %d,
"in range [120, 130)": %d,
Expand All @@ -443,7 +439,7 @@ HTTP2Telemetry{
"in range [180, infinity)": %d
}
}`, t.Request_seen, t.Response_seen, t.End_of_stream, t.End_of_stream_rst, t.Literal_value_exceeds_frame,
t.Exceeding_max_frames_to_filter, t.Exceeding_max_interesting_frames, t.Fragmented_frame_count_headers, t.Fragmented_frame_count_headers_eos, t.Fragmented_frame_count_data_eos, t.Fragmented_frame_count_rst, t.Path_size_bucket[0], t.Path_size_bucket[1],
t.Exceeding_max_frames_to_filter, t.Exceeding_max_interesting_frames, t.Path_size_bucket[0], t.Path_size_bucket[1],
t.Path_size_bucket[2], t.Path_size_bucket[3], t.Path_size_bucket[4], t.Path_size_bucket[5], t.Path_size_bucket[6],
t.Path_size_bucket[7])
}
24 changes: 8 additions & 16 deletions pkg/network/protocols/http2/telemetry.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,10 +113,6 @@ func (t *kernelTelemetry) update(tel *HTTP2Telemetry, isTLS bool) {
t.literalValueExceedsFrame.add(int64(telemetryDelta.Literal_value_exceeds_frame), isTLS)
t.exceedingMaxInterestingFrames.add(int64(telemetryDelta.Exceeding_max_interesting_frames), isTLS)
t.exceedingMaxFramesToFilter.add(int64(telemetryDelta.Exceeding_max_frames_to_filter), isTLS)
t.fragmentedFrameCountRST.add(int64(telemetryDelta.Fragmented_frame_count_rst), isTLS)
t.fragmentedHeadersFrameEOSCount.add(int64(telemetryDelta.Fragmented_frame_count_headers_eos), isTLS)
t.fragmentedHeadersFrameCount.add(int64(telemetryDelta.Fragmented_frame_count_headers), isTLS)
t.fragmentedDataFrameEOSCount.add(int64(telemetryDelta.Fragmented_frame_count_data_eos), isTLS)
for bucketIndex := range t.pathSizeBucket {
t.pathSizeBucket[bucketIndex].add(int64(telemetryDelta.Path_size_bucket[bucketIndex]), isTLS)
}
Expand All @@ -131,18 +127,14 @@ func (t *kernelTelemetry) Log() {
// Sub generates a new HTTP2Telemetry object by subtracting the values of this HTTP2Telemetry object from the other
func (t *HTTP2Telemetry) Sub(other HTTP2Telemetry) *HTTP2Telemetry {
return &HTTP2Telemetry{
Request_seen: t.Request_seen - other.Request_seen,
Response_seen: t.Response_seen - other.Response_seen,
End_of_stream: t.End_of_stream - other.End_of_stream,
End_of_stream_rst: t.End_of_stream_rst - other.End_of_stream_rst,
Literal_value_exceeds_frame: t.Literal_value_exceeds_frame - other.Literal_value_exceeds_frame,
Exceeding_max_interesting_frames: t.Exceeding_max_interesting_frames - other.Exceeding_max_interesting_frames,
Exceeding_max_frames_to_filter: t.Exceeding_max_frames_to_filter - other.Exceeding_max_frames_to_filter,
Fragmented_frame_count_headers: t.Fragmented_frame_count_headers - other.Fragmented_frame_count_headers,
Fragmented_frame_count_data_eos: t.Fragmented_frame_count_data_eos - other.Fragmented_frame_count_data_eos,
Fragmented_frame_count_rst: t.Fragmented_frame_count_rst - other.Fragmented_frame_count_rst,
Fragmented_frame_count_headers_eos: t.Fragmented_frame_count_headers_eos - other.Fragmented_frame_count_headers_eos,
Path_size_bucket: computePathSizeBucketDifferences(t.Path_size_bucket, other.Path_size_bucket),
Request_seen: t.Request_seen - other.Request_seen,
Response_seen: t.Response_seen - other.Response_seen,
End_of_stream: t.End_of_stream - other.End_of_stream,
End_of_stream_rst: t.End_of_stream_rst - other.End_of_stream_rst,
Literal_value_exceeds_frame: t.Literal_value_exceeds_frame - other.Literal_value_exceeds_frame,
Exceeding_max_interesting_frames: t.Exceeding_max_interesting_frames - other.Exceeding_max_interesting_frames,
Exceeding_max_frames_to_filter: t.Exceeding_max_frames_to_filter - other.Exceeding_max_frames_to_filter,
Path_size_bucket: computePathSizeBucketDifferences(t.Path_size_bucket, other.Path_size_bucket),
}
}

Expand Down
20 changes: 8 additions & 12 deletions pkg/network/protocols/http2/types_linux.go
View file

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

112 changes: 0 additions & 112 deletions pkg/network/usm/usm_http2_monitor_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1250,118 +1250,6 @@ func (s *usmHTTP2Suite) TestRawTraffic() {
}
}

func (s *usmHTTP2Suite) TestFrameSplitIntoMultiplePackets() {
t := s.T()
cfg := s.getCfg()

// Start local server and register its cleanup.
t.Cleanup(startH2CServer(t, authority, s.isTLS))

// Start the proxy server.
proxyProcess, cancel := proxy.NewExternalUnixTransparentProxyServer(t, unixPath, authority, s.isTLS)
t.Cleanup(cancel)
require.NoError(t, proxy.WaitForConnectionReady(unixPath))

tests := []struct {
name string
messageBuilder func() [][]byte
fragmentedFrameCountRST uint64
fragmentedHeadersFrameEOSCount uint64
fragmentedHeadersFrameCount uint64
fragmentedDataFrameEOSCount uint64
}{
{
name: "validate fragmented headers frame with eos",
// The purpose of this test is to validate that we cannot handle reassembled tcp segments.
messageBuilder: func() [][]byte {
const endStream = true
a := newFramer().
writeHeaders(t, 1, usmhttp2.HeadersFrameOptions{Headers: testHeaders(), EndStream: endStream}).
writeData(t, 1, true, emptyBody).bytes()
return [][]byte{
// we split it in 10 bytes in order to split the payload itself.
a[:10],
a[10:],
}
},
fragmentedHeadersFrameEOSCount: 1,
},
{
name: "validate fragmented data frame with eos",
// The purpose of this test is to validate that we cannot handle reassembled tcp segments.
messageBuilder: func() [][]byte {
a := newFramer().
writeHeaders(t, 1, usmhttp2.HeadersFrameOptions{Headers: testHeaders()}).bytes()
b := newFramer().writeData(t, 1, true, []byte("test1234")).bytes()
return [][]byte{
// we split it in 10 bytes in order to split the payload itself.
a,
b[:10],
}
},
fragmentedDataFrameEOSCount: 1,
},
{
name: "validate fragmented rst frame",
// The purpose of this test is to validate that we cannot handle reassembled tcp segments.
messageBuilder: func() [][]byte {
a := newFramer().
writeHeaders(t, 1, usmhttp2.HeadersFrameOptions{Headers: testHeaders()}).bytes()
b := newFramer().writeRSTStream(t, 1, 1).bytes()
return [][]byte{
// we split it in 10 bytes in order to split the payload itself.
a,
b[:10],
}
},
fragmentedFrameCountRST: 1,
},
{
name: "validate fragmented headers frame",
// The purpose of this test is to validate that we cannot handle reassembled tcp segments.
messageBuilder: func() [][]byte {
a := newFramer().
writeHeaders(t, 1, usmhttp2.HeadersFrameOptions{Headers: testHeaders()}).
writeData(t, 1, true, emptyBody).bytes()
return [][]byte{
// we split it in 10 bytes in order to split the payload itself.
a[:10],
a[10:],
}
},
fragmentedHeadersFrameCount: 1,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
usmMonitor := setupUSMTLSMonitor(t, cfg)
if s.isTLS {
utils.WaitForProgramsToBeTraced(t, "go-tls", proxyProcess.Process.Pid)
}

c := dialHTTP2Server(t)

// Composing a message with the number of setting frames we want to send.
require.NoError(t, writeInput(c, 500*time.Millisecond, tt.messageBuilder()...))

assert.Eventually(t, func() bool {
telemetry, err := getHTTP2KernelTelemetry(usmMonitor, s.isTLS)
require.NoError(t, err, "could not get http2 telemetry")
require.Equal(t, telemetry.Fragmented_frame_count_data_eos, tt.fragmentedDataFrameEOSCount, "expected to see %d fragmented data eos frames and got %d", tt.fragmentedDataFrameEOSCount, telemetry.Fragmented_frame_count_data_eos)
require.Equal(t, telemetry.Fragmented_frame_count_headers_eos, tt.fragmentedHeadersFrameEOSCount, "expected to see %d fragmented headers eos frames and got %d", tt.fragmentedHeadersFrameEOSCount, telemetry.Fragmented_frame_count_headers_eos)
require.Equal(t, telemetry.Fragmented_frame_count_rst, tt.fragmentedFrameCountRST, "expected to see %d fragmented rst frames and got %d", tt.fragmentedFrameCountRST, telemetry.Fragmented_frame_count_rst)
require.Equal(t, telemetry.Fragmented_frame_count_headers, tt.fragmentedHeadersFrameCount, "expected to see %d fragmented headers frames and got %d", tt.fragmentedHeadersFrameCount, telemetry.Fragmented_frame_count_headers)
return true

}, time.Second*5, time.Millisecond*100)
if t.Failed() {
ebpftest.DumpMapsTestHelper(t, usmMonitor.DumpMaps, usmhttp2.InFlightMap, "http2_dynamic_table")
dumpTelemetry(t, usmMonitor, s.isTLS)
}
})
}
}

func (s *usmHTTP2Suite) TestDynamicTable() {
t := s.T()
cfg := s.getCfg()
Expand Down

0 comments on commit 1b556e5

Please sign in to comment.