Enable TriggerBindings to validate requests (e.g. GitHub) about triggers HOT 20 CLOSED

I assign myself to work on this. Yet to start. Just following the discussions atm.

from triggers.

If I understand @dlorenc use case, it's about securing webhook? I don't think we can validate token which is set up for webhook using just regex.

@vtereso what do you think about expanding this issue to be about authenticating against GitHub including hashing the payload ?

from triggers.

@khrm I've tried to group together issues related to this: #78 (comment)

It sounds like for this issue our next step will be to create a design for how we express authenticating the caller.

from triggers.

@khrm I thought it might not fit. I might not understand the conditional correctly. I'll look it, again.

from triggers.

As far as the manifestation, I don't have particular feelings one way or the other. It seems like the PipelineRun reconciler is where the only place where Conditions are resolved into TaskRuns. Looking at this briefly, it seems like we would have to recreate some of this, which is less than ideal, but maybe not too bad? Otherwise, we could have a condition section that allows users to spec out PodTemplates/Tasks/TaskRuns or something of the sort to be explicit like @akihikokuroda mentioned.

from triggers.

/assign

from triggers.

Actually, not. Someone else feel free to tackle this. :)
/unassign

from triggers.

/assign

from triggers.

This probably should have been blocked since #47 and #55 are still open. @khrm how are you handling it in the meantime?

from triggers.

Hey @vtereso !

I'm late to the headersMatch discussion - can we add some details here (or even in a separate design doc) about what use cases we need the headerMatch field for and how it would work.

From the example in #42 it looked like the main use case was for filtering based on the type of event, e.g.:

  headerMatch:
    - X-GitHub-Event: issues

But then I asked @dlorenc and he said the most important use case was about verifying that requests actually come from GitHub i.e. matching a token in the header against a previously known token - it's not clear to me from the designs I've seen how we would do that (esp if the matching is just a simple regex as we are discussing in #49) so I'd love to see more details about that if possible.

from triggers.

I have mentioned in the conditionals discussion here, the headers field was an implicit way to access the headers without having to introduce another interpolation variable. I believe the intent was to "match" against anything that could be passed here, which could both designate and authenticate. It has been mentioned that an explicit separation between headers/conditionals might be good for the sake of clarity.

I have thrown around the idea of combining them before, but that would imply introducing another variable to access the headers, which might be fine? From the recent discussion on that conditionals issue (in favor of using containers), headers seem to be overlapping more and more.

from triggers.

@vtereso @bobcatfish

I felt the general use case for headers match would be just simple match to send events to matching TriggerBinding whether it be Github, Gitlab or CE type coming in headers.

If I understand @dlorenc use case, it's about securing webhook? I don't think we can validate token which is set up for webhook using just regex.

Github computes the hash of payload using the above token and then pass the computed hash to the header.
And on triggers side, if we want to be sure, we compute the hash of payload using that token(needs to be stored) and match it with the one received in the header.

PS: I was on leave so the late response. I would be more active from Tuesday.

from triggers.

For the authentication, we can extend the Filter idea in #71 to run the (Tekton)task. We need to create the TriggeringEvent pipeline resource that has the whole event header and payload. The task authenticates the event using the resource and success or failure based on the result.
In this way, all the event source unique handling are in the task definitions. We can have them in the catalog.

from triggers.

Interesting idea to use a Tekton Task to execute the validation 🙂 This sounds to me like an equivalent alternative to the conditionals being discussed in issue49. We could potentially use Tasks instead of Conditionals?

Also, just a thought; instead of creating an "event" PipelineResource to pass the event header & payload to the Task, could we pass them to the Task as parameters?

from triggers.

I thought we can have validation using conditionals. Just different pod for different providers. Like github pod, gitlab pod, etc.
So that if some other event source in future is needed, we don't need to change logic, just have a pod.

Why use tasks over conditionals, @akihikokuroda ?

from triggers.

Also, just a thought; instead of creating an "event" PipelineResource to pass the event header & payload to the Task, could we pass them to the Task as parameters?

Yea. I was thinking of doing the same.

@vtereso @akihikokuroda
But I am still in doubt about whether it's OK to have a taskrun / pipelinerun(which has conditions) every time a new event arrives to check for authentication before proceeding with creation of resources?

from triggers.

@khrm I looked at the implementation of the condition. The condition is executed as a task with one step in it internally. https://github.com/tektoncd/pipeline/blob/830ffc26e6bee0a5d11ce95ec328b2ef448f00de/pkg/reconciler/pipelinerun/pipelinerun.go#L624
Using task explicitly may be more clear and easy to understand.

from triggers.

But I am still in doubt about whether it's OK to have a taskrun / pipelinerun(which has conditions) every time a new event arrives to check for authentication before proceeding with creation of resources?

I had the same reservation, but I have come around. To the point of GitHub authentication being non-trivial, the same can be said about other possible event sources and we don't want to shift this responsibility to Triggers directly. There is nothing stopping us from adding functionality to allow people to raw match/string match/etc. down the line to improve performance should we feel it necessary.

If we consider Triggers as an extension point for runs, it should be reasonable to have run success toggle other runs. In this way, supporting Conditionals/images seems like a good approach since it follows the Triggers paradigm of supporting anything assuming it is configured properly.

from triggers.

Coming in late to all of this! Looks like a lot of discussion around whether or not we should use the condition implementation from pipelines. A few thoughts:

I thought we can have validation using conditionals. Just different pod for different providers. Like github pod, gitlab pod, etc.

+1 I like this!

But I am still in doubt about whether it's OK to have a taskrun / pipelinerun(which has conditions) every time a new event arrives to check for authentication before proceeding with creation of resources?

I think running an entire pipelinerun just to use conditions would be overkill. Though the current implementation is tied to pipelines, we could refactor the condition checking out into something that could be reusable.

I think using TaskRuns is a good starting point. Besides reuse we get some additional features like timeouts, retries, variable substitution support etc. And as @vtereso says, it leaves room for optimization for simpler cases later on if we do determine that scheduling/running TaskRuns does not meet our performance/scaling goals.

As far as the manifestation, I don't have particular feelings one way or the other. It seems like the PipelineRun reconciler is where the only place where Conditions are resolved into TaskRuns. Looking at this briefly, it seems like we would have to recreate some of this, which is less than ideal, but maybe not too bad?

Yeah, I think it would make sense to refactor this out into a package that can be reused by both pipelines, and triggers

Otherwise, we could have a condition section that allows users to spec out PodTemplates/Tasks/TaskRuns or something of the sort to be explicit like @akihikokuroda mentioned.

I think these could work though I'm not sure you'd want to expose the full Task spec (e.g. does it make sense to have pipeline resources here?).

For conditionals in pipelines, we did briefly think about just using regular tasks but decided to go with a more restricted subset (i.e. Condition). A Condition is basically a subset of a Task as @akihikokuroda said. Besides being a single step, it also does not allow things like output resources. So, it might work better though I also like the Filter type proposed in in #71

In summary, I think it makes sense to run TaskRuns to execute the validation/filtering/conditional logic. It would be neat if we could reuse the pipeline condition implementation (though it would require some refactoring). We could let people define the logic either via regular Tasks, or something more specialized like Conditions or maybe even create even more specialized types like Filters. I have a slight preference towards reusing the Condition type though I'm willing to be convinced otherwise!

from triggers.

Follow up with @iancoffey for documentation of Validation.

from triggers.