Bug 1415868 - create hooks and roles for actions as hooks #20

djmitche · 2018-04-25T22:10:19Z

No description provided.

tomprince

This looks good. I've suggested a few changes, but this is involved enough that I think another pass after they are made is probably worthwhile.

tomprince · 2018-04-26T15:41:58Z

src/make-scm-group-roles.js

    var roleId = `mozilla-group:active_scm_level_${level}`;
-    var scopes = projectsWithGroup.map(project => {
+
+    // See https://bugzilla.mozilla.org/show_bug.cgi?id=1415868 for "old" and "new"


yeah, we need to not take scopes away from people until the new stuff works!

I'm more unhappy with the variables names. But if this exists for a short enough period, it won't be a problem.

tomprince · 2018-04-26T15:43:48Z

src/update-action-hook-perms.js

+    .description('update action-related `hooks:trigger-hook:` scopes in mozilla-group:* roles');
+};
+
+EXPECTED_PERMS = [


I wonder if this wants to be in ci-configuration?

Yes, almost all of this does -- but I'm holding that for a follow-up.

tomprince · 2018-04-26T15:46:21Z

src/update-action-hook-perms.js

+    {trustDomain: 'gecko', level: '3', actionPerm: 'relpromo'},
+  ]},
+  {group: 'releng', actions: [
+    {trustDomain: 'gecko', level: '3', actionPerm: 'relpromo'},


Probably want to give comm/relpromo here too.

Also, at least releng (but potentially wider) will want level 1/2 scopes for this too. We eventually want to be able to do staging release on try, and I've already been doing it a bit.

Oh, I meant to change this from relpromo to something more innocuous like purge-cache to start with. I don't want to mess with "scary things" yet! But yes, this will be the place to add such scopes.

tomprince · 2018-04-26T15:50:21Z

src/util/tcyml.js

+
+const CENTRAL_RAW = 'https://hg.mozilla.org/mozilla-central/raw-file/default/';
+
+exports.getCentralTaskclusterYml = async () => {


I think it would make more sense for this to grabbed per-project (even if you only use it for m-c here).

We don't have a hook per project, though -- only per level. We could potentially have one per repo, but then we have a hook per repo, per actionPerm, and we are granting those individual trigger-hook scopes to users, so it's quite an explosion. I don't see a great benefit, either -- it would just leave us running tcadmin on every uplift, if there was something important changing in .taskcluster.yml.

I'm happy if you call it only with mozilla-central (although I think we also need to call it with comm-central), but I think it is worth making the utility general, even if the callers aren't).

tomprince · 2018-04-26T15:50:42Z

src/util/action-hooks.js

@@ -0,0 +1,17 @@
+exports.ACTION_HOOKS = [


ci-configuration?

tomprince · 2018-04-26T16:11:47Z

src/make-gecko-action-hooks.js

+      tasks_for: 'action',
+      action: {
+        $merge: [
+          {$eval: 'payload.decision.action'},


Would there be any value in changing this so that the stuff that can be overridden is in a sub-key here, instead of merging? This will require some .taskcluster.yml refactoring, but I think it will be less error prone.

There are only two, and one of them (cb_name) is only sometimes overridden. So I'm not sure that'd be less confusing. Also, a sub-key would still require merging..

We could have (in python python psuedo-code):

repo_scope = {...} if action.actionPerm == 'generic': cb_name = action.pop('cb_name') else: cb_name = action.actionPerm task = { ... 'action': { 'repo_scope': repo_scope, 'cb_name': {'$eval': 'payload.action.cb_name', 'params': {'$eval': 'payload.action.params'}, }, ... }

with bikesheading over param.

I laid out the three contexts concerned here in this gist.

My initial reaction to your comment is that stuffWeWillNotOverride would be a better name than params. It seems to be designing the data structure to suit the software constructing it (and I'm not even convinced the proposed design suits any better). We could potentially flatten out all of the stuff in the action subkey.

I'd like to introduce minimal churn here, and keep risk of breaking existing (kind=task) actions as low as possible. Is that risk worthwhile?

My concern is less about what the code looks like than about security; making it harder to future changes to accidentally let the security sensitive values be overridden (which suggests the name should be something like untrusted, maybe).

To be clear, I don't think the code as-is is insecure. I just think that it would be easy for somebody to not realize the security impact of the code (for example, that the order of merging is critical). By moving the security critical values outside of the user-provided dictionary, I think that makes mistakes here less likely.

Hm, I see. So, a few things to change:

add detail to hooks' triggerSchema to indiciate specific keys in action, push, and repository

don't include repo_scope in the hookPayload (and don't generate it in the in-tree code)

Rework these overrides to be a little clearer that we either take the given value, or force it, and why (with some comments).

…lines

djmitche · 2018-05-01T14:46:58Z

OK, this is substantially updated to reflect reviews and conversation on the topic.

tomprince

This looks good! Thanks for the changes. I've just got a minor nit about an unclear comment.

tomprince · 2018-05-01T15:12:58Z

src/make-gecko-action-hooks.js

+        taskGroupId: '${payload.decision.action.taskGroupId}',
+        // repo_scope is calculated in the hook's JSON-e rendering; note that this is limited
+        // by the hook's `hook-id:..` role, too
+        repo_scope: 'assume:repo:${payload.decision.repository.url[8:]}:action:' + action.actionPerm,


It took me a few minute to convince myself that this was safe. I think it would make sense to specifically call out that this can only allow scopes that are also given to the hook-id:* role, so that the validation of the repository url happens there.

Thanks - updated

Also, simplify some logic by requiring that repository.url not have a trailing slash

jonasfj · 2018-05-01T17:27:30Z

src/util/action-hooks.js

+    trustDomain: 'gecko',
+    level: '1',
+    actionPerm: 'purge-caches',
+    groups: ['taskcluster', 'vpn_sheriff'],


I see no reason this should be so limited :)

But it's a good example nonetheless...

Yeah, we can work out the permissions later -- this is almost just placeholder stuff right now.

djmitche added 4 commits April 19, 2018 17:11

Bug 1415868 - rename to make-scm-group-roles and do all levels

8657fa9

Bug 1415868 - add new action roles but leave the repo roles

ac13602

Bug 1415868 - manage hook access in other mozilla-group roles

5702e13

Bug 1415868 - make a generic editHook function

12331d2

djmitche self-assigned this Apr 25, 2018

djmitche requested review from jonasfj and tomprince April 25, 2018 22:10

tomprince suggested changes Apr 26, 2018

View reviewed changes

djmitche force-pushed the bug1415868 branch from 29e675e to 5822b11 Compare April 27, 2018 22:23

djmitche added 5 commits May 1, 2018 10:46

Bug 1415868 - read actions from a list of actions

baa4ae1

Bug 1415868 - make push-specific roles and grant in-tree:.. to them

3306f08

Bug 1415868 - make getTaskclusterYml project-agnostic

4cf345c

Bug 1415868 - make diff display better, with context and no stray new…

f145436

…lines

Bug 1415868 - add much more detail to triggerSchema

05e6959

djmitche force-pushed the bug1415868 branch from 025fce2 to 7f2ec36 Compare May 1, 2018 14:46

djmitche requested a review from tomprince May 1, 2018 14:47

tomprince approved these changes May 1, 2018

View reviewed changes

djmitche added 5 commits May 1, 2018 11:44

Bug 1415868 - specify each part of the 'action' property directly

fb8e94a

Also, simplify some logic by requiring that repository.url not have a trailing slash

Bug 1415868 - drop unnecessary repo_scope

9bf15a4

Bug 1415868 - drop unnecessary task property

7d39f55

Bug 1415868 - use the taskId value from hooks for ownTaskId

e848d1e

Bug 1415868 - use comm-central's .taskcluster.yml for comm-central hooks

2eb4ce8

djmitche force-pushed the bug1415868 branch from 7f2ec36 to 2eb4ce8 Compare May 1, 2018 15:44

jonasfj approved these changes May 1, 2018

View reviewed changes

djmitche merged commit 1562e45 into master May 1, 2018


		const CENTRAL_RAW = 'https://hg.mozilla.org/mozilla-central/raw-file/default/';

		exports.getCentralTaskclusterYml = async () => {

Bug 1415868 - create hooks and roles for actions as hooks #20

Bug 1415868 - create hooks and roles for actions as hooks #20

Uh oh!

Conversation

djmitche commented Apr 25, 2018

Uh oh!

tomprince 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!

djmitche commented May 1, 2018

Uh oh!

tomprince 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!

Uh oh!