From 65d6293367734ae3d13f4139f5efc38ef1bb7b92 Mon Sep 17 00:00:00 2001 From: Dibyo Mukherjee Date: Fri, 13 Nov 2020 13:08:26 -0500 Subject: [PATCH] Move all core interceptors to new interface This commit migrates the GitHub, GitLab, BitBucket interceptors to use the new InterceptorRequest/Response types. Since these interceptors do not modify the event body, this change is not a breaking change. In addition, this commit also refactors the tests for the interceptors by splitting valid and invalid scenarios into separate test cases. Part of #271 and #778 Signed-off-by: Dibyo Mukherjee --- pkg/interceptors/bitbucket/bitbucket.go | 71 +-- pkg/interceptors/bitbucket/bitbucket_test.go | 440 ++++++++++-------- pkg/interceptors/cel/cel.go | 23 +- pkg/interceptors/github/github.go | 77 ++-- pkg/interceptors/github/github_test.go | 462 +++++++++++-------- pkg/interceptors/gitlab/gitlab.go | 63 +-- pkg/interceptors/gitlab/gitlab_test.go | 407 +++++++++------- pkg/interceptors/interceptors.go | 35 ++ pkg/interceptors/interceptors_test.go | 114 ++++- pkg/sink/sink.go | 9 +- test/signature.go | 19 + test/signature_test.go | 20 + 12 files changed, 1043 insertions(+), 697 deletions(-) create mode 100644 test/signature.go create mode 100644 test/signature_test.go diff --git a/pkg/interceptors/bitbucket/bitbucket.go b/pkg/interceptors/bitbucket/bitbucket.go index 2db7d954bd3..d0925364dbb 100644 --- a/pkg/interceptors/bitbucket/bitbucket.go +++ b/pkg/interceptors/bitbucket/bitbucket.go @@ -17,12 +17,14 @@ limitations under the License. package bitbucket import ( - "bytes" - "errors" + "context" "fmt" - "io/ioutil" "net/http" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gh "github.com/google/go-github/v31/github" triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" "github.com/tektoncd/triggers/pkg/interceptors" @@ -30,65 +32,64 @@ import ( "k8s.io/client-go/kubernetes" ) +var _ triggersv1.InterceptorInterface = (*Interceptor)(nil) + type Interceptor struct { - KubeClientSet kubernetes.Interface - Logger *zap.SugaredLogger - Bitbucket *triggersv1.BitbucketInterceptor - TriggerNamespace string + KubeClientSet kubernetes.Interface + Logger *zap.SugaredLogger } -func NewInterceptor(bh *triggersv1.BitbucketInterceptor, k kubernetes.Interface, ns string, l *zap.SugaredLogger) interceptors.Interceptor { +func NewInterceptor(k kubernetes.Interface, l *zap.SugaredLogger) interceptors.Interceptor { return &Interceptor{ - Logger: l, - Bitbucket: bh, - KubeClientSet: k, - TriggerNamespace: ns, + Logger: l, + KubeClientSet: k, } } func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, error) { - payload := []byte{} - var err error + return nil, fmt.Errorf("executeTrigger() is deprecated. Call Process() instead") +} - if request.Body != nil { - defer request.Body.Close() - payload, err = ioutil.ReadAll(request.Body) - if err != nil { - return nil, fmt.Errorf("failed to read request body: %w", err) - } +func (w *Interceptor) Process(ctx context.Context, r *triggersv1.InterceptorRequest) *triggersv1.InterceptorResponse { + p := triggersv1.BitbucketInterceptor{} + if err := interceptors.UnmarshalParams(r.InterceptorParams, &p); err != nil { + return interceptors.Fail(status.Newf(codes.InvalidArgument, "failed to parse interceptor params: %v", err)) } + headers := interceptors.Canonical(r.Header) // Validate secrets first before anything else, if set - if w.Bitbucket.SecretRef != nil { - header := request.Header.Get("X-Hub-Signature") + if p.SecretRef != nil { + header := headers.Get("X-Hub-Signature") if header == "" { - return nil, errors.New("no X-Hub-Signature header set") + return interceptors.Fail(status.New(codes.InvalidArgument, "no X-Hub-Signature header set")) } - secretToken, err := interceptors.GetSecretToken(request, w.KubeClientSet, w.Bitbucket.SecretRef, w.TriggerNamespace) + ns, _ := triggersv1.ParseTriggerID(r.Context.TriggerID) + secret, err := w.KubeClientSet.CoreV1().Secrets(ns).Get(ctx, p.SecretRef.SecretName, metav1.GetOptions{}) if err != nil { - return nil, err + return interceptors.Fail(status.New(codes.Internal, fmt.Sprintf("error getting secret: %v", err))) } - if err := gh.ValidateSignature(header, payload, secretToken); err != nil { - return nil, err + secretToken := secret.Data[p.SecretRef.SecretKey] + + if err := gh.ValidateSignature(header, r.Body, secretToken); err != nil { + return interceptors.Fail(status.New(codes.FailedPrecondition, err.Error())) } } // Next see if the event type is in the allow-list - if w.Bitbucket.EventTypes != nil { - actualEvent := request.Header.Get("X-Event-Key") + if p.EventTypes != nil { + actualEvent := http.Header(r.Header).Get("X-Event-Key") isAllowed := false - for _, allowedEvent := range w.Bitbucket.EventTypes { + for _, allowedEvent := range p.EventTypes { if actualEvent == allowedEvent { isAllowed = true break } } if !isAllowed { - return nil, fmt.Errorf("event type %s is not allowed", actualEvent) + return interceptors.Fail(status.Newf(codes.FailedPrecondition, "event type %s is not allowed", actualEvent)) } } - return &http.Response{ - Header: request.Header, - Body: ioutil.NopCloser(bytes.NewBuffer(payload)), - }, nil + return &triggersv1.InterceptorResponse{ + Continue: true, + } } diff --git a/pkg/interceptors/bitbucket/bitbucket_test.go b/pkg/interceptors/bitbucket/bitbucket_test.go index 1f2ce69c3ea..6675fd5a227 100644 --- a/pkg/interceptors/bitbucket/bitbucket_test.go +++ b/pkg/interceptors/bitbucket/bitbucket_test.go @@ -17,15 +17,13 @@ limitations under the License. package bitbucket import ( - "bytes" - "io" - "io/ioutil" + "encoding/json" "net/http" "testing" "go.uber.org/zap/zaptest" - "github.com/google/go-cmp/cmp" + "github.com/tektoncd/triggers/test" triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" corev1 "k8s.io/api/core/v1" @@ -34,230 +32,298 @@ import ( rtesting "knative.dev/pkg/reconciler/testing" ) -func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) { - type args struct { - payload io.ReadCloser - secret *corev1.Secret - signature string - eventType string +func TestInterceptor_Process_ShouldContinue(t *testing.T) { + var ( + emptyJSONBody = json.RawMessage(`{}`) + secretToken = "secret" + ) + emptyBodyHMACSignature, err := test.HMACHeader(secretToken, emptyJSONBody) + if err != nil { + t.Fatalf("could not generate HMAC header: %v", err) } + tests := []struct { - name string - Bitbucket *triggersv1.BitbucketInterceptor - args args - want []byte - wantErr bool - }{ - { - name: "no secret", - Bitbucket: &triggersv1.BitbucketInterceptor{}, - args: args{ - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), - signature: "foo", + name string + interceptorParams *triggersv1.BitbucketInterceptor + payload []byte + secret *corev1.Secret + signature string + eventType string + }{{ + name: "no secret", + interceptorParams: &triggersv1.BitbucketInterceptor{}, + payload: json.RawMessage(`{}`), + signature: "foo", + }, { + name: "valid header for secret", + interceptorParams: &triggersv1.BitbucketInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - want: []byte("somepayload"), - wantErr: false, }, - { - name: "invalid header for secret", - Bitbucket: &triggersv1.BitbucketInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + signature: emptyBodyHMACSignature, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - signature: "foo", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secrettoken"), - }, - }, - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + Data: map[string][]byte{ + "token": []byte(secretToken), }, - wantErr: true, }, - { - name: "valid header for secret", - Bitbucket: &triggersv1.BitbucketInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + payload: json.RawMessage(`{}`), + }, { + name: "matching event", + interceptorParams: &triggersv1.BitbucketInterceptor{ + EventTypes: []string{"pr:opened", "repo:refs_changed"}, + }, + payload: json.RawMessage(`{}`), + eventType: "repo:refs_changed", + }, { + name: "valid header for secret and matching event", + interceptorParams: &triggersv1.BitbucketInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - // This was generated by using SHA1 and hmac from go stdlib on secret and payload. - // https://play.golang.org/p/otp1o_cJTd7 for a sample. - signature: "sha1=38e005ef7dd3faee13204505532011257023654e", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secret"), - }, + EventTypes: []string{"pr:opened", "repo:refs_changed"}, + }, + signature: emptyBodyHMACSignature, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", + }, + Data: map[string][]byte{ + "token": []byte(secretToken), + }, + }, + eventType: "repo:refs_changed", + payload: json.RawMessage(`{}`), + }, { + name: "nil body does not panic", + interceptorParams: &triggersv1.BitbucketInterceptor{}, + payload: nil, + signature: "foo", + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + logger := zaptest.NewLogger(t) + kubeClient := fakekubeclient.Get(ctx) + + w := &Interceptor{ + KubeClientSet: kubeClient, + Logger: logger.Sugar(), + } + + req := &triggersv1.InterceptorRequest{ + Body: tt.payload, + Header: http.Header{ + "Content-Type": []string{"application/json"}, + }, + InterceptorParams: map[string]interface{}{ + "eventTypes": tt.interceptorParams.EventTypes, + "secretRef": tt.interceptorParams.SecretRef, + }, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", }, - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + } + + if tt.eventType != "" { + req.Header["X-Event-Key"] = []string{tt.eventType} + } + if tt.signature != "" { + req.Header["X-Hub-Signature"] = []string{tt.signature} + } + if tt.secret != nil { + if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(ctx, tt.secret, metav1.CreateOptions{}); err != nil { + t.Error(err) + } + } + res := w.Process(ctx, req) + if !res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be true but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) + } + }) + } +} + +func TestInterceptor_Process_ShouldNotContinue(t *testing.T) { + var ( + emptyJSONBody = json.RawMessage(`{}`) + secretToken = "secret" + ) + emptyBodyHMACSignature, err := test.HMACHeader(secretToken, emptyJSONBody) + if err != nil { + t.Fatalf("could not generate HMAC header: %v", err) + } + + tests := []struct { + name string + interceptorParams *triggersv1.BitbucketInterceptor + payload []byte + secret *corev1.Secret + signature string + eventType string + }{{ + name: "invalid header for secret", + interceptorParams: &triggersv1.BitbucketInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - wantErr: false, - want: []byte("somepayload"), }, - { - name: "matching event", - Bitbucket: &triggersv1.BitbucketInterceptor{ - EventTypes: []string{"pr:opened", "repo:refs_changed"}, + signature: "foo", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), - eventType: "repo:refs_changed", + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - wantErr: false, - want: []byte("somepayload"), }, - { - name: "no matching event", - Bitbucket: &triggersv1.BitbucketInterceptor{ - EventTypes: []string{"pr:opened", "repo:refs_changed"}, + payload: json.RawMessage(`{}`), + }, { + name: "no X-Hub-Signature header for secret", + interceptorParams: &triggersv1.BitbucketInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), - eventType: "event", - }, - wantErr: true, }, - { - name: "valid header for secret and matching event", - Bitbucket: &triggersv1.BitbucketInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, - EventTypes: []string{"pr:opened", "repo:refs_changed"}, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - // This was generated by using SHA1 and hmac from go stdlib on secret and payload. - // https://play.golang.org/p/otp1o_cJTd7 for a sample. - signature: "sha1=38e005ef7dd3faee13204505532011257023654e", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secret"), - }, - }, - eventType: "repo:refs_changed", - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - wantErr: false, - want: []byte("somepayload"), }, - { - name: "valid header for secret, but no matching event", - Bitbucket: &triggersv1.BitbucketInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, - EventTypes: []string{"pr:opened", "repo:refs_changed"}, + payload: json.RawMessage(`{}`), + }, { + name: "no matching event", + interceptorParams: &triggersv1.BitbucketInterceptor{ + EventTypes: []string{"pr:opened", "repo:refs_changed"}, + }, + payload: json.RawMessage(`{}`), + eventType: "event", + }, { + name: "invalid header for secret, but matching event", + interceptorParams: &triggersv1.BitbucketInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - // This was generated by using SHA1 and hmac from go stdlib on secret and payload. - // https://play.golang.org/p/otp1o_cJTd7 for a sample. - signature: "sha1=38e005ef7dd3faee13204505532011257023654e", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secret"), - }, - }, - eventType: "event", - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + EventTypes: []string{"pr:opened", "repo:refs_changed"}, + }, + signature: "foo", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", + }, + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - wantErr: true, }, - { - name: "invalid header for secret, but matching event", - Bitbucket: &triggersv1.BitbucketInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, - EventTypes: []string{"pr:opened", "repo:refs_changed"}, + eventType: "pr:opened", + payload: json.RawMessage(`{}`), + }, { + name: "valid header for secret, but no matching event", + interceptorParams: &triggersv1.BitbucketInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - signature: "foo", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secrettoken"), - }, - }, - eventType: "pr:opened", - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + EventTypes: []string{"pr:opened", "repo:refs_changed"}, + }, + signature: emptyBodyHMACSignature, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - wantErr: true, - }, { - name: "nil body does not panic", - Bitbucket: &triggersv1.BitbucketInterceptor{}, - args: args{ - payload: nil, - signature: "foo", + Data: map[string][]byte{ + "token": []byte(secretToken), }, - want: []byte{}, - wantErr: false, }, - } + eventType: "event", + payload: json.RawMessage(`{}`), + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) logger := zaptest.NewLogger(t) kubeClient := fakekubeclient.Get(ctx) - request := &http.Request{ - Body: tt.args.payload, + + w := &Interceptor{ + KubeClientSet: kubeClient, + Logger: logger.Sugar(), + } + + req := &triggersv1.InterceptorRequest{ + Body: tt.payload, Header: http.Header{ "Content-Type": []string{"application/json"}, }, + InterceptorParams: map[string]interface{}{ + "eventTypes": tt.interceptorParams.EventTypes, + "secretRef": tt.interceptorParams.SecretRef, + }, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", + }, } - if tt.args.eventType != "" { - request.Header.Add("X-Event-Key", tt.args.eventType) + + if tt.eventType != "" { + req.Header["X-Event-Key"] = []string{tt.eventType} } - if tt.args.signature != "" { - request.Header.Add("X-Hub-Signature", tt.args.signature) + if tt.signature != "" { + req.Header["X-Hub-Signature"] = []string{tt.signature} } - if tt.args.secret != nil { - if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(ctx, tt.args.secret, metav1.CreateOptions{}); err != nil { + if tt.secret != nil { + if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(ctx, tt.secret, metav1.CreateOptions{}); err != nil { t.Error(err) } } - w := &Interceptor{ - KubeClientSet: kubeClient, - Bitbucket: tt.Bitbucket, - Logger: logger.Sugar(), - TriggerNamespace: metav1.NamespaceDefault, - } - resp, err := w.ExecuteTrigger(request) - if err != nil { - if !tt.wantErr { - t.Errorf("Interceptor.ExecuteTrigger() error = %v, wantErr %v", err, tt.wantErr) - } - return - } - - got, err := ioutil.ReadAll(resp.Body) - if err != nil { - t.Fatalf("error reading response body %v", err) - } - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("Interceptor.ExecuteTrigger (-want, +got) = %s", diff) + res := w.Process(ctx, req) + if res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be false but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) } }) } } + +func TestInterceptor_Process_InvalidParams(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + logger := zaptest.NewLogger(t) + kubeClient := fakekubeclient.Get(ctx) + + w := &Interceptor{ + KubeClientSet: kubeClient, + Logger: logger.Sugar(), + } + + req := &triggersv1.InterceptorRequest{ + Body: json.RawMessage(`{}`), + Header: http.Header{ + "Content-Type": []string{"application/json"}, + }, + InterceptorParams: map[string]interface{}{ + "blah": func() {}, + }, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", + }, + } + + res := w.Process(ctx, req) + if res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be false but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) + } +} diff --git a/pkg/interceptors/cel/cel.go b/pkg/interceptors/cel/cel.go index 0ae2306d280..26243d646f9 100644 --- a/pkg/interceptors/cel/cel.go +++ b/pkg/interceptors/cel/cel.go @@ -23,6 +23,8 @@ import ( "net/http" "reflect" + "github.com/tektoncd/triggers/pkg/interceptors" + "google.golang.org/grpc/codes" "google.golang.org/grpc/status" @@ -60,8 +62,6 @@ var ( mapType = reflect.TypeOf(&structpb.Struct{}) ) -type params = triggersv1.CELInterceptor - // NewInterceptor creates a prepopulated Interceptor. func NewInterceptor(k kubernetes.Interface, l *zap.SugaredLogger) *Interceptor { return &Interceptor{ @@ -124,23 +124,12 @@ func makeEvalContext(body []byte, h http.Header, url string) (map[string]interfa } func (w *Interceptor) Process(ctx context.Context, r *triggersv1.InterceptorRequest) *triggersv1.InterceptorResponse { - b, err := json.Marshal(r.InterceptorParams) - if err != nil { - return &triggersv1.InterceptorResponse{ - Continue: false, - Status: status.Newf(codes.InvalidArgument, "failed to marshal json: %v", err), - } + p := triggersv1.CELInterceptor{} + if err := interceptors.UnmarshalParams(r.InterceptorParams, &p); err != nil { + return interceptors.Fail(status.Newf(codes.InvalidArgument, "failed to parse interceptor params: %v", err)) } - p := params{} - if err := json.Unmarshal(b, &p); err != nil { - // Should never happen since Unmarshal only returns err if json is invalid which we already check above - return &triggersv1.InterceptorResponse{ - Continue: false, - Status: status.Newf(codes.InvalidArgument, "invalid json: %v", err), - } - } - ns, _ := triggersv1.ParseTriggerID(r.Context.TriggerID) + ns, _ := triggersv1.ParseTriggerID(r.Context.TriggerID) env, err := makeCelEnv(ns, w.KubeClientSet) if err != nil { return &triggersv1.InterceptorResponse{ diff --git a/pkg/interceptors/github/github.go b/pkg/interceptors/github/github.go index cee14380a90..9a3ea901b48 100644 --- a/pkg/interceptors/github/github.go +++ b/pkg/interceptors/github/github.go @@ -17,12 +17,15 @@ limitations under the License. package github import ( - "bytes" + "context" "errors" "fmt" - "io/ioutil" "net/http" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + gh "github.com/google/go-github/v31/github" triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" "github.com/tektoncd/triggers/pkg/interceptors" @@ -30,72 +33,72 @@ import ( "k8s.io/client-go/kubernetes" ) +var _ triggersv1.InterceptorInterface = (*Interceptor)(nil) + // ErrInvalidContentType is returned when the content-type is not a JSON body. var ErrInvalidContentType = errors.New("form parameter encoding not supported, please change the hook to send JSON payloads") type Interceptor struct { - KubeClientSet kubernetes.Interface - Logger *zap.SugaredLogger - GitHub *triggersv1.GitHubInterceptor - TriggerNamespace string + KubeClientSet kubernetes.Interface + Logger *zap.SugaredLogger } -func NewInterceptor(gh *triggersv1.GitHubInterceptor, k kubernetes.Interface, ns string, l *zap.SugaredLogger) interceptors.Interceptor { +func NewInterceptor(k kubernetes.Interface, l *zap.SugaredLogger) interceptors.Interceptor { return &Interceptor{ - Logger: l, - GitHub: gh, - KubeClientSet: k, - TriggerNamespace: ns, + Logger: l, + KubeClientSet: k, } } func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, error) { - payload := []byte{} - var err error - if v := request.Header.Get("Content-Type"); v == "application/x-www-form-urlencoded" { - return nil, ErrInvalidContentType - } + return nil, fmt.Errorf("executeTrigger() is deprecated. Call Process() instead") +} - if request.Body != nil { - defer request.Body.Close() - payload, err = ioutil.ReadAll(request.Body) - if err != nil { - return nil, fmt.Errorf("failed to read request body: %w", err) - } +func (w *Interceptor) Process(ctx context.Context, r *triggersv1.InterceptorRequest) *triggersv1.InterceptorResponse { + headers := interceptors.Canonical(r.Header) + if v := headers.Get("Content-Type"); v == "application/x-www-form-urlencoded" { + return interceptors.Fail(status.New(codes.InvalidArgument, ErrInvalidContentType.Error())) } + p := triggersv1.GitHubInterceptor{} + if err := interceptors.UnmarshalParams(r.InterceptorParams, &p); err != nil { + return interceptors.Fail(status.Newf(codes.InvalidArgument, "failed to parse interceptor params: %v", err)) + } // Validate secrets first before anything else, if set - if w.GitHub.SecretRef != nil { - header := request.Header.Get("X-Hub-Signature") + if p.SecretRef != nil { + header := headers.Get("X-Hub-Signature") if header == "" { - return nil, errors.New("no X-Hub-Signature header set") + return interceptors.Fail(status.New(codes.FailedPrecondition, "no X-Hub-Signature header set")) } - secretToken, err := interceptors.GetSecretToken(request, w.KubeClientSet, w.GitHub.SecretRef, w.TriggerNamespace) + + ns, _ := triggersv1.ParseTriggerID(r.Context.TriggerID) + secret, err := w.KubeClientSet.CoreV1().Secrets(ns).Get(ctx, p.SecretRef.SecretName, metav1.GetOptions{}) if err != nil { - return nil, err + return interceptors.Fail(status.Newf(codes.FailedPrecondition, "error getting secret: %v", err)) } - if err := gh.ValidateSignature(header, payload, secretToken); err != nil { - return nil, err + secretToken := secret.Data[p.SecretRef.SecretKey] + + if err := gh.ValidateSignature(header, r.Body, secretToken); err != nil { + return interceptors.Fail(status.New(codes.FailedPrecondition, err.Error())) } } // Next see if the event type is in the allow-list - if w.GitHub.EventTypes != nil { - actualEvent := request.Header.Get("X-GitHub-Event") + if p.EventTypes != nil { + actualEvent := headers.Get("X-GitHub-Event") isAllowed := false - for _, allowedEvent := range w.GitHub.EventTypes { + for _, allowedEvent := range p.EventTypes { if actualEvent == allowedEvent { isAllowed = true break } } if !isAllowed { - return nil, fmt.Errorf("event type %s is not allowed", actualEvent) + return interceptors.Fail(status.Newf(codes.FailedPrecondition, "event type %s is not allowed", actualEvent)) } } - return &http.Response{ - Header: request.Header, - Body: ioutil.NopCloser(bytes.NewBuffer(payload)), - }, nil + return &triggersv1.InterceptorResponse{ + Continue: true, + } } diff --git a/pkg/interceptors/github/github_test.go b/pkg/interceptors/github/github_test.go index 025100575b3..e220812d407 100644 --- a/pkg/interceptors/github/github_test.go +++ b/pkg/interceptors/github/github_test.go @@ -17,17 +17,13 @@ limitations under the License. package github import ( - "bytes" - "io" - "io/ioutil" + "encoding/json" "net/http" "testing" - "go.uber.org/zap/zaptest" - - "github.com/google/go-cmp/cmp" - triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" + "github.com/tektoncd/triggers/test" + "go.uber.org/zap/zaptest" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" fakekubeclient "knative.dev/pkg/client/injection/kube/client/fake" @@ -35,228 +31,266 @@ import ( ) func TestInterceptor_ExecuteTrigger_Signature(t *testing.T) { - type args struct { - payload io.ReadCloser - secret *corev1.Secret - signature string - eventType string + var ( + emptyJSONBody = json.RawMessage(`{}`) + secretToken = "secret" + ) + emptyBodyHMACSignature, err := test.HMACHeader(secretToken, emptyJSONBody) + if err != nil { + t.Fatalf("could not generate HMAC header: %v", err) } tests := []struct { - name string - GitHub *triggersv1.GitHubInterceptor - args args - want []byte - wantErr bool - }{ - { - name: "no secret", - GitHub: &triggersv1.GitHubInterceptor{}, - args: args{ - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), - signature: "foo", + name string + interceptorParams *triggersv1.GitHubInterceptor + payload []byte + secret *corev1.Secret + signature string + eventType string + }{{ + name: "no secret", + interceptorParams: &triggersv1.GitHubInterceptor{}, + payload: emptyJSONBody, + signature: "foo", + }, { + name: "valid header for secret", + interceptorParams: &triggersv1.GitHubInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - want: []byte("somepayload"), - wantErr: false, }, - { - name: "invalid header for secret", - GitHub: &triggersv1.GitHubInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + // signature is based on the secret token below and the payload + signature: emptyBodyHMACSignature, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - signature: "foo", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secrettoken"), - }, - }, - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + Data: map[string][]byte{ + "token": []byte(secretToken), }, - wantErr: true, }, - { - name: "valid header for secret", - GitHub: &triggersv1.GitHubInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + payload: emptyJSONBody, + }, { + name: "no secret, matching event", + interceptorParams: &triggersv1.GitHubInterceptor{ + EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + }, + + payload: emptyJSONBody, + eventType: "YOUR_EVENT", + }, { + name: "valid header for secret and matching event", + interceptorParams: &triggersv1.GitHubInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", + }, + EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + }, + + // signature is based on the secret token below and the payload + signature: emptyBodyHMACSignature, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", + }, + Data: map[string][]byte{ + "token": []byte("secret"), }, - args: args{ - // This was generated by using SHA1 and hmac from go stdlib on secret and payload. - // https://play.golang.org/p/otp1o_cJTd7 for a sample. - signature: "sha1=38e005ef7dd3faee13204505532011257023654e", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secret"), - }, + }, + eventType: "MY_EVENT", + payload: emptyJSONBody, + }, { + name: "nil body does not panic", + interceptorParams: &triggersv1.GitHubInterceptor{}, + payload: nil, + signature: "foo", + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + logger := zaptest.NewLogger(t) + kubeClient := fakekubeclient.Get(ctx) + + req := &triggersv1.InterceptorRequest{ + Body: tt.payload, + Header: http.Header{ + "Content-Type": []string{"application/json"}, + }, + InterceptorParams: map[string]interface{}{ + "eventTypes": tt.interceptorParams.EventTypes, + "secretRef": tt.interceptorParams.SecretRef, }, - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", + }, + } + if tt.eventType != "" { + req.Header["X-GITHUB-EVENT"] = []string{tt.eventType} + } + if tt.signature != "" { + req.Header["X-Hub-Signature"] = []string{tt.signature} + } + if tt.secret != nil { + if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(ctx, tt.secret, metav1.CreateOptions{}); err != nil { + t.Error(err) + } + } + w := &Interceptor{ + KubeClientSet: kubeClient, + Logger: logger.Sugar(), + } + res := w.Process(ctx, req) + + if !res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be true but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) + } + }) + } +} + +func TestInterceptor_ExecuteTrigger_ShouldNotContinue(t *testing.T) { + var ( + emptyJSONBody = json.RawMessage(`{}`) + secretToken = "secret" + ) + emptyBodyHMACSignature, err := test.HMACHeader(secretToken, emptyJSONBody) + if err != nil { + t.Fatalf("could not generate HMAC header: %v", err) + } + tests := []struct { + name string + interceptorParams *triggersv1.GitHubInterceptor + payload []byte + secret *corev1.Secret + signature string + eventType string + }{{ + name: "invalid signature header", + interceptorParams: &triggersv1.GitHubInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - wantErr: false, - want: []byte("somepayload"), }, - { - name: "no secret, matching event", - GitHub: &triggersv1.GitHubInterceptor{ - EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + signature: "foo", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), - eventType: "YOUR_EVENT", + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - wantErr: false, - want: []byte("somepayload"), }, - { - name: "no secret, failing event", - GitHub: &triggersv1.GitHubInterceptor{ - EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, - }, - args: args{ - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), - eventType: "OTHER_EVENT", + payload: emptyJSONBody, + }, { + name: "missing signature header", + interceptorParams: &triggersv1.GitHubInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - wantErr: true, }, - { - name: "valid header for secret and matching event", - GitHub: &triggersv1.GitHubInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, - EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - // This was generated by using SHA1 and hmac from go stdlib on secret and payload. - // https://play.golang.org/p/otp1o_cJTd7 for a sample. - signature: "sha1=38e005ef7dd3faee13204505532011257023654e", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secret"), - }, - }, - eventType: "MY_EVENT", - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - wantErr: false, - want: []byte("somepayload"), }, - { - name: "valid header for secret, failing event", - GitHub: &triggersv1.GitHubInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, - EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + payload: emptyJSONBody, + }, { + name: "no secret, failing event", + interceptorParams: &triggersv1.GitHubInterceptor{ + EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + }, + + payload: emptyJSONBody, + eventType: "OTHER_EVENT", + }, { + name: "valid header for secret, failing event", + interceptorParams: &triggersv1.GitHubInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - // This was generated by using SHA1 and hmac from go stdlib on secret and payload. - // https://play.golang.org/p/otp1o_cJTd7 for a sample. - signature: "sha1=38e005ef7dd3faee13204505532011257023654e", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secret"), - }, - }, - eventType: "OTHER_EVENT", - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + }, + signature: emptyBodyHMACSignature, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", + }, + Data: map[string][]byte{ + "token": []byte("secret"), }, - wantErr: true, }, - { - name: "invalid header for secret, matching event", - GitHub: &triggersv1.GitHubInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, - EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + eventType: "OTHER_EVENT", + payload: emptyJSONBody, + }, { + name: "invalid header for secret, matching event", + interceptorParams: &triggersv1.GitHubInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - signature: "foo", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secrettoken"), - }, - }, - eventType: "MY_EVENT", - payload: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + EventTypes: []string{"MY_EVENT", "YOUR_EVENT"}, + }, + signature: "foo", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - wantErr: true, - }, { - name: "nil body does not panic", - GitHub: &triggersv1.GitHubInterceptor{}, - args: args{ - payload: nil, - signature: "foo", + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - want: []byte{}, - wantErr: false, }, - } + eventType: "MY_EVENT", + payload: emptyJSONBody, + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) logger := zaptest.NewLogger(t) kubeClient := fakekubeclient.Get(ctx) - request := &http.Request{ - Body: tt.args.payload, + + req := &triggersv1.InterceptorRequest{ + Body: tt.payload, Header: http.Header{ "Content-Type": []string{"application/json"}, }, + InterceptorParams: map[string]interface{}{ + "eventTypes": tt.interceptorParams.EventTypes, + "secretRef": tt.interceptorParams.SecretRef, + }, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", + }, } - if tt.args.eventType != "" { - request.Header.Add("X-GITHUB-EVENT", tt.args.eventType) + if tt.eventType != "" { + req.Header["X-GITHUB-EVENT"] = []string{tt.eventType} } - if tt.args.signature != "" { - request.Header.Add("X-Hub-Signature", tt.args.signature) + if tt.signature != "" { + req.Header["X-Hub-Signature"] = []string{tt.signature} } - if tt.args.secret != nil { - if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(ctx, tt.args.secret, metav1.CreateOptions{}); err != nil { + if tt.secret != nil { + if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(ctx, tt.secret, metav1.CreateOptions{}); err != nil { t.Error(err) } } w := &Interceptor{ - KubeClientSet: kubeClient, - GitHub: tt.GitHub, - Logger: logger.Sugar(), - TriggerNamespace: metav1.NamespaceDefault, - } - resp, err := w.ExecuteTrigger(request) - if err != nil { - if !tt.wantErr { - t.Errorf("Interceptor.ExecuteTrigger() error = %v, wantErr %v", err, tt.wantErr) - } - return + KubeClientSet: kubeClient, + Logger: logger.Sugar(), } + res := w.Process(ctx, req) - got, err := ioutil.ReadAll(resp.Body) - if err != nil { - t.Fatalf("error reading response body %v", err) - } - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("Interceptor.ExecuteTrigger (-want, +got) = %s", diff) + if res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be false but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) } }) } @@ -266,21 +300,59 @@ func TestInterceptor_ExecuteTrigger_with_invalid_content_type(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) logger := zaptest.NewLogger(t) kubeClient := fakekubeclient.Get(ctx) - request := &http.Request{ - Body: ioutil.NopCloser(bytes.NewBufferString("somepayload")), + req := &triggersv1.InterceptorRequest{ + Body: json.RawMessage(`{}`), Header: http.Header{ "Content-Type": []string{"application/x-www-form-urlencoded"}, "X-Hub-Signature": []string{"foo"}, }, + InterceptorParams: map[string]interface{}{}, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", + }, } w := &Interceptor{ - KubeClientSet: kubeClient, - GitHub: &triggersv1.GitHubInterceptor{}, - Logger: logger.Sugar(), - TriggerNamespace: metav1.NamespaceDefault, + KubeClientSet: kubeClient, + Logger: logger.Sugar(), + } + res := w.Process(ctx, req) + if res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be : %t but got %t.\n Status.Err(): %v", false, res.Continue, res.Status.Err()) } - _, err := w.ExecuteTrigger(request) - if err != ErrInvalidContentType { - t.Fatalf("got error %v, want %v", err, ErrInvalidContentType) + if res.Status.Message() != ErrInvalidContentType.Error() { + t.Fatalf("got error %v, want %v", res.Status.Err(), ErrInvalidContentType) + } +} + +func TestInterceptor_Process_InvalidParams(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + logger := zaptest.NewLogger(t) + kubeClient := fakekubeclient.Get(ctx) + + w := &Interceptor{ + KubeClientSet: kubeClient, + Logger: logger.Sugar(), + } + + req := &triggersv1.InterceptorRequest{ + Body: json.RawMessage(`{}`), + Header: http.Header{ + "Content-Type": []string{"application/json"}, + }, + InterceptorParams: map[string]interface{}{ + "blah": func() {}, + }, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", + }, + } + + res := w.Process(ctx, req) + if res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be false but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) } } diff --git a/pkg/interceptors/gitlab/gitlab.go b/pkg/interceptors/gitlab/gitlab.go index ae5c832c195..236069e340b 100644 --- a/pkg/interceptors/gitlab/gitlab.go +++ b/pkg/interceptors/gitlab/gitlab.go @@ -17,11 +17,15 @@ limitations under the License. package gitlab import ( + "context" "crypto/subtle" - "errors" "fmt" "net/http" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/tektoncd/triggers/pkg/interceptors" triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" @@ -30,56 +34,63 @@ import ( "k8s.io/client-go/kubernetes" ) +var _ triggersv1.InterceptorInterface = (*Interceptor)(nil) + type Interceptor struct { - KubeClientSet kubernetes.Interface - Logger *zap.SugaredLogger - GitLab *triggersv1.GitLabInterceptor - TriggerNamespace string + KubeClientSet kubernetes.Interface + Logger *zap.SugaredLogger } -func NewInterceptor(gl *triggersv1.GitLabInterceptor, k kubernetes.Interface, ns string, l *zap.SugaredLogger) interceptors.Interceptor { +func NewInterceptor(k kubernetes.Interface, l *zap.SugaredLogger) interceptors.Interceptor { return &Interceptor{ - Logger: l, - GitLab: gl, - KubeClientSet: k, - TriggerNamespace: ns, + Logger: l, + KubeClientSet: k, } } func (w *Interceptor) ExecuteTrigger(request *http.Request) (*http.Response, error) { - // Validate the secret first, if set. - if w.GitLab.SecretRef != nil { - header := request.Header.Get("X-GitLab-Token") + return nil, fmt.Errorf("executeTrigger() is deprecated. Call Process() instead") +} + +func (w *Interceptor) Process(ctx context.Context, r *triggersv1.InterceptorRequest) *triggersv1.InterceptorResponse { + p := triggersv1.GitLabInterceptor{} + if err := interceptors.UnmarshalParams(r.InterceptorParams, &p); err != nil { + return interceptors.Fail(status.Newf(codes.InvalidArgument, "failed to parse interceptor params: %v", err)) + } + + headers := interceptors.Canonical(r.Header) + if p.SecretRef != nil { + header := headers.Get("X-GitLab-Token") if header == "" { - return nil, errors.New("no X-GitLab-Token header set") + return interceptors.Fail(status.New(codes.InvalidArgument, "no X-GitLab-Token header set")) } - secretToken, err := interceptors.GetSecretToken(request, w.KubeClientSet, w.GitLab.SecretRef, w.TriggerNamespace) + ns, _ := triggersv1.ParseTriggerID(r.Context.TriggerID) + secret, err := w.KubeClientSet.CoreV1().Secrets(ns).Get(ctx, p.SecretRef.SecretName, metav1.GetOptions{}) if err != nil { - return nil, err + return interceptors.Fail(status.New(codes.Internal, fmt.Sprintf("error getting secret: %v", err))) } + secretToken := secret.Data[p.SecretRef.SecretKey] // Make sure to use a constant time comparison here. if subtle.ConstantTimeCompare([]byte(header), secretToken) == 0 { - return nil, errors.New("Invalid X-GitLab-Token") + return interceptors.Fail(status.New(codes.InvalidArgument, "Invalid X-GitLab-Token")) } } - if w.GitLab.EventTypes != nil { - actualEvent := request.Header.Get("X-GitLab-Event") + if p.EventTypes != nil { + actualEvent := headers.Get("X-GitLab-Event") isAllowed := false - for _, allowedEvent := range w.GitLab.EventTypes { + for _, allowedEvent := range p.EventTypes { if actualEvent == allowedEvent { isAllowed = true break } } if !isAllowed { - return nil, fmt.Errorf("event type %s is not allowed", actualEvent) + return interceptors.Fail(status.Newf(codes.FailedPrecondition, "event type %s is not allowed", actualEvent)) } } - - return &http.Response{ - Header: request.Header, - Body: request.Body, - }, nil + return &triggersv1.InterceptorResponse{ + Continue: true, + } } diff --git a/pkg/interceptors/gitlab/gitlab_test.go b/pkg/interceptors/gitlab/gitlab_test.go index 06bc9083682..be7720dd2cc 100644 --- a/pkg/interceptors/gitlab/gitlab_test.go +++ b/pkg/interceptors/gitlab/gitlab_test.go @@ -17,16 +17,13 @@ limitations under the License. package gitlab import ( - "bytes" "context" - "io/ioutil" + "encoding/json" "net/http" "testing" "go.uber.org/zap/zaptest" - "github.com/google/go-cmp/cmp" - triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -34,215 +31,277 @@ import ( rtesting "knative.dev/pkg/reconciler/testing" ) -func TestInterceptor_ExecuteTrigger(t *testing.T) { - type args struct { - payload []byte - secret *corev1.Secret - token string - eventType string - } +func TestInterceptor_ExecuteTrigger_ShouldContinue(t *testing.T) { tests := []struct { - name string - GitLab *triggersv1.GitLabInterceptor - args args - want []byte - wantErr bool - }{ - { - name: "no secret", - GitLab: &triggersv1.GitLabInterceptor{}, - args: args{ - payload: []byte("somepayload"), - token: "foo", - }, - want: []byte("somepayload"), - wantErr: false, - }, - { - name: "invalid header for secret", - GitLab: &triggersv1.GitLabInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + name string + interceptorParams *triggersv1.GitLabInterceptor + payload []byte + secret *corev1.Secret + token string + eventType string + }{{ + name: "no secret", + interceptorParams: &triggersv1.GitLabInterceptor{}, + + payload: []byte("somepayload"), + token: "foo", + }, { + name: "valid header for secret", + interceptorParams: &triggersv1.GitLabInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - token: "foo", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secrettoken"), - }, - }, - payload: []byte("somepayload"), + }, + + token: "secret", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", + }, + Data: map[string][]byte{ + "token": []byte("secret"), }, - wantErr: true, }, - { - name: "valid header for secret", - GitLab: &triggersv1.GitLabInterceptor{ - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + payload: []byte("somepayload"), + }, { + name: "valid event", + interceptorParams: &triggersv1.GitLabInterceptor{ + EventTypes: []string{"foo", "bar"}, + }, + + eventType: "foo", + payload: []byte("somepayload"), + }, { + name: "valid event, valid secret", + interceptorParams: &triggersv1.GitLabInterceptor{ + EventTypes: []string{"foo", "bar"}, + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", + }, + }, + eventType: "bar", + payload: []byte("somepayload"), + token: "secrettoken", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - token: "secret", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secret"), - }, + Data: map[string][]byte{ + "token": []byte("secrettoken"), + }, + }, + }} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + logger := zaptest.NewLogger(t) + kubeClient := fakekubeclient.Get(ctx) + req := &triggersv1.InterceptorRequest{ + Body: tt.payload, + Header: http.Header{ + "Content-Type": []string{"application/json"}, + }, + InterceptorParams: map[string]interface{}{ + "eventTypes": tt.interceptorParams.EventTypes, + "secretRef": tt.interceptorParams.SecretRef, + }, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", }, - payload: []byte("somepayload"), + } + if tt.token != "" { + req.Header["X-GitLab-Token"] = []string{tt.token} + } + if tt.eventType != "" { + req.Header["X-GitLab-Event"] = []string{tt.eventType} + } + if tt.secret != nil { + if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.Background(), tt.secret, metav1.CreateOptions{}); err != nil { + t.Error(err) + } + } + w := &Interceptor{ + KubeClientSet: kubeClient, + Logger: logger.Sugar(), + } + res := w.Process(ctx, req) + if !res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be : true but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) + } + + }) + } +} + +func TestInterceptor_ExecuteTrigger_ShouldNotContinue(t *testing.T) { + tests := []struct { + name string + interceptorParams *triggersv1.GitLabInterceptor + payload []byte + secret *corev1.Secret + token string + eventType string + }{{ + name: "invalid header for secret", + interceptorParams: &triggersv1.GitLabInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - wantErr: false, - want: []byte("somepayload"), }, - { - name: "valid event", - GitLab: &triggersv1.GitLabInterceptor{ - EventTypes: []string{"foo", "bar"}, + + token: "foo", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - eventType: "foo", - payload: []byte("somepayload"), + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - wantErr: false, - want: []byte("somepayload"), }, - { - name: "invalid event", - GitLab: &triggersv1.GitLabInterceptor{ - EventTypes: []string{"foo", "bar"}, + payload: []byte("somepayload"), + }, { + name: "missing header for secret", + interceptorParams: &triggersv1.GitLabInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - eventType: "baz", - payload: []byte("somepayload"), - }, - wantErr: true, }, - { - name: "valid event, invalid secret", - GitLab: &triggersv1.GitLabInterceptor{ - EventTypes: []string{"foo", "bar"}, - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", }, - args: args{ - eventType: "bar", - payload: []byte("somepayload"), - token: "foo", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secrettoken"), - }, - }, + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - wantErr: true, }, - { - name: "invalid event, valid secret", - GitLab: &triggersv1.GitLabInterceptor{ - EventTypes: []string{"foo", "bar"}, - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + payload: []byte("somepayload"), + }, { + name: "invalid event", + interceptorParams: &triggersv1.GitLabInterceptor{ + EventTypes: []string{"foo", "bar"}, + }, + + eventType: "baz", + payload: []byte("somepayload"), + }, { + name: "valid event, invalid secret", + interceptorParams: &triggersv1.GitLabInterceptor{ + EventTypes: []string{"foo", "bar"}, + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - eventType: "baz", - payload: []byte("somepayload"), - token: "secrettoken", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secrettoken"), - }, - }, + }, + + eventType: "bar", + payload: []byte("somepayload"), + token: "foo", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", + }, + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - wantErr: true, }, - { - name: "valid event, valid secret", - GitLab: &triggersv1.GitLabInterceptor{ - EventTypes: []string{"foo", "bar"}, - SecretRef: &triggersv1.SecretRef{ - SecretName: "mysecret", - SecretKey: "token", - }, + }, { + name: "invalid event, valid secret", + interceptorParams: &triggersv1.GitLabInterceptor{ + EventTypes: []string{"foo", "bar"}, + SecretRef: &triggersv1.SecretRef{ + SecretName: "mysecret", + SecretKey: "token", }, - args: args{ - eventType: "bar", - payload: []byte("somepayload"), - token: "secrettoken", - secret: &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: "mysecret", - }, - Data: map[string][]byte{ - "token": []byte("secrettoken"), - }, - }, + }, + + eventType: "baz", + payload: []byte("somepayload"), + token: "secrettoken", + secret: &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "mysecret", + }, + Data: map[string][]byte{ + "token": []byte("secrettoken"), }, - want: []byte("somepayload"), }, - } + }} for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) logger := zaptest.NewLogger(t) kubeClient := fakekubeclient.Get(ctx) - request := &http.Request{ - Body: ioutil.NopCloser(bytes.NewReader(tt.args.payload)), + req := &triggersv1.InterceptorRequest{ + Body: tt.payload, Header: http.Header{ "Content-Type": []string{"application/json"}, }, + InterceptorParams: map[string]interface{}{ + "eventTypes": tt.interceptorParams.EventTypes, + "secretRef": tt.interceptorParams.SecretRef, + }, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", + }, } - if tt.args.token != "" { - request.Header.Add("X-GitLab-Token", tt.args.token) + if tt.token != "" { + req.Header["X-GitLab-Token"] = []string{tt.token} } - if tt.args.eventType != "" { - request.Header.Add("X-GitLab-Event", tt.args.eventType) + if tt.eventType != "" { + req.Header["X-interceptorParams-Event"] = []string{tt.eventType} } - if tt.args.secret != nil { - if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.Background(), tt.args.secret, metav1.CreateOptions{}); err != nil { + if tt.secret != nil { + if _, err := kubeClient.CoreV1().Secrets(metav1.NamespaceDefault).Create(context.Background(), tt.secret, metav1.CreateOptions{}); err != nil { t.Error(err) } } w := &Interceptor{ - KubeClientSet: kubeClient, - GitLab: tt.GitLab, - Logger: logger.Sugar(), - TriggerNamespace: metav1.NamespaceDefault, + KubeClientSet: kubeClient, + Logger: logger.Sugar(), } - resp, err := w.ExecuteTrigger(request) - if err != nil { - if !tt.wantErr { - t.Errorf("Interceptor.ExecuteTrigger() error = %v, wantErr %v", err, tt.wantErr) - } - return - } - - got, err := ioutil.ReadAll(resp.Body) - if err != nil { - t.Fatalf("error reading response: %v", err) - } - defer resp.Body.Close() - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("Interceptor.ExecuteTrigger (-want, +got) = %s", diff) + res := w.Process(ctx, req) + if res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be false but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) } }) } } + +func TestInterceptor_Process_InvalidParams(t *testing.T) { + ctx, _ := rtesting.SetupFakeContext(t) + logger := zaptest.NewLogger(t) + kubeClient := fakekubeclient.Get(ctx) + + w := &Interceptor{ + KubeClientSet: kubeClient, + Logger: logger.Sugar(), + } + + req := &triggersv1.InterceptorRequest{ + Body: json.RawMessage(`{}`), + Header: http.Header{ + "Content-Type": []string{"application/json"}, + }, + InterceptorParams: map[string]interface{}{ + "blah": func() {}, + }, + Context: &triggersv1.TriggerContext{ + EventURL: "https://testing.example.com", + EventID: "abcde", + TriggerID: "namespaces/default/triggers/example-trigger", + }, + } + + res := w.Process(ctx, req) + if res.Continue { + t.Fatalf("Interceptor.Process() expected res.Continue to be false but got %t. \nStatus.Err(): %v", res.Continue, res.Status.Err()) + } +} diff --git a/pkg/interceptors/interceptors.go b/pkg/interceptors/interceptors.go index 4f5e322a19c..b6c07374b6e 100644 --- a/pkg/interceptors/interceptors.go +++ b/pkg/interceptors/interceptors.go @@ -18,9 +18,13 @@ package interceptors import ( "context" + "encoding/json" + "fmt" "net/http" "path" + "google.golang.org/grpc/status" + triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes" @@ -118,3 +122,34 @@ func GetInterceptorParams(i *triggersv1.EventInterceptor) map[string]interface{} return ip } + +// Fail constructs a InterceptorResponse that should not continue further processing. +func Fail(status *status.Status) *triggersv1.InterceptorResponse { + return &triggersv1.InterceptorResponse{ + Continue: false, + Status: status, + } +} + +// Canonical updates the map keys to use the Canonical name +func Canonical(h map[string][]string) http.Header { + c := map[string][]string{} + for k, v := range h { + c[http.CanonicalHeaderKey(k)] = v + } + return http.Header(c) +} + +// UnmarshalParams unmarshalls the passed in InterceptorParams into the provided param struct +func UnmarshalParams(ip map[string]interface{}, p interface{}) error { + b, err := json.Marshal(ip) + if err != nil { + return fmt.Errorf("failed to marshal json: %w", err) + } + + if err := json.Unmarshal(b, &p); err != nil { + // Should never happen since Unmarshall only returns err if json is invalid which we already check above + return fmt.Errorf("invalid json: %w", err) + } + return nil +} diff --git a/pkg/interceptors/interceptors_test.go b/pkg/interceptors/interceptors_test.go index 8cd6f014dd7..9c4b3c3116b 100644 --- a/pkg/interceptors/interceptors_test.go +++ b/pkg/interceptors/interceptors_test.go @@ -20,6 +20,7 @@ import ( "context" "fmt" "net/http" + "strings" "testing" pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" @@ -156,25 +157,100 @@ func TestGetInterceptorParams(t *testing.T) { } } +func TestCanonical(t *testing.T) { + for _, tc := range []struct { + name string + in map[string][]string + want map[string][]string + }{{ + name: "all uppercase", + in: map[string][]string{ + "X-ABC": {"foo"}, + }, + want: map[string][]string{ + "X-Abc": {"foo"}, + }, + }, { + name: "all lowercase", + in: map[string][]string{ + "x-abc": {"a", "v"}, + }, + want: map[string][]string{ + "X-Abc": {"a", "v"}, + }, + }} { + t.Run(tc.name, func(t *testing.T) { + got := Canonical(tc.in) + want := http.Header(tc.want) + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("Canonical() failed. Diff (-want/+got): %s", diff) + } + }) + } +} + +func TestUnmarshalParam(t *testing.T) { + in := map[string]interface{}{ + "secretKey": "key", + "secretName": "name", + } + + got := triggersv1.SecretRef{} + if err := UnmarshalParams(in, &got); err != nil { + t.Fatalf("UnmarshalParams() unexpected error: %v", err) + } + + want := triggersv1.SecretRef{ + SecretKey: "key", + SecretName: "name", + } + + if diff := cmp.Diff(want, got); diff != "" { + t.Fatalf("UnmarshalParams() failed. Diff (-want/+got): %s", diff) + } +} + +func TestGetInterceptorParams_Error(t *testing.T) { + for _, tc := range []struct { + ip map[string]interface{} + p interface{} + wantErrMsg string + }{{ + ip: map[string]interface{}{ + "secretKey": func() {}, + }, + p: triggersv1.SecretRef{}, + wantErrMsg: "failed to marshal json", + }} { + t.Run(tc.wantErrMsg, func(t *testing.T) { + err := UnmarshalParams(tc.ip, &tc.p) + if err == nil { + t.Fatalf("UnmarshalParams() expected error but got nil") + } + + if !strings.Contains(err.Error(), tc.wantErrMsg) { + t.Fatalf("UnmarshalParams() expected err to contain %s but got %s", tc.wantErrMsg, err.Error()) + } + }) + } +} + func Test_GetSecretToken(t *testing.T) { tests := []struct { name string cache map[string]interface{} wanted []byte - }{ - { - name: "no matching cache entry exists", - cache: make(map[string]interface{}), - wanted: []byte("secret from API"), - }, - { - name: "a matching cache entry exists", - cache: map[string]interface{}{ - fmt.Sprintf("secret/%s/test-secret/token", testNS): []byte("secret from cache"), - }, - wanted: []byte("secret from cache"), + }{{ + name: "no matching cache entry exists", + cache: make(map[string]interface{}), + wanted: []byte("secret from API"), + }, { + name: "a matching cache entry exists", + cache: map[string]interface{}{ + fmt.Sprintf("secret/%s/test-secret/token", testNS): []byte("secret from cache"), }, - } + wanted: []byte("secret from cache"), + }} for _, tt := range tests { t.Run(tt.name, func(rt *testing.T) { @@ -182,7 +258,10 @@ func Test_GetSecretToken(t *testing.T) { ctx, _ := rtesting.SetupFakeContext(t) kubeClient := fakekubeclient.Get(ctx) - secretRef := makeSecretRef() + secretRef := triggersv1.SecretRef{ + SecretKey: "token", + SecretName: "test-secret", + } if _, err := kubeClient.CoreV1().Secrets(testNS).Create(context.Background(), makeSecret("secret from API"), metav1.CreateOptions{}); err != nil { rt.Error(err) @@ -212,13 +291,6 @@ func makeSecret(secretText string) *corev1.Secret { } } -func makeSecretRef() triggersv1.SecretRef { - return triggersv1.SecretRef{ - SecretKey: "token", - SecretName: "test-secret", - } -} - func setCache(req *http.Request, vals map[string]interface{}) *http.Request { return req.WithContext(context.WithValue(req.Context(), requestCacheKey, vals)) } diff --git a/pkg/sink/sink.go b/pkg/sink/sink.go index ade156ba9df..3b89221c78f 100644 --- a/pkg/sink/sink.go +++ b/pkg/sink/sink.go @@ -264,20 +264,19 @@ func (r Sink) ExecuteInterceptors(t triggersv1.Trigger, in *http.Request, event }, } - var interceptorResponse *triggersv1.InterceptorResponse for _, i := range t.Spec.Interceptors { var interceptor interceptors.Interceptor switch { case i.Webhook != nil: interceptor = webhook.NewInterceptor(i.Webhook, r.HTTPClient, t.Namespace, log) case i.GitHub != nil: - interceptor = github.NewInterceptor(i.GitHub, r.KubeClientSet, t.Namespace, log) + interceptor = github.NewInterceptor(r.KubeClientSet, log) case i.GitLab != nil: - interceptor = gitlab.NewInterceptor(i.GitLab, r.KubeClientSet, t.Namespace, log) + interceptor = gitlab.NewInterceptor(r.KubeClientSet, log) case i.CEL != nil: interceptor = cel.NewInterceptor(r.KubeClientSet, log) case i.Bitbucket != nil: - interceptor = bitbucket.NewInterceptor(i.Bitbucket, r.KubeClientSet, t.Namespace, log) + interceptor = bitbucket.NewInterceptor(r.KubeClientSet, log) default: return nil, nil, nil, fmt.Errorf("unknown interceptor type: %v", i) } @@ -286,7 +285,7 @@ func (r Sink) ExecuteInterceptors(t triggersv1.Trigger, in *http.Request, event if interceptorInterface, ok := interceptor.(triggersv1.InterceptorInterface); ok { // Set per interceptor config params to the request request.InterceptorParams = interceptors.GetInterceptorParams(i) - interceptorResponse = interceptorInterface.Process(context.Background(), &request) + interceptorResponse := interceptorInterface.Process(context.Background(), &request) if !interceptorResponse.Continue { return nil, nil, interceptorResponse, nil } diff --git a/test/signature.go b/test/signature.go new file mode 100644 index 00000000000..68bb6998edb --- /dev/null +++ b/test/signature.go @@ -0,0 +1,19 @@ +package test + +import ( + "crypto/hmac" + "crypto/sha1" //nolint + "encoding/hex" + "fmt" +) + +// HMACHeader generates a X-Hub-Signature header given a secret token and the request body +// See https://developer.github.com/webhooks/securing/#validating-payloads-from-github +func HMACHeader(secret string, body []byte) (string, error) { + h := hmac.New(sha1.New, []byte(secret)) + _, err := h.Write(body) + if err != nil { + return "", err + } + return fmt.Sprintf("sha1=%s", hex.EncodeToString(h.Sum(nil))), nil +} diff --git a/test/signature_test.go b/test/signature_test.go new file mode 100644 index 00000000000..6936f9996c6 --- /dev/null +++ b/test/signature_test.go @@ -0,0 +1,20 @@ +package test_test + +import ( + "encoding/json" + "testing" + + "github.com/tektoncd/triggers/test" +) + +func TestHMACHeader(t *testing.T) { + got, err := test.HMACHeader("secret", json.RawMessage(`{}`)) + if err != nil { + t.Fatalf("HMACHeader() error: %v", err) + } + // Generated from https://play.golang.org/p/OlkBawQQPiJ + want := "sha1=5d61605c3feea9799210ddcb71307d4ba264225f" + if want != got { + t.Fatalf("HMACHeader(). Want: %s Got: %s", want, got) + } +}