1286989 - Add ability to trigger a hook with modified parameters

Status: RESOLVED FIXED

(Taskcluster :: Services, defect)

Description

[Anthony Miyaguchi [:amiyaguchi]]

Currently hooks are immutable, predefined tasks that can be periodically scheduled. It would be useful to allow for some degree of flexibility by allowing for parameterized task definitions.

This would be useful for creating nightly builds. There would be a hook to launch a modified decision-task at the tip revision of a repo. If the nightly build happens to break, someone would want to manually re-trigger the same hook with a modified `GECKO_HEAD_REV`.

This is currently not possible with the current API. We would want to let people with the permission to trigger the job have the ability to edit a predefined set of task attributes. For example, I should be able to edit task.payload.env.GECKO_HEAD_REV, but I should not be able to arbitrarily edit task.payload.command, since this hook may be operating with scopes that are well beyond mine. 

Allowing edits to the a task that will get run at an elevated privilege may be of some concern, so there should be a way to lock down the attributes that are considered safe to use. However, adding parameters would give hooks some needed flexibility.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

We need to be able to parameterize tasks to support triggering from pulse messages, as well.  So we'll incorporate some kind of templating language (e.g., mustache).  The re-triggering could accept a JSON blob for containing those parameters.

That said, a templating language isn't a good security boundary -- it would be pretty easy to formulate a JSON injection attack (similar to SQL injection).  So if this is a critical security boundary, access should be limited with scopes.  What is the threat you're looking to mitigate here?

Comment 2

[Anthony Miyaguchi [:amiyaguchi]]

I was just thinking about the case where this could expose the role that starts a nightly build process and signing to somehow sign arbitrary binaries. I don't think this a problem as long as the parameters are carefully chosen and the scopes are appropriately assigned.

But some other thoughts, the JSON blob could be validated against a schema using taskcluster-lib-validate which could limit the opportunities to pass on invalid or unintended data to the template.

Comment 3

[Dustin J. Mitchell [:dustin] (he/him)]

Yes, schema validation will help (it will be validated against the task definition schema).  I'm still not willing to consider that a security boundary, though.  Consider {"GECKO_HEAD_REV": "1 `echo $SOME_SECRET_VALUE`"} -- that would still validate against the schema, but a shell script might be tripped up later.

So, from a security standpoint, I think you should require the inputs to the task to be as trusted as the task itself.  That is, someone who can specify task inputs should also be able to edit the task.  Of course, those can still be separate scopes if, for example, you want to discourage sheriffs from editing the task definition.

By the way, bug 1225243 is the bug for triggering from pulse.  If you want to make a blocking bug to add templating support, that'd be great.  If you accidentally wrote a patch for that bug too I certainly would be happy to accidentally review it.

Comment 5

[Dustin J. Mitchell [:dustin] (he/him)]

(In reply to Dustin J. Mitchell [:dustin] from comment #3)
> Yes, schema validation will help (it will be validated against the task
> definition schema).  I'm still not willing to consider that a security
> boundary, though.  Consider {"GECKO_HEAD_REV": "1 `echo
> $SOME_SECRET_VALUE`"} -- that would still validate against the schema, but a
> shell script might be tripped up later.

I'm willing to consider this now.  The schema would need to be written carefully (so GECKO_HEAD_REV has to be /([0-9a-f]{32}|default)/ for example).

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

This is finished now.  Triggering fully supports JSON-e, with schemas and everything.