From 0f5541fe1fa7cec7769d095f4289debd943feb64 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Thu, 9 Jul 2026 09:10:59 +0200 Subject: [PATCH 01/16] Trigger secret won't use a default value --- jenkins/webhook-proxy/.gitignore | 1 + jenkins/webhook-proxy/main.go | 9 +-------- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/jenkins/webhook-proxy/.gitignore b/jenkins/webhook-proxy/.gitignore index e9257c24b..323618849 100644 --- a/jenkins/webhook-proxy/.gitignore +++ b/jenkins/webhook-proxy/.gitignore @@ -1,3 +1,4 @@ webhook-proxy_linux_amd64 webhook-proxy_darwin_amd64 webhook-proxy_windows_amd64.exe +local/ diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index c0dbcad09..c12a48bab 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -29,7 +29,6 @@ const ( pipelineConfigFilename = "pipeline.json.tmpl" repoBaseEnvVar = "REPO_BASE" triggerSecretEnvVar = "TRIGGER_SECRET" - triggerSecretDefault = "secret101" jenkinsfilePathDefault = "Jenkinsfile" protectedBranchesEnvVar = "PROTECTED_BRANCHES" protectedBranchesDefault = "master,develop,production,staging,release/" @@ -176,13 +175,7 @@ func main() { triggerSecret := os.Getenv(triggerSecretEnvVar) if len(triggerSecret) == 0 { - triggerSecret = triggerSecretDefault - log.Println( - "WARN:", - triggerSecretEnvVar, - "not set, using default value:", - triggerSecretDefault, - ) + log.Fatalln("Exiting due to missing trigger secret.") } openShiftAPIHost := os.Getenv(openShiftAPIHostEnvVar) From 1aa63073d5b2c9950d00fef8dab4af829ee3e4f9 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Thu, 9 Jul 2026 14:02:45 +0200 Subject: [PATCH 02/16] Add validation for cross-project pull requests in webhook handler --- jenkins/webhook-proxy/main.go | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index c12a48bab..8a7f78027 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -300,6 +300,9 @@ func (s *Server) HandleRoot() http.HandlerFunc { Repository repository `json:"repository"` DisplayID string `json:"displayId"` } `json:"fromRef"` + ToRef struct { + Repository repository `json:"repository"` + } `json:"toRef"` } `json:"pullRequest"` } @@ -448,6 +451,16 @@ func (s *Server) HandleRoot() http.HandlerFunc { return } } else if req.EventKey == "pr:opened" || req.EventKey == "pr:merged" || req.EventKey == "pr:declined" || req.EventKey == "pr:deleted" { + if req.PullRequest.FromRef.Repository.Project.Key != req.PullRequest.ToRef.Repository.Project.Key { + msg := fmt.Sprintf( + "Cross-project PR rejected: source project %q does not match target project %q", + req.PullRequest.FromRef.Repository.Project.Key, + req.PullRequest.ToRef.Repository.Project.Key, + ) + log.Println(requestID, msg) + http.Error(w, msg, http.StatusBadRequest) + return + } repo = req.PullRequest.FromRef.Repository.Slug if component == "" { component = extractComponent(repo, project) From d60bd7a2d7f28fa77cb12f409652e1dfd43fd483 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Thu, 9 Jul 2026 14:10:29 +0200 Subject: [PATCH 03/16] Add test for rejecting cross-project pull requests in webhook handler --- jenkins/webhook-proxy/main_test.go | 49 ++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index abb099163..6f9f1b312 100644 --- a/jenkins/webhook-proxy/main_test.go +++ b/jenkins/webhook-proxy/main_test.go @@ -338,6 +338,55 @@ func TestHandleRootReadsRequests(t *testing.T) { } } +func TestRejectsCrossProjectPR(t *testing.T) { + ts, _ := testServer() + defer ts.Close() + + tests := map[string]struct { + payloadFile string + expectedStatusCode int + wantCrossProject bool + }{ + "same-project PR merged is allowed": { + payloadFile: "pr-merged-payload.json", + expectedStatusCode: http.StatusOK, + wantCrossProject: false, + }, + "same-project PR declined is allowed": { + payloadFile: "pr-declined-payload.json", + expectedStatusCode: http.StatusOK, + wantCrossProject: false, + }, + "cross-project PR is rejected": { + payloadFile: "pr-cross-project-payload.json", + expectedStatusCode: http.StatusBadRequest, + wantCrossProject: true, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + f, err := os.Open("testdata/fixtures/" + tc.payloadFile) + if err != nil { + t.Fatal(err) + } + res, err := http.Post(ts.URL+"?trigger_secret=s3cr3t", "application/json", f) + if err != nil { + t.Fatal(err) + } + body, _ := io.ReadAll(res.Body) + res.Body.Close() + + if res.StatusCode != tc.expectedStatusCode { + t.Fatalf("Got status %d, want %d (body: %s)", res.StatusCode, tc.expectedStatusCode, body) + } + if tc.wantCrossProject && !strings.Contains(string(body), "Cross-project PR rejected") { + t.Fatalf("Expected cross-project rejection message in body, got: %s", body) + } + }) + } +} + func TestSkipsPayloads(t *testing.T) { // The expected events depend on the values in the payload files. tests := map[string]struct { From 20d006b841d755cc60474513216b6bf88491c670 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Thu, 9 Jul 2026 14:19:59 +0200 Subject: [PATCH 04/16] Update Dockerfile to use specific image digest for ubi-micro and adjust user permissions --- jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml | 4 +++- jenkins/webhook-proxy/Dockerfile | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml b/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml index ce2c83b93..823d9330a 100644 --- a/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml +++ b/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml @@ -123,7 +123,9 @@ objects: dnsPolicy: ClusterFirst restartPolicy: Always schedulerName: default-scheduler - securityContext: {} + securityContext: + runAsNonRoot: true + runAsUser: 1001 serviceAccount: '${JENKINS_SERVICE_NAME}' serviceAccountName: '${JENKINS_SERVICE_NAME}' terminationGracePeriodSeconds: 30 diff --git a/jenkins/webhook-proxy/Dockerfile b/jenkins/webhook-proxy/Dockerfile index 9fd0f65b9..f361f9686 100644 --- a/jenkins/webhook-proxy/Dockerfile +++ b/jenkins/webhook-proxy/Dockerfile @@ -14,7 +14,7 @@ WORKDIR /home/webhook-proxy RUN CGO_ENABLED=0 go build -o webhook-proxy # Final stage -FROM registry.access.redhat.com/ubi9/ubi-micro:latest +FROM registry.access.redhat.com/ubi9/ubi-micro@sha256:35de56a9413112f1474e392ebc35e0cf6f0fb484c8e8877bbae59b513694b41f # Set default ALLOWED_EXTERNAL_PROJECTS env var ARG allowedExternalProjects=opendevstack @@ -29,6 +29,8 @@ COPY pipeline.json.tmpl /home/webhook-proxy/pipeline.json.tmpl WORKDIR /home/webhook-proxy +USER 1001 + EXPOSE 8080 CMD ./webhook-proxy From 521785c458191388b6f3af4c30bd5556bf93cd3a Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Thu, 9 Jul 2026 14:37:26 +0200 Subject: [PATCH 05/16] Remove trigger secret from logs. --- jenkins/webhook-proxy/main.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index 8a7f78027..5105d02b8 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -116,7 +116,7 @@ type ocClient struct { HTTPClient *http.Client OpenShiftAPIBaseURL string Token string - OpenShiftAppDomain string + OpenShiftAppDomain string } // Server represents this service, and is a global. @@ -628,7 +628,7 @@ func (s *Server) HandleRoot() http.HandlerFunc { log.Println(requestID, "No remaining instances found") return } - if i == s.MaxDeletionChecks - 1 { + if i == s.MaxDeletionChecks-1 { log.Println(requestID, "Reached maximum iterations, stopping checks") } } @@ -661,11 +661,17 @@ func (c *ocClient) Forward(e *Event, triggerSecret string) (int, []byte, error) e.Pipeline, triggerSecret, ) + redactedURL := fmt.Sprintf( + "%s/namespaces/%s/buildconfigs/%s/webhooks/[REDACTED]/generic", + c.OpenShiftAPIBaseURL, + e.Namespace, + e.Pipeline, + ) c.CheckJenkinsAvailability(e) c.CheckDocGenAvailability(e) - log.Println(e.RequestID, "Forwarding to", url) + log.Println(e.RequestID, "Forwarding to", redactedURL) p := struct { Env []EnvPair `json:"env"` @@ -927,7 +933,7 @@ func newClient(openShiftAPIHost string, triggerSecret string, openShiftAppDomain HTTPClient: secureClient, OpenShiftAPIBaseURL: baseURL, Token: token, - OpenShiftAppDomain: openShiftAppDomain, + OpenShiftAppDomain: openShiftAppDomain, }, nil } From cfa20906590e385ec1ea2fa4a87defe4acc3e5a1 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Fri, 10 Jul 2026 09:33:50 +0200 Subject: [PATCH 06/16] Enhance validation and escaping in webhook handler and template processing - Introduced regex patterns for validating repository names and branch names. - Updated event validation to reject unsafe characters and ensure proper field lengths. - Refactored template parsing to safely encode user-controlled values, preventing JSON injection. - Added comprehensive tests for event validation and template rendering. --- jenkins/webhook-proxy/main.go | 43 +++- jenkins/webhook-proxy/main_test.go | 244 ++++++++++++++++++++++- jenkins/webhook-proxy/pipeline.json.tmpl | 6 +- 3 files changed, 286 insertions(+), 7 deletions(-) diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index 5105d02b8..3884bc0a4 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -21,6 +21,13 @@ import ( "time" ) +var ( + // safeNameRegex permits characters that are valid in repository / component names. + safeNameRegex = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) + // safeBranchRegex permits characters that are valid in git branch / ref names. + safeBranchRegex = regexp.MustCompile(`^[a-zA-Z0-9._/\-+@]+$`) +) + const ( namespaceFile = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" tokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" @@ -323,7 +330,7 @@ func (s *Server) HandleRoot() http.HandlerFunc { log.Println(requestID, "-----") init.Do(func() { - tmpl, err = template.ParseFiles(pipelineConfigFilename) + tmpl, err = parsePipelineTemplate(pipelineConfigFilename) }) if err != nil { log.Println(requestID, err.Error()) @@ -887,7 +894,7 @@ func (c *ocClient) do(req *http.Request) (*http.Response, error) { return c.HTTPClient.Do(req) } -// IsValid performs basic snaity checks for event values. +// IsValid performs basic sanity checks for event values. func (e *Event) IsValid() bool { // Only forward and delete are recognized right now. if e.Kind != "forward" && e.Kind != "delete" { @@ -897,7 +904,21 @@ func (e *Event) IsValid() bool { if len(e.Pipeline) < 3 { return false } - return len(e.Namespace) > 0 && len(e.Repo) > 0 && len(e.Component) > 0 && len(e.Branch) > 0 + if len(e.Namespace) == 0 || len(e.Repo) == 0 || len(e.Component) == 0 || len(e.Branch) == 0 { + return false + } + // Reject JSON metacharacters and other unsafe characters in fields that are + // interpolated into the BuildConfig template to prevent JSON injection. + if !safeNameRegex.MatchString(e.Repo) { + return false + } + if !safeNameRegex.MatchString(e.Component) { + return false + } + if !safeBranchRegex.MatchString(e.Branch) { + return false + } + return true } func (e *Event) String() string { @@ -946,6 +967,22 @@ func getBuildConfig(tmpl *template.Template, data BuildConfigData) (*bytes.Buffe return b, nil } +// parsePipelineTemplate parses the pipeline BuildConfig template and registers +// the jsonStr function used to safely encode user-controlled string values. +func parsePipelineTemplate(filename string) (*template.Template, error) { + return template.New(filename).Funcs(template.FuncMap{ + // jsonStr JSON-encodes s and strips the surrounding quotes so it can + // be safely interpolated inside an existing JSON string literal. + "jsonStr": func(s string) (string, error) { + b, err := json.Marshal(s) + if err != nil { + return "", err + } + return string(b[1 : len(b)-1]), nil + }, + }).ParseFiles(filename) +} + func getSecureClient() (*http.Client, error) { // Load CA cert caCert, err := os.ReadFile(caCert) diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index 6f9f1b312..9d3473de8 100644 --- a/jenkins/webhook-proxy/main_test.go +++ b/jenkins/webhook-proxy/main_test.go @@ -907,7 +907,7 @@ func TestNotFound(t *testing.T) { } func TestGetBuildConfig(t *testing.T) { - tmpl, err := template.ParseFiles(pipelineConfigFilename) + tmpl, err := parsePipelineTemplate(pipelineConfigFilename) if err != nil { t.Error(err) } @@ -937,6 +937,248 @@ func TestGetBuildConfig(t *testing.T) { } } +func TestIsValid(t *testing.T) { + tests := map[string]struct { + event Event + want bool + }{ + "valid forward event": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "my-repo", Component: "my-repo", Branch: "main", Pipeline: "my-repo-main"}, + want: true, + }, + "valid delete event": { + event: Event{Kind: "delete", Namespace: "bar-cd", Repo: "my-repo", Component: "my-repo", Branch: "feature/ABC-123", Pipeline: "my-repo-123"}, + want: true, + }, + "branch with dots and plus": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: "repo", Branch: "release/v1.2.3+hotfix", Pipeline: "repo-v123"}, + want: true, + }, + "unknown kind": { + event: Event{Kind: "unknown", Namespace: "bar-cd", Repo: "repo", Component: "repo", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + "pipeline too short": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: "repo", Branch: "main", Pipeline: "ab"}, + want: false, + }, + "empty namespace": { + event: Event{Kind: "forward", Namespace: "", Repo: "repo", Component: "repo", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + "empty repo": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "", Component: "repo", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + "empty component": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: "", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + "empty branch": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: "repo", Branch: "", Pipeline: "repo-main"}, + want: false, + }, + // JSON injection via Repo + "repo with double-quote": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: `evil"repo`, Component: "repo", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + "repo with backslash": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: `evil\repo`, Component: "repo", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + "repo with opening brace": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "evil{repo", Component: "repo", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + "repo with newline": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "evil\nrepo", Component: "repo", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + // JSON injection via Component + "component with double-quote": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: `evil"comp`, Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + "component with closing brace": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: "evil}comp", Branch: "main", Pipeline: "repo-main"}, + want: false, + }, + // JSON injection via Branch + "branch with double-quote": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: "repo", Branch: `main"injected`, Pipeline: "repo-main"}, + want: false, + }, + "branch with backslash": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: "repo", Branch: `main\ninjected`, Pipeline: "repo-main"}, + want: false, + }, + "branch with null byte": { + event: Event{Kind: "forward", Namespace: "bar-cd", Repo: "repo", Component: "repo", Branch: "main\x00injected", Pipeline: "repo-main"}, + want: false, + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + got := tc.event.IsValid() + if got != tc.want { + t.Fatalf("IsValid() = %v, want %v (event: %+v)", got, tc.want, tc.event) + } + }) + } +} + +func TestParsePipelineTemplateJsonStrEscaping(t *testing.T) { + tmpl, err := parsePipelineTemplate(pipelineConfigFilename) + if err != nil { + t.Fatal(err) + } + + // renderAndParse renders the template and returns the fully-parsed JSON map. + // It fails the test if rendering or JSON parsing fails, which catches cases + // where an unescaped injection string broke the JSON structure. + renderAndParse := func(t *testing.T, data BuildConfigData) map[string]interface{} { + t.Helper() + b, err := getBuildConfig(tmpl, data) + if err != nil { + t.Fatalf("getBuildConfig error: %v", err) + } + var out map[string]interface{} + if err := json.Unmarshal(b.Bytes(), &out); err != nil { + t.Fatalf("rendered template is not valid JSON: %v\noutput:\n%s", err, b.String()) + } + return out + } + + t.Run("double-quote in GitURI is escaped to valid JSON", func(t *testing.T) { + // An unescaped " in the URI breaks out of the JSON string and makes the + // document invalid — json.Unmarshal would fail inside renderAndParse. + data := BuildConfigData{ + Name: "repo-main", TriggerSecret: "s3cr3t", + GitURI: `https://domain.com/proj/repo"injected.git`, + Branch: "main", JenkinsfilePath: "Jenkinsfile", + Env: "[]", ResourceVersion: "0", + } + out := renderAndParse(t, data) + // Verify the URI value round-trips with the literal " character preserved. + uri := out["spec"].(map[string]interface{})["source"].(map[string]interface{})["git"].(map[string]interface{})["uri"].(string) + if !strings.Contains(uri, `"injected`) { + t.Fatalf("expected escaped quote preserved in URI value, got: %s", uri) + } + }) + + t.Run("double-quote in Branch cannot break out of JSON string", func(t *testing.T) { + // Without escaping, branch value `main","secret":"leaked` would inject a + // new JSON key. After escaping it becomes a single string value. + injectedBranch := `main","secret":"leaked` + data := BuildConfigData{ + Name: "repo-main", TriggerSecret: "s3cr3t", + GitURI: "https://domain.com/proj/repo.git", + Branch: injectedBranch, + JenkinsfilePath: "Jenkinsfile", + Env: "[]", ResourceVersion: "0", + } + out := renderAndParse(t, data) + // Injected key must not surface as a top-level or spec-level field. + if _, ok := out["secret"]; ok { + t.Fatal("injection succeeded: 'secret' key found at top level") + } + // The ref value must equal the full injection string as data. + ref := out["spec"].(map[string]interface{})["source"].(map[string]interface{})["git"].(map[string]interface{})["ref"].(string) + if ref != injectedBranch { + t.Fatalf("branch value not preserved: got %q, want %q", ref, injectedBranch) + } + }) + + t.Run("backslash in JenkinsfilePath is escaped to valid JSON", func(t *testing.T) { + // An unescaped backslash followed by " would produce an invalid JSON escape. + injectedPath := `Jenkinsfile\","injected":true,"x":"` + data := BuildConfigData{ + Name: "repo-main", TriggerSecret: "s3cr3t", + GitURI: "https://domain.com/proj/repo.git", + Branch: "main", + JenkinsfilePath: injectedPath, + Env: "[]", ResourceVersion: "0", + } + out := renderAndParse(t, data) + // Injected key must not appear. + strategy := out["spec"].(map[string]interface{})["strategy"].(map[string]interface{})["jenkinsPipelineStrategy"].(map[string]interface{}) + if _, ok := strategy["injected"]; ok { + t.Fatal("injection succeeded: 'injected' key found in jenkinsPipelineStrategy") + } + // The jenkinsfilePath value must equal the full injection string as data. + jfp := strategy["jenkinsfilePath"].(string) + if jfp != injectedPath { + t.Fatalf("jenkinsfilePath value not preserved: got %q, want %q", jfp, injectedPath) + } + }) +} + +func TestHandleRootRejectsInjectionPayloads(t *testing.T) { + ts, mc := testServer() + defer ts.Close() + + tests := map[string]struct { + slug string // repository.slug in the Bitbucket payload + displayID string // changes[0].ref.displayId + }{ + "double-quote in repo slug": { + slug: `repo"evil`, + displayID: "main", + }, + "JSON metachar in branch displayId": { + slug: "repository", + displayID: `main","kind":"delete`, + }, + "backslash in branch displayId": { + slug: "repository", + displayID: `main\ninjected`, + }, + "null byte in branch displayId": { + slug: "repository", + displayID: "main\x00injected", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + payload := map[string]interface{}{ + "eventKey": "repo:refs_changed", + "repository": map[string]interface{}{ + "slug": tc.slug, + "project": map[string]interface{}{ + "key": "BAR", + }, + }, + "changes": []map[string]interface{}{ + { + "type": "UPDATE", + "ref": map[string]interface{}{ + "displayId": tc.displayID, + "type": "BRANCH", + }, + }, + }, + } + body, err := json.Marshal(payload) + if err != nil { + t.Fatal(err) + } + res, err := http.Post(ts.URL+"?trigger_secret=s3cr3t", "application/json", bytes.NewReader(body)) + if err != nil { + t.Fatal(err) + } + io.ReadAll(res.Body) + res.Body.Close() + + if res.StatusCode != http.StatusBadRequest { + t.Fatalf("Got status %d, want %d; mock event: %v", res.StatusCode, http.StatusBadRequest, mc.Event) + } + }) + } +} + func TestExtractComponent(t *testing.T) { tests := map[string]struct { repository string diff --git a/jenkins/webhook-proxy/pipeline.json.tmpl b/jenkins/webhook-proxy/pipeline.json.tmpl index e1097b581..d4ace2516 100644 --- a/jenkins/webhook-proxy/pipeline.json.tmpl +++ b/jenkins/webhook-proxy/pipeline.json.tmpl @@ -20,8 +20,8 @@ "source": { "type": "Git", "git": { - "uri": "{{.GitURI}}", - "ref": "{{.Branch}}" + "uri": "{{.GitURI | jsonStr}}", + "ref": "{{.Branch | jsonStr}}" }, "sourceSecret": { "name": "cd-user-with-password" @@ -30,7 +30,7 @@ "strategy": { "type": "JenkinsPipeline", "jenkinsPipelineStrategy": { - "jenkinsfilePath": "{{.JenkinsfilePath}}", + "jenkinsfilePath": "{{.JenkinsfilePath | jsonStr}}", "env": {{.Env}} } }, From 58d957933796220aa6472429b44eee5e15c7e0c1 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Fri, 10 Jul 2026 12:27:25 +0200 Subject: [PATCH 07/16] Add validation for jenkinsfile_path parameter in webhook handler --- jenkins/webhook-proxy/main.go | 10 ++++++ jenkins/webhook-proxy/main_test.go | 52 ++++++++++++++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index 3884bc0a4..ac0841c87 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -26,6 +26,11 @@ var ( safeNameRegex = regexp.MustCompile(`^[a-zA-Z0-9._-]+$`) // safeBranchRegex permits characters that are valid in git branch / ref names. safeBranchRegex = regexp.MustCompile(`^[a-zA-Z0-9._/\-+@]+$`) + // safeJenkinsfilePathRegex permits only relative path segments composed of + // safe characters. Each segment must begin with an alphanumeric character, + // which implicitly rejects absolute paths ("/..."), path traversal ("../"), + // and hidden-file tricks ("."). + safeJenkinsfilePathRegex = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*(?:/[a-zA-Z0-9][a-zA-Z0-9._-]*)*$`) ) const ( @@ -349,6 +354,11 @@ func (s *Server) HandleRoot() http.HandlerFunc { jenkinsfilePath := jenkinsfilePathDefault jenkinsfilePathParam := queryValues.Get("jenkinsfile_path") if jenkinsfilePathParam != "" { + if !safeJenkinsfilePathRegex.MatchString(jenkinsfilePathParam) { + log.Println(requestID, "jenkinsfile_path param rejected:", jenkinsfilePathParam) + http.Error(w, "Invalid jenkinsfile_path", http.StatusBadRequest) + return + } jenkinsfilePath = jenkinsfilePathParam } diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index 9d3473de8..7267d2588 100644 --- a/jenkins/webhook-proxy/main_test.go +++ b/jenkins/webhook-proxy/main_test.go @@ -1179,6 +1179,58 @@ func TestHandleRootRejectsInjectionPayloads(t *testing.T) { } } +func TestJenkinsfilePathValidation(t *testing.T) { + ts, _ := testServer() + defer ts.Close() + + validPayload := func() io.Reader { + b, _ := os.ReadFile("testdata/fixtures/repo-refs-changed-payload.json") + return bytes.NewReader(b) + } + + tests := map[string]struct { + path string + wantStatus int + }{ + // Valid paths + "default (no param)": {"", http.StatusOK}, + "simple filename": {"Jenkinsfile", http.StatusOK}, + "one directory deep": {"ci/Jenkinsfile", http.StatusOK}, + "multiple segments": {"a/b/c/Jenkinsfile", http.StatusOK}, + "filename with dots and dash": {"ci/My-Jenkinsfile.groovy", http.StatusOK}, + // Invalid – path traversal + "double-dot traversal": {"../evil/Jenkinsfile", http.StatusBadRequest}, + "traversal in middle": {"ci/../../../etc/passwd", http.StatusBadRequest}, + // Invalid – absolute path + "absolute path": {"/etc/passwd", http.StatusBadRequest}, + // Invalid – hidden file / dot-start segment + "hidden file": {".hidden/Jenkinsfile", http.StatusBadRequest}, + // Invalid – JSON metacharacters + "double-quote in path": {`ci/"injected`, http.StatusBadRequest}, + "backslash in path": {`ci\Jenkinsfile`, http.StatusBadRequest}, + // Invalid – space + "space in path": {"ci/My Jenkinsfile", http.StatusBadRequest}, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + url := ts.URL + "/?trigger_secret=s3cr3t" + if tc.path != "" { + url += "&jenkinsfile_path=" + tc.path + } + res, err := http.Post(url, "application/json", validPayload()) + if err != nil { + t.Fatal(err) + } + io.ReadAll(res.Body) + res.Body.Close() + if res.StatusCode != tc.wantStatus { + t.Fatalf("jenkinsfile_path=%q: got status %d, want %d", tc.path, res.StatusCode, tc.wantStatus) + } + }) + } +} + func TestExtractComponent(t *testing.T) { tests := map[string]struct { repository string From ceecf0208f53cf9b4d63cb76a72c2457cb52665e Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Fri, 10 Jul 2026 13:39:50 +0200 Subject: [PATCH 08/16] Validate target project in webhook handler and update test payloads for project key consistency --- jenkins/webhook-proxy/main.go | 8 ++ .../fixtures/pr-cross-project-payload.json | 106 ++++++++++++++++++ .../fixtures/pr-declined-payload.json | 4 +- .../testdata/fixtures/pr-merged-payload.json | 4 +- 4 files changed, 118 insertions(+), 4 deletions(-) create mode 100644 jenkins/webhook-proxy/testdata/fixtures/pr-cross-project-payload.json diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index ac0841c87..56eb1c747 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -468,6 +468,14 @@ func (s *Server) HandleRoot() http.HandlerFunc { return } } else if req.EventKey == "pr:opened" || req.EventKey == "pr:merged" || req.EventKey == "pr:declined" || req.EventKey == "pr:deleted" { + // Validate the target (toRef) project against the server's allowed + // projects. This is the authoritative check; the root-level + // repository field is absent in PR payloads so it cannot be used. + _, prProjectErr := s.readProjectParam(req.PullRequest.ToRef.Repository.Project.Key, requestID) + if prProjectErr != nil { + http.Error(w, prProjectErr.Error(), http.StatusBadRequest) + return + } if req.PullRequest.FromRef.Repository.Project.Key != req.PullRequest.ToRef.Repository.Project.Key { msg := fmt.Sprintf( "Cross-project PR rejected: source project %q does not match target project %q", diff --git a/jenkins/webhook-proxy/testdata/fixtures/pr-cross-project-payload.json b/jenkins/webhook-proxy/testdata/fixtures/pr-cross-project-payload.json new file mode 100644 index 000000000..cbbe198c4 --- /dev/null +++ b/jenkins/webhook-proxy/testdata/fixtures/pr-cross-project-payload.json @@ -0,0 +1,106 @@ +{ + "eventKey": "pr:merged", + "date": "2017-09-19T10:39:36+1000", + "actor": { + "name": "user", + "emailAddress": "user@example.com", + "id": 2, + "displayName": "User", + "active": true, + "slug": "user", + "type": "NORMAL" + }, + "pullRequest": { + "id": 9, + "version": 2, + "title": "file edited online with Bitbucket", + "state": "MERGED", + "open": false, + "closed": true, + "createdDate": 1505781560908, + "updatedDate": 1505781576361, + "closedDate": 1505781576361, + "fromRef": { + "id": "refs/heads/admin/file-1505781548644", + "displayId": "admin/file-1505781548644", + "latestCommit": "45f9690c928915a5e1c4366d5ee1985eea03f05d", + "repository": { + "slug": "repository", + "id": 84, + "name": "repository", + "scmId": "git", + "state": "AVAILABLE", + "statusMessage": "Available", + "forkable": true, + "project": { + "key": "ATTACKER", + "id": 84, + "name": "project", + "public": false, + "type": "NORMAL" + }, + "public": false + } + }, + "toRef": { + "id": "refs/heads/master", + "displayId": "master", + "latestCommit": "8d2ad38c918fa6943859fca2176c89ea98b92a21", + "repository": { + "slug": "repository", + "id": 84, + "name": "repository", + "scmId": "git", + "state": "AVAILABLE", + "statusMessage": "Available", + "forkable": true, + "project": { + "key": "BAR", + "id": 84, + "name": "project", + "public": false, + "type": "NORMAL" + }, + "public": false + } + }, + "locked": false, + "author": { + "user": { + "name": "admin", + "emailAddress": "admin@example.com", + "id": 1, + "displayName": "Administrator", + "active": true, + "slug": "admin", + "type": "NORMAL" + }, + "role": "AUTHOR", + "approved": false, + "status": "UNAPPROVED" + }, + "reviewers": [], + "participants": [ + { + "user": { + "name": "user", + "emailAddress": "user@example.com", + "id": 2, + "displayName": "User", + "active": true, + "slug": "user", + "type": "NORMAL" + }, + "role": "PARTICIPANT", + "approved": false, + "status": "UNAPPROVED" + } + ], + "properties": { + "mergeCommit": { + "displayId": "7e48f426f0a", + "id": "7e48f426f0a6e47c5b5e862c31be6ca965f82c9c" + } + } + } +} diff --git a/jenkins/webhook-proxy/testdata/fixtures/pr-declined-payload.json b/jenkins/webhook-proxy/testdata/fixtures/pr-declined-payload.json index b15f717c3..1c9bb4ad0 100644 --- a/jenkins/webhook-proxy/testdata/fixtures/pr-declined-payload.json +++ b/jenkins/webhook-proxy/testdata/fixtures/pr-declined-payload.json @@ -33,7 +33,7 @@ "statusMessage": "Available", "forkable": true, "project": { - "key": "PROJ", + "key": "BAR", "id": 84, "name": "project", "public": false, @@ -55,7 +55,7 @@ "statusMessage": "Available", "forkable": true, "project": { - "key": "PROJ", + "key": "BAR", "id": 84, "name": "project", "public": false, diff --git a/jenkins/webhook-proxy/testdata/fixtures/pr-merged-payload.json b/jenkins/webhook-proxy/testdata/fixtures/pr-merged-payload.json index 4b78a7f44..49496e6a6 100644 --- a/jenkins/webhook-proxy/testdata/fixtures/pr-merged-payload.json +++ b/jenkins/webhook-proxy/testdata/fixtures/pr-merged-payload.json @@ -33,7 +33,7 @@ "statusMessage": "Available", "forkable": true, "project": { - "key": "PROJ", + "key": "BAR", "id": 84, "name": "project", "public": false, @@ -55,7 +55,7 @@ "statusMessage": "Available", "forkable": true, "project": { - "key": "PROJ", + "key": "BAR", "id": 84, "name": "project", "public": false, From 40347d9cb119ca2649a42784be9ae42dd2555d33 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Fri, 10 Jul 2026 14:25:51 +0200 Subject: [PATCH 09/16] Fix as the cluster assign an user and provides conflicts --- jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml | 1 - 1 file changed, 1 deletion(-) diff --git a/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml b/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml index 823d9330a..c6aa638f2 100644 --- a/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml +++ b/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml @@ -125,7 +125,6 @@ objects: schedulerName: default-scheduler securityContext: runAsNonRoot: true - runAsUser: 1001 serviceAccount: '${JENKINS_SERVICE_NAME}' serviceAccountName: '${JENKINS_SERVICE_NAME}' terminationGracePeriodSeconds: 30 From ebd40e9e9e9fdcff1e837aad040e3c6189fb0596 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Mon, 13 Jul 2026 08:54:16 +0200 Subject: [PATCH 10/16] Add redirect handling to secure HTTP client to prevent token leakage --- jenkins/webhook-proxy/main.go | 10 ++++- jenkins/webhook-proxy/main_test.go | 61 ++++++++++++++++++++++++++++++ 2 files changed, 70 insertions(+), 1 deletion(-) diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index 56eb1c747..3be586365 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -1021,7 +1021,15 @@ func getSecureClient() (*http.Client, error) { // left as nil. https://go.dev/pkg/crypto/tls/ // tlsConfig.BuildNameToCertificate() transport := &http.Transport{TLSClientConfig: tlsConfig} - return &http.Client{Transport: transport, Timeout: 10 * time.Second}, nil + return &http.Client{ + Transport: transport, + Timeout: 10 * time.Second, + // Never follow redirects: the SA Bearer token must not be forwarded + // to a redirected host (SSRF / token-leakage mitigation). + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + }, nil } func getFileContent(filename string) (string, error) { diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index 7267d2588..1fcdbfae1 100644 --- a/jenkins/webhook-proxy/main_test.go +++ b/jenkins/webhook-proxy/main_test.go @@ -1263,3 +1263,64 @@ func TestExtractComponent(t *testing.T) { }) } } + +// TestOcClientDoesNotFollowRedirects verifies the SSRF / SA-token-leakage +// mitigation: the ocClient must never follow HTTP redirects so that the +// Authorization: Bearer header is not forwarded to an attacker-controlled host. +// +// The test wires up two servers: +// - redirectSrv returns a 302 pointing at capturesSrv +// - captureSrv records whether it received an Authorization header +// +// If the client followed the redirect, captureSrv would receive the Bearer +// token. With CheckRedirect returning http.ErrUseLastResponse the client +// stops at the 302 and captureSrv is never reached. +func TestOcClientDoesNotFollowRedirects(t *testing.T) { + bearerToken := "super-secret-sa-token" + + // captureSrv records any Authorization header it receives. + capturedAuth := "" + captureSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + capturedAuth = r.Header.Get("Authorization") + w.WriteHeader(http.StatusOK) + })) + defer captureSrv.Close() + + // redirectSrv issues a 302 redirect to captureSrv. + redirectSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Redirect(w, r, captureSrv.URL, http.StatusFound) + })) + defer redirectSrv.Close() + + // Build an ocClient with the same CheckRedirect policy as getSecureClient. + c := &ocClient{ + HTTPClient: &http.Client{ + Timeout: 10 * time.Second, + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + }, + OpenShiftAPIBaseURL: redirectSrv.URL, + Token: bearerToken, + } + + req, err := http.NewRequest("GET", redirectSrv.URL+"/test", nil) + if err != nil { + t.Fatal(err) + } + res, err := c.do(req) + if err != nil { + t.Fatalf("do() returned unexpected error: %v", err) + } + defer res.Body.Close() + + // The client must stop at the redirect response, not follow it. + if res.StatusCode != http.StatusFound { + t.Fatalf("expected status %d (redirect not followed), got %d", http.StatusFound, res.StatusCode) + } + + // captureSrv must never have been called, so the Bearer token was not leaked. + if capturedAuth != "" { + t.Fatalf("SA Bearer token was leaked to redirect target: Authorization=%q", capturedAuth) + } +} From f6e8c18f186cbc46fae4d01dcdf7b1dcb6328b4b Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Mon, 13 Jul 2026 09:10:11 +0200 Subject: [PATCH 11/16] Enhance branch protection checks to be case-insensitive in webhook handler --- jenkins/webhook-proxy/main.go | 5 +++-- jenkins/webhook-proxy/main_test.go | 22 ++++++++++++++++++++++ 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index 3be586365..e90e46b69 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -1091,14 +1091,15 @@ func makePipelineName(project string, component string, branch string) string { } func isProtectedBranch(protectedBranches []string, branch string) bool { + lowerBranch := strings.ToLower(branch) for _, b := range protectedBranches { if b == "*" { return true } - if strings.HasSuffix(b, "/") && strings.HasPrefix(branch, b) { + if strings.HasSuffix(b, "/") && strings.HasPrefix(lowerBranch, b) { return true } - if b == branch { + if b == lowerBranch { return true } } diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index 1fcdbfae1..72a263fda 100644 --- a/jenkins/webhook-proxy/main_test.go +++ b/jenkins/webhook-proxy/main_test.go @@ -12,6 +12,7 @@ import ( "strings" "testing" "text/template" + "time" ) // SETUP @@ -162,6 +163,27 @@ func TestIsProtectedBranch(t *testing.T) { "feature/v2", false, }, + // Case-insensitive protection: "Master" must match configured "master" (F-11) + { + []string{"master"}, + "Master", + true, + }, + { + []string{"master"}, + "MASTER", + true, + }, + { + []string{"develop"}, + "DEVELOP", + true, + }, + { + []string{"master", "release/"}, + "Release/v1", + true, + }, } for _, tt := range tests { From a9d1501461ec39fd0276c1c6e6199a1d2a18f1a1 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Mon, 13 Jul 2026 09:15:01 +0200 Subject: [PATCH 12/16] Use constant-time comparison for trigger_secret in webhook handler to enhance security --- jenkins/webhook-proxy/main.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index e90e46b69..c8477fec8 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -3,6 +3,7 @@ package main import ( "bytes" "crypto/sha1" + "crypto/subtle" "crypto/tls" "crypto/x509" "encoding/json" @@ -345,7 +346,7 @@ func (s *Server) HandleRoot() http.HandlerFunc { queryValues := r.URL.Query() triggerSecretParam := queryValues.Get("trigger_secret") - if triggerSecretParam != s.TriggerSecret { + if subtle.ConstantTimeCompare([]byte(triggerSecretParam), []byte(s.TriggerSecret)) != 1 { log.Println(requestID, "trigger_secret param not given / not matching") http.Error(w, "Not authorized", http.StatusUnauthorized) return From 9890feba3a5bae49897d1e1d4e58c175a1b2e3d9 Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Mon, 13 Jul 2026 09:21:50 +0200 Subject: [PATCH 13/16] Edit Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index bc68941f8..26b8b103d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,7 @@ ## Unreleased ### Added +- Webhook proxy security enhancements ([#1398](https://github.com/opendevstack/ods-core/pull/1398/)) ### Changed From f5caae535cf2bff39906dd145da2af6800ccffad Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Mon, 13 Jul 2026 09:24:49 +0200 Subject: [PATCH 14/16] Fix linting problems in test file. --- jenkins/webhook-proxy/main_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index 72a263fda..6ef7ea2bc 100644 --- a/jenkins/webhook-proxy/main_test.go +++ b/jenkins/webhook-proxy/main_test.go @@ -1191,7 +1191,7 @@ func TestHandleRootRejectsInjectionPayloads(t *testing.T) { if err != nil { t.Fatal(err) } - io.ReadAll(res.Body) + _, _ = io.ReadAll(res.Body) res.Body.Close() if res.StatusCode != http.StatusBadRequest { @@ -1244,7 +1244,7 @@ func TestJenkinsfilePathValidation(t *testing.T) { if err != nil { t.Fatal(err) } - io.ReadAll(res.Body) + _, _ = io.ReadAll(res.Body) res.Body.Close() if res.StatusCode != tc.wantStatus { t.Fatalf("jenkinsfile_path=%q: got status %d, want %d", tc.path, res.StatusCode, tc.wantStatus) From 0ab87a7353e45407af1433dd89b90cd01e64343d Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Mon, 13 Jul 2026 13:55:41 +0200 Subject: [PATCH 15/16] Add IP range validation for webhook requests --- .../deploy/jenkins-webhook-proxy.yml | 9 ++ jenkins/webhook-proxy/main.go | 92 ++++++++++++++ jenkins/webhook-proxy/main_test.go | 114 ++++++++++++++++++ 3 files changed, 215 insertions(+) diff --git a/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml b/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml index c6aa638f2..34e529e97 100644 --- a/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml +++ b/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml @@ -23,6 +23,13 @@ parameters: - name: MAX_DELETION_CHECKS value: '10' required: true +- name: ALLOWED_WEBHOOK_IP_RANGES + description: >- + Comma-separated list of allowed source IP addresses or CIDR ranges for + incoming webhook requests (e.g. "185.166.140.0/22,10.0.0.1"). + Leave empty to allow all source IPs. + value: '' + required: true - name: WEBHOOK_PROXY_CPU_REQUEST value: 25m - name: WEBHOOK_PROXY_CPU_LIMIT @@ -103,6 +110,8 @@ objects: value: ${OPENSHIFT_APPS_BASEDOMAIN} - name: MAX_DELETION_CHECKS value: ${MAX_DELETION_CHECKS} + - name: ALLOWED_WEBHOOK_IP_RANGES + value: ${ALLOWED_WEBHOOK_IP_RANGES} - name: TRIGGER_SECRET valueFrom: secretKeyRef: diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index c8477fec8..c17f9f87c 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -12,6 +12,7 @@ import ( "io" "log" "math/rand" + "net" "net/http" "os" "regexp" @@ -57,6 +58,7 @@ const ( maxDeletionChecksDefault = "10" allowedChangeRefTypesEnvVar = "ALLOWED_CHANGE_REF_TYPES" allowedChangeRefTypesDefault = "BRANCH" + allowedWebhookIPRangesEnvVar = "ALLOWED_WEBHOOK_IP_RANGES" namespaceSuffix = "-cd" letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" ) @@ -142,6 +144,7 @@ type Server struct { AcceptedEvents []string AllowedExternalProjects []string AllowedChangeRefTypes []string + AllowedWebhookIPRanges []*net.IPNet RepoBase string MaxDeletionChecks int } @@ -253,6 +256,16 @@ func main() { maxDeletionChecks = envMaxDeletionChecks } + envAllowedWebhookIPRanges := os.Getenv(allowedWebhookIPRangesEnvVar) + if len(envAllowedWebhookIPRanges) == 0 { + log.Fatalln(allowedWebhookIPRangesEnvVar, "is required but not set") + } + allowedWebhookIPRanges, parseErr := parseIPRanges(envAllowedWebhookIPRanges) + if parseErr != nil { + log.Fatalln("Invalid", allowedWebhookIPRangesEnvVar, ":", parseErr) + } + log.Println("INFO:", allowedWebhookIPRangesEnvVar, "set to", envAllowedWebhookIPRanges) + client, err := newClient(openShiftAPIHost, triggerSecret, openShiftAppDomain) if err != nil { log.Fatalln(err) @@ -279,6 +292,7 @@ func main() { AcceptedEvents: acceptedEvents, AllowedExternalProjects: allowedExternalProjects, AllowedChangeRefTypes: allowedChangeRefTypes, + AllowedWebhookIPRanges: allowedWebhookIPRanges, RepoBase: repoBase, MaxDeletionChecks: maxDeletionChecksInt, } @@ -335,6 +349,13 @@ func (s *Server) HandleRoot() http.HandlerFunc { requestID := randStringBytes(6) log.Println(requestID, "-----") + // TODO(debug): remove before merging + log.Println(requestID, "DEBUG remote-addr="+r.RemoteAddr, + "X-Forwarded-For="+r.Header.Get("X-Forwarded-For"), + "X-Real-IP="+r.Header.Get("X-Real-IP"), + "resolved-ip="+fmt.Sprintf("%s", requestIP(r)), + ) + init.Do(func() { tmpl, err = parsePipelineTemplate(pipelineConfigFilename) }) @@ -344,6 +365,15 @@ func (s *Server) HandleRoot() http.HandlerFunc { return } + if len(s.AllowedWebhookIPRanges) > 0 { + ip := requestIP(r) + if ip == nil || !isIPAllowed(s.AllowedWebhookIPRanges, ip) { + log.Println(requestID, "request from disallowed IP:", ip, "(remote-addr="+r.RemoteAddr+")") + http.Error(w, "Forbidden", http.StatusForbidden) + return + } + } + queryValues := r.URL.Query() triggerSecretParam := queryValues.Get("trigger_secret") if subtle.ConstantTimeCompare([]byte(triggerSecretParam), []byte(s.TriggerSecret)) != 1 { @@ -1107,6 +1137,68 @@ func isProtectedBranch(protectedBranches []string, branch string) bool { return false } +// parseIPRanges parses a comma-separated list of CIDRs and bare IPs into +// []*net.IPNet. Bare IPs are automatically expanded to /32 (IPv4) or /128 +// (IPv6) host routes. +func parseIPRanges(raw string) ([]*net.IPNet, error) { + var networks []*net.IPNet + for _, entry := range strings.Split(raw, ",") { + entry = strings.TrimSpace(entry) + if entry == "" { + continue + } + if !strings.Contains(entry, "/") { + ip := net.ParseIP(entry) + if ip == nil { + return nil, fmt.Errorf("invalid IP address: %q", entry) + } + if ip.To4() != nil { + entry = entry + "/32" + } else { + entry = entry + "/128" + } + } + _, network, err := net.ParseCIDR(entry) + if err != nil { + return nil, fmt.Errorf("invalid CIDR %q: %s", entry, err) + } + networks = append(networks, network) + } + return networks, nil +} + +// requestIP extracts the client IP from the request. It trusts the +// X-Forwarded-For header (first entry, set by the OpenShift router/ingress) +// when present, then X-Real-IP, and finally falls back to RemoteAddr. +func requestIP(r *http.Request) net.IP { + if xff := r.Header.Get("X-Forwarded-For"); xff != "" { + parts := strings.SplitN(xff, ",", 2) + if ip := net.ParseIP(strings.TrimSpace(parts[0])); ip != nil { + return ip + } + } + if xri := r.Header.Get("X-Real-IP"); xri != "" { + if ip := net.ParseIP(strings.TrimSpace(xri)); ip != nil { + return ip + } + } + host, _, err := net.SplitHostPort(r.RemoteAddr) + if err != nil { + return net.ParseIP(r.RemoteAddr) + } + return net.ParseIP(host) +} + +// isIPAllowed reports whether ip is contained in any of the allowed networks. +func isIPAllowed(allowedRanges []*net.IPNet, ip net.IP) bool { + for _, network := range allowedRanges { + if network.Contains(ip) { + return true + } + } + return false +} + // extractComponent returns the component part of the given repository. // The component is equal to the repository without any project prefix. func extractComponent(repository, project string) string { diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index 6ef7ea2bc..62a8e14d0 100644 --- a/jenkins/webhook-proxy/main_test.go +++ b/jenkins/webhook-proxy/main_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "io" "log" + "net" "net/http" "net/http/httptest" "os" @@ -1346,3 +1347,116 @@ func TestOcClientDoesNotFollowRedirects(t *testing.T) { t.Fatalf("SA Bearer token was leaked to redirect target: Authorization=%q", capturedAuth) } } + +func TestParseIPRanges(t *testing.T) { + tests := []struct { + name string + input string + wantLen int + wantErr bool + }{ + {"single IPv4 CIDR", "192.168.1.0/24", 1, false}, + {"bare IPv4 address", "10.0.0.1", 1, false}, + {"bare IPv6 address", "::1", 1, false}, + {"multiple entries", "10.0.0.0/8,172.16.0.0/12,192.168.0.0/16", 3, false}, + {"entries with spaces", " 10.0.0.1 , 10.0.0.2 ", 2, false}, + {"empty string", "", 0, false}, + {"invalid CIDR", "not-an-ip", 0, true}, + {"invalid prefix length", "10.0.0.0/33", 0, true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := parseIPRanges(tc.input) + if tc.wantErr && err == nil { + t.Fatal("expected error, got nil") + } + if !tc.wantErr && err != nil { + t.Fatalf("unexpected error: %s", err) + } + if len(got) != tc.wantLen { + t.Fatalf("got %d networks, want %d", len(got), tc.wantLen) + } + }) + } +} + +func TestIsIPAllowed(t *testing.T) { + ranges, err := parseIPRanges("192.168.1.0/24,10.0.0.1") + if err != nil { + t.Fatal(err) + } + tests := []struct { + ip string + want bool + }{ + {"192.168.1.100", true}, + {"192.168.1.1", true}, + {"10.0.0.1", true}, + {"10.0.0.2", false}, + {"172.16.0.1", false}, + {"8.8.8.8", false}, + } + for _, tc := range tests { + ip := net.ParseIP(tc.ip) + if got := isIPAllowed(ranges, ip); got != tc.want { + t.Errorf("isIPAllowed(%s) = %v, want %v", tc.ip, got, tc.want) + } + } +} + +func TestAllowedWebhookIPRanges(t *testing.T) { + mc := &mockClient{} + ranges, _ := parseIPRanges("192.168.1.0/24") + server := &Server{ + Client: mc, + Namespace: "bar-cd", + Project: "bar", + TriggerSecret: "s3cr3t", + AcceptedEvents: []string{"repo:refs_changed"}, + AllowedChangeRefTypes: []string{"BRANCH"}, + AllowedWebhookIPRanges: ranges, + RepoBase: "https://domain.com", + MaxDeletionChecks: 1, + } + ts := httptest.NewServer(server.HandleRoot()) + defer ts.Close() + + body := `{"eventKey":"repo:refs_changed","repository":{"slug":"my-repo","project":{"key":"bar"}},"changes":[{"type":"UPDATE","ref":{"displayId":"main","type":"BRANCH"}}]}` + + // httptest server binds to 127.0.0.1 and r.RemoteAddr will be 127.0.0.1:PORT, + // which is NOT in 192.168.1.0/24 → expect 403. + res, err := http.Post(ts.URL+"?trigger_secret=s3cr3t", "application/json", strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + _, _ = io.ReadAll(res.Body) + res.Body.Close() + if res.StatusCode != http.StatusForbidden { + t.Fatalf("expected 403 Forbidden from disallowed IP, got %d", res.StatusCode) + } + + // With no IP restriction configured → same request must not be 403. + server2 := &Server{ + Client: mc, + Namespace: "bar-cd", + Project: "bar", + TriggerSecret: "s3cr3t", + AcceptedEvents: []string{"repo:refs_changed"}, + AllowedChangeRefTypes: []string{"BRANCH"}, + AllowedWebhookIPRanges: nil, + RepoBase: "https://domain.com", + MaxDeletionChecks: 1, + } + ts2 := httptest.NewServer(server2.HandleRoot()) + defer ts2.Close() + + res2, err := http.Post(ts2.URL+"?trigger_secret=s3cr3t", "application/json", strings.NewReader(body)) + if err != nil { + t.Fatal(err) + } + _, _ = io.ReadAll(res2.Body) + res2.Body.Close() + if res2.StatusCode == http.StatusForbidden { + t.Fatalf("expected non-403 when no IP restriction configured, got %d", res2.StatusCode) + } +} From 0fefb19dd535bb98c22ebcfc103744e6646fe3ac Mon Sep 17 00:00:00 2001 From: Jorge Romero Date: Mon, 20 Jul 2026 14:45:12 +0200 Subject: [PATCH 16/16] Enhance security --- jenkins/webhook-proxy/main.go | 45 +++++++++++++++--------------- jenkins/webhook-proxy/main_test.go | 20 +++++++++++-- 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index c17f9f87c..7f2aa4a1d 100644 --- a/jenkins/webhook-proxy/main.go +++ b/jenkins/webhook-proxy/main.go @@ -33,6 +33,9 @@ var ( // which implicitly rejects absolute paths ("/..."), path traversal ("../"), // and hidden-file tricks ("."). safeJenkinsfilePathRegex = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*(?:/[a-zA-Z0-9][a-zA-Z0-9._-]*)*$`) + // safeTriggerSecretRegex matches a UUID as produced by Java's + // UUID.randomUUID().toString(): 8-4-4-4-12 lowercase hex digits. + safeTriggerSecretRegex = regexp.MustCompile(`^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$`) ) const ( @@ -119,7 +122,7 @@ type buildConfig struct { // Client makes requests, e.g. to create and delete pipelines, or to forward // event payloads. type Client interface { - Forward(e *Event, triggerSecret string) (int, []byte, error) + Forward(e *Event) (int, []byte, error) GetPipeline(e *Event) (bool, []byte, error) CreateOrUpdatePipeline(exists bool, tmpl *template.Template, e *Event, data BuildConfigData) (int, error) DeletePipeline(e *Event) error @@ -132,6 +135,7 @@ type ocClient struct { OpenShiftAPIBaseURL string Token string OpenShiftAppDomain string + TriggerSecret string } // Server represents this service, and is a global. @@ -190,8 +194,8 @@ func main() { } triggerSecret := os.Getenv(triggerSecretEnvVar) - if len(triggerSecret) == 0 { - log.Fatalln("Exiting due to missing trigger secret.") + if !safeTriggerSecretRegex.MatchString(triggerSecret) { + log.Fatalln("Trigger secret must be a valid UUID (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx).") } openShiftAPIHost := os.Getenv(openShiftAPIHostEnvVar) @@ -264,6 +268,10 @@ func main() { if parseErr != nil { log.Fatalln("Invalid", allowedWebhookIPRangesEnvVar, ":", parseErr) } + if len(allowedWebhookIPRanges) == 0 { + log.Fatalln("No valid IP ranges found in", allowedWebhookIPRangesEnvVar) + } + log.Println("INFO:", allowedWebhookIPRangesEnvVar, "set to", envAllowedWebhookIPRanges) client, err := newClient(openShiftAPIHost, triggerSecret, openShiftAppDomain) @@ -349,13 +357,6 @@ func (s *Server) HandleRoot() http.HandlerFunc { requestID := randStringBytes(6) log.Println(requestID, "-----") - // TODO(debug): remove before merging - log.Println(requestID, "DEBUG remote-addr="+r.RemoteAddr, - "X-Forwarded-For="+r.Header.Get("X-Forwarded-For"), - "X-Real-IP="+r.Header.Get("X-Real-IP"), - "resolved-ip="+fmt.Sprintf("%s", requestIP(r)), - ) - init.Do(func() { tmpl, err = parsePipelineTemplate(pipelineConfigFilename) }) @@ -365,13 +366,11 @@ func (s *Server) HandleRoot() http.HandlerFunc { return } - if len(s.AllowedWebhookIPRanges) > 0 { - ip := requestIP(r) - if ip == nil || !isIPAllowed(s.AllowedWebhookIPRanges, ip) { - log.Println(requestID, "request from disallowed IP:", ip, "(remote-addr="+r.RemoteAddr+")") - http.Error(w, "Forbidden", http.StatusForbidden) - return - } + ip := requestIP(r) + if ip == nil || !isIPAllowed(s.AllowedWebhookIPRanges, ip) { + log.Println(requestID, "request from disallowed IP:", ip, "(remote-addr="+r.RemoteAddr+")") + http.Error(w, "Forbidden", http.StatusForbidden) + return } queryValues := r.URL.Query() @@ -638,7 +637,7 @@ func (s *Server) HandleRoot() http.HandlerFunc { return } } - forwardStatusCode, forwardBody, forwardErr := s.Client.Forward(event, s.TriggerSecret) + forwardStatusCode, forwardBody, forwardErr := s.Client.Forward(event) if forwardErr != nil { log.Println(requestID, forwardErr) http.Error(w, http.StatusText(http.StatusInternalServerError), http.StatusInternalServerError) @@ -709,13 +708,13 @@ func (s *Server) readProjectParam(projectParam string, requestID string) (string } // Forward forwards a webhook event payload to the correct pipeline. -func (c *ocClient) Forward(e *Event, triggerSecret string) (int, []byte, error) { +func (c *ocClient) Forward(e *Event) (int, []byte, error) { url := fmt.Sprintf( "%s/namespaces/%s/buildconfigs/%s/webhooks/%s/generic", c.OpenShiftAPIBaseURL, e.Namespace, e.Pipeline, - triggerSecret, + c.TriggerSecret, ) redactedURL := fmt.Sprintf( "%s/namespaces/%s/buildconfigs/%s/webhooks/[REDACTED]/generic", @@ -1004,6 +1003,7 @@ func newClient(openShiftAPIHost string, triggerSecret string, openShiftAppDomain OpenShiftAPIBaseURL: baseURL, Token: token, OpenShiftAppDomain: openShiftAppDomain, + TriggerSecret: triggerSecret, }, nil } @@ -1122,15 +1122,14 @@ func makePipelineName(project string, component string, branch string) string { } func isProtectedBranch(protectedBranches []string, branch string) bool { - lowerBranch := strings.ToLower(branch) for _, b := range protectedBranches { if b == "*" { return true } - if strings.HasSuffix(b, "/") && strings.HasPrefix(lowerBranch, b) { + if strings.HasSuffix(b, "/") && strings.HasPrefix(strings.ToLower(branch), strings.ToLower(b)) { return true } - if b == lowerBranch { + if strings.EqualFold(b, branch) { return true } } diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index 62a8e14d0..ed87c3add 100644 --- a/jenkins/webhook-proxy/main_test.go +++ b/jenkins/webhook-proxy/main_test.go @@ -22,6 +22,14 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } +func localhostIPRanges() []*net.IPNet { + ranges, err := parseIPRanges("127.0.0.0/8,::1/128") + if err != nil { + panic(err) + } + return ranges +} + func TestMakePipelineName(t *testing.T) { tests := map[string]struct { project string @@ -205,7 +213,7 @@ type mockClient struct { Event *Event } -func (c *mockClient) Forward(e *Event, triggerSecret string) (int, []byte, error) { +func (c *mockClient) Forward(e *Event) (int, []byte, error) { c.Event = e return 200, nil, nil } @@ -241,6 +249,7 @@ func testServer() (*httptest.Server, *mockClient) { AllowedChangeRefTypes: []string{"BRANCH"}, RepoBase: "https://domain.com", MaxDeletionChecks: 10, + AllowedWebhookIPRanges: localhostIPRanges(), } return httptest.NewServer(server.HandleRoot()), mc } @@ -448,6 +457,7 @@ func TestSkipsPayloads(t *testing.T) { AllowedChangeRefTypes: []string{"BRANCH"}, RepoBase: "https://domain.com", MaxDeletionChecks: 10, + AllowedWebhookIPRanges: localhostIPRanges(), } ts := httptest.NewServer(server.HandleRoot()) defer ts.Close() @@ -559,6 +569,7 @@ func TestNamespaceRestriction(t *testing.T) { AllowedChangeRefTypes: []string{"BRANCH"}, RepoBase: "https://domain.com", MaxDeletionChecks: 10, + AllowedWebhookIPRanges: localhostIPRanges(), } ts := httptest.NewServer(s.HandleRoot()) defer ts.Close() @@ -683,10 +694,11 @@ func TestForward(t *testing.T) { HTTPClient: &http.Client{}, OpenShiftAPIBaseURL: apiStub.URL, Token: "foo", + TriggerSecret: "s3cr3t", } // Ensure the response from OpenShift is forwarded as-is to the client - actualOpenshiftStatusCode, actualOpenshiftResponse, err := c.Forward(tc.event, "s3cr3t") + actualOpenshiftStatusCode, actualOpenshiftResponse, err := c.Forward(tc.event) if err != nil { t.Fatal(err) } @@ -871,6 +883,7 @@ func TestBuildEndpoint(t *testing.T) { AllowedChangeRefTypes: []string{"BRANCH"}, RepoBase: "https://domain.com", MaxDeletionChecks: 10, + AllowedWebhookIPRanges: localhostIPRanges(), } server := httptest.NewServer(s.HandleRoot()) @@ -916,6 +929,7 @@ func TestNotFound(t *testing.T) { AllowedChangeRefTypes: []string{"BRANCH"}, RepoBase: "https://domain.com", MaxDeletionChecks: 10, + AllowedWebhookIPRanges: localhostIPRanges(), } server := httptest.NewServer(s.HandleRoot()) @@ -1443,7 +1457,7 @@ func TestAllowedWebhookIPRanges(t *testing.T) { TriggerSecret: "s3cr3t", AcceptedEvents: []string{"repo:refs_changed"}, AllowedChangeRefTypes: []string{"BRANCH"}, - AllowedWebhookIPRanges: nil, + AllowedWebhookIPRanges: localhostIPRanges(), RepoBase: "https://domain.com", MaxDeletionChecks: 1, }