diff --git a/pkg/apis/triggers/v1alpha1/interceptor_types.go b/pkg/apis/triggers/v1alpha1/interceptor_types.go index c197fd570..38a06d64b 100644 --- a/pkg/apis/triggers/v1alpha1/interceptor_types.go +++ b/pkg/apis/triggers/v1alpha1/interceptor_types.go @@ -2,7 +2,6 @@ package v1alpha1 import ( "context" - "encoding/json" "fmt" "strings" @@ -20,10 +19,14 @@ type InterceptorInterface interface { // Do not generate DeepCopy(). See #827 // +k8s:deepcopy-gen=false type InterceptorRequest struct { - // Body is the incoming HTTP event body - Body json.RawMessage `json:"body,omitempty"` + // Body is the incoming HTTP event body. We use a "string" representation of the JSON body + // in order to preserve the body exactly as it was sent (including spaces etc.). This is necessary + // for some interceptors e.g. GitHub for validating the body with a signature. + Body string `json:"body,omitempty"` + // Header are the headers for the incoming HTTP event Header map[string][]string `json:"header,omitempty"` + // Extensions are extra values that are added by previous interceptors in a chain Extensions map[string]interface{} `json:"extensions,omitempty"` diff --git a/pkg/interceptors/bitbucket/bitbucket.go b/pkg/interceptors/bitbucket/bitbucket.go index 03e14ab9e..20f38847b 100644 --- a/pkg/interceptors/bitbucket/bitbucket.go +++ b/pkg/interceptors/bitbucket/bitbucket.go @@ -65,7 +65,7 @@ func (w *Interceptor) Process(ctx context.Context, r *triggersv1.InterceptorRequ } secretToken := secret.Data[p.SecretRef.SecretKey] - if err := gh.ValidateSignature(header, r.Body, secretToken); err != nil { + if err := gh.ValidateSignature(header, []byte(r.Body), secretToken); err != nil { return interceptors.Failf(codes.FailedPrecondition, err.Error()) } } diff --git a/pkg/interceptors/bitbucket/bitbucket_test.go b/pkg/interceptors/bitbucket/bitbucket_test.go index 6675fd5a2..d811961f2 100644 --- a/pkg/interceptors/bitbucket/bitbucket_test.go +++ b/pkg/interceptors/bitbucket/bitbucket_test.go @@ -117,7 +117,7 @@ func TestInterceptor_Process_ShouldContinue(t *testing.T) { } req := &triggersv1.InterceptorRequest{ - Body: tt.payload, + Body: string(tt.payload), Header: http.Header{ "Content-Type": []string{"application/json"}, }, @@ -263,7 +263,7 @@ func TestInterceptor_Process_ShouldNotContinue(t *testing.T) { } req := &triggersv1.InterceptorRequest{ - Body: tt.payload, + Body: string(tt.payload), Header: http.Header{ "Content-Type": []string{"application/json"}, }, @@ -308,7 +308,7 @@ func TestInterceptor_Process_InvalidParams(t *testing.T) { } req := &triggersv1.InterceptorRequest{ - Body: json.RawMessage(`{}`), + Body: `{}`, Header: http.Header{ "Content-Type": []string{"application/json"}, }, diff --git a/pkg/interceptors/cel/cel.go b/pkg/interceptors/cel/cel.go index 28ac91657..c7526f8c7 100644 --- a/pkg/interceptors/cel/cel.go +++ b/pkg/interceptors/cel/cel.go @@ -131,8 +131,8 @@ func (w *Interceptor) Process(ctx context.Context, r *triggersv1.InterceptorRequ } var payload = []byte(`{}`) - if r.Body != nil { - payload = r.Body + if r.Body != "" { + payload = []byte(r.Body) } evalContext, err := makeEvalContext(payload, r.Header, r.Context.EventURL, r.Extensions) diff --git a/pkg/interceptors/cel/cel_test.go b/pkg/interceptors/cel/cel_test.go index 65ad99923..8a728c288 100644 --- a/pkg/interceptors/cel/cel_test.go +++ b/pkg/interceptors/cel/cel_test.go @@ -259,7 +259,7 @@ func TestInterceptor_Process(t *testing.T) { Logger: logger.Sugar(), } res := w.Process(ctx, &triggersv1.InterceptorRequest{ - Body: tt.body, + Body: string(tt.body), Header: http.Header{ "Content-Type": []string{"application/json"}, "X-Test": []string{"test-value"}, @@ -356,7 +356,7 @@ func TestInterceptor_Process_Error(t *testing.T) { Logger: logger.Sugar(), } res := w.Process(context.Background(), &triggersv1.InterceptorRequest{ - Body: tt.body, + Body: string(tt.body), Header: http.Header{ "Content-Type": []string{"application/json"}, "X-Test": []string{"test-value"}, @@ -392,7 +392,7 @@ func TestInterceptor_Process_InvalidParams(t *testing.T) { Logger: logger.Sugar(), } res := w.Process(context.Background(), &triggersv1.InterceptorRequest{ - Body: json.RawMessage(`{}`), + Body: `{}`, Header: http.Header{}, InterceptorParams: map[string]interface{}{ "filter": func() {}, // Should fail JSON unmarshal diff --git a/pkg/interceptors/github/github.go b/pkg/interceptors/github/github.go index 747105a3e..c711ee764 100644 --- a/pkg/interceptors/github/github.go +++ b/pkg/interceptors/github/github.go @@ -71,7 +71,7 @@ func (w *Interceptor) Process(ctx context.Context, r *triggersv1.InterceptorRequ } secretToken := secret.Data[p.SecretRef.SecretKey] - if err := gh.ValidateSignature(header, r.Body, secretToken); err != nil { + if err := gh.ValidateSignature(header, []byte(r.Body), secretToken); err != nil { return interceptors.Fail(codes.FailedPrecondition, err.Error()) } } diff --git a/pkg/interceptors/github/github_test.go b/pkg/interceptors/github/github_test.go index 8eadd9310..4261f0f34 100644 --- a/pkg/interceptors/github/github_test.go +++ b/pkg/interceptors/github/github_test.go @@ -113,7 +113,7 @@ func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) { kubeClient := fakekubeclient.Get(ctx) req := &triggersv1.InterceptorRequest{ - Body: tt.payload, + Body: string(tt.payload), Header: http.Header{ "Content-Type": []string{"application/json"}, }, @@ -258,7 +258,7 @@ func TestInterceptor_ExecuteTrigger_ShouldNotContinue(t *testing.T) { kubeClient := fakekubeclient.Get(ctx) req := &triggersv1.InterceptorRequest{ - Body: tt.payload, + Body: string(tt.payload), Header: http.Header{ "Content-Type": []string{"application/json"}, }, @@ -301,7 +301,7 @@ func TestInterceptor_ExecuteTrigger_with_invalid_content_type(t *testing.T) { logger := zaptest.NewLogger(t) kubeClient := fakekubeclient.Get(ctx) req := &triggersv1.InterceptorRequest{ - Body: json.RawMessage(`{}`), + Body: `{}`, Header: http.Header{ "Content-Type": []string{"application/x-www-form-urlencoded"}, "X-Hub-Signature": []string{"foo"}, @@ -337,7 +337,7 @@ func TestInterceptor_Process_InvalidParams(t *testing.T) { } req := &triggersv1.InterceptorRequest{ - Body: json.RawMessage(`{}`), + Body: `{}`, Header: http.Header{ "Content-Type": []string{"application/json"}, }, diff --git a/pkg/interceptors/gitlab/gitlab_test.go b/pkg/interceptors/gitlab/gitlab_test.go index be7720dd2..21a7506ad 100644 --- a/pkg/interceptors/gitlab/gitlab_test.go +++ b/pkg/interceptors/gitlab/gitlab_test.go @@ -18,7 +18,6 @@ package gitlab import ( "context" - "encoding/json" "net/http" "testing" @@ -99,7 +98,7 @@ func TestInterceptor_ExecuteTrigger_ShouldContinue(t *testing.T) { logger := zaptest.NewLogger(t) kubeClient := fakekubeclient.Get(ctx) req := &triggersv1.InterceptorRequest{ - Body: tt.payload, + Body: string(tt.payload), Header: http.Header{ "Content-Type": []string{"application/json"}, }, @@ -238,7 +237,7 @@ func TestInterceptor_ExecuteTrigger_ShouldNotContinue(t *testing.T) { logger := zaptest.NewLogger(t) kubeClient := fakekubeclient.Get(ctx) req := &triggersv1.InterceptorRequest{ - Body: tt.payload, + Body: string(tt.payload), Header: http.Header{ "Content-Type": []string{"application/json"}, }, @@ -286,7 +285,7 @@ func TestInterceptor_Process_InvalidParams(t *testing.T) { } req := &triggersv1.InterceptorRequest{ - Body: json.RawMessage(`{}`), + Body: `{}`, Header: http.Header{ "Content-Type": []string{"application/json"}, }, diff --git a/pkg/interceptors/server/server_test.go b/pkg/interceptors/server/server_test.go index f9c8f83f1..0bf73384f 100644 --- a/pkg/interceptors/server/server_test.go +++ b/pkg/interceptors/server/server_test.go @@ -37,7 +37,7 @@ func TestServer_ServeHTTP(t *testing.T) { name: "valid request that should continue", path: "/cel", req: &v1alpha1.InterceptorRequest{ - Body: json.RawMessage(`{}`), + Body: `{}`, Header: map[string][]string{ "X-Event-Type": {"push"}, }, @@ -53,7 +53,7 @@ func TestServer_ServeHTTP(t *testing.T) { name: "valid request that should not continue", path: "/cel", req: &v1alpha1.InterceptorRequest{ - Body: json.RawMessage(`{}`), + Body: `{}`, Header: map[string][]string{ "X-Event-Type": {"push"}, }, diff --git a/pkg/sink/sink.go b/pkg/sink/sink.go index ab6b8353d..dd9a5d08c 100644 --- a/pkg/sink/sink.go +++ b/pkg/sink/sink.go @@ -254,7 +254,7 @@ func (r Sink) ExecuteInterceptors(t triggersv1.Trigger, in *http.Request, event // request is the request sent to the interceptors in the chain. Each interceptor can set the InterceptorParams field // or add to the Extensions request := triggersv1.InterceptorRequest{ - Body: event, + Body: string(event), Header: in.Header.Clone(), Extensions: map[string]interface{}{}, // Empty extensions for the first interceptor in chain Context: &triggersv1.TriggerContext{ @@ -266,9 +266,8 @@ func (r Sink) ExecuteInterceptors(t triggersv1.Trigger, in *http.Request, event } for _, i := range t.Spec.Interceptors { - if i.Webhook != nil { - // Old style interceptor (everything but CEL at the moment) - body, err := extendBodyWithExtensions(request.Body, request.Extensions) + if i.Webhook != nil { // Old style interceptor + body, err := extendBodyWithExtensions([]byte(request.Body), request.Extensions) if err != nil { return nil, nil, nil, fmt.Errorf("could not merge extensions with body: %w", err) } @@ -292,7 +291,6 @@ func (r Sink) ExecuteInterceptors(t triggersv1.Trigger, in *http.Request, event // Set the next request to be the output of the last response to enable // request chaining. request.Header = res.Header.Clone() - request.Body = payload continue } @@ -329,7 +327,7 @@ func (r Sink) ExecuteInterceptors(t triggersv1.Trigger, in *http.Request, event // Clear interceptorParams for the next interceptor in chain request.InterceptorParams = map[string]interface{}{} } - return request.Body, request.Header, &triggersv1.InterceptorResponse{ + return []byte(request.Body), request.Header, &triggersv1.InterceptorResponse{ Continue: true, Extensions: request.Extensions, }, nil diff --git a/pkg/sink/sink_test.go b/pkg/sink/sink_test.go index 154dfb836..cddfff0f9 100644 --- a/pkg/sink/sink_test.go +++ b/pkg/sink/sink_test.go @@ -29,6 +29,10 @@ import ( "sync" "testing" + "knative.dev/pkg/ptr" + + "github.com/tektoncd/triggers/pkg/interceptors/server" + "go.uber.org/zap" "go.uber.org/zap/zapcore" "go.uber.org/zap/zaptest" @@ -100,6 +104,21 @@ func getSinkAssets(t *testing.T, resources test.Resources, elName string, auth A dynamicClient := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme()) dynamicSet := dynamicclientset.New(tekton.WithClient(dynamicClient)) + // Setup a handler for core interceptors using httptest + coreInterceptors, err := server.NewWithCoreInterceptors(clients.Kube, logger.Sugar()) + if err != nil { + t.Fatalf("failed to initialize core interceptors: %v", err) + } + srv := httptest.NewServer(coreInterceptors) + t.Cleanup(func() { + srv.Close() + }) + httpClient := srv.Client() + u, _ := url.Parse(srv.URL) + httpClient.Transport = &http.Transport{ + Proxy: http.ProxyURL(u), + } + r := Sink{ EventListenerName: elName, EventListenerNamespace: namespace, @@ -107,6 +126,7 @@ func getSinkAssets(t *testing.T, resources test.Resources, elName string, auth A DiscoveryClient: clients.Kube.Discovery(), KubeClientSet: clients.Kube, TriggersClient: clients.Triggers, + HTTPClient: httpClient, Logger: logger.Sugar(), Auth: auth, EventListenerLister: eventlistenerinformer.Get(ctx).Lister(), @@ -529,7 +549,7 @@ func TestHandleEventWithInterceptors(t *testing.T) { Spec: triggersv1.EventListenerSpec{ Triggers: []triggersv1.EventListenerTrigger{{ Bindings: []*triggersv1.EventListenerBinding{{Ref: "tb", Kind: "TriggerBinding"}}, - Template: &triggersv1.EventListenerTemplate{Name: "tt"}, + Template: &triggersv1.EventListenerTemplate{Ref: ptr.String("tt")}, Interceptors: []*triggersv1.EventInterceptor{{ GitHub: &triggersv1.GitHubInterceptor{ SecretRef: &triggersv1.SecretRef{ @@ -578,7 +598,11 @@ func TestHandleEventWithInterceptors(t *testing.T) { // This was generated by using SHA1 and hmac from go stdlib on secret and payload. // https://play.golang.org/p/8D2E-Yz3zWf for a sample. // TODO(dibyom): Add helper method that does this instead of link above - req.Header.Add("X-Hub-Signature", "sha1=c0f3a2bbd1cdb062ba4f54b2a1cad3d171b7a129") + sigHeader, err := test.HMACHeader("secret", eventBody) + if err != nil { + t.Fatalf("failed to generate Signature header: %v", err) + } + req.Header.Add("X-Hub-Signature", sigHeader) resp, err := http.DefaultClient.Do(req) if err != nil { @@ -1018,11 +1042,7 @@ func TestExecuteInterceptor_error(t *testing.T) { } func TestExecuteInterceptor_NotContinue(t *testing.T) { - logger := zaptest.NewLogger(t) - s := Sink{ - Logger: logger.Sugar(), - } - + s, _ := getSinkAssets(t, test.Resources{}, "el-name", DefaultAuthOverride{}) trigger := triggersv1.Trigger{ Spec: triggersv1.TriggerSpec{ Interceptors: []*triggersv1.EventInterceptor{{ @@ -1038,7 +1058,7 @@ func TestExecuteInterceptor_NotContinue(t *testing.T) { URL: url, }, json.RawMessage(`{"head": "blah"}`), - logger.Sugar(), + s.Logger, "eventID") if err != nil { t.Fatalf("ExecuteInterceptor() unexpected error: %v", err) @@ -1073,6 +1093,7 @@ func (f *echoInterceptor) ServeHTTP(w http.ResponseWriter, r *http.Request) { } } +// TODO: Fix with a mux so that same HTTP client can serve both webhook and core interceptors func TestExecuteInterceptor_ExtensionChaining(t *testing.T) { echoServer := &echoInterceptor{} srv := httptest.NewServer(echoServer) @@ -1135,9 +1156,9 @@ func TestExecuteInterceptor_ExtensionChaining(t *testing.T) { "truncated_sha": "abcde", }, } - var gotBody map[string]interface{} + var gotBody interface{} if err := json.Unmarshal(resp, &gotBody); err != nil { - t.Fatalf("json.Unmarshal response body : %v\n Interceptor response is: %+v", err, iresp) + t.Fatalf("json.Unmarshal response body : %v\n Response is: %+v. \n", err, resp) } if diff := cmp.Diff(wantBody, gotBody); diff != "" { @@ -1209,19 +1230,16 @@ func TestHandleEventWithInterceptorsAndTriggerAuth(t *testing.T) { for _, testCase := range []struct { userVal string statusCode int - }{ - { - userVal: userWithoutPermissions, - statusCode: http.StatusUnauthorized, - }, - { - userVal: userWithPermissions, - statusCode: http.StatusCreated, - }, - { - userVal: userWithForbiddenAccess, - statusCode: http.StatusForbidden, - }, + }{{ + userVal: userWithoutPermissions, + statusCode: http.StatusUnauthorized, + }, { + userVal: userWithPermissions, + statusCode: http.StatusCreated, + }, { + userVal: userWithForbiddenAccess, + statusCode: http.StatusForbidden, + }, } { eventBody := json.RawMessage(`{"head_commit": {"id": "testrevision"}, "repository": {"url": "testurl"}, "foo": "bar\t\r\nbaz昨"}`) tb, tt := getResources(t, "$(body.repository.url)")