From f74aea398984668fba31740a382bb7470fe933ca Mon Sep 17 00:00:00 2001 From: Santiago Greco Date: Wed, 2 Sep 2026 13:46:51 +0000 Subject: [PATCH] [release-4.21] OCPBUGS-116250: Validate chart URL in /api/helm/verify Reject invalid chart URLs before chart verification to prevent authenticated SSRF. Jira: https://redhat.atlassian.net/browse/OCPBUGS-116250 Assisted-by Chai Bot --- pkg/helm/handlers/handlerChartVerifier.go | 19 +++ .../handlers/handler_chartVerifier_test.go | 108 +++++++++++++++++- 2 files changed, 124 insertions(+), 3 deletions(-) diff --git a/pkg/helm/handlers/handlerChartVerifier.go b/pkg/helm/handlers/handlerChartVerifier.go index fa1fc397c82..bb3791a12cc 100644 --- a/pkg/helm/handlers/handlerChartVerifier.go +++ b/pkg/helm/handlers/handlerChartVerifier.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/http" + "regexp" "github.com/openshift/console/pkg/auth" "github.com/openshift/console/pkg/helm/actions" @@ -15,6 +16,13 @@ import ( "k8s.io/client-go/rest" ) +var ( + dnsLabel = `[a-zA-Z0-9](?:[a-zA-Z0-9-]*[a-zA-Z0-9])?` + hostPort = dnsLabel + `(?:\.` + dnsLabel + `)*\.?(?::\d+)?` + ociURLRe = regexp.MustCompile(`(?i)^oci://` + hostPort) + httpURLRe = regexp.MustCompile(`(?i)^https?://` + hostPort + `/.+\.(?:tar\.gz|tgz)$`) +) + // helmHandlers provides handlers to handle helm related requests type verifierHandlers struct { ApiServerHost string @@ -47,6 +55,13 @@ func (h *verifierHandlers) restConfig(bearerToken string) *rest.Config { Transport: h.Transport, } } + +// isValidChartURL validates chart URLs using RFC-compliant hostname labels. +// Accepts oci:/// and http(s):///.tgz|tar.gz URLs. +func isValidChartURL(raw string) bool { + return ociURLRe.MatchString(raw) || httpURLRe.MatchString(raw) +} + func (h *verifierHandlers) HandleChartVerifier(user *auth.User, w http.ResponseWriter, r *http.Request) { var req HelmVerifierRequest @@ -55,6 +70,10 @@ func (h *verifierHandlers) HandleChartVerifier(user *auth.User, w http.ResponseW serverutils.SendResponse(w, http.StatusBadRequest, serverutils.ApiError{Err: fmt.Sprintf("Failed to parse request: %v", err)}) return } + if !isValidChartURL(req.ChartUrl) { + serverutils.SendResponse(w, http.StatusBadRequest, serverutils.ApiError{Err: "invalid chart URL: must be oci:// or http(s)://*.tgz"}) + return + } conf := h.getActionConfigurations(h.ApiServerHost, "default", user.Token, &h.Transport) resp, err := h.chartVerifier(req.ChartUrl, req.Values, conf) if err != nil { diff --git a/pkg/helm/handlers/handler_chartVerifier_test.go b/pkg/helm/handlers/handler_chartVerifier_test.go index 7eed97f3786..53ba61d18d9 100644 --- a/pkg/helm/handlers/handler_chartVerifier_test.go +++ b/pkg/helm/handlers/handler_chartVerifier_test.go @@ -13,6 +13,8 @@ import ( var fakeReportSummary = `{"passed":"0","failed":"0","messages":null}` +const validChartURL = "https://example.com/charts/mychart-1.0.0.tgz" + func fakeVerifierHandler() verifierHandlers { return verifierHandlers{ getActionConfigurations: getFakeActionConfigurations, @@ -21,12 +23,15 @@ func fakeVerifierHandler() verifierHandlers { func fakeChartVerification(reportSummary string, err error) func(chartUrl string, values map[string]interface{}, conf *action.Configuration) (string, error) { return func(chartUrl string, values map[string]interface{}, conf *action.Configuration) (r string, er error) { - return r, err + return reportSummary, err } } func TestHelmHandlers_HandleChartVerifier(t *testing.T) { + validBody := `{"chart_url":"` + validChartURL + `"}` + tests := []struct { name string + body string expectedResponse string ReportSummary string error @@ -34,15 +39,17 @@ func TestHelmHandlers_HandleChartVerifier(t *testing.T) { }{ { name: "Error occurred", + body: validBody, expectedResponse: `{"error":"Failed to verify chart: Chart path is invalid"}`, error: errors.New("Chart path is invalid"), httpStatusCode: http.StatusBadGateway, }, { name: "Successful chart verification", + body: validBody, ReportSummary: fakeReportSummary, httpStatusCode: http.StatusOK, - expectedResponse: ``, + expectedResponse: fakeReportSummary, }, } for _, tt := range tests { @@ -50,7 +57,7 @@ func TestHelmHandlers_HandleChartVerifier(t *testing.T) { handlers := fakeVerifierHandler() handlers.chartVerifier = fakeChartVerification(tt.ReportSummary, tt.error) - request := httptest.NewRequest("", "/foo", strings.NewReader("{}")) + request := httptest.NewRequest("", "/foo", strings.NewReader(tt.body)) response := httptest.NewRecorder() handlers.HandleChartVerifier(&auth.User{}, response, request) @@ -66,3 +73,98 @@ func TestHelmHandlers_HandleChartVerifier(t *testing.T) { }) } } + +func TestHelmHandlers_HandleChartVerifier_AcceptsValidURLs(t *testing.T) { + tests := []struct { + name string + url string + }{ + {"valid OCI registry", "oci://ghcr.io/charts/mychart:1.0.0"}, + {"valid OCI registry with port", "oci://registry.example.com:5000/charts/mychart"}, + {"valid HTTPS tgz", validChartURL}, + {"valid HTTP tgz", "http://example.com/charts/mychart-1.0.0.tgz"}, + {"valid HTTPS tar.gz", "https://example.com/charts/mychart-1.0.0.tar.gz"}, + {"valid HTTP IPv4 tgz", "http://172.28.1.76:8849/chart.tgz"}, + {"valid HTTP localhost tgz", "http://localhost/chart.tgz"}, + {"valid HTTP loopback tgz", "http://127.0.0.1/chart.tgz"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + handlers := fakeVerifierHandler() + actionConfigCalled := false + verifierCalled := false + handlers.getActionConfigurations = func(string, string, string, *http.RoundTripper) *action.Configuration { + actionConfigCalled = true + return &action.Configuration{} + } + handlers.chartVerifier = func(chartURL string, values map[string]interface{}, conf *action.Configuration) (string, error) { + verifierCalled = true + return fakeReportSummary, nil + } + + request := httptest.NewRequest(http.MethodPost, "/api/helm/verify", strings.NewReader(`{"chart_url":"`+tt.url+`"}`)) + response := httptest.NewRecorder() + + handlers.HandleChartVerifier(&auth.User{}, response, request) + + if response.Code != http.StatusOK { + t.Errorf("expected status 200 but got %v", response.Code) + } + if !actionConfigCalled { + t.Error("expected action configuration to be created") + } + if !verifierCalled { + t.Error("expected chart verifier to be called") + } + }) + } +} + +func TestHelmHandlers_HandleChartVerifier_RejectsInvalidURLs(t *testing.T) { + tests := []struct { + name string + body string + }{ + {"rejects internal IP without tgz", `{"chart_url":"http://172.28.1.76:8849/nacos"}`}, + {"rejects non-tgz HTTP URL", `{"chart_url":"http://example.com/charts/mychart"}`}, + {"rejects empty chart URL", `{"chart_url":""}`}, + {"rejects hostless OCI URL", `{"chart_url":"oci:///chart"}`}, + {"rejects ftp scheme", `{"chart_url":"ftp://example.com/chart.tgz"}`}, + {"rejects file scheme", `{"chart_url":"file:///etc/passwd"}`}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + handlers := fakeVerifierHandler() + actionConfigCalled := false + verifierCalled := false + handlers.getActionConfigurations = func(string, string, string, *http.RoundTripper) *action.Configuration { + actionConfigCalled = true + return &action.Configuration{} + } + handlers.chartVerifier = func(chartURL string, values map[string]interface{}, conf *action.Configuration) (string, error) { + verifierCalled = true + return fakeReportSummary, nil + } + + request := httptest.NewRequest(http.MethodPost, "/api/helm/verify", strings.NewReader(tt.body)) + response := httptest.NewRecorder() + + handlers.HandleChartVerifier(&auth.User{}, response, request) + + if response.Code != http.StatusBadRequest { + t.Errorf("expected status 400 but got %v", response.Code) + } + if response.Body.String() != `{"error":"invalid chart URL: must be oci:// or http(s)://*.tgz"}` { + t.Errorf("unexpected response body: %s", response.Body.String()) + } + if actionConfigCalled { + t.Error("did not expect action configuration to be created") + } + if verifierCalled { + t.Error("did not expect chart verifier to be called") + } + }) + } +}