diff --git a/.golangci.yml b/.golangci.yml index 31a5661223..b4c4e828d0 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -14,6 +14,9 @@ run: build-tags: - e2e linters-settings: + govet: + disable: + - inline gocritic: disabled-checks: - unlambda @@ -50,7 +53,7 @@ linters: # - execinquery - exhaustive #- exhaustruct - - exportloopref + - copyloopvar - forbidigo - forcetypeassert #- funlen diff --git a/.tekton/go.yaml b/.tekton/go.yaml index feaddbbedb..7c03e94d30 100644 --- a/.tekton/go.yaml +++ b/.tekton/go.yaml @@ -61,6 +61,8 @@ spec: value: $(workspaces.source.path)/go-build-cache/cache - name: GOMODCACHE value: $(workspaces.source.path)/go-build-cache/mod + - name: GOTOOLCHAIN + value: go1.21.13 workingDir: $(workspaces.source.path) script: | #!/usr/bin/env bash @@ -81,6 +83,8 @@ spec: value: $(workspaces.source.path)/go-build-cache/cache - name: GOMODCACHE value: $(workspaces.source.path)/go-build-cache/mod + - name: GOTOOLCHAIN + value: go1.21.13 - name: GITHUB_REPOSITORY value: "{{repo_owner}}/{{repo_name}}" - name: GITHUB_PULL_REQUEST_ID @@ -96,13 +100,15 @@ spec: chmod +x ./codecov ./codecov -P $GITHUB_PULL_REQUEST_ID -C {{revision}} -v - name: lint - image: golangci/golangci-lint:latest + image: golang:1.21 workingDir: $(workspaces.source.path) env: - name: GOCACHE value: $(workspaces.source.path)/go-build-cache/cache - name: GOMODCACHE value: $(workspaces.source.path)/go-build-cache/mod + - name: GOTOOLCHAIN + value: go1.21.13 - name: GOLANGCILINT_CACHE value: $(workspaces.source.path)/go-build-cache/golangci-cache script: | diff --git a/Makefile b/Makefile index 71b233f8cf..4e05458fb7 100644 --- a/Makefile +++ b/Makefile @@ -1,5 +1,6 @@ TARGET_NAMESPACE=pipelines-as-code -GOLANGCI_LINT=golangci-lint +GOLANGCI_LINT_VERSION ?= v1.59.1 +GO_TOOLCHAIN ?= go1.21.13 GOFUMPT=gofumpt TKN_BINARY_NAME := tkn TKN_BINARY_URL := https://tekton.dev/docs/cli/\#installation @@ -9,7 +10,18 @@ GO = go TIMEOUT_UNIT = 20m TIMEOUT_E2E = 45m GO_TEST_FLAGS += +GOTOOLCHAIN ?= $(GO_TOOLCHAIN) +export GOTOOLCHAIN SHELL := bash +TOPDIR := $(shell git rev-parse --show-toplevel) +TMPDIR := $(TOPDIR)/tmp +GOLANGCI_LINT_OS ?= $(shell uname -s | tr '[:upper:]' '[:lower:]') +GOLANGCI_LINT_ARCH ?= $(shell uname -m | sed -e 's/x86_64/amd64/' -e 's/aarch64/arm64/') +GOLANGCI_LINT_PACKAGE := golangci-lint-$(patsubst v%,%,$(GOLANGCI_LINT_VERSION))-$(GOLANGCI_LINT_OS)-$(GOLANGCI_LINT_ARCH) +GOLANGCI_LINT_DIR := $(TMPDIR)/golangci-lint/$(GOLANGCI_LINT_VERSION) +GOLANGCI_LINT_BIN := $(GOLANGCI_LINT_DIR)/golangci-lint +GOLANGCI_LINT ?= $(GOLANGCI_LINT_BIN) +GOLANGCI_LINT_EXTRA_ARGS ?= --concurrency=1 PY_FILES := $(shell find . -type f -regex ".*\.py" -print) SH_FILES := $(shell find hack/ -type f -regex ".*\.sh" -print) @@ -80,13 +92,21 @@ html-coverage: ## generate html coverage lint: lint-go lint-yaml lint-md lint-python lint-shell ## run all linters .PHONY: lint-go -lint-go: ## runs go linter on all go files +lint-go: golangci-lint ## runs go linter on all go files @echo "Linting go files..." - @$(GOLANGCI_LINT) run ./... --modules-download-mode=vendor \ + @$(GOLANGCI_LINT) run $(GOLANGCI_LINT_EXTRA_ARGS) ./... --modules-download-mode=vendor \ --max-issues-per-linter=0 \ --max-same-issues=0 \ --timeout $(TIMEOUT_UNIT) +.PHONY: golangci-lint +golangci-lint: $(GOLANGCI_LINT_BIN) ## download pinned golangci-lint into tmp + +$(GOLANGCI_LINT_BIN): + @mkdir -p $(GOLANGCI_LINT_DIR) + @echo "Downloading golangci-lint $(GOLANGCI_LINT_VERSION) for $(GOLANGCI_LINT_OS)-$(GOLANGCI_LINT_ARCH)" + @curl -fsSL "https://github.com/golangci/golangci-lint/releases/download/$(GOLANGCI_LINT_VERSION)/$(GOLANGCI_LINT_PACKAGE).tar.gz" | tar -xz -C "$(GOLANGCI_LINT_DIR)" --strip-components=1 "$(GOLANGCI_LINT_PACKAGE)/golangci-lint" + .PHONY: lint-yaml lint-yaml: ${YAML_FILES} ## runs yamllint on all yaml files @echo "Linting yaml files..." @@ -192,5 +212,3 @@ generated: update-golden fumpt ## generate all files that needs to be generated .PHONY: clean clean: ## clean build artifacts rm -fR bin - - diff --git a/go.mod b/go.mod index f3cf087f99..3036e447d2 100644 --- a/go.mod +++ b/go.mod @@ -48,7 +48,7 @@ require ( require ( github.com/coreos/go-oidc/v3 v3.9.0 // indirect - github.com/go-jose/go-jose/v3 v3.0.3 // indirect + github.com/go-jose/go-jose/v3 v3.0.5 // indirect ) replace ( diff --git a/go.sum b/go.sum index e473ddf0c9..8a68dc5ecb 100644 --- a/go.sum +++ b/go.sum @@ -746,8 +746,8 @@ github.com/go-fonts/stix v0.1.0/go.mod h1:w/c1f0ldAUlJmLBvlbkvVXLAD+tAMqobIIQpmn github.com/go-gl/glfw v0.0.0-20190409004039-e6da0acd62b1/go.mod h1:vR7hzQXu2zJy9AVAgeJqvqgH9Q5CA+iKCZ2gyEVpxRU= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20191125211704-12ad95a8df72/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= github.com/go-gl/glfw/v3.3/glfw v0.0.0-20200222043503-6f7a984d4dc4/go.mod h1:tQ2UAYgL5IevRw8kRxooKSPJfGvJ9fJQFa0TUsXzTg8= -github.com/go-jose/go-jose/v3 v3.0.3 h1:fFKWeig/irsp7XD2zBxvnmA/XaRWp5V3CBsZXJF7G7k= -github.com/go-jose/go-jose/v3 v3.0.3/go.mod h1:5b+7YgP7ZICgJDBdfjZaIt+H/9L9T/YQrVfLAMboGkQ= +github.com/go-jose/go-jose/v3 v3.0.5 h1:BLLJWbC4nMZOfuPVxoZIxeYsn6Nl2r1fITaJ78UQlVQ= +github.com/go-jose/go-jose/v3 v3.0.5/go.mod h1:5b+7YgP7ZICgJDBdfjZaIt+H/9L9T/YQrVfLAMboGkQ= github.com/go-kit/kit v0.8.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/kit v0.9.0/go.mod h1:xBxKIO96dXMWWy0MnWVtmwkA9/13aqxPnvrjFYMA2as= github.com/go-kit/log v0.1.0/go.mod h1:zbhenjAZHb184qTLMA9ZjW7ThYL0H2mk7Q6pNt4vbaY= diff --git a/pkg/adapter/incoming.go b/pkg/adapter/incoming.go index e8568f8aee..4ece7dd45f 100644 --- a/pkg/adapter/incoming.go +++ b/pkg/adapter/incoming.go @@ -58,7 +58,7 @@ func (l *listener) detectIncoming(ctx context.Context, req *http.Request, payloa if req.URL.Path != "/incoming" { return false, nil, nil } - l.logger.Infof("incoming request has been requested: %v", req.URL) + l.logger.Infof("incoming request has been requested: %v", req.URL.Path) if pipelineRun == "" || repository == "" || querySecret == "" || branch == "" { err := fmt.Errorf("missing query URL argument: pipelinerun, branch, repository, secret: '%s' '%s' '%s' '%s'", pipelineRun, branch, repository, querySecret) return false, nil, err @@ -112,6 +112,7 @@ func (l *listener) detectIncoming(ctx context.Context, req *http.Request, payloa return false, nil, err } l.event.Provider.URL = enterpriseURL + l.event.GHEURL = enterpriseURL l.event.Provider.Token = token l.event.InstallationID = installationID // Github app is not installed for provided repository url diff --git a/pkg/adapter/incoming_test.go b/pkg/adapter/incoming_test.go index 6536ac43ff..9e45f9ba16 100644 --- a/pkg/adapter/incoming_test.go +++ b/pkg/adapter/incoming_test.go @@ -1,9 +1,10 @@ package adapter import ( + "context" "fmt" + "io" "net/http" - "net/http/httptest" "os" "reflect" "strings" @@ -33,6 +34,13 @@ import ( "github.com/openshift-pipelines/pipelines-as-code/pkg/test/kubernetestint" ) +func newRequest(t *testing.T, method, target string, body io.Reader) *http.Request { + t.Helper() + req, err := http.NewRequestWithContext(context.Background(), method, target, body) + assert.NilError(t, err) + return req +} + const fakePrivateKey = `-----BEGIN RSA PRIVATE KEY----- MIICXAIBAAKBgQC6GorZBeri0eVERMZQDFh5E1RMPjFk9AevaWr27yJse6eiUlos gY2L2vcZKLOrdvVR+TLLapIMFfg1E1qVr1iTHP3IiSCs1uW6NKDmxEQc9Uf/fG9c @@ -682,7 +690,7 @@ func Test_listener_detectIncoming(t *testing.T) { } // make a new request - req := httptest.NewRequest(tt.args.method, + req := newRequest(t, tt.args.method, fmt.Sprintf("http://localhost%s?repository=%s&secret=%s&pipelinerun=%s&branch=%s", tt.args.queryURL, tt.args.queryRepository, tt.args.querySecret, tt.args.queryPipelineRun, tt.args.queryBranch), strings.NewReader(tt.args.incomingBody)) diff --git a/pkg/adapter/sinker.go b/pkg/adapter/sinker.go index 5ab3f7b06d..21043dbafa 100644 --- a/pkg/adapter/sinker.go +++ b/pkg/adapter/sinker.go @@ -45,12 +45,7 @@ func (s *sinker) processEventPayload(ctx context.Context, request *http.Request) } func (s *sinker) processEvent(ctx context.Context, request *http.Request) error { - if s.event.EventType == "incoming" { - if request.Header.Get("X-GitHub-Enterprise-Host") != "" { - s.event.Provider.URL = request.Header.Get("X-GitHub-Enterprise-Host") - s.event.GHEURL = request.Header.Get("X-GitHub-Enterprise-Host") - } - } else { + if s.event.EventType != "incoming" { if err := s.processEventPayload(ctx, request); err != nil { return err } diff --git a/pkg/provider/github/app/token.go b/pkg/provider/github/app/token.go index c22f05faeb..c269dd5978 100644 --- a/pkg/provider/github/app/token.go +++ b/pkg/provider/github/app/token.go @@ -4,10 +4,13 @@ import ( "context" "fmt" "net/http" + "net/url" + "strings" "time" "github.com/golang-jwt/jwt/v4" gt "github.com/google/go-github/v61/github" + "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/keys" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" "github.com/openshift-pipelines/pipelines-as-code/pkg/params" "github.com/openshift-pipelines/pipelines-as-code/pkg/provider/github" @@ -47,11 +50,20 @@ func (ip *Install) GetAndUpdateInstallationID(ctx context.Context) (string, stri return "", "", 0, err } + if ip.ghClient.APIURL == nil { + return "", "", 0, fmt.Errorf("github client APIURL is nil") + } apiURL := *ip.ghClient.APIURL - enterpriseHost = ip.request.Header.Get("X-GitHub-Enterprise-Host") - if enterpriseHost != "" { - // NOTE: Hopefully this works even when the ghe URL is on another host than the api URL - apiURL = "https://" + enterpriseHost + "/api/v3" + repoURL, err := url.Parse(ip.repo.Spec.URL) + if err != nil { + return "", "", 0, fmt.Errorf("failed to parse repository URL: %w", err) + } + enterpriseHost = "" + if repoURL.Host != "" && repoURL.Host != "github.com" { + enterpriseHost = repoURL.Host + if apiURL == keys.PublicGithubAPIURL { + apiURL = fmt.Sprintf("https://%s/api/v3", strings.TrimSuffix(enterpriseHost, "/")) + } } logger := logging.FromContext(ctx) diff --git a/pkg/provider/github/app/token_test.go b/pkg/provider/github/app/token_test.go index 7fd39bc125..a9a1f850bc 100644 --- a/pkg/provider/github/app/token_test.go +++ b/pkg/provider/github/app/token_test.go @@ -1,9 +1,10 @@ package app import ( + "context" "fmt" + "io" "net/http" - "net/http/httptest" "strings" "testing" @@ -22,6 +23,13 @@ import ( rtesting "knative.dev/pkg/reconciler/testing" ) +func newLocalhostGetRequest(t *testing.T, body io.Reader) *http.Request { + t.Helper() + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://localhost", body) + assert.NilError(t, err) + return req +} + const fakePrivateKey = `-----BEGIN RSA PRIVATE KEY----- MIICXAIBAAKBgQC6GorZBeri0eVERMZQDFh5E1RMPjFk9AevaWr27yJse6eiUlos gY2L2vcZKLOrdvVR+TLLapIMFfg1E1qVr1iTHP3IiSCs1uW6NKDmxEQc9Uf/fG9c @@ -144,7 +152,7 @@ func Test_GenerateJWT(t *testing.T) { }, } - ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, tt.namespace.GetName()) + ip := NewInstallation(newLocalhostGetRequest(t, strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, tt.namespace.GetName()) token, err := ip.GenerateJWT(ctx) if tt.wantErr { assert.Assert(t, err != nil) @@ -205,10 +213,10 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { ctx = info.StoreCurrentControllerName(ctx, "default") ctx = info.StoreNS(ctx, testNamespace.GetName()) - ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName()) + ip := NewInstallation(newLocalhostGetRequest(t, strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName()) jwtToken, err := ip.GenerateJWT(ctx) assert.NilError(t, err) - req := httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")) + req := newLocalhostGetRequest(t, strings.NewReader("")) repo := &v1alpha1.Repository{ ObjectMeta: metav1.ObjectMeta{ Name: "repo", @@ -256,6 +264,81 @@ func Test_GetAndUpdateInstallationID(t *testing.T) { assert.Equal(t, token, wantToken) } +func TestGetAndUpdateInstallationIDIgnoresEnterpriseHostHeader(t *testing.T) { + tdata := testclient.Data{ + Namespaces: []*corev1.Namespace{testNamespace}, + Secret: []*corev1.Secret{validSecret}, + } + wantToken := "GOODTOKEN" + wantID := 120 + orgName := "org" + repoName := "repo" + + fakeghclient, mux, serverURL, teardown := ghtesthelper.SetupGH() + defer teardown() + + ctx, _ := rtesting.SetupFakeContext(t) + stdata, _ := testclient.SeedTestData(t, ctx, tdata) + logger, _ := logger.GetLogger() + run := ¶ms.Run{ + Clients: clients.Clients{ + Log: logger, + PipelineAsCode: stdata.PipelineAsCode, + Kube: stdata.Kube, + }, + Info: info.Info{ + Pac: &info.PacOpts{ + Settings: settings.Settings{}, + }, + Controller: &info.ControllerInfo{Secret: validSecret.GetName()}, + }, + } + ctx = info.StoreCurrentControllerName(ctx, "default") + ctx = info.StoreNS(ctx, testNamespace.GetName()) + + jwtInstall := NewInstallation(newLocalhostGetRequest(t, strings.NewReader("")), run, &v1alpha1.Repository{}, &github.Provider{}, testNamespace.GetName()) + jwtToken, err := jwtInstall.GenerateJWT(ctx) + assert.NilError(t, err) + + mux.HandleFunc("/app/installations", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Authorization", "Bearer 12345") + w.Header().Set("Accept", "application/vnd.github+json") + _, _ = fmt.Fprintf(w, `[{"id":%d}]`, wantID) + }) + mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", wantID), func(w http.ResponseWriter, r *http.Request) { + testMethod(t, r, "POST") + w.Header().Set("Authorization", "Bearer "+jwtToken) + w.Header().Set("Accept", "application/vnd.github+json") + _, _ = fmt.Fprintf(w, `{"token": "%s"}`, wantToken) + }) + mux.HandleFunc("/installation/repositories", func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Authorization", "Bearer 12345") + w.Header().Set("Accept", "application/vnd.github+json") + _, _ = fmt.Fprintf(w, `{"total_count": 1,"repositories": [{"id":1,"html_url": "https://github.com/%s/%s"}]}`, orgName, repoName) + }) + + repo := &v1alpha1.Repository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "repo", + }, + Spec: v1alpha1.RepositorySpec{ + URL: fmt.Sprintf("https://github.com/%s/%s", orgName, repoName), + }, + } + + gprovider := &github.Provider{Client: fakeghclient, APIURL: &serverURL, Run: run} + t.Setenv("PAC_GIT_PROVIDER_TOKEN_APIURL", serverURL+"/api/v3") + + req := newLocalhostGetRequest(t, strings.NewReader("")) + req.Header.Set("X-GitHub-Enterprise-Host", "127.0.0.1:1") + ip := NewInstallation(req, run, repo, gprovider, testNamespace.GetName()) + enterpriseURL, token, installationID, err := ip.GetAndUpdateInstallationID(ctx) + assert.NilError(t, err) + assert.Equal(t, enterpriseURL, "") + assert.Equal(t, installationID, int64(wantID)) + assert.Equal(t, token, wantToken) +} + func testMethod(t *testing.T, r *http.Request, want string) { t.Helper() if got := r.Method; got != want { @@ -296,7 +379,7 @@ func Test_ListRepos(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) gprovider := &github.Provider{Client: fakeclient} - ip := NewInstallation(httptest.NewRequest(http.MethodGet, "http://localhost", strings.NewReader("")), + ip := NewInstallation(newLocalhostGetRequest(t, strings.NewReader("")), ¶ms.Run{}, repo, gprovider, testNamespace.GetName()) exist, err := ip.matchRepos(ctx) assert.NilError(t, err) diff --git a/pkg/provider/github/github.go b/pkg/provider/github/github.go index 4a7d507035..3b9b969194 100644 --- a/pkg/provider/github/github.go +++ b/pkg/provider/github/github.go @@ -292,6 +292,34 @@ func (v *Provider) SetClient(ctx context.Context, run *params.Run, event *info.E } } + // Handle GitHub App token scoping for both global and repo-level configuration + if event.InstallationID > 0 { + token := "" + if repo != nil && v.pacInfo != nil && v.Logger != nil && v.eventEmitter != nil { + v.Logger.Debugf("setupAuthenticatedClient: scoping github app token") + scopedToken, err := ScopeTokenToListOfRepos(ctx, v, v.pacInfo, repo, run, event, v.eventEmitter, v.Logger) + if err != nil { + return fmt.Errorf("failed to scope token: %w", err) + } + token = scopedToken + } + // If Global and Repo level configurations are not provided then lets not override the provider token. + if token != "" { + event.Provider.Token = token + } else if len(v.RepositoryIDs) > 0 { + // We need to keep the token unscoped until ScopeTokenToListOfRepos so that CreateToken can + // look up the extra repos from the configmap. + // Token is scoped to only the calling repo if no additional scoping repos are configured + // so that no unwanted remote tasks are executed. + ns := info.GetNS(ctx) + scopedToken, err := v.GetAppToken(ctx, run.Clients.Kube, event.Provider.URL, event.InstallationID, ns) + if err != nil { + return fmt.Errorf("failed to scope token to triggering repository: %w", err) + } + event.Provider.Token = scopedToken + } + } + return nil } diff --git a/pkg/provider/github/parse_payload.go b/pkg/provider/github/parse_payload.go index bbf893a59c..b531d999cd 100644 --- a/pkg/provider/github/parse_payload.go +++ b/pkg/provider/github/parse_payload.go @@ -1,11 +1,13 @@ package github import ( + "bytes" "context" "encoding/json" "errors" "fmt" "net/http" + "net/url" "os" "path" "strconv" @@ -24,6 +26,12 @@ import ( "k8s.io/client-go/kubernetes" ) +const ( + githubAppTokenMintBlockedLog = "[SECURITY] Blocked GitHub App token minting before webhook signature validation completed" + githubAppTokenExfiltrationBlockedLog = "[SECURITY][CRITICAL] Averted GitHub App credential exfiltration attempt before token minting" + controllerWebhookSecretKey = "webhook.secret" +) + // GetAppIDAndPrivateKey retrieves the GitHub application ID and private key from a secret in the specified namespace. // It takes a context, namespace, and Kubernetes client as input parameters. // It returns the application ID (int64), private key ([]byte), and an error if any. @@ -90,6 +98,35 @@ func (v *Provider) GetAppToken(ctx context.Context, kube kubernetes.Interface, g return token, err } +func validateAppWebhookSignature(ctx context.Context, run *params.Run, event *info.Event) error { + signature := event.Request.Header.Get(github.SHA256SignatureHeader) + if signature == "" { + signature = event.Request.Header.Get(github.SHA1SignatureHeader) + } + if signature == "" || signature == "sha1=" { + return fmt.Errorf("no signature has been detected, for security reason we are not allowing webhooks that has no secret") + } + + var err error + event.Provider.WebhookSecret, err = getCurrentNSWebhookSecret(ctx, run) + if err != nil { + return err + } + if event.Provider.WebhookSecret == "" { + return fmt.Errorf("no webhook secret has been set in controller secret") + } + return github.ValidateSignature(signature, event.Request.Payload, []byte(event.Provider.WebhookSecret)) +} + +func getCurrentNSWebhookSecret(ctx context.Context, run *params.Run) (string, error) { + ns := info.GetNS(ctx) + secret, err := run.Clients.Kube.CoreV1().Secrets(ns).Get(ctx, run.Info.Controller.Secret, metav1.GetOptions{}) + if err != nil { + return "", err + } + return strings.TrimSpace(string(secret.Data[controllerWebhookSecretKey])), nil +} + func (v *Provider) parseEventType(request *http.Request, event *info.Event) error { event.EventType = request.Header.Get("X-GitHub-Event") if event.EventType == "" { @@ -111,18 +148,91 @@ type Payload struct { Installation struct { ID *int64 `json:"id"` } `json:"installation"` + Repository struct { + HTMLURL string `json:"html_url"` + ID *int64 `json:"id"` + } `json:"repository"` } -func getInstallationIDFromPayload(payload string) (int64, error) { +func getInstallationAndRepoIDFromPayload(payload string) (int64, int64, error) { var data Payload err := json.Unmarshal([]byte(payload), &data) if err != nil { - return -1, err + return -1, -1, err } + + var installationID int64 = -1 + var repoID int64 = -1 if data.Installation.ID != nil { - return *data.Installation.ID, nil + installationID = *data.Installation.ID + } + + if data.Repository.ID != nil { + repoID = *data.Repository.ID } - return -1, nil + + return installationID, repoID, nil +} + +func validateEnterpriseHostMatchesPayload(gheURL, payload string) error { + if gheURL == "" { + return nil + } + if !strings.HasPrefix(gheURL, "https://") && !strings.HasPrefix(gheURL, "http://") { + gheURL = "https://" + gheURL + } + enterpriseURL, err := url.Parse(gheURL) + if err != nil || enterpriseURL.Host == "" { + return fmt.Errorf("invalid X-GitHub-Enterprise-Host header") + } + + var data Payload + if err := json.Unmarshal([]byte(payload), &data); err != nil { + return err + } + if data.Repository.HTMLURL == "" { + return fmt.Errorf("repository HTML URL is missing in payload, cannot validate enterprise host") + } + repoURL, err := url.Parse(data.Repository.HTMLURL) + if err != nil || repoURL.Host == "" { + return fmt.Errorf("invalid repository URL in GitHub payload") + } + if !strings.EqualFold(enterpriseURL.Host, repoURL.Host) { + return fmt.Errorf("github enterprise host %q does not match repository host %q", enterpriseURL.Host, repoURL.Host) + } + return nil +} + +func (v *Provider) logBlockedGitHubAppTokenMint(request *http.Request, event *info.Event, installationID int64, reason string, err error) { + if v.Logger == nil { + return + } + v.Logger.Errorw(githubAppTokenExfiltrationBlockedLog, + "severity", "critical", + "security-impact", "github-app-jwt-exfiltration-blocked", + "reason", reason, + "error", err, + "event-type", event.EventType, + "installation-id", installationID, + "github-enterprise-host-present", request.Header.Get("X-GitHub-Enterprise-Host") != "", + "remote-addr", request.RemoteAddr, + ) +} + +func (v *Provider) logGitHubAppTokenMintValidationFailure(request *http.Request, event *info.Event, installationID int64, reason string, err error) { + if v.Logger == nil { + return + } + v.Logger.Warnw(githubAppTokenMintBlockedLog, + "severity", "warning", + "security-impact", "github-app-token-mint-blocked", + "reason", reason, + "error", err, + "event-type", event.EventType, + "installation-id", installationID, + "github-enterprise-host-present", request.Header.Get("X-GitHub-Enterprise-Host") != "", + "remote-addr", request.RemoteAddr, + ) } // ParsePayload will parse the payload and return the event @@ -139,32 +249,46 @@ func getInstallationIDFromPayload(payload string) (int64, error) { // app on a github org which has a mixed of private and public repos and some of // the public users should not have access to the private repos. // -// Another thing: The payload is protected by the webhook signature so it cannot be tempered but even tho if it's -// tempered with and somehow a malicious user found the token and set their own github endpoint to hijack and -// exfiltrate the token, it would fail since the jwt token generation will fail, so we are safe here. -// a bit too far fetched but i don't see any way we can exploit this. +// Validate the webhook signature before generating an app token because the token +// request signs a GitHub App JWT locally and sends it to the selected API host. func (v *Provider) ParsePayload(ctx context.Context, run *params.Run, request *http.Request, payload string) (*info.Event, error) { // ParsePayload is really happening before SetClient so let's set this at first here. // Only apply for GitHub provider since we do fancy token creation at payload parsing v.Run = run event := info.NewEvent() + event.Request = &info.Request{ + Header: request.Header, + Payload: bytes.TrimSpace([]byte(payload)), + } systemNS := info.GetNS(ctx) if err := v.parseEventType(request, event); err != nil { return nil, err } - installationIDFrompayload, err := getInstallationIDFromPayload(payload) + installationIDFrompayload, repoIDFromPayload, err := getInstallationAndRepoIDFromPayload(payload) if err != nil { return nil, err } if installationIDFrompayload != -1 { var err error + if err := validateAppWebhookSignature(ctx, run, event); err != nil { + v.logGitHubAppTokenMintValidationFailure(request, event, installationIDFrompayload, "webhook-signature-validation-failed", err) + return nil, err + } + if err := validateEnterpriseHostMatchesPayload(event.Provider.URL, payload); err != nil { + v.logBlockedGitHubAppTokenMint(request, event, installationIDFrompayload, "enterprise-host-validation-failed", err) + return nil, err + } // TODO: move this out of here when we move al config inside context if event.Provider.Token, err = v.GetAppToken(ctx, run.Clients.Kube, event.Provider.URL, installationIDFrompayload, systemNS); err != nil { return nil, err } } + if repoIDFromPayload > 0 { + v.RepositoryIDs = []int64{repoIDFromPayload} + } + eventInt, err := github.ParseWebHook(event.EventType, []byte(payload)) if err != nil { return nil, err @@ -321,6 +445,7 @@ func (v *Provider) handleReRequestEvent(ctx context.Context, event *github.Check // we allow the rerequest user here, not the push user, i guess it's // fine because you can't do a rereq without being a github owner? runevent.Sender = event.GetSender().GetLogin() + v.RepositoryIDs = []int64{event.GetRepo().GetID()} return runevent, nil } runevent.PullRequestNumber = event.GetCheckRun().GetCheckSuite().PullRequests[0].GetNumber() @@ -348,6 +473,7 @@ func (v *Provider) handleCheckSuites(ctx context.Context, event *github.CheckSui // we allow the rerequest user here, not the push user, i guess it's // fine because you can't do a rereq without being a github owner? runevent.Sender = event.GetSender().GetLogin() + v.RepositoryIDs = []int64{event.GetRepo().GetID()} return runevent, nil // return nil, fmt.Errorf("check suite event is not supported for push events") } @@ -403,6 +529,7 @@ func (v *Provider) handleCommitCommentEvent(ctx context.Context, event *github.C runevent.EventType = "push" runevent.TriggerTarget = "push" runevent.TriggerComment = event.GetComment().GetBody() + v.RepositoryIDs = []int64{event.GetRepo().GetID()} // Set main as default branch to runevent.HeadBranch, runevent.BaseBranch runevent.HeadBranch, runevent.BaseBranch = "main", "main" diff --git a/pkg/provider/github/parse_payload_test.go b/pkg/provider/github/parse_payload_test.go index 86b7f1b42c..e0a063022b 100644 --- a/pkg/provider/github/parse_payload_test.go +++ b/pkg/provider/github/parse_payload_test.go @@ -2,6 +2,9 @@ package github import ( "context" + "crypto/hmac" + "crypto/sha256" + "encoding/hex" "encoding/json" "fmt" "net/http" @@ -11,7 +14,6 @@ import ( "github.com/google/go-github/v61/github" "gotest.tools/v3/assert" - "gotest.tools/v3/env" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" rtesting "knative.dev/pkg/reconciler/testing" @@ -53,6 +55,12 @@ var sampleRepo = &github.Repository{ var testInstallationID = int64(1) +func githubSHA256Signature(secret string, payload []byte) string { + hm := hmac.New(sha256.New, []byte(secret)) + hm.Write(payload) + return "sha256=" + hex.EncodeToString(hm.Sum(nil)) +} + var samplePRevent = github.PullRequestEvent{ PullRequest: &github.PullRequest{ Head: &github.PullRequestBranch{ @@ -614,6 +622,7 @@ func TestAppTokenGeneration(t *testing.T) { secretName := "pipelines-as-code-secret" ctx, _ := rtesting.SetupFakeContext(t) + webhookSecret := "webhook-secret" vaildSecret, _ := testclient.SeedTestData(t, ctx, testclient.Data{ Secret: []*corev1.Secret{ { @@ -624,6 +633,7 @@ func TestAppTokenGeneration(t *testing.T) { Data: map[string][]byte{ "github-application-id": []byte("12345"), "github-private-key": []byte(fakePrivateKey), + "webhook.secret": []byte(webhookSecret), }, }, }, @@ -640,6 +650,7 @@ func TestAppTokenGeneration(t *testing.T) { Data: map[string][]byte{ "github-application-id": []byte("abcd"), "github-private-key": []byte(fakePrivateKey), + "webhook.secret": []byte(webhookSecret), }, }, }, @@ -656,6 +667,7 @@ func TestAppTokenGeneration(t *testing.T) { Data: map[string][]byte{ "github-application-id": []byte("12345"), "github-private-key": []byte("invalid-key"), + "webhook.secret": []byte(webhookSecret), }, }, }, @@ -668,17 +680,49 @@ func TestAppTokenGeneration(t *testing.T) { wantErrSubst string nilClient bool seedData testclient.Clients - envs map[string]string resultBaseURL string checkInstallIDs []int64 extraRepoInstallIDs map[string]string + omitSignature bool + enterpriseHost string + payload string + wantLogMessage string }{ { - name: "secret not found", - ctx: ctxNoSecret, - ctxNS: "foo", - seedData: noSecret, - wantErrSubst: `secrets "pipelines-as-code-secret" not found`, + name: "secret not found", + ctx: ctxNoSecret, + ctxNS: "foo", + seedData: noSecret, + wantErrSubst: `secrets "pipelines-as-code-secret" not found`, + wantLogMessage: githubAppTokenMintBlockedLog, + }, + { + ctx: ctx, + name: "missing webhook signature", + ctxNS: testNamespace, + seedData: vaildSecret, + omitSignature: true, + wantErrSubst: "no signature has been detected", + wantLogMessage: githubAppTokenMintBlockedLog, + }, + { + ctx: ctx, + name: "enterprise host does not match signed repository payload", + ctxNS: testNamespace, + seedData: vaildSecret, + enterpriseHost: "127.0.0.1:1", + wantErrSubst: `github enterprise host "127.0.0.1:1" does not match repository host "github.com"`, + wantLogMessage: githubAppTokenExfiltrationBlockedLog, + }, + { + ctx: ctx, + name: "enterprise host with missing repository HTML URL", + ctxNS: testNamespace, + seedData: vaildSecret, + enterpriseHost: "127.0.0.1:1", + payload: fmt.Sprintf(`{"installation":{"id":%d},"repository":{}}`, testInstallationID), + wantErrSubst: "repository HTML URL is missing in payload, cannot validate enterprise host", + wantLogMessage: githubAppTokenExfiltrationBlockedLog, }, { ctx: ctx, @@ -726,8 +770,6 @@ func TestAppTokenGeneration(t *testing.T) { mux.HandleFunc(fmt.Sprintf("/app/installations/%d/access_tokens", testInstallationID), func(w http.ResponseWriter, _ *http.Request) { _, _ = fmt.Fprint(w, "{}") }) - envRemove := env.PatchAll(t, tt.envs) - defer envRemove() // adding installation id to event to enforce client creation samplePRevent.Installation = &github.Installation{ @@ -746,9 +788,12 @@ func TestAppTokenGeneration(t *testing.T) { } jeez, _ := json.Marshal(samplePRevent) - logger, _ := logger.GetLogger() + if tt.payload != "" { + jeez = []byte(tt.payload) + } + testLogger, observedLogs := logger.GetLogger() gprovider := Provider{ - Logger: logger, + Logger: testLogger, Client: fakeghclient, pacInfo: &info.PacOpts{ Settings: settings.Settings{}, @@ -756,14 +801,17 @@ func TestAppTokenGeneration(t *testing.T) { } request := &http.Request{Header: map[string][]string{}} request.Header.Set("X-GitHub-Event", "pull_request") - // a bit of a pain but works - request.Header.Set("X-GitHub-Enterprise-Host", serverURL) - tt.envs = make(map[string]string) - tt.envs["PAC_GIT_PROVIDER_TOKEN_APIURL"] = serverURL + "/api/v3" + if !tt.omitSignature { + request.Header.Set(github.SHA256SignatureHeader, githubSHA256Signature(webhookSecret, jeez)) + } + if tt.enterpriseHost != "" { + request.Header.Set("X-GitHub-Enterprise-Host", tt.enterpriseHost) + } + t.Setenv("PAC_GIT_PROVIDER_TOKEN_APIURL", serverURL+"/api/v3") run := ¶ms.Run{ Clients: clients.Clients{ - Log: logger, + Log: testLogger, Kube: tt.seedData.Kube, }, @@ -799,6 +847,16 @@ func TestAppTokenGeneration(t *testing.T) { if tt.wantErrSubst != "" { assert.Assert(t, err != nil) assert.ErrorContains(t, err, tt.wantErrSubst) + if tt.wantLogMessage != "" { + found := false + for _, entry := range observedLogs.All() { + if entry.Message == tt.wantLogMessage { + found = true + break + } + } + assert.Assert(t, found, "expected log message %q for blocked GitHub App token mint", tt.wantLogMessage) + } return } assert.NilError(t, err) diff --git a/pkg/resolve/remote.go b/pkg/resolve/remote.go index 359dc74e5c..0f789d1983 100644 --- a/pkg/resolve/remote.go +++ b/pkg/resolve/remote.go @@ -50,7 +50,7 @@ func getRemotes(ctx context.Context, rt *matcher.RemoteTasks, types TektonTypes) rt.Logger.Debugf("skipping already fetched task %s in annotations on pipelinerun %s", task.GetName(), pipelinerun.GetName()) continue } - remoteType.Tasks = append(remoteType.Tasks, task) + remoteType.Tasks = append(remoteType.Tasks, task.DeepCopy()) } // get the pipeline from the remote annotation if any @@ -60,7 +60,7 @@ func getRemotes(ctx context.Context, rt *matcher.RemoteTasks, types TektonTypes) } if remotePipeline != nil { - remoteType.Pipelines = append(remoteType.Pipelines, remotePipeline) + remoteType.Pipelines = append(remoteType.Pipelines, remotePipeline.DeepCopy()) } } @@ -79,7 +79,7 @@ func getRemotes(ctx context.Context, rt *matcher.RemoteTasks, types TektonTypes) rt.Logger.Infof("skipping remote task %s from remote pipeline %s as already defined in pipelinerun", remoteTask.GetName(), pipeline.GetName()) continue } - remoteType.Tasks = append(remoteType.Tasks, remoteTask) + remoteType.Tasks = append(remoteType.Tasks, remoteTask.DeepCopy()) } } diff --git a/vendor/modules.txt b/vendor/modules.txt index d373ca1947..af2ca76ced 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -99,7 +99,7 @@ github.com/gfleury/go-bitbucket-v1 # github.com/go-fed/httpsig v1.1.0 ## explicit; go 1.13 github.com/go-fed/httpsig -# github.com/go-jose/go-jose/v3 v3.0.3 +# github.com/go-jose/go-jose/v3 v3.0.5 ## explicit; go 1.12 # github.com/go-kit/log v0.2.1 ## explicit; go 1.17