feat: Add support for optional namespace parameter in incoming webhook
#2290
+274
−91
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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"` | ||
| } | ||
|
Comment on lines
+28
to
+45
There was a problem hiding this comment. @aThorp96 I see a lot done for deprecation, can we have it in separate commit for better tracking? There was a problem hiding this comment. Yeah i agree i think we can add the feature in one PR and refactoring and depreactioin in the other There was a problem hiding this comment. Yeah I can factor that into a separate PR. |
||
|
|
||
| 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 { | ||
| 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) { | ||
| if req.URL.Path != "/incoming" { | ||
| return false, nil, nil | ||
| } | ||
|
|
||
| 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.") | ||
| } | ||
| if err != nil { | ||
| return false, nil, err | ||
| } | ||
|
|
||
| repo, err := matcher.GetRepoByName(ctx, l.run, payload.RepoName, payload.Namespace) | ||
|
There was a problem hiding this comment. If no namespace is provided, the zero value |
||
| 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", 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", payload.RepoName) | ||
| } | ||
|
|
||
| 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", 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", 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(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 = 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" | ||
|
|
||
| // 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) { | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this is new behavior and not particularly related to the principal change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we should use a X- prefixed header for that?