From 9ab2d7fbb2952b9916c095703a6b2939d8bcfe3d Mon Sep 17 00:00:00 2001 From: Gabe Montero Date: Fri, 28 Feb 2020 12:56:45 -0500 Subject: [PATCH] introduce authentication/authorization at the EventListenerTrigger level (with default back to existing EventListener level) --- cmd/eventlistenersink/main.go | 1 + docs/clustertriggerbindings.md | 4 +- docs/eventlisteners.md | 138 ++++++++-- docs/triggerbindings.md | 2 +- docs/triggertemplates.md | 2 +- .../clusterrole.yaml | 3 +- .../triggerbinding-roles/role.yaml | 3 +- .../triggers/v1alpha1/event_listener_types.go | 9 + .../v1alpha1/zz_generated.deepcopy.go | 5 + pkg/resources/create.go | 5 + pkg/sink/auth_override.go | 135 +++++++++ pkg/sink/sink.go | 46 +++- pkg/sink/sink_test.go | 260 +++++++++++++++++- test/builder/eventlistener.go | 10 + test/controller.go | 11 + test/eventlistener_test.go | 92 +++++-- 16 files changed, 669 insertions(+), 57 deletions(-) create mode 100644 pkg/sink/auth_override.go diff --git a/cmd/eventlistenersink/main.go b/cmd/eventlistenersink/main.go index 2c7604ee6..bab92f7f9 100644 --- a/cmd/eventlistenersink/main.go +++ b/cmd/eventlistenersink/main.go @@ -92,6 +92,7 @@ func main() { EventListenerName: sinkArgs.ElName, EventListenerNamespace: sinkArgs.ElNamespace, Logger: logger, + Auth: sink.DefaultAuthOverride{}, } // Listen and serve diff --git a/docs/clustertriggerbindings.md b/docs/clustertriggerbindings.md index 1bbf9d2d7..25abec3fa 100644 --- a/docs/clustertriggerbindings.md +++ b/docs/clustertriggerbindings.md @@ -12,7 +12,6 @@ designed to encourage reusability clusterwide. You can reference a ClusterTriggerBinding in any EventListener in any namespace. - ```YAML apiVersion: triggers.tekton.dev/v1alpha1 kind: ClusterTriggerBinding @@ -28,6 +27,7 @@ spec: value: $(header.Content-Type) ``` + You can specify multiple ClusterTriggerBindings in a Trigger. You can use a ClusterTriggerBinding in multiple Triggers. @@ -35,7 +35,6 @@ In case of using a ClusterTriggerBinding, the `Binding` kind should be added. The default kind is TriggerBinding which represents a namespaced TriggerBinding. - ```YAML --- apiVersion: triggers.tekton.dev/v1alpha1 @@ -54,3 +53,4 @@ spec: template: name: pipeline-template ``` + diff --git a/docs/eventlisteners.md b/docs/eventlisteners.md index 3acbe4ea0..2970d9868 100644 --- a/docs/eventlisteners.md +++ b/docs/eventlisteners.md @@ -15,13 +15,74 @@ Tekton resources. In addition, EventListeners allow lightweight event processing using [Event Interceptors](#Interceptors). - [Syntax](#syntax) + - [ServiceAccountName](#serviceAccountName) - [Triggers](#triggers) - [Interceptors](#Interceptors) - - [ServiceAccountName](#serviceAccountName) - [Logging](#logging) - [Labels](#labels) - [Examples](#examples) +## Multi-Tenant Concerns + +The EventListener is effectively an additional form of client into Tekton, versus what +example usage via `kubectl` or `tkn` which you have seen elsewhere. In particular, the HTTP based +events bypass the normal Kubernetes authentication path you get via `kubeconfig` files +and the `kubectl config` family of commands. + +As such, there are set of items to consider when deciding how to + +- best expose (each) EventListener in your cluster to the outside world. +- best control how (each) EventListener and the underlying API Objects described below access, create, +and update Tekton related API Objects in your cluster. + +Minimally, each EventListener has its [ServiceAccountName](#serviceAccountName) as noted below and all +events coming over the "Sink" result in any Tekton resource interactions being done with the permissions +assigned to that ServiceAccount. + +However, if you need differing levels of permissions over a set of Tekton resources across the various +[Triggers](#triggers) and [Interceptors](#Interceptors), where not all Triggers or Interceptors can +manipulate certain Tekton Resources in the same way, a simple, single EventListener will not suffice. + +Your options at that point are as follows: + +### Multiple EventListers (One EventListener Per Namespace) + +You can create multiple EventListener objects, where your set of Triggers and Interceptors are spread out across the +EventListeners. + +If you create each of those EventListeners in their own namespace, it becomes easy to assign +varying permissions to the ServiceAccount of each one to serve your needs. And often times namespace +creation is coupled with a default set of ServiceAccounts and Secrets that are also defined. +So conceivably some administration steps are taken care of. You just update the permissions +of the automatically created ServiceAccounts. + +Possible drawbacks: +- Namespaces with associated Secrets and ServiceAccounts in an aggregate sense prove to be the most expensive +items in Kubernetes underlying `etcd` store. In larger clusters `etcd` storage capacity can become a concern. +- Multiple EventListeners means multiple HTTP ports that must be exposed to the external entities accessing +the "Sink". If you happen to have a HTTP Firewall between your Cluster and external entities, that means more +administrative cost, opening ports in the firewall for each Service, unless you can employ Kubernetes `Ingress` to +serve as a routing abstraction layer for your set of EventListeners. + +### Multiple EventListeners (Multiple EventListeners per Namespace) + +Multiple EventListeners per namespace will most likely mean more ServiceAccount/Secret/RBAC manipulation for +the administrator, as some of the built in generation of those artifacts as part of namespace creation are not +applicable. + +However you will save some on the `etcd` storage costs by reducing the number of namespaces. + +Multiple EventListeners and potential Firewall concerns still apply (again unless you employ `Ingress`). + +### ServiceAccount per EventListenerTrigger + +Being able to set a ServiceAccount on an EventListenerTrigger allows for finer grained permissions as well. + +You still have to create the additional ServiceAccounts. + +But staying within 1 namespace and minimizing the number of EventListeners with their associated "Sinks" minimizes +concerns around `etcd` storage and port considerations with Firewalls if `Ingress` is not utilized. + ## Syntax To define a configuration file for an `EventListener` resource, you can specify @@ -47,30 +108,6 @@ the following fields: [kubernetes-overview]: https://kubernetes.io/docs/concepts/overview/working-with-objects/kubernetes-objects/#required-fields -### Triggers - -The `triggers` field is required. Each EventListener can consist of one or more -`triggers`. A Trigger consists of: - -- `name` - (Optional) a valid - [Kubernetes name](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) -- [`interceptors`](#interceptors) - (Optional) list of interceptors to use -- `bindings` - A list of names of `TriggerBindings` to use -- `template` - The name of `TriggerTemplate` to use - -```yaml -triggers: - - name: trigger-1 - interceptors: - - github: - eventTypes: ["pull_request"] - bindings: - - name: pipeline-binding - - name: message-binding - template: - name: pipeline-template -``` - ### ServiceAccountName The `serviceAccountName` field is required. The ServiceAccount that the @@ -90,7 +127,8 @@ rules: resources: ["eventlisteners", "triggerbindings", "triggertemplates"] verbs: ["get"] - apiGroups: [""] - resources: ["configmaps", "secrets"] # secrets are only needed for Github/Gitlab interceptors + # secrets are only needed for Github/Gitlab interceptors, serviceaccounts only for per trigger authorization + resources: ["configmaps", "secrets", "serviceaccounts"] verbs: ["get", "list", "watch"] # Permissions to create resources in associated TriggerTemplates - apiGroups: ["tekton.dev"] @@ -103,6 +141,52 @@ If your EventListener is using ServiceAccount with a [ClusterRole instead](../examples/role-resources/clustertriggerbinding-roles/clusterrole.yaml). +### Triggers + +The `triggers` field is required. Each EventListener can consist of one or more +`triggers`. A Trigger consists of: + +- `name` - (Optional) a valid + [Kubernetes name](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#syntax-and-character-set) +- [`interceptors`](#interceptors) - (Optional) list of interceptors to use +- `bindings` - A list of names of `TriggerBindings` to use +- `template` - The name of `TriggerTemplate` to use + +```yaml +triggers: + - name: trigger-1 + interceptors: + - github: + eventTypes: ["pull_request"] + bindings: + - name: pipeline-binding + - name: message-binding + template: + name: pipeline-template +``` + +Also, to support multi-tenant styled scenarios, where an administrator may not want all triggers to have +the same permissions as the `EventListener`, a service account can optionally be set at the trigger level +and used if present in place of the `EventListener` service account when creating resources: + +```yaml +triggers: + - name: trigger-1 + serviceAccount: + name: trigger-1-sa + namespace: event-listener-namespace + interceptors: + - github: + eventTypes: ["pull_request"] + bindings: + - name: pipeline-binding + - name: message-binding + template: + name: pipeline-template +``` + +The default ClusterRole for the EventLister allows for reading ServiceAccounts from any namespace. + ### ServiceType The `serviceType` field is optional. EventListener sinks are exposed via @@ -414,7 +498,6 @@ processed, and the `overlays` applied. Optionally, no `filter` expression can be provided, and the `overlays` will be applied to the incoming body. - ```YAML @@ -542,6 +625,7 @@ spec: value: $(body.pull_request.head.short_sha) ``` + ## Examples For complete examples, see diff --git a/docs/triggerbindings.md b/docs/triggerbindings.md index 30f30d144..b19ed6fd5 100644 --- a/docs/triggerbindings.md +++ b/docs/triggerbindings.md @@ -12,7 +12,6 @@ parameters. The separation of `TriggerBinding`s from `TriggerTemplate`s was deliberate to encourage reuse between them. - ```YAML apiVersion: triggers.tekton.dev/v1alpha1 kind: TriggerBinding @@ -28,6 +27,7 @@ spec: value: $(header.Content-Type) ``` + `TriggerBinding`s are connected to `TriggerTemplate`s within an [`EventListener`](eventlisteners.md), which is where the pod is actually instantiated that "listens" for the respective events. diff --git a/docs/triggertemplates.md b/docs/triggertemplates.md index 5f538a40b..651cc0ec1 100644 --- a/docs/triggertemplates.md +++ b/docs/triggertemplates.md @@ -11,7 +11,6 @@ A `TriggerTemplate` is a resource that can template resources. **anywhere** within the resource template. - ```YAML apiVersion: triggers.tekton.dev/v1alpha1 kind: TriggerTemplate @@ -53,6 +52,7 @@ spec: value: $(params.gitrepositoryurl) ``` + Similar to [Pipelines](https://github.com/tektoncd/pipeline/blob/master/docs/pipelines.md),`TriggerTemplate`s do not do any actual work, but instead act as the blueprint for what resources diff --git a/examples/role-resources/clustertriggerbinding-roles/clusterrole.yaml b/examples/role-resources/clustertriggerbinding-roles/clusterrole.yaml index 5604e5c26..6719dd22b 100644 --- a/examples/role-resources/clustertriggerbinding-roles/clusterrole.yaml +++ b/examples/role-resources/clustertriggerbinding-roles/clusterrole.yaml @@ -8,7 +8,8 @@ rules: resources: ["clustertriggerbindings", "eventlisteners", "triggerbindings", "triggertemplates"] verbs: ["get"] - apiGroups: [""] - resources: ["configmaps", "secrets"] # secrets are only needed for Github/Gitlab interceptors + # secrets are only needed for Github/Gitlab interceptors, serviceaccounts only for per trigger authorization + resources: ["configmaps", "secrets", "serviceaccounts"] verbs: ["get", "list", "watch"] # Permissions to create resources in associated TriggerTemplates - apiGroups: ["tekton.dev"] diff --git a/examples/role-resources/triggerbinding-roles/role.yaml b/examples/role-resources/triggerbinding-roles/role.yaml index b05c2f362..9abc1e4fa 100644 --- a/examples/role-resources/triggerbinding-roles/role.yaml +++ b/examples/role-resources/triggerbinding-roles/role.yaml @@ -8,7 +8,8 @@ rules: resources: ["eventlisteners", "triggerbindings", "triggertemplates"] verbs: ["get"] - apiGroups: [""] - resources: ["configmaps", "secrets"] # secrets are only needed for Github/Gitlab interceptors + # secrets are only needed for Github/Gitlab interceptors, serviceaccounts only for per trigger authorization + resources: ["configmaps", "secrets", "serviceaccounts"] verbs: ["get", "list", "watch"] # Permissions to create resources in associated TriggerTemplates - apiGroups: ["tekton.dev"] diff --git a/pkg/apis/triggers/v1alpha1/event_listener_types.go b/pkg/apis/triggers/v1alpha1/event_listener_types.go index e181e657c..23e15f324 100644 --- a/pkg/apis/triggers/v1alpha1/event_listener_types.go +++ b/pkg/apis/triggers/v1alpha1/event_listener_types.go @@ -67,6 +67,15 @@ type EventListenerTrigger struct { // +optional Name string `json:"name,omitempty"` Interceptors []*EventInterceptor `json:"interceptors,omitempty"` + // ServiceAccount optionally associates credentials with each trigger; + // more granular authorization for + // who is allowed to utilize the associated pipeline + // vs. defaulting to whatever permissions are associated + // with the entire EventListener and associated sink facilitates + // multi-tenant model based scenarios + // TODO do we want to restrict this to the event listener namespace and just ask for the service account name here? + // +optional + ServiceAccount *corev1.ObjectReference `json:"serviceAccount,omitempty"` } // EventInterceptor provides a hook to intercept and pre-process events diff --git a/pkg/apis/triggers/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/triggers/v1alpha1/zz_generated.deepcopy.go index b2e67a0ec..794d51432 100644 --- a/pkg/apis/triggers/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/triggers/v1alpha1/zz_generated.deepcopy.go @@ -337,6 +337,11 @@ func (in *EventListenerTrigger) DeepCopyInto(out *EventListenerTrigger) { } } } + if in.ServiceAccount != nil { + in, out := &in.ServiceAccount, &out.ServiceAccount + *out = new(v1.ObjectReference) + **out = **in + } return } diff --git a/pkg/resources/create.go b/pkg/resources/create.go index 2462d6dfa..b0a6d4241 100644 --- a/pkg/resources/create.go +++ b/pkg/resources/create.go @@ -21,6 +21,8 @@ import ( "fmt" "strings" + kerrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/client-go/dynamic" "go.uber.org/zap" @@ -99,6 +101,9 @@ func Create(logger *zap.SugaredLogger, rt json.RawMessage, triggerName, eventID, logger.Infof("For event ID %q creating resource %v", eventID, gvr) if _, err := dc.Resource(gvr).Namespace(namespace).Create(data, metav1.CreateOptions{}); err != nil { + if kerrors.IsUnauthorized(err) || kerrors.IsForbidden(err) { + return err + } return fmt.Errorf("couldn't create resource with group version kind %q: %v", gvr, err) } return nil diff --git a/pkg/sink/auth_override.go b/pkg/sink/auth_override.go new file mode 100644 index 000000000..f19df37b6 --- /dev/null +++ b/pkg/sink/auth_override.go @@ -0,0 +1,135 @@ +package sink + +import ( + triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" + dynamicClientset "github.com/tektoncd/triggers/pkg/client/dynamic/clientset" + "github.com/tektoncd/triggers/pkg/client/dynamic/clientset/tekton" + "go.uber.org/zap" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + discoveryclient "k8s.io/client-go/discovery" + "k8s.io/client-go/dynamic" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/rest" +) + +//AuthOverride is an interface that constructs a discovery client for the ServerResourceInterface +//and a dynamic client for the Tekton Resources, using the token provide as the bearer token in the +//REST config used to build those client. The other non-credential related parameters for the +//REST client used are copied from the in cluster config of the event sink. +type AuthOverride interface { + OverrideAuthentication(token string, + log *zap.SugaredLogger, + defaultDiscoveryClient discoveryclient.ServerResourcesInterface, + defaultDynamicClient dynamic.Interface) (discoveryClient discoveryclient.ServerResourcesInterface, + dynamicClient dynamic.Interface, + err error) +} + +func isServiceAccountToken(secret *corev1.Secret, sa *corev1.ServiceAccount) bool { + if secret.Type != corev1.SecretTypeServiceAccountToken { + return false + } + + name := secret.Annotations[corev1.ServiceAccountNameKey] + uid := secret.Annotations[corev1.ServiceAccountUIDKey] + if name != sa.Name { + // Name must match + return false + } + if len(uid) > 0 && uid != string(sa.UID) { + // If UID is specified, it must match + return false + } + + return true +} + +func (r Sink) retrieveAuthToken(saRef *corev1.ObjectReference, eventLog *zap.SugaredLogger) (string, error) { + if saRef == nil { + return "", nil + } + + if len(saRef.Name) == 0 || len(saRef.Namespace) == 0 { + return "", nil + } + + var log *zap.SugaredLogger + if eventLog != nil { + log = eventLog.With(zap.String(triggersv1.TriggerLabelKey, "retriveAuthToken")) + } + + sa, err := r.KubeClientSet.CoreV1().ServiceAccounts(saRef.Namespace).Get(saRef.Name, metav1.GetOptions{}) + if err != nil { + if log != nil { + log.Error(err) + } + return "", err + } + var savedErr error + for _, ref := range sa.Secrets { + // secret ref namespace most likely won't be set, as the secret can only reside in + // sa's namespace, so use that + secret, err := r.KubeClientSet.CoreV1().Secrets(saRef.Namespace).Get(ref.Name, metav1.GetOptions{}) + if err != nil { + if log != nil { + log.Error(err) + } + savedErr = err + continue + } + if isServiceAccountToken(secret, sa) { + token, exists := secret.Data[corev1.ServiceAccountTokenKey] + if log != nil { + log.Debugf("retrieveAuthToken found SA auth: %v", exists) + } + if !exists { + continue + } + + return string(token), nil + } + } + return "", savedErr +} + +func newConfig(newToken string, config *rest.Config) *rest.Config { + // first clean out all user credentials from the pods' in cluster config + newConfig := rest.AnonymousClientConfig(config) + // add the token from our interceptors + newConfig.BearerToken = newToken + return newConfig +} + +type DefaultAuthOverride struct { +} + +func (r DefaultAuthOverride) OverrideAuthentication(token string, + log *zap.SugaredLogger, + defaultDiscoverClient discoveryclient.ServerResourcesInterface, + defaultDynamicClient dynamic.Interface) (discoveryClient discoveryclient.ServerResourcesInterface, + dynamicClient dynamic.Interface, + err error) { + dynamicClient = defaultDynamicClient + discoveryClient = defaultDiscoverClient + clusterConfig, err := rest.InClusterConfig() + if err != nil { + log.Errorf("overrideAuthentication: problem getting in cluster config: %#v\n", err) + return + } + clusterConfig = newConfig(token, clusterConfig) + dc, err := dynamic.NewForConfig(clusterConfig) + if err != nil { + log.Errorf("overrideAuthentication: problem getting dynamic client set: %#v\n", err) + return + } + kubeClient, err := kubernetes.NewForConfig(clusterConfig) + if err != nil { + log.Errorf("overrideAuthentication: problem getting kube client: %#v\n", err) + return + } + dynamicClient = dynamicClientset.New(tekton.WithClient(dc)) + discoveryClient = kubeClient.Discovery() + + return +} diff --git a/pkg/sink/sink.go b/pkg/sink/sink.go index 18e988149..30d459ba8 100644 --- a/pkg/sink/sink.go +++ b/pkg/sink/sink.go @@ -36,6 +36,7 @@ import ( "github.com/tektoncd/triggers/pkg/resources" "github.com/tektoncd/triggers/pkg/template" "go.uber.org/zap" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" discoveryclient "k8s.io/client-go/discovery" "k8s.io/client-go/dynamic" @@ -55,6 +56,7 @@ type Sink struct { EventListenerName string EventListenerNamespace string Logger *zap.SugaredLogger + Auth AuthOverride } // Response defines the HTTP body that the Sink responds to events with. @@ -93,6 +95,13 @@ func (r Sink) HandleEvent(response http.ResponseWriter, request *http.Request) { go func(t triggersv1.EventListenerTrigger) { localRequest := request.Clone(request.Context()) if err := r.processTrigger(&t, localRequest, event, eventID, eventLog); err != nil { + if kerrors.IsUnauthorized(err) { + result <- http.StatusUnauthorized + return + } + if kerrors.IsForbidden(err) { + result <- http.StatusForbidden + } result <- http.StatusAccepted return } @@ -105,6 +114,13 @@ func (r Sink) HandleEvent(response http.ResponseWriter, request *http.Request) { code := http.StatusAccepted for i := 0; i < len(el.Spec.Triggers); i++ { thiscode := <-result + // current take - if someone is doing unauthorized stuff, we abort immediately; + // unauthorized should be the final status code vs. the less than comparison + // below around accepted vs. created + if thiscode == http.StatusUnauthorized || thiscode == http.StatusForbidden { + code = thiscode + break + } if thiscode < code { code = thiscode } @@ -130,6 +146,7 @@ func (r Sink) processTrigger(t *triggersv1.EventListenerTrigger, request *http.R finalPayload, header, err := r.executeInterceptors(t, request, event, eventID, log) if err != nil { + log.Error(err) return err } @@ -149,7 +166,12 @@ func (r Sink) processTrigger(t *triggersv1.EventListenerTrigger, request *http.R } log.Info("params: %+v", params) resources := template.ResolveResources(rt.TriggerTemplate, params) - if err := r.createResources(resources, t.Name, eventID); err != nil { + token, err := r.retrieveAuthToken(t.ServiceAccount, eventLog) + if err != nil { + log.Error(err) + return err + } + if err := r.createResources(token, resources, t.Name, eventID, log); err != nil { log.Error(err) return err } @@ -188,6 +210,7 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R log.Error(err) return nil, nil, err } + // Set the next request to be the output of the last response to enable // request chaining. request = &http.Request{ @@ -204,9 +227,26 @@ func (r Sink) executeInterceptors(t *triggersv1.EventListenerTrigger, in *http.R return payload, resp.Header, nil } -func (r Sink) createResources(res []json.RawMessage, triggerName, eventID string) error { +func (r Sink) createResources(token string, res []json.RawMessage, triggerName, eventID string, log *zap.SugaredLogger) error { + discoveryClient := r.DiscoveryClient + dynamicClient := r.DynamicClient + var err error + if len(token) > 0 { + // So at start up the discovery and dynamic clients are created using the in cluster config + // of this pod (i.e. using the credentials of the serviceaccount associated with the EventListener) + + // However, we also have a ServiceAccount/Secret/SecretKey reference with each EventListenerTrigger to allow + // for more fine grained authorization control around the resources we create below. + discoveryClient, dynamicClient, err = r.Auth.OverrideAuthentication(token, log, r.DiscoveryClient, r.DynamicClient) + if err != nil { + log.Errorf("problem cloning rest config: %#v", err) + return err + } + } + for _, rr := range res { - if err := resources.Create(r.Logger, rr, triggerName, eventID, r.EventListenerName, r.EventListenerNamespace, r.DiscoveryClient, r.DynamicClient); err != nil { + if err := resources.Create(r.Logger, rr, triggerName, eventID, r.EventListenerName, r.EventListenerNamespace, discoveryClient, dynamicClient); err != nil { + log.Errorf("problem creating obj: %#v", err) return err } } diff --git a/pkg/sink/sink_test.go b/pkg/sink/sink_test.go index 5b1ecac9c..99b254f48 100644 --- a/pkg/sink/sink_test.go +++ b/pkg/sink/sink_test.go @@ -25,13 +25,21 @@ import ( "net/url" "strconv" "strings" + "sync" "testing" + fakekubeclientset "k8s.io/client-go/kubernetes/fake" + + "go.uber.org/zap" + discoveryclient "k8s.io/client-go/discovery" + "k8s.io/client-go/dynamic" + "github.com/google/go-cmp/cmp" "github.com/google/go-cmp/cmp/cmpopts" "github.com/gorilla/mux" pipelinev1alpha1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1" + pipelinev1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1" "github.com/tektoncd/pipeline/pkg/logging" triggersv1 "github.com/tektoncd/triggers/pkg/apis/triggers/v1alpha1" dynamicclientset "github.com/tektoncd/triggers/pkg/client/dynamic/clientset" @@ -40,9 +48,11 @@ import ( "github.com/tektoncd/triggers/test" bldr "github.com/tektoncd/triggers/test/builder" corev1 "k8s.io/api/core/v1" + kerrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" fakedynamic "k8s.io/client-go/dynamic/fake" ktesting "k8s.io/client-go/testing" rtesting "knative.dev/pkg/reconciler/testing" @@ -70,7 +80,7 @@ func comparePR(x, y pipelinev1alpha1.PipelineResource) bool { // getSinkAssets seeds test resources and returns a testable Sink and a dynamic // client. The returned client is used to creating the fake resources and can be // used to check if the correct resources were created. -func getSinkAssets(t *testing.T, resources test.Resources, elName string) (Sink, *fakedynamic.FakeDynamicClient) { +func getSinkAssets(t *testing.T, resources test.Resources, elName string, auth AuthOverride) (Sink, *fakedynamic.FakeDynamicClient) { t.Helper() ctx, _ := rtesting.SetupFakeContext(t) clients := test.SeedResources(t, ctx, resources) @@ -88,6 +98,7 @@ func getSinkAssets(t *testing.T, resources test.Resources, elName string) (Sink, KubeClientSet: clients.Kube, TriggersClient: clients.Triggers, Logger: logger, + Auth: auth, } return r, dynamicClient } @@ -197,7 +208,7 @@ func TestHandleEvent(t *testing.T) { EventListeners: []*triggersv1.EventListener{el}, } - sink, dynamicClient := getSinkAssets(t, resources, el.Name) + sink, dynamicClient := getSinkAssets(t, resources, el.Name, DefaultAuthOverride{}) ts := httptest.NewServer(http.HandlerFunc(sink.HandleEvent)) defer ts.Close() @@ -318,7 +329,7 @@ func TestHandleEventWithInterceptors(t *testing.T) { Secrets: []*corev1.Secret{secret}, } - sink, dynamicClient := getSinkAssets(t, resources, el.Name) + sink, dynamicClient := getSinkAssets(t, resources, el.Name, DefaultAuthOverride{}) ts := httptest.NewServer(http.HandlerFunc(sink.HandleEvent)) defer ts.Close() @@ -466,7 +477,7 @@ func TestHandleEventWithWebhookInterceptors(t *testing.T) { Proxy: http.ProxyURL(u), } - sink, dynamicClient := getSinkAssets(t, resources, el.Name) + sink, dynamicClient := getSinkAssets(t, resources, el.Name, DefaultAuthOverride{}) sink.HTTPClient = srv.Client() ts := httptest.NewServer(http.HandlerFunc(sink.HandleEvent)) @@ -668,3 +679,244 @@ func TestExecuteInterceptor_error(t *testing.T) { t.Error("expected sequential interceptor to not be called") } } + +const userWithPermissions = "user-with-permissions" +const userWithoutPermissions = "user-with-no-permissions" + +func TestRetriveveAuthToken(t *testing.T) { + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: userWithoutPermissions, + UID: types.UID(userWithoutPermissions), + Annotations: map[string]string{ + corev1.ServiceAccountNameKey: userWithoutPermissions, + corev1.ServiceAccountUIDKey: userWithoutPermissions, + }, + }, + Type: corev1.SecretTypeServiceAccountToken, + Data: map[string][]byte{ + corev1.ServiceAccountTokenKey: []byte(userWithoutPermissions), + }, + } + sa := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: userWithoutPermissions, + Namespace: userWithoutPermissions, + UID: types.UID(userWithoutPermissions), + }, + Secrets: []corev1.ObjectReference{ + { + Name: userWithoutPermissions, + Namespace: userWithoutPermissions, + }, + }, + } + + kubeClient := fakekubeclientset.NewSimpleClientset() + test.AddTektonResources(kubeClient) + if err := kubeClient.CoreV1().Secrets(userWithoutPermissions).Delete(userWithoutPermissions, &metav1.DeleteOptions{}); err != nil && !kerrors.IsNotFound(err) { + t.Fatalf("Error deleting secret %v: %s", secret, err.Error()) + } + if _, err := kubeClient.CoreV1().Secrets(userWithoutPermissions).Create(secret); err != nil { + t.Fatalf("Error creating secret %v: %s", secret, err.Error()) + } + if err := kubeClient.CoreV1().ServiceAccounts(userWithoutPermissions).Delete(userWithoutPermissions, &metav1.DeleteOptions{}); err != nil && !kerrors.IsNotFound(err) { + t.Fatalf("Error delete sa %v: %s", sa, err.Error()) + } + if _, err := kubeClient.CoreV1().ServiceAccounts(userWithoutPermissions).Create(sa); err != nil { + t.Fatalf("Error creating sa %v: %s", sa, err.Error()) + } + + r := Sink{ + KubeClientSet: kubeClient, + } + + token, err := r.retrieveAuthToken(&corev1.ObjectReference{Name: userWithoutPermissions, Namespace: userWithoutPermissions}, nil) + if err != nil { + t.Fatalf(err.Error()) + } + if token != userWithoutPermissions { + t.Fatalf("got token %s instead of %s", token, userWithoutPermissions) + } +} + +type fakeAuth struct { +} + +var triggerAuthWG sync.WaitGroup + +func (r fakeAuth) OverrideAuthentication(token string, + log *zap.SugaredLogger, + defaultDiscoverClient discoveryclient.ServerResourcesInterface, + defaultDynamicClient dynamic.Interface) (discoveryClient discoveryclient.ServerResourcesInterface, + dynamicClient dynamic.Interface, + err error) { + + if token == userWithoutPermissions { + dynamicClient := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme()) + dynamicClient.PrependReactor("*", "*", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) { + defer triggerAuthWG.Done() + return true, nil, kerrors.NewUnauthorized(token + " unauthorized") + }) + dynamicSet := dynamicclientset.New(tekton.WithClient(dynamicClient)) + return defaultDiscoverClient, dynamicSet, nil + } + + return defaultDiscoverClient, defaultDynamicClient, nil +} + +func TestHandleEventWithInterceptorsAndTriggerAuth(t *testing.T) { + for _, testCase := range []struct { + userVal string + statusCode int + }{ + { + userVal: userWithoutPermissions, + statusCode: http.StatusUnauthorized, + }, + { + userVal: userWithPermissions, + statusCode: http.StatusCreated, + }, + } { + eventBody := json.RawMessage(`{"head_commit": {"id": "testrevision"}, "repository": {"url": "testurl"}, "foo": "bar\t\r\nbaz昨"}`) + + pipelineResource := pipelinev1alpha1.PipelineResource{ + TypeMeta: metav1.TypeMeta{ + APIVersion: "tekton.dev/v1alpha1", + Kind: "PipelineResource", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "my-pipelineresource", + Namespace: namespace, + }, + Spec: pipelinev1alpha1.PipelineResourceSpec{ + Type: pipelinev1.PipelineResourceTypeGit, + Params: []pipelinev1.ResourceParam{{ + Name: "url", + Value: "$(params.url)", + }}, + }, + } + pipelineResourceBytes, err := json.Marshal(pipelineResource) + if err != nil { + t.Fatalf("Error unmarshalling pipelineResource: %s", err) + } + + tt := bldr.TriggerTemplate("tt", namespace, + bldr.TriggerTemplateSpec( + bldr.TriggerTemplateParam("url", "", ""), + bldr.TriggerResourceTemplate(runtime.RawExtension{Raw: pipelineResourceBytes}), + )) + tb := bldr.TriggerBinding("tb", namespace, + bldr.TriggerBindingSpec( + bldr.TriggerBindingParam("url", "$(body.repository.url)"), + )) + + authSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: testCase.userVal, + Namespace: testCase.userVal, + UID: types.UID(testCase.userVal), + Annotations: map[string]string{ + corev1.ServiceAccountNameKey: testCase.userVal, + corev1.ServiceAccountUIDKey: testCase.userVal, + }, + }, + Type: corev1.SecretTypeServiceAccountToken, + Data: map[string][]byte{ + corev1.ServiceAccountTokenKey: []byte(testCase.userVal), + }, + } + authSA := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: testCase.userVal, + Namespace: testCase.userVal, + UID: types.UID(testCase.userVal), + }, + Secrets: []corev1.ObjectReference{ + { + Name: testCase.userVal, + Namespace: testCase.userVal, + }, + }, + } + + el := &triggersv1.EventListener{ + ObjectMeta: metav1.ObjectMeta{ + Name: "el", + Namespace: namespace, + }, + Spec: triggersv1.EventListenerSpec{ + Triggers: []triggersv1.EventListenerTrigger{{ + ServiceAccount: &corev1.ObjectReference{ + Namespace: testCase.userVal, + Name: testCase.userVal, + }, + Bindings: []*triggersv1.EventListenerBinding{{Name: "tb", Kind: "TriggerBinding"}}, + Template: triggersv1.EventListenerTemplate{Name: "tt"}, + Interceptors: []*triggersv1.EventInterceptor{{ + GitHub: &triggersv1.GitHubInterceptor{ + SecretRef: &triggersv1.SecretRef{ + SecretKey: "secretKey", + SecretName: "secret", + Namespace: namespace, + }, + EventTypes: []string{"pull_request"}, + }, + }}, + }}, + }, + } + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "secret", + Namespace: namespace, + }, + Data: map[string][]byte{ + "secretKey": []byte("secret"), + }, + } + + resources := test.Resources{ + TriggerBindings: []*triggersv1.TriggerBinding{tb}, + TriggerTemplates: []*triggersv1.TriggerTemplate{tt}, + EventListeners: []*triggersv1.EventListener{el}, + Secrets: []*corev1.Secret{secret, authSecret}, + ServiceAccounts: []*corev1.ServiceAccount{authSA}, + } + sink, dynamicClient := getSinkAssets(t, resources, el.Name, fakeAuth{}) + ts := httptest.NewServer(http.HandlerFunc(sink.HandleEvent)) + defer ts.Close() + + triggerAuthWG.Add(1) + + dynamicClient.PrependReactor("*", "*", func(action ktesting.Action) (handled bool, ret runtime.Object, err error) { + defer triggerAuthWG.Done() + return false, nil, nil + }) + + req, err := http.NewRequest("POST", ts.URL, bytes.NewReader(eventBody)) + if err != nil { + t.Fatalf("Error creating Post request: %s", err) + } + req.Header.Add("Content-Type", "application/json") + req.Header.Add("X-Github-Event", "pull_request") + // 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. + req.Header.Add("X-Hub-Signature", "sha1=c0f3a2bbd1cdb062ba4f54b2a1cad3d171b7a129") + + resp, err := http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("Error sending Post request: %v", err) + } + + if resp.StatusCode != testCase.statusCode { + t.Fatalf("Response code doesn't match: expected status code %d vs. actual %d, entire statuts %v", + testCase.statusCode, + resp.StatusCode, + resp.Status) + } + } + +} diff --git a/test/builder/eventlistener.go b/test/builder/eventlistener.go index f4ec03038..02664fbbb 100644 --- a/test/builder/eventlistener.go +++ b/test/builder/eventlistener.go @@ -151,6 +151,16 @@ func EventListenerTriggerName(name string) EventListenerTriggerOp { } } +// EventListenerTriggerServiceAccount set the specified ServiceAccount of the EventListenerTrigger. +func EventListenerTriggerServiceAccount(saName, namespace string) EventListenerTriggerOp { + return func(trigger *v1alpha1.EventListenerTrigger) { + trigger.ServiceAccount = &corev1.ObjectReference{ + Namespace: saName, + Name: namespace, + } + } +} + // EventListenerTriggerBinding adds a Binding to the Trigger in EventListenerSpec Triggers. func EventListenerTriggerBinding(name, kind, apiVersion string) EventListenerTriggerOp { return func(trigger *v1alpha1.EventListenerTrigger) { diff --git a/test/controller.go b/test/controller.go index d4c6c4307..fc698eb58 100644 --- a/test/controller.go +++ b/test/controller.go @@ -45,6 +45,7 @@ import ( fakeconfigmapinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/configmap/fake" fakesecretinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/secret/fake" fakeserviceinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/service/fake" + fakeserviceaccountinformer "knative.dev/pkg/client/injection/kube/informers/core/v1/serviceaccount/fake" "knative.dev/pkg/controller" ) @@ -60,6 +61,7 @@ type Resources struct { Services []*corev1.Service ConfigMaps []*corev1.ConfigMap Secrets []*corev1.Secret + ServiceAccounts []*corev1.ServiceAccount } // Clients holds references to clients which are useful for reconciler tests. @@ -99,6 +101,7 @@ func SeedResources(t *testing.T, ctx context.Context, r Resources) Clients { serviceInformer := fakeserviceinformer.Get(ctx) configMapInformer := fakeconfigmapinformer.Get(ctx) secretInformer := fakesecretinformer.Get(ctx) + saInformer := fakeserviceaccountinformer.Get(ctx) // Create Namespaces for _, ns := range r.Namespaces { @@ -173,6 +176,14 @@ func SeedResources(t *testing.T, ctx context.Context, r Resources) Clients { t.Fatal(err) } } + for _, sa := range r.ServiceAccounts { + if err := saInformer.Informer().GetIndexer().Add(sa); err != nil { + t.Fatal(err) + } + if _, err := c.Kube.CoreV1().ServiceAccounts(sa.Namespace).Create(sa); err != nil { + t.Fatal(err) + } + } c.Kube.ClearActions() c.Triggers.ClearActions() diff --git a/test/eventlistener_test.go b/test/eventlistener_test.go index e87274262..31d814f75 100644 --- a/test/eventlistener_test.go +++ b/test/eventlistener_test.go @@ -42,6 +42,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" knativetest "knative.dev/pkg/test" ) @@ -66,8 +67,8 @@ func TestEventListenerCreate(t *testing.T) { c, namespace := setup(t) t.Parallel() - defer cleanup(t, c, namespace) - knativetest.CleanupOnInterrupt(func() { cleanup(t, c, namespace) }, t.Logf) + defer cleanup(t, c, namespace, "my-eventlistener") + knativetest.CleanupOnInterrupt(func() { cleanup(t, c, namespace, "my-eventlistener") }, t.Logf) t.Log("Start EventListener e2e test") @@ -190,7 +191,7 @@ func TestEventListenerCreate(t *testing.T) { Verbs: []string{"create"}, }, { APIGroups: []string{""}, - Resources: []string{"configmaps"}, + Resources: []string{"configmaps", "serviceaccounts", "secrets"}, Verbs: []string{"get", "list", "watch"}, }, }, @@ -367,24 +368,62 @@ func TestEventListenerCreate(t *testing.T) { } } - // Delete EventListener - err = c.TriggersClient.TriggersV1alpha1().EventListeners(namespace).Delete(el.Name, &metav1.DeleteOptions{}) + // Now let's override auth at the trigger level and make sure we get a permission problem + + // create SA/secret with insufficient permissions to set at trigger level + userWithoutPermissions := "user-with-no-permissions" + triggerSA := &corev1.ServiceAccount{ + ObjectMeta: metav1.ObjectMeta{ + Name: userWithoutPermissions, + Namespace: namespace, + UID: types.UID(userWithoutPermissions), + }, + } + + _, err = c.KubeClient.CoreV1().ServiceAccounts(namespace).Create(triggerSA) if err != nil { - t.Fatalf("Failed to delete EventListener: %s", err) + t.Fatalf("Error creating trigger SA: %s", err.Error()) } - t.Log("Deleted EventListener") - // Verify the EventListener's Deployment is deleted - if err = WaitFor(deploymentNotExist(t, c, namespace, fmt.Sprintf("%s-%s", eventReconciler.GeneratedResourcePrefix, el.Name))); err != nil { - t.Fatalf("Failed to delete EventListener Deployment: %s", err) + if err := WaitFor(func() (bool, error) { + el, err := c.TriggersClient.TriggersV1alpha1().EventListeners(namespace).Get(el.Name, metav1.GetOptions{}) + if err != nil { + return false, nil + } + for i, trigger := range el.Spec.Triggers { + trigger.ServiceAccount = &corev1.ObjectReference{ + Namespace: namespace, + Name: userWithoutPermissions, + } + el.Spec.Triggers[i] = trigger + } + _, err = c.TriggersClient.TriggersV1alpha1().EventListeners(namespace).Update(el) + if err != nil { + return false, nil + } + return true, nil + }); err != nil { + t.Fatalf("Failed to update EventListener for trigger auth test: %s", err) } - t.Log("EventListener's Deployment was deleted") - // Verify the EventListener's Service is deleted - if err = WaitFor(serviceNotExist(t, c, namespace, fmt.Sprintf("%s-%s", eventReconciler.GeneratedResourcePrefix, el.Name))); err != nil { - t.Fatalf("Failed to delete EventListener Service: %s", err) + // Verify the EventListener is ready with the new update + if err := WaitFor(eventListenerReady(t, c, namespace, el.Name)); err != nil { + t.Fatalf("EventListener not ready after trigger auth update: %s", err) + } + // Send POST request to EventListener sink + req, err = http.NewRequest("POST", fmt.Sprintf("http://127.0.0.1:%s", portString), bytes.NewBuffer(eventBodyJSON)) + if err != nil { + t.Fatalf("Error creating POST request for trigger auth: %s", err) + } + req.Header.Add("Content-Type", "application/json") + resp, err = http.DefaultClient.Do(req) + if err != nil { + t.Fatalf("Error sending POST request for trigger auth: %s", err) + } + + if resp.StatusCode != http.StatusUnauthorized && resp.StatusCode != http.StatusForbidden { + t.Errorf("sink did not return 401/403 response. Got status code: %d", resp.StatusCode) } - t.Log("EventListener's Service was deleted") } // The structure of this field corresponds to values for the `license` key in @@ -424,9 +463,28 @@ func compareParamsWithLicenseJSON(x, y v1alpha1.ResourceParam) bool { return false } -func cleanup(t *testing.T, c *clients, n string) { +func cleanup(t *testing.T, c *clients, namespace, elName string) { t.Helper() - tearDown(t, c, n) + tearDown(t, c, namespace) + // Delete EventListener + err := c.TriggersClient.TriggersV1alpha1().EventListeners(namespace).Delete(elName, &metav1.DeleteOptions{}) + if err != nil { + t.Fatalf("Failed to delete EventListener: %s", err) + } + t.Log("Deleted EventListener") + + // Verify the EventListener's Deployment is deleted + if err = WaitFor(deploymentNotExist(t, c, namespace, fmt.Sprintf("%s-%s", eventReconciler.GeneratedResourcePrefix, elName))); err != nil { + t.Fatalf("Failed to delete EventListener Deployment: %s", err) + } + t.Log("EventListener's Deployment was deleted") + + // Verify the EventListener's Service is deleted + if err = WaitFor(serviceNotExist(t, c, namespace, fmt.Sprintf("%s-%s", eventReconciler.GeneratedResourcePrefix, elName))); err != nil { + t.Fatalf("Failed to delete EventListener Service: %s", err) + } + t.Log("EventListener's Service was deleted") + // Cleanup cluster-scoped resources t.Logf("Deleting cluster-scoped resources") if err := c.KubeClient.RbacV1().ClusterRoles().Delete("my-role", &metav1.DeleteOptions{}); err != nil {