in_ebpf: Implement openssl trace - #11793
Conversation
📝 WalkthroughWalkthroughThe PR adds OpenSSL TLS tracing to the in_ebpf plugin: new TLS event types and payloads, a generated eBPF probe for SSL handshake/read/write/shutdown, a userspace handler and registration, and runtime test coverage. ChangesOpenSSL TLS tracing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
d4f8c70 to
df24026
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
df24026 to
ad57117
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
plugins/in_ebpf/traces/includes/common/events.h (1)
170-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
tls_handshake_eventandtls_io_eventare byte-for-byte identical.Both structs share the same fields (
ssl_ptr,__s64 latency_ns,int ret), and the downstream handler branches onev->typepurely to select between two interchangeable layouts. A singlestruct tls_event(with both union members aliasing it, or a single member) would remove the duplication and the redundant branching inhandler.c.Keeping them split is acceptable if you intend the two payloads to diverge later; otherwise consider consolidating.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_ebpf/traces/includes/common/events.h` around lines 170 - 181, The two event payload structs are duplicated and should be consolidated if they are meant to stay identical. Update the shared event model in events.h by introducing a single tls_event layout (or a shared alias/union-backed type) and then update the downstream logic in handler.c to use that common type instead of branching between tls_handshake_event and tls_io_event for the same fields. Keep the split only if you plan to add different payloads later; otherwise remove the redundant struct definitions and redundant type-based handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/in_ebpf/traces/openssl/handler.c`:
- Around line 32-43: `encode_openssl_event` currently treats any unknown
`ev->type` as `openssl_tls_handshake`, which can produce inconsistent payloads
when later fields are encoded as `tls_io`. Update the event-type handling in
`encode_openssl_event` to explicitly accept the known TLS read/write/shutdown
cases, add a dedicated handshake branch for `EVENT_TYPE_TLS_HANDSHAKE`, and
return an error for any unsupported `ev->type`. Make the same validation
consistent across the body/status/message encoding paths so only valid event
types are encoded and no default fallback is used.
---
Nitpick comments:
In `@plugins/in_ebpf/traces/includes/common/events.h`:
- Around line 170-181: The two event payload structs are duplicated and should
be consolidated if they are meant to stay identical. Update the shared event
model in events.h by introducing a single tls_event layout (or a shared
alias/union-backed type) and then update the downstream logic in handler.c to
use that common type instead of branching between tls_handshake_event and
tls_io_event for the same fields. Keep the split only if you plan to add
different payloads later; otherwise remove the redundant struct definitions and
redundant type-based handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 80c20ac5-6008-490d-95b5-30131ebd69f9
📒 Files selected for processing (9)
plugins/in_ebpf/CMakeLists.txtplugins/in_ebpf/traces/includes/common/encoder.hplugins/in_ebpf/traces/includes/common/events.hplugins/in_ebpf/traces/openssl/bpf.c.inplugins/in_ebpf/traces/openssl/handler.cplugins/in_ebpf/traces/openssl/handler.hplugins/in_ebpf/traces/traces.htests/runtime/CMakeLists.txttests/runtime/in_ebpf_openssl_handler.c
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad571174bd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
```plugins/in_ebpf/traces/openssl/bpf.c.in (1)
45-59: 🔒 Security & Privacy | 🟠 MajorDo not hard-code
mntns_idto 0; retrieve it and optionally filter by mount namespace.The
fill_commonfunction hard-codesevent->common.mntns_idto 0, ignoring the included<gadget/mntns_filter.h>. This prevents container-aware filtering and causes event metadata to report unscoped namespaces. Align with the pattern used inplugins/in_ebpf/traces/vfs/bpf.candplugins/in_ebpf/traces/tcp/bpf.cto correctly capture the namespace ID.<details> <summary>Reference pattern from plugins/in_ebpf/traces/tcp/bpf.c</summary> ```c __u64 mntns_id = gadget_get_mntns_id(); if (gadget_should_discard_mntns_id(mntns_id)) { return 0; } // ... event->common.mntns_id = mntns_id;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_ebpf/traces/openssl/bpf.c.in` around lines 45 - 59, The fill_common helper in the OpenSSL eBPF trace is hard-coding event->common.mntns_id to 0 instead of reading the current mount namespace and honoring namespace filtering. Update fill_common to follow the same gadget_get_mntns_id and gadget_should_discard_mntns_id pattern used in vfs/tcp trace programs, and assign the retrieved mntns_id to event->common.mntns_id so container-scoped metadata is correct.
🧹 Nitpick comments (1)
plugins/in_ebpf/traces/openssl/bpf.c.in (1)
192-203: 🩺 Stability & Availability | 🔵 TrivialVerify strict symbol requirements for uprobe attachment
Although fallback probes for
SSL_readandSSL_writeare present in the code, ensure the eBPF skeleton attachment logic (trace_openssl) is configured to ignore failures for missingSSL_read_ex/SSL_write_exsymbols. If the loader treats the absence of these specific symbols as a fatal error, the plugin will fail to start on older OpenSSL versions despite the fallback definitions being available. Confirm that missing_exsymbols do not abort the initialization process.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_ebpf/traces/openssl/bpf.c.in` around lines 192 - 203, The eBPF loader for trace_openssl should not fail hard when SSL_read_ex/SSL_write_ex are missing, since fallback SSL_read/SSL_write probes already exist. Update the attachment logic in trace_openssl to treat missing _ex symbols as optional and continue initialization, while still attaching the available fallback probes. Make sure the probe setup around trace_uprobe_ssl_read_ex and trace_uretprobe_ssl_read_ex ignores these specific attachment failures instead of aborting startup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@plugins/in_ebpf/traces/openssl/bpf.c.in`:
- Around line 45-59: The fill_common helper in the OpenSSL eBPF trace is
hard-coding event->common.mntns_id to 0 instead of reading the current mount
namespace and honoring namespace filtering. Update fill_common to follow the
same gadget_get_mntns_id and gadget_should_discard_mntns_id pattern used in
vfs/tcp trace programs, and assign the retrieved mntns_id to
event->common.mntns_id so container-scoped metadata is correct.
---
Nitpick comments:
In `@plugins/in_ebpf/traces/openssl/bpf.c.in`:
- Around line 192-203: The eBPF loader for trace_openssl should not fail hard
when SSL_read_ex/SSL_write_ex are missing, since fallback SSL_read/SSL_write
probes already exist. Update the attachment logic in trace_openssl to treat
missing _ex symbols as optional and continue initialization, while still
attaching the available fallback probes. Make sure the probe setup around
trace_uprobe_ssl_read_ex and trace_uretprobe_ssl_read_ex ignores these specific
attachment failures instead of aborting startup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a67f0122-3093-4b04-8b4d-f2db25163c5e
📒 Files selected for processing (3)
plugins/in_ebpf/traces/openssl/bpf.c.inplugins/in_ebpf/traces/openssl/handler.ctests/runtime/in_ebpf_openssl_handler.c
🚧 Files skipped from review as they are similar to previous changes (1)
- plugins/in_ebpf/traces/openssl/handler.c
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
3028035 to
e89c53f
Compare
Signed-off-by: Hiroshi Hatake <hiroshi@chronosphere.io>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/in_ebpf/CMakeLists.txt (1)
47-53: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDistro-specific fallback and no existence check can silently bake an invalid uprobe path.
The resolved
LIBSSL_PATHis interpolated into the BPFSEC("uprobe/@LIBSSL_PATH@:SSL_*")strings at build time, so an unreachable path only fails later when libbpf tries to attach. Two gaps here:
- The fallback
/lib64/libssl.so.3is RHEL/Fedora-specific. Debian/Ubuntu place the runtime object at/usr/lib/<arch>/libssl.so.3, so the fallback resolves to a nonexistent file on those distros.- Nothing validates that the resolved path actually exists; only the static-archive case (lines 56-60) emits a diagnostic.
Consider validating existence and surfacing a clearer error/warning when nothing usable was found:
♻️ Suggested validation
if (NOT LIBSSL_PATH) set(LIBSSL_PATH "/lib64/libssl.so.3") endif() endif() +if (NOT EXISTS "${LIBSSL_PATH}") + message(WARNING + "eBPF OpenSSL uprobes resolved a libssl path that does not exist: ${LIBSSL_PATH}. " + "Set FLB_IN_EBPF_LIBSSL_PATH to the target host libssl shared object.") +endif()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@plugins/in_ebpf/CMakeLists.txt` around lines 47 - 53, The `LIBSSL_PATH` fallback logic in the CMake setup can silently select a distro-specific path that does not exist, which later breaks the `SEC("uprobe/@LIBSSL_PATH@:SSL_*")` probe targets. Update the resolution flow around the `find_library(LIBSSL_PATH ...)` and `/lib64/libssl.so.3` fallback to validate that the chosen path exists before accepting it. If no usable shared object is found, emit a clear warning or fatal diagnostic instead of baking an invalid path into the BPF build, and keep the existing static-archive handling consistent with this validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@plugins/in_ebpf/CMakeLists.txt`:
- Around line 47-53: The `LIBSSL_PATH` fallback logic in the CMake setup can
silently select a distro-specific path that does not exist, which later breaks
the `SEC("uprobe/@LIBSSL_PATH@:SSL_*")` probe targets. Update the resolution
flow around the `find_library(LIBSSL_PATH ...)` and `/lib64/libssl.so.3`
fallback to validate that the chosen path exists before accepting it. If no
usable shared object is found, emit a clear warning or fatal diagnostic instead
of baking an invalid path into the BPF build, and keep the existing
static-archive handling consistent with this validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2a7900a-538a-45dc-b5dd-5b987eda3f4a
📒 Files selected for processing (1)
plugins/in_ebpf/CMakeLists.txt
2 participants
In this PR, I implemented OpenSSL's uprobe traces for read, write, handshake, and shutdown.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Configuration/Compatibility
libsslpath.Tests