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 diff --git a/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml b/jenkins/ocp-config/deploy/jenkins-webhook-proxy.yml index ce2c83b93..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: @@ -123,7 +132,8 @@ objects: dnsPolicy: ClusterFirst restartPolicy: Always schedulerName: default-scheduler - securityContext: {} + securityContext: + runAsNonRoot: true serviceAccount: '${JENKINS_SERVICE_NAME}' serviceAccountName: '${JENKINS_SERVICE_NAME}' terminationGracePeriodSeconds: 30 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/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 diff --git a/jenkins/webhook-proxy/main.go b/jenkins/webhook-proxy/main.go index c0dbcad09..7f2aa4a1d 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" @@ -11,6 +12,7 @@ import ( "io" "log" "math/rand" + "net" "net/http" "os" "regexp" @@ -21,6 +23,21 @@ 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._/\-+@]+$`) + // 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._-]*)*$`) + // 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 ( namespaceFile = "/var/run/secrets/kubernetes.io/serviceaccount/namespace" tokenFile = "/var/run/secrets/kubernetes.io/serviceaccount/token" @@ -29,7 +46,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/" @@ -45,6 +61,7 @@ const ( maxDeletionChecksDefault = "10" allowedChangeRefTypesEnvVar = "ALLOWED_CHANGE_REF_TYPES" allowedChangeRefTypesDefault = "BRANCH" + allowedWebhookIPRangesEnvVar = "ALLOWED_WEBHOOK_IP_RANGES" namespaceSuffix = "-cd" letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" ) @@ -105,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 @@ -117,7 +134,8 @@ type ocClient struct { HTTPClient *http.Client OpenShiftAPIBaseURL string Token string - OpenShiftAppDomain string + OpenShiftAppDomain string + TriggerSecret string } // Server represents this service, and is a global. @@ -130,6 +148,7 @@ type Server struct { AcceptedEvents []string AllowedExternalProjects []string AllowedChangeRefTypes []string + AllowedWebhookIPRanges []*net.IPNet RepoBase string MaxDeletionChecks int } @@ -175,14 +194,8 @@ func main() { } triggerSecret := os.Getenv(triggerSecretEnvVar) - if len(triggerSecret) == 0 { - triggerSecret = triggerSecretDefault - log.Println( - "WARN:", - triggerSecretEnvVar, - "not set, using default value:", - triggerSecretDefault, - ) + if !safeTriggerSecretRegex.MatchString(triggerSecret) { + log.Fatalln("Trigger secret must be a valid UUID (xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx).") } openShiftAPIHost := os.Getenv(openShiftAPIHostEnvVar) @@ -247,6 +260,20 @@ 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) + } + 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) if err != nil { log.Fatalln(err) @@ -273,6 +300,7 @@ func main() { AcceptedEvents: acceptedEvents, AllowedExternalProjects: allowedExternalProjects, AllowedChangeRefTypes: allowedChangeRefTypes, + AllowedWebhookIPRanges: allowedWebhookIPRanges, RepoBase: repoBase, MaxDeletionChecks: maxDeletionChecksInt, } @@ -307,6 +335,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"` } @@ -327,7 +358,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()) @@ -335,9 +366,16 @@ func (s *Server) HandleRoot() http.HandlerFunc { 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() 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 @@ -346,6 +384,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 } @@ -455,6 +498,24 @@ 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", + 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) @@ -576,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) @@ -622,7 +683,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") } } @@ -647,19 +708,25 @@ 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", + 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"` @@ -875,7 +942,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" { @@ -885,7 +952,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 { @@ -921,7 +1002,8 @@ func newClient(openShiftAPIHost string, triggerSecret string, openShiftAppDomain HTTPClient: secureClient, OpenShiftAPIBaseURL: baseURL, Token: token, - OpenShiftAppDomain: openShiftAppDomain, + OpenShiftAppDomain: openShiftAppDomain, + TriggerSecret: triggerSecret, }, nil } @@ -934,6 +1016,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) @@ -954,7 +1052,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) { @@ -1020,10 +1126,72 @@ func isProtectedBranch(protectedBranches []string, branch string) bool { if b == "*" { return true } - if strings.HasSuffix(b, "/") && strings.HasPrefix(branch, b) { + if strings.HasSuffix(b, "/") && strings.HasPrefix(strings.ToLower(branch), strings.ToLower(b)) { return true } - if b == branch { + if strings.EqualFold(b, branch) { + return true + } + } + 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 } } diff --git a/jenkins/webhook-proxy/main_test.go b/jenkins/webhook-proxy/main_test.go index abb099163..ed87c3add 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" @@ -12,6 +13,7 @@ import ( "strings" "testing" "text/template" + "time" ) // SETUP @@ -20,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 @@ -162,6 +172,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 { @@ -182,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 } @@ -218,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 } @@ -338,6 +370,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 { @@ -376,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() @@ -487,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() @@ -611,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) } @@ -799,6 +883,7 @@ func TestBuildEndpoint(t *testing.T) { AllowedChangeRefTypes: []string{"BRANCH"}, RepoBase: "https://domain.com", MaxDeletionChecks: 10, + AllowedWebhookIPRanges: localhostIPRanges(), } server := httptest.NewServer(s.HandleRoot()) @@ -844,6 +929,7 @@ func TestNotFound(t *testing.T) { AllowedChangeRefTypes: []string{"BRANCH"}, RepoBase: "https://domain.com", MaxDeletionChecks: 10, + AllowedWebhookIPRanges: localhostIPRanges(), } server := httptest.NewServer(s.HandleRoot()) @@ -858,7 +944,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) } @@ -888,6 +974,300 @@ 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 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 @@ -920,3 +1300,177 @@ 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) + } +} + +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: localhostIPRanges(), + 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) + } +} 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}} } }, 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,