Bug 1415868 - docs for actions with kind=hook #250

djmitche · 2018-04-12T22:47:02Z

You might want to take this commit-by-commit.

Mostly I'd like some thought and feedback on the security implications. The risk is that someone makes a try job with the retrigger action configured to run hook project-gecko/in-tree-action-3-relpromo with a payload that promotes some malicious try build as Firefox 666. Then some innocent test task turns orange on the try job and they kindly ask someone in releng or relman to click "retrigger" for that task.

My thinking is that a "generic" actions UI would run the user through two dialogs for an hook action:

Enter the task input so that it matches the schema
See and confirm the hookId, hookGroupId, and rendered payload
The first can be skipped if there's no schema, of course.

Treeherder, however, can do a bit better:

Use authorizedScopes=['mozilla-group:scm_level_1'] for try, 2 for level-2 repos, etc.
For hard-wired things like 'r', check the hookGroupId/hookId
Do the two-phase confirmation for everything else.

This moves some of the more verbose schema descriptions out into the manual, leaving the schema quite a bit shorter. It will get longer when a new kind is added!

jonasfj

Over all I LOVE this!!!

Most of my comments are suggestions, but refactoring the schema is probably a good idea.

jonasfj · 2018-04-13T19:14:15Z

src/manual/using/actions/schema.yml

+              - task
+          - type: object
+            properties:
+              kind: hook


this doesn't work

jonasfj · 2018-04-13T19:16:20Z

src/manual/using/actions/schema.yml

-          items:
-            type: object
-            additionalProperties:
+      allOf:


I propose refactoring the schema into:

oneOf: - type: object properties: kind: {enum: task} ... - type: object properties: kind: {enum: hook} ...

even the two kinds share some properties, just duplicate those properties. It's okay to document them twice.
The allOf + anyOf refactoring is harder to do in your head.

Another way to reference definitions with $ref, this is okay if we render it using a viewer...

Putting definitions to be referenced under a top-level property definitions: {"mydef": ....} is convention in json schemas. Then reference it with $ref: '#/definitions/mydef'

jonasfj · 2018-04-13T19:23:22Z

src/manual/using/actions/ui.md

+decision task couldn't do.
+
+Instead, the user interface should require the user to confirm each action,
+displaying the hookGroupId, hookId, and hookPayload.


Please add something like:

Furthermore, hook implementation should be designed to guard against malicious input,
which could be triggered by phishing an unsuspecting administrator to trigger a hook action
from a low privileged task with bad intend, causing an evil hookPayload.

jonasfj · 2018-04-13T19:23:54Z

src/manual/using/actions/ui.md

+
+* it can limit the scopes available using
+  [authorizedScopes](https://docs.taskcluster.net/manual/design/apis/hawk/authorized-scopes)
+  based on some other definiitve information about the task -- for example,


*definiitve

jonasfj · 2018-04-13T19:25:06Z

src/manual/using/actions/spec.md

+
+The "task" kind requires that the user invoking the task has all of the scopes
+necessary to run the action task (and to create any further tasks). Such scopes
+might allow a user to perform undesirable actions with the Taskcluster API.


Granting such scopes allow a user to perform undesirable actions with the Taskcluster API

jonasfj · 2018-04-13T19:27:10Z

src/manual/using/actions/spec.md

+`hook-id:<hookGroupId>/<hookId>`. The method also verifies its payload against
+the hook's `triggerSchema`.  This allows a controlled privilege escalation:
+only clients with the appropriate `trigger-hook` scope may trigger a hook, and
+only if they provide a payload matching the schema. If both of those conditions


only if they provide a payload matching the schema

^ this is not a security assertion, as the attacker controls the schema in actions.json

Maybe rephrase this section... I love the sentence "This allows a controlled privilege escalation".

But I'm a little confused about the rest. As an action designer, using a hook means I can grant the user more restricted scopes. And I can use the hook to create a controlled privileged escalation, where my hook implementation sanitizes untrusted input.

I meant matching triggerSchema, which the attacker does not control. What is the confusion?

Okay, this is the triggerSchema in hooks.. I didn't see that got added.

Curious, will that validate input from both HTTP triggers and pulse triggers...

No, triggerSchema only applies to triggers. I think we decided to add an additional pulseMessageSchema, but I don't recall. Anyway, that part (pulse) isn't implemented yet.

jonasfj · 2018-04-13T19:33:22Z

src/manual/using/actions/spec.md

 JSON-e rendering performed by the hooks service.  Do not confuse the two
 operations!

+### Choosing a Kind


Some of this belong more in action design guidelines than the action specification.

This is a nit, but it would be nice to corner these discussion topics and examples into a separate document.
But I'm okay with this here too :)

I can move it to a guidelines.md..

jonasfj · 2018-04-13T19:38:45Z

src/manual/using/actions/spec.md

+
+Once the hook is complete and functional, only then should you design the
+`actions.json` entry to trigger it. An "hook" action is just a template for how
+to call `hooks.triggerHook` and does not offer any level of security.


At this point you should remember that the schema is only for documentation, and you can rely on the integrity of actions.json for security.

Yeah, cannot rely on...

jonasfj · 2018-04-13T19:39:30Z

src/manual/using/actions/spec.md

+are met, then the resulting task may execute with permissions the originating
+client does not posses.
+
+To design a hook-based action, start with the hook and consider the minimal


Could this be prefixed **Example, ** (in markdown)

That way it's easy to see that I don't care :)

It's not an example, though..

Could it have an informal title or something...

jonasfj · 2018-04-13T19:45:37Z

src/manual/using/actions/ui.md

