change InterceptorRequest.Body to string

Storing the body as json.RawMessagae can lead to loss of information about
spaces in the incoming body when the InterceptorRequest is marshaled as JSON
before sending it over HTTP.

This is because Go's `json.Marshal` will encode `[]byte` as a base64 string and
base64 does not preserve spaces. Adding a custom `MarshalJSON` does not help
here since `MarshalJSON` will return a `[]byte` that will then get compacted as
base64 by `json.Marshal`.

This loss of spaces can be problematic for some use cases. For instance, the
GitHub payload signature validation requires us to compare using the exact body
as it was sent. Otherwise, the validation fails. Note that this will only be a
problem when we marshal the InterceptorRequest i.e. when we move the core
interceptors out to its own server.

Using a string means that the string will be quoted e.g. `{\"foo\": \"bar\"}`.
Go should take care of unquoting it during the unmarshaling process. However,
intercetpor authors using a different language will have to manually unquote
the string by parsing it as a JSON object.

Part of tektoncd#271, 867

Signed-off-by: Dibyo Mukherjee <dibyo@google.com>

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"`
 

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())
 		}
 	}

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"},
 		},

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)

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

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())
 		}
 	}

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"},
 		},

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"},
 		},

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"},
 			},

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

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,13 +104,29 @@ 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,
 		DynamicClient:               dynamicSet,
 		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)")