Securing triggering about triggers HOT 9 CLOSED

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.

from triggers.

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.

from triggers.

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

• #80 to document setting up TLS
• #45 matching event headers / GitHub auth
• #71 separating validation / filtering from binding

from triggers.

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 🙂

from triggers.

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 🤔

from triggers.

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.

from triggers.

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

from triggers.

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.

from triggers.

Waiting for documentation from @vtereso to close out this issue

from triggers.