+### "hook" Actions
+
+Hook actions have no such luxury, as they are intended to do things the
+decision task couldn't do.


We could add that UIs triggering hook actions should validate that the decision task posses the scope:

decision-task:reference-hook-action:<hookGroupId>/<hookId>

Obviously, the UI would have to call auth.expandScope(decisionTask.scopes) to determine the scopes.

This would protect against the case where a try-decision task is tweaked to add an action that triggers the hook to release firefox. As scm_level_1 doesn't have permission to reference said action.

@djmitche, @imbstack, maybe think this over ^, I'll admit my initial thought is that this is too complicated. That this is something we'll get wrong every time we add a new hook action. On the hand scope errors like this should be easy to make visible and understand.

Hm, I like that idea. It does sound like a good bit of work to implement, but not too bad..

It is probably worth looking at what the releng scriptworkers are doing. @escapewindow might have some thoughts on this.

It is probably worth looking at what the releng scriptworkers are doing. @escapewindow might have some thoughts on this.

That's a bit of a different situation -- recognizing a "legitimate" decision task for Firefox. The spec here is more general, and could apply to any project, Firefox or not, with any kind of decision task.

imbstack · 2018-04-13T19:43:41Z

src/manual/using/actions/spec.md


-Variables
---------
+In practice it's encourage that consumers employ a facility that can generate


imbstack · 2018-04-13T19:55:15Z

src/manual/using/actions/schema.yml

+                relevant. Thus, what context the action should be presented to the
+                end-user.
+            schema:
+              $ref: http://json-schema.org/schema


Will this be a valid external reference for things that don't fetch external references?

I don't know - I didn't change this part. This schema file is just to make Jonas happy anyway, and isn't machine-interpreted..

imbstack · 2018-04-13T20:04:18Z

src/manual/using/actions/ui.md

+decision task couldn't do.
+
+Instead, the user interface should require the user to confirm each action,
+displaying the hookGroupId, hookId, and hookPayload.


Not that we necessarily need to do this now, but I wonder if we could make this even more solid. Very poorly thought out ideas:

require linking to that hook in the tools site?

suggest displaying a "you have run this hook N times before" the way browsers do with ssl/website stuff

none of the above!

Uh, maybe require the UI display the name, description and owner properties of the hook.

That way a warning can be added to the description property indicating that the hook could do something bad... Stuff like that would smart..

djmitche · 2018-04-13T20:36:38Z

I really like the decision-task-scopes thing, but I'm hesitant to introduce a new top-level scope name (decision-task:..). I see that giving the decision task hooks:trigger-hook:.. would be bad (as we do not want a rogue L3 push to be able to start a release, for example). The solution is very similar to that for kind=task.

I can overcome my hesitation if necessary..

jonasfj · 2018-04-16T15:43:28Z

@djmitche, yeah, I don't like making a lot of new top-level scopes either...
Could we make some that are repo specific... or is this a scope that's like treeherder:... or something...

Maybe the top-level scope is related to actions.... The idea that there is a top-level scope that a task will need in-order to expose actions isn't crazy. But let's talk about how to mitigate phishing on vidyo or something.

jonasfj · 2018-04-16T18:47:51Z

src/manual/using/actions/schema.yml

+          - task
+      - type: object
+        properties:
+          kind: {enum: hook}


This has to be kind: {enum: ['hook']}

good point - I trusted #250 (comment) :)

jonasfj · 2018-04-16T19:30:25Z

src/manual/using/actions/schema.yml

+      anyOf:
+      - type: object
+        properties:
+          kind: {enum: task}


this should be {enum: ['task']}

djmitche · 2018-04-16T19:47:54Z

We settled on scope in-tree:hook-action:<hookGroupId>/<hookId>.

Note that there's no reason to allow the user to work around this in the UI. If you want to test a hook, just call hooks.triggerHook yourself.

djmitche added 3 commits April 12, 2018 22:01

Bug 1415868 - refactor actions doc to allow multiple kinds

b421e65

This moves some of the more verbose schema descriptions out into the manual, leaving the schema quite a bit shorter. It will get longer when a new kind is added!

Bug 1415868 - document kind=hook

6c9e7b4

Bug 1415868 - add a section on choosing a kind, security concerns

5e6625c

djmitche self-assigned this Apr 12, 2018

djmitche requested review from imbstack, jonasfj and tomprince April 12, 2018 22:47

jonasfj suggested changes Apr 13, 2018

View reviewed changes

imbstack reviewed Apr 13, 2018

View reviewed changes

djmitche force-pushed the bug1415868 branch from ad7e5cb to 5ac129e Compare April 13, 2018 23:45

jonasfj reviewed Apr 16, 2018

View reviewed changes

Bug 1415868 - refactor docs based on review comments

53c748b

djmitche force-pushed the bug1415868 branch from 5ac129e to 53c748b Compare April 16, 2018 20:44

djmitche requested a review from jonasfj April 16, 2018 20:57

use a decision-task scope check for hook actions

d7fd97c

djmitche force-pushed the bug1415868 branch from d1ebaca to d7fd97c Compare April 17, 2018 14:49

jonasfj approved these changes Apr 18, 2018

View reviewed changes

djmitche merged commit b4b6d0e into taskcluster:master Apr 18, 2018

Bug 1415868 - docs for actions with kind=hook #250

Bug 1415868 - docs for actions with kind=hook #250

Uh oh!

Conversation

djmitche commented Apr 12, 2018

Uh oh!

jonasfj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djmitche commented Apr 13, 2018

Uh oh!

jonasfj commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

djmitche commented Apr 16, 2018

Uh oh!

Reviewers

Assignees

Labels