diff --git a/docs/content/docs/guide/incoming_webhook.md b/docs/content/docs/guide/incoming_webhook.md index ef5fe8ce11..59d22f022f 100644 --- a/docs/content/docs/guide/incoming_webhook.md +++ b/docs/content/docs/guide/incoming_webhook.md @@ -7,7 +7,8 @@ weight: 50 Pipelines-as-Code supports the concept of incoming webhook URL. It lets you trigger PipelineRuns in a Repository using a shared secret and URL, -instead of creating a new code iteration. +instead of creating a new code iteration. This allows users to trigger +PipelineRuns using an HTTP request, e.g. with `curl` or from a webservice. ## Incoming Webhook URL @@ -33,6 +34,20 @@ values are: Whereas for `github-apps` this does not need to be added. {{< /hint >}} +### Required Parameters + +Whether using the recommended POST request body or deprecated QueryParams, +the `/incoming` request accepts the following parameters: + +| Parameter | Type | Description | Required | +|-------------|--------|--------------------------------------------------------------------------------------|---------------------------------------------------| +|`repository` |`string`| Name of Repository CR | `true` | +|`namespace` |`string`| Namespace with the Repository CR | When Repository name is not unique in the cluster | +|`branch` |`string`| Branch configured for incoming webhook | `true` | +|`pipelinerun`|`string`| Name (or generateName) of PipelineRun, used to match PipelineRun definition | `true` | +|`secret` |`string`| Secret key referenced by the Repository CR in desired incoming webhook configuration | `true` | +|`params` |`json` | Parameters to override in PipelineRun context | `false` | + ### GitHub App The example below illustrates the use of GitHub App to trigger a PipelineRun diff --git a/pkg/adapter/adapter.go b/pkg/adapter/adapter.go index d1a4043398..dc86a9f01b 100644 --- a/pkg/adapter/adapter.go +++ b/pkg/adapter/adapter.go @@ -3,6 +3,7 @@ package adapter import ( "context" "encoding/json" + "errors" "fmt" "io" "net/http" @@ -171,8 +172,17 @@ func (l listener) handleEvent(ctx context.Context) http.HandlerFunc { isIncoming, targettedRepo, err := l.detectIncoming(ctx, request, payload) if err != nil { - l.logger.Errorf("error processing incoming webhook: %v", err) - return + if errors.Is(err, errDeprecatedRequestMode) { + // TODO: change this to a request failure once the deprecation is removed + // In this specific case the error can be non-nil while the rest of the return values are valid + response.Header().Add("Deprecation", "true") + } else { + if errors.Is(err, errMissingFields) { + l.writeResponse(response, http.StatusBadRequest, err.Error()) + } + l.logger.Errorf("error processing incoming webhook: %v", err) + return + } } if isIncoming { diff --git a/pkg/adapter/incoming.go b/pkg/adapter/incoming.go index 2869995f54..775912a985 100644 --- a/pkg/adapter/incoming.go +++ b/pkg/adapter/incoming.go @@ -4,8 +4,10 @@ import ( "context" "crypto/subtle" "encoding/json" + "errors" "fmt" "net/http" + "slices" apincoming "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/incoming" "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" @@ -23,6 +25,68 @@ import ( "go.uber.org/zap" ) +var errDeprecatedRequestMode = errors.New("requesting with secret name in query parameters is deprecated and will not be supported in future versions") + +var errMissingFields = errors.New("missing required fields") + +func errMissingSpecificFields(fields []string) error { + return fmt.Errorf("%w: %s", errMissingFields, fields) +} + +type incomingPayload struct { + legacyMode bool // indicates the request was made using the deprecated queryparams method + + RepoName string `json:"repository"` + Namespace string `json:"namespace,omitempty"` // Optional unless Repository name is not unique + Branch string `json:"branch"` + PipelineRun string `json:"pipelinerun"` + Secret string `json:"secret"` + Params map[string]any `json:"params"` +} + +func (payload *incomingPayload) validate() error { + missingFields := []string{} + + for field, value := range map[string]string{ + "repository": payload.RepoName, + "branch": payload.Branch, + "pipelinerun": payload.PipelineRun, + "secret": payload.Secret, + } { + if value == "" { + missingFields = append(missingFields, field) + } + } + + if len(missingFields) > 0 { + return errMissingSpecificFields(missingFields) + } + return nil +} + +// parseIncomingPayload parses and validates the incoming payload. +func parseIncomingPayload(request *http.Request, payloadBody []byte) (incomingPayload, error) { + parsedPayload := incomingPayload{ + RepoName: request.URL.Query().Get("repository"), + Branch: request.URL.Query().Get("branch"), + PipelineRun: request.URL.Query().Get("pipelinerun"), + Secret: request.URL.Query().Get("secret"), + Namespace: request.URL.Query().Get("namespace"), + legacyMode: true, + } + + if parsedPayload.validate() != nil { + if request.Method == http.MethodPost && request.Header.Get("Content-Type") == "application/json" && len(payloadBody) > 0 { + parsedPayload = incomingPayload{legacyMode: false} + if err := json.Unmarshal(payloadBody, &parsedPayload); err != nil { + return parsedPayload, fmt.Errorf("invalid JSON body for incoming webhook: %w", err) + } + } + } + + return parsedPayload, parsedPayload.validate() +} + func compareSecret(incomingSecret, secretValue string) bool { return subtle.ConstantTimeCompare([]byte(incomingSecret), []byte(secretValue)) != 0 } @@ -36,86 +100,55 @@ func applyIncomingParams(req *http.Request, payloadBody []byte, params []string) return apincoming.Payload{}, fmt.Errorf("error parsing incoming payload, not the expected format?: %w", err) } for k := range payload.Params { - allowed := false - for _, allowedP := range params { - if k == allowedP { - allowed = true - break - } - } - if !allowed { + if !slices.Contains(params, k) { return apincoming.Payload{}, fmt.Errorf("param %s is not allowed in incoming webhook CR", k) } } return payload, nil } +// detectIncoming checks if the request is for an "incoming" webhook request. +// If the request is for an "incoming" webhook request the request is parsed and matched to the expected +// repository. +// +// If the request is made using a deprecated mode, errDeprecatedRequestMode may be returned alongside otherwise valid return values. func (l *listener) detectIncoming(ctx context.Context, req *http.Request, payloadBody []byte) (bool, *v1alpha1.Repository, error) { - // Support both legacy (URL query) and new (POST body) secret passing - repository := req.URL.Query().Get("repository") - branch := req.URL.Query().Get("branch") - pipelineRun := req.URL.Query().Get("pipelinerun") - querySecret := req.URL.Query().Get("secret") - legacyMode := false - if req.URL.Path != "/incoming" { return false, nil, nil } - // If not all required query params are present, try to parse from JSON body - if repository == "" || branch == "" || pipelineRun == "" || querySecret == "" { - if req.Method == http.MethodPost && req.Header.Get("Content-Type") == "application/json" && len(payloadBody) > 0 { - var body struct { - Repository string `json:"repository"` - Branch string `json:"branch"` - PipelineRun string `json:"pipelinerun"` - Secret string `json:"secret"` - Params map[string]any `json:"params"` - } - if err := json.Unmarshal(payloadBody, &body); err == nil { - repository = body.Repository - branch = body.Branch - pipelineRun = body.PipelineRun - querySecret = body.Secret - } else { - return false, nil, fmt.Errorf("invalid JSON body for incoming webhook: %w", err) - } - } else { - return false, nil, fmt.Errorf("missing query URL argument: pipelinerun, branch, repository, secret: '%s' '%s' '%s' '%s'", pipelineRun, branch, repository, querySecret) - } - } else { - legacyMode = true - } - - if legacyMode { + l.logger.Infof("incoming request has been requested: %v", req.URL) + payload, err := parseIncomingPayload(req, payloadBody) + if payload.legacyMode { + // Log this, even if the request is invalid l.logger.Warnf("[SECURITY] Incoming webhook used legacy URL-based secret passing. This is insecure and will be deprecated. Please use POST body instead.") } - - l.logger.Infof("incoming request has been requested: %v", req.URL) - if pipelineRun == "" || repository == "" || querySecret == "" || branch == "" { - err := fmt.Errorf("missing query URL argument: pipelinerun, branch, repository, secret: '%s' '%s' '%s' '%s'", pipelineRun, branch, repository, querySecret) + if err != nil { return false, nil, err } - repo, err := matcher.GetRepo(ctx, l.run, repository) + repo, err := matcher.GetRepoByName(ctx, l.run, payload.RepoName, payload.Namespace) if err != nil { + if errors.Is(err, matcher.ErrRepositoryNameConflict) { + return false, nil, fmt.Errorf("%w: %w", err, errMissingSpecificFields([]string{"namespace"})) + } return false, nil, fmt.Errorf("error getting repo: %w", err) } if repo == nil { - return false, nil, fmt.Errorf("cannot find repository %s", repository) + return false, nil, fmt.Errorf("cannot find repository %s", payload.RepoName) } if repo.Spec.Incomings == nil { - return false, nil, fmt.Errorf("you need to have incoming webhooks rules in your repo spec, repo: %s", repository) + return false, nil, fmt.Errorf("you need to have incoming webhooks rules in your repo spec, repo: %s", payload.RepoName) } - hook := matcher.IncomingWebhookRule(branch, *repo.Spec.Incomings) + hook := matcher.IncomingWebhookRule(payload.Branch, *repo.Spec.Incomings) if hook == nil { - return false, nil, fmt.Errorf("branch '%s' has not matched any rules in repo incoming webhooks spec: %+v", branch, *repo.Spec.Incomings) + return false, nil, fmt.Errorf("branch '%s' has not matched any rules in repo incoming webhooks spec: %+v", payload.Branch, *repo.Spec.Incomings) } // log incoming request - l.logger.Infof("incoming request targeting pipelinerun %s on branch %s for repository %s has been accepted", pipelineRun, branch, repository) + l.logger.Infof("incoming request targeting pipelinerun %s on branch %s for repository %s has been accepted", payload.PipelineRun, payload.Branch, payload.RepoName) secretOpts := ktypes.GetSecretOpt{ Namespace: repo.Namespace, @@ -131,7 +164,7 @@ func (l *listener) detectIncoming(ctx context.Context, req *http.Request, payloa } // TODO: move to somewhere common to share between gitlab and here - if !compareSecret(querySecret, secretValue) { + if !compareSecret(payload.Secret, secretValue) { return false, nil, fmt.Errorf("secret passed to the webhook does not match the incoming webhook secret set on repository CR in secret %s", hook.Secret.Name) } @@ -164,14 +197,19 @@ func (l *listener) detectIncoming(ctx context.Context, req *http.Request, payloa // eventType and vice versa, but keeping as is for now. l.event.EventType = "incoming" l.event.TriggerTarget = "push" - l.event.TargetPipelineRun = pipelineRun - l.event.HeadBranch = branch - l.event.BaseBranch = branch + l.event.TargetPipelineRun = payload.PipelineRun + l.event.HeadBranch = payload.Branch + l.event.BaseBranch = payload.Branch l.event.Request.Header = req.Header l.event.Request.Payload = payloadBody l.event.URL = repo.Spec.URL l.event.Sender = "incoming" - return true, repo, nil + + // The caller can handle this error accordingly + if payload.legacyMode { + err = errDeprecatedRequestMode + } + return true, repo, err } func (l *listener) processIncoming(targetRepo *v1alpha1.Repository) (provider.Interface, *zap.SugaredLogger, error) { diff --git a/pkg/adapter/incoming_test.go b/pkg/adapter/incoming_test.go index 0aa1ceb2b6..a7cd58b0f2 100644 --- a/pkg/adapter/incoming_test.go +++ b/pkg/adapter/incoming_test.go @@ -104,6 +104,7 @@ func Test_listener_detectIncoming(t *testing.T) { querySecret string queryBranch string queryHeaders http.Header + queryNamespace string incomingBody string secretResult map[string]string } @@ -150,6 +151,64 @@ func Test_listener_detectIncoming(t *testing.T) { queryBranch: "main", }, }, + { + name: "good/incoming with namespace", + want: true, + args: args{ + secretResult: map[string]string{"good-secret": "verysecrete"}, + data: testclient.Data{ + Repositories: []*v1alpha1.Repository{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-good", + Namespace: "bad-namespace", + }, + Spec: v1alpha1.RepositorySpec{ + URL: goodURL, + Incomings: &[]v1alpha1.Incoming{ + { + Targets: []string{"main"}, + Secret: v1alpha1.Secret{ + Name: "good-secret", + }, + }, + }, + GitProvider: &v1alpha1.GitProvider{ + Type: "github", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-good", + Namespace: "good-namespace", + }, + Spec: v1alpha1.RepositorySpec{ + URL: goodURL, + Incomings: &[]v1alpha1.Incoming{ + { + Targets: []string{"main"}, + Secret: v1alpha1.Secret{ + Name: "good-secret", + }, + }, + }, + GitProvider: &v1alpha1.GitProvider{ + Type: "github", + }, + }, + }, + }, + }, + method: "GET", + queryURL: "/incoming", + queryRepository: "test-good", + querySecret: "verysecrete", + queryPipelineRun: "pipelinerun1", + queryBranch: "main", + queryNamespace: "good-namespace", + }, + }, { name: "good/incoming with body", want: true, @@ -318,6 +377,61 @@ func Test_listener_detectIncoming(t *testing.T) { }, wantErr: true, }, + { + name: "bad/multiple repos with name", + args: args{ + queryURL: "/incoming", + queryRepository: "repo", + queryPipelineRun: "pr", + querySecret: "secret", + queryBranch: "main", + data: testclient.Data{ + Repositories: []*v1alpha1.Repository{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "repo", + Namespace: "bad-namespace", + }, + Spec: v1alpha1.RepositorySpec{ + URL: goodURL, + Incomings: &[]v1alpha1.Incoming{ + { + Targets: []string{"main"}, + Secret: v1alpha1.Secret{ + Name: "good-secret", + }, + }, + }, + GitProvider: &v1alpha1.GitProvider{ + Type: "github", + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "repo", + Namespace: "good-namespace", + }, + Spec: v1alpha1.RepositorySpec{ + URL: goodURL, + Incomings: &[]v1alpha1.Incoming{ + { + Targets: []string{"main"}, + Secret: v1alpha1.Secret{ + Name: "good-secret", + }, + }, + }, + GitProvider: &v1alpha1.GitProvider{ + Type: "github", + }, + }, + }, + }, + }, + }, + wantErr: true, + }, { name: "bad/no incomings", args: args{ @@ -683,8 +797,8 @@ func Test_listener_detectIncoming(t *testing.T) { // make a new request req := httptest.NewRequest(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), + fmt.Sprintf("http://localhost%s?repository=%s&secret=%s&pipelinerun=%s&branch=%s&namespace=%s", tt.args.queryURL, + tt.args.queryRepository, tt.args.querySecret, tt.args.queryPipelineRun, tt.args.queryBranch, tt.args.queryNamespace), strings.NewReader(tt.args.incomingBody)) req.Header = tt.args.queryHeaders got, _, err := l.detectIncoming(ctx, req, []byte(tt.args.incomingBody)) @@ -949,18 +1063,18 @@ func Test_detectIncoming_legacy_warning(t *testing.T) { }, } tests := []struct { - name string - req *http.Request - body []byte - expectWarning bool + name string + req *http.Request + body []byte + legacyMode bool }{ { name: "legacy mode - params in URL", req: httptest.NewRequest(http.MethodPost, "http://localhost/incoming?repository=test-good&secret=verysecrete&pipelinerun=pipelinerun1&branch=main", strings.NewReader("")), - body: nil, - expectWarning: true, + body: nil, + legacyMode: true, }, { name: "new mode - params in JSON body", @@ -978,8 +1092,8 @@ func Test_detectIncoming_legacy_warning(t *testing.T) { r.Header.Set("Content-Type", "application/json") return r }(), - body: []byte(`{"repository":"test-good","branch":"main","pipelinerun":"pipelinerun2","secret":"verysecrete","params":{"foo":"bar"}}`), - expectWarning: false, + body: []byte(`{"repository":"test-good","branch":"main","pipelinerun":"pipelinerun2","secret":"verysecrete","params":{"foo":"bar"}}`), + legacyMode: false, }, } @@ -995,19 +1109,18 @@ func Test_detectIncoming_legacy_warning(t *testing.T) { event: info.NewEvent(), } got, _, err := l.detectIncoming(ctx, tt.req, tt.body) - assert.NilError(t, err) - assert.Assert(t, got) - found := false - for _, entry := range observedLogs.All() { - if strings.Contains(entry.Message, "[SECURITY] Incoming webhook used legacy URL-based secret passing") { - found = true - break - } + if tt.legacyMode { + assert.ErrorIs(t, err, errDeprecatedRequestMode) + } else { + assert.NilError(t, err) } - if tt.expectWarning { - assert.Assert(t, found, "expected security warning log for legacy URL-based secret passing") + assert.Assert(t, got) + + warningLogFound := observedLogs.FilterMessageSnippet("[SECURITY] Incoming webhook used legacy URL-based secret passing").Len() != 0 + if tt.legacyMode { + assert.Assert(t, warningLogFound, "expected security warning log for legacy URL-based secret passing") } else { - assert.Assert(t, !found, "did not expect security warning log for new mode") + assert.Assert(t, !warningLogFound, "did not expect security warning log for new mode") } }) } diff --git a/pkg/matcher/repo_runinfo_matcher.go b/pkg/matcher/repo_runinfo_matcher.go index 40e0993580..bfdbe1ef42 100644 --- a/pkg/matcher/repo_runinfo_matcher.go +++ b/pkg/matcher/repo_runinfo_matcher.go @@ -2,6 +2,7 @@ package matcher import ( "context" + "errors" "strings" apipac "github.com/openshift-pipelines/pipelines-as-code/pkg/apis/pipelinesascode/v1alpha1" @@ -11,6 +12,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) +var ErrRepositoryNameConflict = errors.New("multiple repositories exist with the given name") + func MatchEventURLRepo(ctx context.Context, cs *params.Run, event *info.Event, ns string) (*apipac.Repository, error) { repositories, err := cs.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(ns).List( ctx, metav1.ListOptions{}) @@ -28,21 +31,25 @@ func MatchEventURLRepo(ctx context.Context, cs *params.Run, event *info.Event, n return nil, nil } -// GetRepo get a repo by name anywhere on a cluster. -func GetRepo(ctx context.Context, cs *params.Run, repoName string) (*apipac.Repository, error) { - repositories, err := cs.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories("").List( - ctx, metav1.ListOptions{}) +// GetRepoByName get a repo by name anywhere on a cluster. +// Parameter 'ns' may optionally be supplied in case of a naming conflict. +func GetRepoByName(ctx context.Context, cs *params.Run, repoName string, ns string) (*apipac.Repository, error) { + repositories, err := cs.Clients.PipelineAsCode.PipelinesascodeV1alpha1().Repositories(ns).List( + ctx, metav1.ListOptions{ + FieldSelector: "metadata.name==" + repoName, + }) if err != nil { return nil, err } - for i := len(repositories.Items) - 1; i >= 0; i-- { - repo := repositories.Items[i] - if repo.GetName() == repoName { - return &repo, nil - } - } - return nil, nil + switch len(repositories.Items) { + case 0: + return nil, nil + case 1: + return &repositories.Items[0], nil + default: + return nil, ErrRepositoryNameConflict + } } // IncomingWebhookRule will match a rule to an incoming rule, currently a rule is a target branch.