Closed Bug 1465842 Opened 6 years ago Closed 6 years ago

Create ci-configuration/scope-grants.yml

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: dustin, Assigned: dustin)

References

(Blocks 1 open bug)

Details

Attachments

(9 files, 1 obsolete file)

46 bytes, text/x-phabricator-request
bstack
: review+
tomprince
: review+
pmoore
: review+
Details | Review
46 bytes, text/x-phabricator-request
bstack
: review+
tomprince
: review+
pmoore
: review+
Details | Review
46 bytes, text/x-phabricator-request
bstack
: review+
tomprince
: review+
pmoore
: review+
Details | Review
46 bytes, text/x-phabricator-request
bstack
: review+
pmoore
: review+
Details | Review
46 bytes, text/x-phabricator-request
bstack
: review+
pmoore
: review+
tomprince
: review+
Details | Review
46 bytes, text/x-phabricator-request
bstack
: review+
tomprince
: review+
pmoore
: review+
Details | Review
46 bytes, text/x-phabricator-request
bstack
: review+
pmoore
: review+
tomprince
: review+
Details | Review
46 bytes, text/x-phabricator-request
bstack
: review+
pmoore
: review+
Details | Review
4.61 MB, application/json
Details
Jonas, is this something you could work on?

This is a "medium" priority, as gecko's complex role configuration is blocking a few changes that would help make things more secure, and in particular blocking the big change to remove repo scopes from user credentials after actions-as-hooks lands.
Flags: needinfo?(jopsen)
Flags: needinfo?(jopsen)
also, I have no idea why all that junk got copied into comment 0.
OK, so I think I have an idea how to implement this.  However, I don't want to screw it up.  So, to test, I'd like to start with a bunch of scopesets and make sure they expand to the same thing before and after.

Which means I need a way to simulate scope expansion locally.

Happily, I already wrote something (in Python!) back when doing the parameterized roles work.  I even still have it on a spare git branch.

Unhappily, its performance doesn't quite match that of taskcluster-auth.  I suspect that one of the more subtle changes we made in the parameterized role support is to blame, but I haven't quite figured it out.

Anyway, with that in place, I'll build a temporary harness to do some before-and-after comparisons (using the "current" and "generated" resources), so I can be sure that the set of grants I invent has exactly the same effect as the current hodge-podge.
A bunch of progress in https://hg.mozilla.org/users/dmitchell_mozilla.com/ci-admin/pushloghtml?changeset=7191ec114ed27eb9c6b1ca887b8a84a5e65f9870

Notably:
 * manage moz-tree:* roles
 * simulate scope expansion locally!
 * `ci-admin check`
 * ability to do some basic has-a-scope / doesn't-have-a-scope checks that will make the sec team happy /cc ulfr

This last bit is pretty cool.  I only really added `ci-admin check` so that I could verify my scope-grants.yml doesn't have any effect, but then I needed a better excuse to land it.  Checking scopes is just such an excuse!
Attachment #8995316 - Attachment description: Bug 1465842: refactor ciadmin.util into sub-modules Depends on D2143 → Bug 1465842: refactor ciadmin.util into sub-modules
This includes a resolver that emulates taskcluster-auth, and will be used to
make some checks about the result of proposed role changes.

Depends on D2436
TODO: once .taskcluster.yml is merged, run this as well

Depends on D2438
This is never going to be a super-secure check, but will help make sure we
don't do anything boneheaded.

Depends on D2439
If only because this avoids naming conflicts between modules defining the data
structure and modules generating resources based on those data structures..

Depends on D2442
In the same commit, this removes all other code that sets repo roles or user
roles, as otherwise they try to generate the same resources.  Checks in place
during development (but not to land) ensure that actual scopes do not change.

Depends on D2443
OK, I think I phabricated correctly, more-or-less.  What I'm looking for is a 50% review of these changes.  I'm also hoping to get Brian and Pete more familiar with ci-admin / ci-configuration.

This patch sequence accomplishes a few things:

 * centralizes administration of Firefox-related (taken broadly to include nss and TB) roles
 * makes those roles self-serve (via patch - someone still has to apply them)
 * allows some automated access checks like secops would prefer
 * prepares us to reduce scopes assigned to users (once actions as hooks are landed..)

