Securing triggering

[akihikokuroda]

Expected Behavior

The triggering event must be communicated from the event source to the triggering in a secure way and the event must be authenticated before the processing.

1. TLS should be optionally enabled for the communication.
2. The event / payload must be authenticated in the way provided by the event source.

Actual Behavior

I believe that these are necessary in the production environment so must be implemented some how but my question is if this is in the scope of the triggers.

The TLS can be implemented by defining the ingress or route for the event listener service. They are terminate the TLS and send the plain event to the event listener.

The authentication needs some additional code in the processing of the event (I just looked at the github webhook case, so far). I can think of 2 ways to approach this. The first one is to add an hook in the event listener for plugins to pre-process the events. The other one is add a sidecar to the event listener pod to receive the event and authenticate the event before pass to the event listener.

Steps to Reproduce the Problem

Additional Info

Github info:
https://developer.github.com/webhooks/securing/
https://developer.github.com/webhooks/creating/

[ncskier]

Thanks for creating this issue. Security is a very important topic that we've been discussing recently, so it's crucial to document these issues/security concerns 👍

I misunderstood the GitHub validation issue; I thought it was just a string match (like GitLab webhooks seem to be), but the hash & compare is more in-depth. Would it be feasible to do this hash & comparison using the Conditions being discussed here? If I understand correctly, we will also want to read the GitHub secret token from a mounted Secret; is this something that Conditions will support?
@bobcatfish @dlorenc @vtereso

As for setting up the ingress or route for the EventListener Service, I'm unsure whether that should be Triggers' responsibility. I'm starting to think that it might be best to just create a ClusterIP Service, and let the user configure things on top of the Service however the user wants 🤔

[akihikokuroda]

I agree about the TLS. When the user create the eventlistener CD, he/she should create the ingress route and uses the external address of them to the webhook configuration.

I looked at the knative code that handling the github authentication

It is using

gopkg.in/go-playground/webhooks.v5/github/github.go

in the eventing-contrib github so it's github unique code.

The core of the code is only a few line like this:

       if len(hook.secret) > 0 {
        signature := r.Header.Get("X-Hub-Signature")
                if len(signature) == 0 {
                        return nil, ErrMissingHubSignatureHeader
                }
                mac := hmac.New(sha1.New, []byte(hook.secret))
                _, _ = mac.Write(payload)
                expectedMAC := hex.EncodeToString(mac.Sum(nil))


                if !hmac.Equal([]byte(signature[5:]), []byte(expectedMAC)) {
                        return nil, ErrHMACVerificationFailed
                }
        }

So it is not too bad to include this in the eventlistener.

[vincent-pli]

I guess there should be some third part components stand in front of EventListener to handle the authentication, like knative/event-contrib/githubAdapter or gitlabsource.

I think EventLister should not cover it, since different event source has different authentication, we can not handle them all.

[ncskier]

In an effort to track all of the issues related to security, issue45 also mentions the GitHub hash authentication (number 2 in this issue). FYI @khrm 🙂

[bobcatfish]

In the interests of organizing this a bit, I've turned this issue into an "epic" (visible via zenhub) with these issues in it:

• Add clear docs on how to set up TLS correctly for EventListener #80 to document setting up TLS
• Enable TriggerBindings to validate requests (e.g. GitHub) #45 matching event headers / GitHub auth
• Proposal to separate Validation / Filtering from TriggerBinding #71 separating validation / filtering from binding

[bobcatfish]

And depending on how we decide to implement #45 we might want to bring in #49 as well

[EliZucker]

since different event source has different authentication, we can not handle them all.

I tend to agree with @vincent-pli. This issue goes back to the discussion of how much event-specific code we want to build into the EventListener. Broadly, my understanding is that we want the EventListener to accept event-agnostic HTTP messages while hopefully allowing users to take in simple HTTP-native event messages (from services like Slack or GitHub) without a more advanced event-specific adapter.

Imagine we include GitHub hash validation in the EventListener. Then, imagine a bit later we also decide to include Slack hash validation (because Slack webhooks also natively emit HTTP messages). Well, looking at how to verify slack requests, it seems like the process is unique in some ways:

[STEP 2:] Concatenate the version number, the timestamp, and the body of the request to form a basestring. Use a colon as the delimiter between the three elements. For example, v0:123456789:command=/weather&text=94070. The version number right now is always v0.
[STEP 3:] With the help of HMAC SHA256 implemented in your favorite programming, hash the above basestring, using the Slack Signing Secret as the key.

Personally, I could see this getting out of hand pretty quickly. We might eventually add validation logic specific to HTTP messages from sendgrid, twilio, hubspot, etc (I just googled services that emit events over http). Eventually, we have our own internal library of event-specific adapters like knative eventing and argo events do. Also note that this is just for HTTP-emitting events. Presumably, more intricate event-specific code will have to exist somewhere for events that don't send their data over HTTP -- like cron jobs or Kafka off the top of my head.

So my take on this is to keep the EventListener as event-agnostic as possible. An approach for this might be to remove headermatching and just have separate prewritten conditionals in a catalog for secure verification of some specific http event types.

My highly-opinionated view is that when Tekton Triggers becomes fully developed, and we have created or recommend an existing catalog of event-specific adapters (that emit http messages, probably cloudevents), accepting events through lightweight adapters will be the approach most users take despite the generic HTTP-parsing ability we have. For instance, something like the Github adapter from knative-contrib that automatically configures a github webhook for the user and abstracts away any security validation details.

[vincent-pli]

My highly-opinionated view is that when Tekton Triggers becomes fully developed, and we have created or recommend an existing catalog of event-specific adapters

Agree with @EliZucker
Just like Knative/event-contrib, but seems recently there is a trend to move components in Knative/event-contrib to Knative/event, it make them heavier to adopt by us. so, I think we need consider create ours.

[ncskier]

Waiting for documentation from @vtereso to close out this issue