OCPBUGS-90080: Validate chart URL in /api/helm/verify to prevent SSRF#16786
OCPBUGS-90080: Validate chart URL in /api/helm/verify to prevent SSRF#16786fsgreco wants to merge 1 commit into
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
openshift-ci-robot
added
jira/severity-important
|
@fsgreco: This pull request references Jira Issue OCPBUGS-90080, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThe chart URL validator is exported and reused by chart retrieval and installation. Chart verification now rejects invalid chart URLs with HTTP 400 before downstream processing, with tests covering valid payloads and invalid URL cases. ChangesChart URL validation
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant HandleChartVerifier
participant IsValidChartURL
participant chartVerifier
Client->>HandleChartVerifier: submit chart_url
HandleChartVerifier->>IsValidChartURL: validate chart URL
IsValidChartURL-->>HandleChartVerifier: validation result
HandleChartVerifier->>chartVerifier: verify valid chart URL
chartVerifier-->>Client: verification response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ 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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fsgreco 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 |
![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)
pkg/helm/handlers/handler_chartVerifier_test.go (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify the HTTP method for the test request.
httptest.NewRequestdefaults to"GET"when the method is passed as"". Since chart verification is typically aPOSToperation (as correctly implemented in your newTestHelmHandlers_HandleChartVerifier_RejectsInvalidURLstest), consider explicitly passing"POST"here for accuracy.♻️ Proposed fix
- request := httptest.NewRequest("", "/foo", strings.NewReader(tt.body)) + request := httptest.NewRequest("POST", "/foo", strings.NewReader(tt.body))🤖 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/helm/handlers/handler_chartVerifier_test.go` at line 58, Update the httptest.NewRequest call in the chart verification test to explicitly use the POST method instead of an empty method string, matching the operation being tested and the existing TestHelmHandlers_HandleChartVerifier_RejectsInvalidURLs behavior.
🤖 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 `@pkg/helm/actions/install_chart.go`:
- Around line 61-64: Anchor ociURLRe in IsValidChartURL to require a complete
valid URL and prevent trailing-character or `@-authority` bypasses. In the chart
verifier handler, add strict net/url parsing and net.ParseIP checks that reject
loopback, private, and unspecified addresses without changing install_chart’s
existing localhost allowance. Update
pkg/helm/handlers/handler_chartVerifier_test.go:80 to append .tgz to the
internal-IP test payload.
---
Nitpick comments:
In `@pkg/helm/handlers/handler_chartVerifier_test.go`:
- Line 58: Update the httptest.NewRequest call in the chart verification test to
explicitly use the POST method instead of an empty method string, matching the
operation being tested and the existing
TestHelmHandlers_HandleChartVerifier_RejectsInvalidURLs behavior.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cdd2ea91-847e-480e-ace3-83b1214071b7
📒 Files selected for processing (5)
pkg/helm/actions/get_chart.gopkg/helm/actions/install_chart.gopkg/helm/actions/install_chart_test.gopkg/helm/handlers/handlerChartVerifier.gopkg/helm/handlers/handler_chartVerifier_test.go
a905d80 to
d6016da
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg/helm/handlers/handlerChartVerifier.go`:
- Around line 52-68: Harden SSRF validation in isPrivateURL and the downstream
chart fetch: validate every address returned by DNS, include CGNAT
(100.64.0.0/10) and other restricted ranges in the blocklist, and configure the
HTTP client used by h.chartVerifier with a net.Dialer.Control hook that rejects
restricted destination IPs at connection time, ensuring requests cannot rely on
the previously validated hostname resolution.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 031f1497-6220-4f00-93dd-1019422add81
📒 Files selected for processing (5)
pkg/helm/actions/get_chart.gopkg/helm/actions/install_chart.gopkg/helm/actions/install_chart_test.gopkg/helm/handlers/handlerChartVerifier.gopkg/helm/handlers/handler_chartVerifier_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/helm/actions/get_chart.go
- pkg/helm/actions/install_chart.go
- pkg/helm/actions/install_chart_test.go
d6016da to
bbfb522
Compare
| } | ||
| return false | ||
| } | ||
| return ip.IsLoopback() || ip.IsPrivate() || ip.IsUnspecified() || ip.IsLinkLocalUnicast() |
There was a problem hiding this comment.
There is a possibility that customers would be hosting their own registries. So we may get private, loopback or localhost IPs.
There was a problem hiding this comment.
Thanks for the feedback. I had some doubts about the IP check since the format validation (IsValidChartURL) already blocks the reported attack. I'll remove isPrivateURL and keep just the format check
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/jira refresh |
|
@fsgreco: This pull request references Jira Issue OCPBUGS-90080, which is invalid:
Comment DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@fsgreco: This pull request references Jira Issue OCPBUGS-90080, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
The `/api/helm/verify` endpoint accepted arbitrary chart_url values without validation, allowing authenticated users to make the backend issue requests to internal network addresses. I reused the existing IsValidChartURL check (already enforced on install and get-chart paths) to reject URLs that are not oci:// or http(s)://*.tgz before the backend attempts any connection. Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
bbfb522 to
2df62f3
Compare
|
@fsgreco: This pull request references Jira Issue OCPBUGS-90080, which is valid. 3 validation(s) were run on this bug
DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/helm/handlers/handler_chartVerifier_test.go (2)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
"POST"instead of an empty method string.
httptest.NewRequestdefaults to"GET"when the method parameter is an empty string. Since this handler processes a JSON body (which is typical of aPOSTrequest) and your new test block explicitly uses"POST", consider specifying it here for consistency.♻️ Proposed refactor
- request := httptest.NewRequest("", "/foo", strings.NewReader(tt.body)) + request := httptest.NewRequest("POST", "/foo", strings.NewReader(tt.body))🤖 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/helm/handlers/handler_chartVerifier_test.go` at line 58, Update the httptest.NewRequest call in the handler test to pass "POST" instead of an empty method string, keeping the existing URL and request body unchanged and consistent with the test's explicit POST request.
75-85: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winTest against format validation bypasses.
Consider adding test cases where the
.tgzextension is placed in a query parameter or URL fragment. This ensures that the format validation securely prevents attackers from bypassing the check to hit arbitrary non-tgz endpoints (like internal metadata or REST APIs).♻️ Proposed test additions
{"rejects empty chart_url", `{"chart_url":""}`}, {"rejects ftp scheme", `{"chart_url":"ftp://example.com/chart.tgz"}`}, {"rejects file scheme", `{"chart_url":"file:///etc/passwd"}`}, + {"rejects tgz in query param", `{"chart_url":"http://127.0.0.1/metadata?file=.tgz"}`}, + {"rejects tgz in fragment", `{"chart_url":"http://127.0.0.1/metadata#.tgz"}`}, }🤖 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/helm/handlers/handler_chartVerifier_test.go` around lines 75 - 85, Add cases to TestHelmHandlers_HandleChartVerifier_RejectsInvalidURLs covering URLs with “.tgz” only in the query string and only in the fragment, while the actual path is a non-tgz endpoint. Assert both are rejected, preserving coverage against format-validation bypasses.
🤖 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 `@pkg/helm/handlers/handlerChartVerifier.go`:
- Around line 58-61: Update actions.IsValidChartURL, used by the chart
verification handler, to parse the URL and validate the parsed u.Path for the
required .tgz suffix rather than applying httpURLRe to the raw URL. Ensure query
strings and fragments such as ?ignore=.tgz or #.tgz cannot satisfy the suffix
check while preserving valid oci:// and http(s):// chart URLs.
---
Nitpick comments:
In `@pkg/helm/handlers/handler_chartVerifier_test.go`:
- Line 58: Update the httptest.NewRequest call in the handler test to pass
"POST" instead of an empty method string, keeping the existing URL and request
body unchanged and consistent with the test's explicit POST request.
- Around line 75-85: Add cases to
TestHelmHandlers_HandleChartVerifier_RejectsInvalidURLs covering URLs with
“.tgz” only in the query string and only in the fragment, while the actual path
is a non-tgz endpoint. Assert both are rejected, preserving coverage against
format-validation bypasses.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 545e5121-d987-4594-b91c-636b1c8e7a47
📒 Files selected for processing (5)
pkg/helm/actions/get_chart.gopkg/helm/actions/install_chart.gopkg/helm/actions/install_chart_test.gopkg/helm/handlers/handlerChartVerifier.gopkg/helm/handlers/handler_chartVerifier_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/helm/actions/get_chart.go
- pkg/helm/actions/install_chart_test.go
- pkg/helm/actions/install_chart.go
|
@fsgreco: all tests passed! 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. |
3 participants
Analysis / Root cause:
The
/api/helm/verifyendpoint accepts achart_urlparameter and passes it directly to the chart-verifier library without URL validation. It allows authenticated users to force the console backend to issue requests to arbitrary internal network addresses (SSRF). The install and get-chart paths already validate URLs viaIsValidChartURL, but the verify handler was missing this check.Solution description:
I exported the existing
isValidChartURLfunction and call it inHandleChartVerifierbefore invoking the chart verifier. Invalid URLs now return 400 instead of being forwarded.Screenshots / screen recording:
N/A — backend-only change, no UI impact.
Test setup:
Run the console bridge locally with
--user-auth=disabledagainst any cluster.Test cases:
curl -X POST http://localhost:9000/api/helm/verify -H 'Content-Type: application/json' -H 'X-CSRFToken: test' -b 'csrf-token=test' -d '{"chart_url": "http://172.28.1.76:8849/nacos"}'→ 400invalid chart URLcurl -X POST http://localhost:9000/api/helm/verify -H 'Content-Type: application/json' -H 'X-CSRFToken: test' -b 'csrf-token=test' -d '{"chart_url": "https://charts.helm.sh/stable/argo-cd-2.3.5.tgz"}'→ passes validation (502 chart not found, but not 400)Browser conformance:
N/A — backend-only change.
Additional info:
Jira: https://redhat.atlassian.net/browse/OCPBUGS-90080
Summary by CodeRabbit
chart_urlvalidation and returns clear HTTP 400 errors for invalid inputs (including disallowed URL schemes and other unsupported URL formats).chart_urlvalidation is now applied consistently across chart loading and installation workflows.chart_urlrequests are rejected with HTTP 400.