I have a few things on my TODO list, on which I'd like feedback, too:

* add `ciadmin check` to .taskcluster.yml and run it periodically
* support multiple grant files (so, maybe grants.yml has `grants-from: [nss-grants.yml, action-grants.yml, ..]`)
* support utility roles ({grant: [<scopes>] to: {role: <roleId>}}) and manage 
  - lists of workers
  - sccache access
  - nightly-scopes (currently hand-managed!)
  - etc.
* break out the huge grants at the beginning of grants.yml into more manageable, topical chunks
  -- possibly using utility roles like lists of workers
* use parameterized roles automatically
  -- detect when a scope fits one of a few patterns, and automatically use a parameterized role fitting that pattern

I'm not sure the parameterized roles is necessary.  As written, this is going to put a *lot* of scopes in the various repo roles' `.scopes` property, but the resulting `.expandedScopes` will not really change (that's what the checks verify). The scope resolver in tc-auth basically deals with expanded scopes.  So I'm not sure the new arrangement would be any less efficient, or even any harder to read in the UI.  Thoughts?

I don't intend to land this until I get back on August 19, so take your time :)
Flags: needinfo?(pmoore)
Flags: needinfo?(bstack)
Comment on attachment 8995316 [details]
Bug 1465842: refactor ciadmin.util into sub-modules

Brian Stack [:bstack] has approved the revision.

https://phabricator.services.mozilla.com/D2436
Attachment #8995316 - Flags: review+
Comment on attachment 8995317 [details]
Bug 1465842: Add scope utilities

Brian Stack [:bstack] has approved the revision.

https://phabricator.services.mozilla.com/D2437
Attachment #8995317 - Flags: review+
Comment on attachment 8995318 [details]
Bug 1465842: generate moz-tree:* roles, too

Brian Stack [:bstack] has approved the revision.
Attachment #8995318 - Flags: review+
Comment on attachment 8995319 [details]
Bug 1465842: add 'ci-admin check', upgrade requirements

Brian Stack [:bstack] has approved the revision.
Attachment #8995319 - Flags: review+
Comment on attachment 8995321 [details]
Bug 1465842: test for signing scopes

Brian Stack [:bstack] has approved the revision.
Attachment #8995321 - Flags: review+
Comment on attachment 8995325 [details]
Bug 1465842: add grants and remove other scope specifications

Brian Stack [:bstack] has approved the revision.
Attachment #8995325 - Flags: review+
Comment on attachment 8995323 [details]
Bug 1465842: move ciconfig into sub-modules

Brian Stack [:bstack] has approved the revision.
Attachment #8995323 - Flags: review+
Comment on attachment 8995324 [details]
Bug 1465842: implement grants.yml

Brian Stack [:bstack] has approved the revision.
Attachment #8995324 - Flags: review+
I've reviewed all but the DO NOT LAND one. If that needs a review, let me know on monday!
Flags: needinfo?(bstack)
Comment on attachment 8995316 [details]
Bug 1465842: refactor ciadmin.util into sub-modules

Tom Prince [:tomprince] has approved the revision.
Attachment #8995316 - Flags: review+
Comment on attachment 8995317 [details]
Bug 1465842: Add scope utilities

Tom Prince [:tomprince] has approved the revision.
Attachment #8995317 - Flags: review+
Comment on attachment 8995323 [details]
Bug 1465842: move ciconfig into sub-modules

Tom Prince [:tomprince] has approved the revision.
Attachment #8995323 - Flags: review+
Depends on: 1470942
I talked to Pete today, and he will give this a look.  I also checked in with jhford and wcosta, and I'd love to hear any thoughts either of you have, but I won't block waiting on it (so don't feel pressured to comment).
Looking at this now!
Flags: needinfo?(pmoore)
Comment on attachment 8995316 [details]
Bug 1465842: refactor ciadmin.util into sub-modules

Pete Moore [:pmoore][:pete] has approved the revision.
Attachment #8995316 - Flags: review+
Comment on attachment 8995317 [details]
Bug 1465842: Add scope utilities

Pete Moore [:pmoore][:pete] has approved the revision.
Attachment #8995317 - Flags: review+
Comment on attachment 8995319 [details]
Bug 1465842: add 'ci-admin check', upgrade requirements

Pete Moore [:pmoore][:pete] has approved the revision.
Attachment #8995319 - Flags: review+
Comment on attachment 8995321 [details]
Bug 1465842: test for signing scopes

Pete Moore [:pmoore][:pete] has approved the revision.
Attachment #8995321 - Flags: review+
Comment on attachment 8995322 [details]
Bug 1465842: test that changes have no effect DO NOT LAND

Pete Moore [:pmoore][:pete] has approved the revision.
Attachment #8995322 - Flags: review+
Comment on attachment 8995323 [details]
Bug 1465842: move ciconfig into sub-modules

Pete Moore [:pmoore][:pete] has approved the revision.
Attachment #8995323 - Flags: review+
Comment on attachment 8995324 [details]
Bug 1465842: implement grants.yml

Pete Moore [:pmoore][:pete] has approved the revision.
Attachment #8995324 - Flags: review+
(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #14)
> * add `ciadmin check` to .taskcluster.yml and run it periodically

I'll do this once I add .taskcluster.yml (bug 1486431)

> * support multiple grant files (so, maybe grants.yml has `grants-from:
> [nss-grants.yml, action-grants.yml, ..]`)

I'm not sure this is a good idea, since the file contents are fetched remotely as raw files.  Maybe that's a bad idea, but I don't want to revisit the question right now.

> * support utility roles ({grant: [<scopes>] to: {role: <roleId>}}) and
> manage 
>   - lists of workers
>   - sccache access
>   - nightly-scopes (currently hand-managed!)
>   - etc.

Follow-up - bug 1486544.

> * break out the huge grants at the beginning of grants.yml into more
> manageable, topical chunks
>   -- possibly using utility roles like lists of workers

Follow-up work, to be chipped away at as necessary.

> * use parameterized roles automatically
>   -- detect when a scope fits one of a few patterns, and automatically use a
> parameterized role fitting that pattern

I don't think this is a good idea -- it sounds fragile and complex without any real benefit.
Attachment #8995319 - Attachment description: Bug 1465842: add 'ci-admin check', upgrade requirements (TODO) → Bug 1465842: add 'ci-admin check', upgrade requirements
Attachment #8995322 - Attachment is obsolete: true
Attachment #8995322 - Flags: review+
Comment on attachment 8995321 [details]
Bug 1465842: test for signing scopes

Tom Prince [:tomprince] has approved the revision.
Attachment #8995321 - Flags: review+
Blocks: 1431948
Comment on attachment 8995324 [details]
Bug 1465842: implement grants.yml

Tom Prince [:tomprince] has approved the revision.
Attachment #8995324 - Flags: review+
Comment on attachment 8995318 [details]
Bug 1465842: generate moz-tree:* roles, too

Tom Prince [:tomprince] has approved the revision.
Attachment #8995318 - Flags: review+
Comment on attachment 8995325 [details]
Bug 1465842: add grants and remove other scope specifications

Pete Moore [:pmoore][:pete] has approved the revision.
Attachment #8995325 - Flags: review+
OK!  Everything is landed, and I'm now applying the changes.  I'm trying to do so in an order that will not cause anything to fail.
Changes landed for try at 16:19UTC
..and a try task created successfully:
  https://tools.taskcluster.net/groups/fX75fyZrS_WPmSwxwh1qGw/tasks/fX75fyZrS_WPmSwxwh1qGw/details

It appears that treeherder ingestion is behind, as I don't see a D for this or any other try job since the top of the hour, but that is decidedly unrelated to this work.  And now that I've written that, I see more D's -- so it's just treeherder lag.
Attached file roles.json
listRoles output as of this time (in case I need to roll back :crossed fingers:)
I've now seen an autoland task created.  No other errors in the mozilla-taskcluster logs.  So I think this is good!
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Blocks: 1470625
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: