Fix pprof broad exposition#996
Conversation
When pprof was enabled, it was exposed on ":(port)" which stands for all net interfaces. Settings are changed so the user controls what address to use. For example, they can configure it with "127.0.0.1:6060" to restrict to the local loop (more secured), or keep ":6060" or "0.0.0.0:6060" for a broader exposition. Documented those security aspects. Note that this setting is not exposed in netobserv API, but can still be configured using the environment variable. Additionally, the pprof server now uses the common safeguards against resource exhaustion attacks *Breaking changes*: - config for pprof changed from env "PROFILE_PORT" to "PPROF_ADDR", now including the listening address.
|
Warning Review limit reached
More reviews will be available in 38 minutes and 50 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughIntroduces ChangesShared HTTP server defaults and PPROF_ADDR migration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 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)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 2 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@cmd/netobserv-ebpf-agent.go`:
- Line 16: The import statement at line 16 of cmd/netobserv-ebpf-agent.go is
importing from the external flowlogs-pipeline repository, but the local
pkg/server package should be used instead for consistency with how it is
imported in pkg/prometheus/prom_server.go. Replace the external import path
"github.com/netobserv/flowlogs-pipeline/pkg/server" with the local package
import path to ensure the codebase uses the local source consistently and avoids
divergence.
- Around line 49-53: The PProf listener is bypassing the MaxBytesHandler
body-size guard because the http.Server passed to server.Default() at line 52
has a nil Handler field. When Handler is nil, the MaxBytesHandler wrapping in
pkg/server/common.go:15 is skipped, and the DefaultServeMux (registered by the
net/http/pprof blank import) is used unprotected by ListenAndServe(). Explicitly
set the Handler field on the http.Server passed to server.Default() to
http.DefaultServeMux so that the MaxBytesHandler protection is applied to the
PProf endpoints.
In `@docs/profiling.md`:
- Around line 22-24: The fenced code block containing the kubectl port-forward
command is missing a language declaration, which violates the MD040 markdown
linting rule. Add "bash" after the opening triple backticks (```) on line 22 to
specify that the code block contains bash code, changing it from triple
backticks with no language to triple backticks followed by "bash".
In `@pkg/prometheus/prom_server.go`:
- Line 53: The explicit MinVersion set to tls.VersionTLS12 at line 41 prevents
server.Default from enforcing TLS 1.3 when called at line 53, as server.Default
only applies TLS 1.3 when MinVersion is unset. Either remove the explicit TLS
1.2 configuration entirely to allow server.Default to apply TLS 1.3, or upgrade
the MinVersion to tls.VersionTLS13 to match the intended security baseline used
elsewhere in the codebase.
In `@pkg/server/common.go`:
- Line 1: The Go file in pkg/server/common.go is missing the required Apache v2
license header at the beginning. Add the standard Apache v2 license header
comment block before the `package server` declaration to comply with the coding
guidelines for Go files.
- Around line 13-46: The `Default` function in pkg/server/common.go lacks unit
test coverage. Create a test file (pkg/server/common_test.go) using Go's
standard testing package with test cases that verify the `Default` function
correctly sets all default timeout values, MaxHeaderBytes, TLSConfig settings,
and disables HTTP/2. Include a test case that verifies the handler is wrapped
with http.MaxBytesHandler when srv.Handler is not nil, and another that confirms
the warning is logged when srv.Handler is nil. Ensure all conditional branches
in the `Default` function are covered by your tests.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d23fcb02-97a6-4dea-baef-b6da48cbd7a7
📒 Files selected for processing (6)
cmd/netobserv-ebpf-agent.godocs/config.mddocs/profiling.mdpkg/config/config.gopkg/prometheus/prom_server.gopkg/server/common.go
| ``` | ||
| oc -n netobserv-privileged port-forward <netobserv-ebpf-agent pod name> 6060 | ||
| kubectl -n netobserv-privileged port-forward <netobserv-ebpf-agent pod name> 6060 | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language to the fenced command block.
Line 22 should declare a language (e.g., bash) to satisfy MD040.
Suggested fix
- ```
+ ```bash
kubectl -n netobserv-privileged port-forward <netobserv-ebpf-agent pod name> 6060
```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ``` | |
| oc -n netobserv-privileged port-forward <netobserv-ebpf-agent pod name> 6060 | |
| kubectl -n netobserv-privileged port-forward <netobserv-ebpf-agent pod name> 6060 | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@docs/profiling.md` around lines 22 - 24, The fenced code block containing the
kubectl port-forward command is missing a language declaration, which violates
the MD040 markdown linting rule. Add "bash" after the opening triple backticks
(```) on line 22 to specify that the code block contains bash code, changing it
from triple backticks with no language to triple backticks followed by "bash".
Source: Linters/SAST tools
| } | ||
| httpServer.Handler = mux | ||
| httpServer = defaultServer(httpServer) | ||
| httpServer = server.Default(httpServer) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify TLS minimum versions on migrated server code paths.
rg -n --type=go 'MinVersion:\s*tls\.VersionTLS1[23]|server\.Default\(' \
pkg/prometheus/prom_server.go pkg/server/common.go cmd/netobserv-ebpf-agent.goRepository: netobserv/netobserv-ebpf-agent
Length of output: 402
🏁 Script executed:
# First, locate and examine the server.Default implementation
ast-grep outline pkg/server/common.go --view expandedRepository: netobserv/netobserv-ebpf-agent
Length of output: 287
🏁 Script executed:
# Read the server.Default function to understand its behavior
cat -n pkg/server/common.goRepository: netobserv/netobserv-ebpf-agent
Length of output: 1628
🏁 Script executed:
sed -n '35,60p' pkg/prometheus/prom_server.goRepository: netobserv/netobserv-ebpf-agent
Length of output: 939
🏁 Script executed:
# Check how server.Default is used elsewhere - cmd/netobserv-ebpf-agent.go from initial search
sed -n '50,60p' cmd/netobserv-ebpf-agent.goRepository: netobserv/netobserv-ebpf-agent
Length of output: 609
Remove explicit TLS 1.2 override or upgrade to TLS 1.3.
Line 41 sets MinVersion: tls.VersionTLS12, preventing server.Default at line 53 from enforcing TLS 1.3 (it only applies when MinVersion == 0). This weakens the intended security baseline—other servers in the codebase use server.Default without explicit TLS config and correctly get TLS 1.3. Either upgrade to TLS 1.3 or remove the override entirely.
Suggested fix
- TLSConfig: &tls.Config{
- MinVersion: tls.VersionTLS12,
- },
+ TLSConfig: &tls.Config{
+ MinVersion: tls.VersionTLS13,
+ },🤖 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 `@pkg/prometheus/prom_server.go` at line 53, The explicit MinVersion set to
tls.VersionTLS12 at line 41 prevents server.Default from enforcing TLS 1.3 when
called at line 53, as server.Default only applies TLS 1.3 when MinVersion is
unset. Either remove the explicit TLS 1.2 configuration entirely to allow
server.Default to apply TLS 1.3, or upgrade the MinVersion to tls.VersionTLS13
to match the intended security baseline used elsewhere in the codebase.
| @@ -0,0 +1,46 @@ | |||
| package server | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add Apache v2 license header to this new Go file.
Line 1 starts directly with package, so the required header is missing.
As per coding guidelines, **/*.go: Go code must use Apache v2 license headers.
🤖 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 `@pkg/server/common.go` at line 1, The Go file in pkg/server/common.go is
missing the required Apache v2 license header at the beginning. Add the standard
Apache v2 license header comment block before the `package server` declaration
to comply with the coding guidelines for Go files.
Source: Coding guidelines
| func Default(srv *http.Server) *http.Server { | ||
| // defaults taken from https://bruinsslot.jp/post/go-secure-webserver/ can be overriden by caller | ||
| if srv.Handler != nil { | ||
| // No more than 2MB body | ||
| srv.Handler = http.MaxBytesHandler(srv.Handler, 2<<20) | ||
| } else { | ||
| slog.Warnf("Handler not yet set on server while securing defaults. Make sure a MaxByte middleware is used.") | ||
| } | ||
| if srv.ReadTimeout == 0 { | ||
| srv.ReadTimeout = 10 * time.Second | ||
| } | ||
| if srv.ReadHeaderTimeout == 0 { | ||
| srv.ReadHeaderTimeout = 5 * time.Second | ||
| } | ||
| if srv.WriteTimeout == 0 { | ||
| srv.WriteTimeout = 10 * time.Second | ||
| } | ||
| if srv.IdleTimeout == 0 { | ||
| srv.IdleTimeout = 120 * time.Second | ||
| } | ||
| if srv.MaxHeaderBytes == 0 { | ||
| srv.MaxHeaderBytes = 1 << 20 // 1MB | ||
| } | ||
| if srv.TLSConfig == nil { | ||
| srv.TLSConfig = &tls.Config{} | ||
| } | ||
| if srv.TLSConfig.MinVersion == 0 { | ||
| srv.TLSConfig.MinVersion = tls.VersionTLS13 | ||
| } | ||
| // Disable http/2 | ||
| srv.TLSNextProto = make(map[string]func(*http.Server, *tls.Conn, http.Handler), 0) | ||
|
|
||
| return srv | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add unit tests for Default before merge.
This is new Go logic on a shared security path, but no test coverage is included in the provided PR files for defaults/middleware behavior.
As per coding guidelines, **/*.go: All new Go code must include unit test coverage using standard Go testing patterns.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 36-36: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures
(missing-ssl-minversion-go)
🤖 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 `@pkg/server/common.go` around lines 13 - 46, The `Default` function in
pkg/server/common.go lacks unit test coverage. Create a test file
(pkg/server/common_test.go) using Go's standard testing package with test cases
that verify the `Default` function correctly sets all default timeout values,
MaxHeaderBytes, TLSConfig settings, and disables HTTP/2. Include a test case
that verifies the handler is wrapped with http.MaxBytesHandler when srv.Handler
is not nil, and another that confirms the warning is logged when srv.Handler is
nil. Ensure all conditional branches in the `Default` function are covered by
your tests.
Source: Coding guidelines
|
@jotak: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
New changes are detected. LGTM label has been removed. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
2 participants
Description
When pprof was enabled, it was exposed on ":(port)" which stands for all net interfaces.
Settings are changed so the user controls what address to use. For example, they can configure it with "127.0.0.1:6060" to restrict to the local loop (more secured), or keep ":6060" or "0.0.0.0:6060" for a broader exposition. Documented those security aspects.
Note that this setting is not exposed in netobserv API, but can still be configured using the environment variable.
Additionally, the pprof server now uses the common safeguards against resource exhaustion attacks
Breaking changes:
Dependencies
Checklist
To run a perfscale test, comment with:
/test ebpf-node-density-heavy-25nodesSummary by CodeRabbit
Configuration Changes
PPROF_ADDR(string listen address) instead ofPROFILE_PORT; profiling is disabled whenPPROF_ADDRisn’t set.Security Improvements
Documentation
PPROF_ADDR, including revised example settings and Kubernetes port-forward guidance.