Skip to content

Fix invalid HTTP2 parsing - #932

Merged
grcevski merged 4 commits into
open-telemetry:mainfrom
grcevski:fix_bad_http2_parse
Nov 26, 2025
Merged

Fix invalid HTTP2 parsing#932
grcevski merged 4 commits into
open-telemetry:mainfrom
grcevski:fix_bad_http2_parse

Conversation

@grcevski

Copy link
Copy Markdown
Contributor

Since HTTP2 (including gRPC) client connections can be long lived, we might never see the HTTP2 preamble in eBPF, so we have code in userspace that detects if a frame might be HTTP2/gRPC.

Since HTTP2/gRPC is binary and a lot of the header values are encoded in the static/dynamic tables, it's possible in practice to match HTTP2 accidentally with random binary text, e.g. something from TLS. Our logic to detect HTTP2 in userspace was too lenient and could cause false positives.

I've re-worked the code with additional logic:

  1. We now validate the values of the headers.
  2. We don't look for status anymore, since status is so easy to match and it's not clear if random value is valid or not. grpc-status is fine, since it's only in the dynamic table and this decoder doesn't have any.
  3. We must match at least keys, since matching :path without :method makes no sense, and matching :method alone is not clear since the constants are all in the static table and can be fooled by binary.

@grcevski
grcevski requested a review from a team as a code owner November 25, 2025 22:05
@codecov

codecov Bot commented Nov 25, 2025

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.48718% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 55.84%. Comparing base (c128d7b) to head (b26b754).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
pkg/ebpf/common/http2grpc_transform.go 88.57% 3 Missing and 1 partial ⚠️
pkg/ebpf/common/tcp_detect_transform.go 0.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #932      +/-   ##
==========================================
+ Coverage   55.26%   55.84%   +0.58%     
==========================================
  Files         253      255       +2     
  Lines       21769    22139     +370     
==========================================
+ Hits        12031    12364     +333     
- Misses       8911     8936      +25     
- Partials      827      839      +12     
Flag Coverage Δ
integration-test 22.88% <0.00%> (-0.40%) ⬇️
integration-test-arm 0.00% <0.00%> (ø)
k8s-integration-test 2.68% <0.00%> (-0.05%) ⬇️
oats-test 0.00% <0.00%> (ø)
unittests 47.12% <79.48%> (+0.72%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@rafaelroquetto rafaelroquetto left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gnarly!

@grcevski
grcevski merged commit 4ca64e8 into open-telemetry:main Nov 26, 2025
53 of 54 checks passed
@grcevski
grcevski deleted the fix_bad_http2_parse branch November 26, 2025 18:23
grcevski added a commit to grafana/opentelemetry-ebpf-instrumentation that referenced this pull request Nov 26, 2025
@MrAlias MrAlias added this to the v0.3.0 milestone Dec 3, 2025
@MrAlias MrAlias mentioned this pull request Dec 3, 2025
marctc pushed a commit to grafana/opentelemetry-ebpf-instrumentation that referenced this pull request Dec 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants