swarming: switch to a 'capability focused' ACL system

swarming: switch to a 'capability focused' ACL system

The main visible ACL change is that privileged_users cannot get the config
anymore.

R=vadimsh@chromium.org,kjlubick@chromium.org
BUG=746557
Review-Url: https://codereview.chromium.org/2984843002
Committed: https://github.com/luci/luci-py/commit/35f786195171478c029b17cf1a0841ac2723ec57

[M-A Ruel, 2017-07-21 19:43:41]

Would like your opinion on the design, if you think it's fine I'll write
acl_test.py to increase coverage.

[Vadim Sh., 2017-07-21 22:01:20]

I like the idea, but I think we need more rigorous structure.

https://codereview.chromium.org/2984843002/diff/1/appengine/swarming/server/a...
File appengine/swarming/server/acl.py (right):

https://codereview.chromium.org/2984843002/diff/1/appengine/swarming/server/a...
appengine/swarming/server/acl.py:64: def can_config_view():
nit: please make it "can_<verb>_<subject>". Will be much more readable.

https://codereview.chromium.org/2984843002/diff/1/appengine/swarming/server/a...
appengine/swarming/server/acl.py:129: def can_id_task_view(identity):
I think this should be merged with 'can_task_view' and should accept a task
being viewed.

def can_view_task(task_request):
  ...

So handlers will look like

@auth.require(acl.can_access)
def result(self, request):
  ....
  if not acl.can_view_task(task):
    ...

If we want fine-grain ACLs like that we need to come up with a crystal clear
data model: what our resources are and what can be done with them.

Ideally, we'll have only 

can_create_task()
can_view_task(task)
can_edit_task(task)

can_create_bot()
can_view_bot(bot)
can_edit_bot(bot)

And listings will be build on top of can_view_bot/can_view_task.  I'm not sure
it is possible to do efficiently in practice, especially considering that some
requests just count number of tasks/bots, without fetching them :(

So, I think, we might need to add new kinds of resource "all tasks" and "all
bots" and have:

can_view_all_tasks()
can_view_all_bots()

And we'll have in the end:

can_access()

can_view_config()
can_edit_config()

can_create_bot()
can_edit_bot(bot)
can_view_bot(bot)
can_view_all_bots()

can_create_task()
can_edit_task(task)
can_view_task(task)
can_view_all_tasks()

Listings and countings will be allowed only for can_*_all_*.

---

If we are serious about sharing single Swarming host by multiple teams, we may
also consider adding bot pool as a resource instead of a bot. Then we'll have:

can_view_pool(pool)  // instead of can_view_bot
can_edit_pool(pool)   // instead of can_create_bot()
can_list_pool(pool)
can_list_all_pools()    // instead of can_list_bots

Though this will require a lot more refactorings to support in UI. Current UI is
very single-tenant :(

https://codereview.chromium.org/2984843002/diff/1/appengine/swarming/server/a...
appengine/swarming/server/acl.py:150: def get_user_type():
We should get rid of this, it doesn't work well in "capabilities based" acl
system, since callers may have a "mix" of capabilities, not fitting in any of
the categories.

[M-A Ruel, 2017-07-24 15:42:52]

I prefer to not add pool related ACL changes in this CL as it is already large
enough and this CL *does* change some ACLs.

Will add tests if the current design looks good to you.

https://codereview.chromium.org/2984843002/diff/1/appengine/swarming/server/a...
File appengine/swarming/server/acl.py (right):

https://codereview.chromium.org/2984843002/diff/1/appengine/swarming/server/a...
appengine/swarming/server/acl.py:64: def can_config_view():
On 2017/07/21 22:01:20, Vadim Sh. wrote:
> nit: please make it "can_<verb>_<subject>". Will be much more readable.

I did ponder between "can_<object>_<mutation>" and "can_<mutation>_<object>" and
preferred the other way but changed, as I prefer to optimize for readability and
whatever others prefer.

https://codereview.chromium.org/2984843002/diff/1/appengine/swarming/server/a...
appengine/swarming/server/acl.py:150: def get_user_type():
Done.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/task_scheduler.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:407: if not acl.can_edit_config():
This one is a bit awkward, not sure how to change.

[Vadim Sh., 2017-07-24 19:16:47]

looks fine, but I want to make another more thorough review pass before
submitting

(so I'll ping this CL a bit later)

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/acl.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/acl.py:144: It is possible that the user can only see
a subset of the tasks.
this line no longer applies

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/task_scheduler.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:407: if not acl.can_edit_config():
On 2017/07/24 15:42:51, M-A Ruel wrote:
> This one is a bit awkward, not sure how to change.

Let's remove this clause. I think we have never used this feature and it was
pointed at as a potential security concern during the review.

So it should be:

if 'id' in dims and 'pool' not in dims:
  raise auth.AuthorizationError(
      'Posting a task with "id" dimension require specifying "pool" dimension')

Strictly speaking we can also try to lookup the dimensions of the bot and check
that the caller is allowed to use at least one pool dimensions specified there.
But it is more code.

[Vadim Sh., 2017-07-24 23:07:26]

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/hand...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:72: raise
endpoints.NotFoundException('%s not found.' % task_id)
strictly speaking we are leaking presence/absence of the task to users that may
not have access to it. But it's probably not a big deal, since our task IDs are
meaningless on their own. Knowing that task 378f8d3fb5962710 exists doesn't
provide much information :)

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:73: if viewing == _VIEW:
can we move these checks outside of 'try/except' block 68-82?

'except' clause there is intended for line 69. I'd prefer if potential
exceptions in acl.can_* don't trigger it.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/hand...
File appengine/swarming/handlers_frontend.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/hand...
appengine/swarming/handlers_frontend.py:126: @auth.require(acl.can_edit_config)
:-/

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/prot...
File appengine/swarming/proto/config.proto (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/prot...
appengine/swarming/proto/config.proto:156: // Membership of this group acts a
superset of privileged_users_group and
this phrase is confusing... how can a membership act? Also "a superset of
<group_name>" reads like a superset of a group (a group that includes another
group), which is not true for 'admins_group'.

either

"Membership in this group grants superset of privileges granted by memberships
in privileged_users_group and bot_bootstrap groups"

or 

"Members of this group are implicitly granted same privileges as member of
privileged_users_group and bot_bootstrap groups"

or 

"Membership grants same privileges as memberships in privileged_users_group and
bot_bootstrap"

or just list all permissions explicitly:

"Grants:
  can_view_task for all tasks
  can_edit_task for all tasks
  can_view_all_tasks
  ...
"

(and same for all other groups). Then reads won't need to do mental set math to
figure out what each group grants.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/acl.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/acl.py:13: """The admins group is a super set of the
privileged users group."""
This statement is false. 

A group is a set of users, by definition.

Admins group (a set of users) is not a super set of privileged users group.
"admins_group" group doesn't include all users in "privileged_users_group"
group.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/acl.py:93: return is_ip_whitelisted_machine() or
_is_privileged_user()
I think it should be _is_admin instead of _is_privileged_user.

Why would users terminate bots? As a reminder, in our main case 'privileged
users' is all googlers.

(As I said before, I think we should get rid of 'privileged' vs 'non-priviliged'
users distinction. We are not using it in practice. Almost all our users are
privileged. It just confuses things. If we want to hide something from
non-googlers we should just run it on internal Swarming instance. Trying to use
privileged vs non-priviliged difference to hide stuff on single Swarming
instance is dangerous, at least until we redo UI to be inherently multi-tenant).

[M-A Ruel, 2017-07-24 23:46:46]

I'll make a separate CL to get rid of 'id' alone, as this is significant amount
of work.

[M-A Ruel, 2017-07-25 13:46:38]

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/hand...
File appengine/swarming/handlers_endpoints.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:72: raise
endpoints.NotFoundException('%s not found.' % task_id)
On 2017/07/24 23:07:25, Vadim Sh. wrote:
> strictly speaking we are leaking presence/absence of the task to users that
may
> not have access to it. But it's probably not a big deal, since our task IDs
are
> meaningless on their own. Knowing that task 378f8d3fb5962710 exists doesn't
> provide much information :)

I do not mind leaking the presence of objects, as the client passed the
can_access() check so we know the client is not anonymous.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/hand...
appengine/swarming/handlers_endpoints.py:73: if viewing == _VIEW:
On 2017/07/24 23:07:25, Vadim Sh. wrote:
> can we move these checks outside of 'try/except' block 68-82?
> 
> 'except' clause there is intended for line 69. I'd prefer if potential
> exceptions in acl.can_* don't trigger it.

True, fixed.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/prot...
File appengine/swarming/proto/config.proto (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/prot...
appengine/swarming/proto/config.proto:156: // Membership of this group acts a
superset of privileged_users_group and
On 2017/07/24 23:07:26, Vadim Sh. wrote:
> this phrase is confusing... how can a membership act? Also "a superset of
> <group_name>" reads like a superset of a group (a group that includes another
> group), which is not true for 'admins_group'.
> 
> either
> 
> "Membership in this group grants superset of privileges granted by memberships
> in privileged_users_group and bot_bootstrap groups"
> 
> or 
> 
> "Members of this group are implicitly granted same privileges as member of
> privileged_users_group and bot_bootstrap groups"
> 
> or 
> 
> "Membership grants same privileges as memberships in privileged_users_group
and
> bot_bootstrap"
> 
> or just list all permissions explicitly:
> 
> "Grants:
>   can_view_task for all tasks
>   can_edit_task for all tasks
>   can_view_all_tasks
>   ...
> "
> 
> (and same for all other groups). Then reads won't need to do mental set math
to
> figure out what each group grants.

Did the Grants: way.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/acl.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/acl.py:13: """The admins group is a super set of the
privileged users group."""
On 2017/07/24 23:07:26, Vadim Sh. wrote:
> This statement is false. 
> 
> A group is a set of users, by definition.
> 
> Admins group (a set of users) is not a super set of privileged users group.
> "admins_group" group doesn't include all users in "privileged_users_group"
> group.

Reworded

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/acl.py:93: return is_ip_whitelisted_machine() or
_is_privileged_user()
On 2017/07/24 23:07:26, Vadim Sh. wrote:
> I think it should be _is_admin instead of _is_privileged_user.
> 
> Why would users terminate bots? As a reminder, in our main case 'privileged
> users' is all googlers.

So that any Googler can quickly take a bot out of rotation, including build
sheriffs.

> (As I said before, I think we should get rid of 'privileged' vs
'non-priviliged'
> users distinction. We are not using it in practice. Almost all our users are
> privileged. It just confuses things. If we want to hide something from
> non-googlers we should just run it on internal Swarming instance. Trying to
use
> privileged vs non-priviliged difference to hide stuff on single Swarming
> instance is dangerous, at least until we redo UI to be inherently
multi-tenant).

Removing groups is out of scope for this CL.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/acl.py:144: It is possible that the user can only see
a subset of the tasks.
On 2017/07/24 19:16:47, Vadim Sh. wrote:
> this line no longer applies

Done.

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
File appengine/swarming/server/task_scheduler.py (right):

https://codereview.chromium.org/2984843002/diff/20001/appengine/swarming/serv...
appengine/swarming/server/task_scheduler.py:407: if not acl.can_edit_config():
On 2017/07/24 19:16:47, Vadim Sh. wrote:
> On 2017/07/24 15:42:51, M-A Ruel wrote:
> > This one is a bit awkward, not sure how to change.
> 
> Let's remove this clause. I think we have never used this feature and it was
> pointed at as a potential security concern during the review.

All removed.

[Vadim Sh., 2017-07-25 17:02:50]

lgtm

https://codereview.chromium.org/2984843002/diff/40001/appengine/swarming/prot...
File appengine/swarming/proto/config.proto (right):

https://codereview.chromium.org/2984843002/diff/40001/appengine/swarming/prot...
appengine/swarming/proto/config.proto:161: optional string admins_group = 1;
nit: \n (and in other fields). With such big multi line comments new lines
between fields will make this stuff more readable.

https://codereview.chromium.org/2984843002/diff/40001/appengine/swarming/prot...
appengine/swarming/proto/config.proto:165: // - bot create: create a token to
annonymously fetch the bot code.
typo: anonymously

[M-A Ruel, 2017-07-25 18:10:00]

Description was changed from

==========
swarming: switch to a 'capability focused' ACL system

R=vadimsh@chromium.org
BUG=746557
==========

to

==========
swarming: switch to a 'capability focused' ACL system

R=vadimsh@chromium.org,kjlubick@chromium.org
BUG=746557
==========

[M-A Ruel, 2017-07-25 18:10:01]

maruel@chromium.org changed reviewers:
+ kjlubick@chromium.org

[M-A Ruel, 2017-07-25 18:11:39]

Can you do a quick re-review of patchset #4? I've tweaked the permissions a bit,
split delete and terminate bot in two, admins only can delete bots now, updated
/server/permissions.

+Kevin for the new way permissions are defined.

https://codereview.chromium.org/2984843002/diff/40001/appengine/swarming/prot...
File appengine/swarming/proto/config.proto (right):

https://codereview.chromium.org/2984843002/diff/40001/appengine/swarming/prot...
appengine/swarming/proto/config.proto:161: optional string admins_group = 1;
On 2017/07/25 17:02:50, Vadim Sh. wrote:
> nit: \n (and in other fields). With such big multi line comments new lines
> between fields will make this stuff more readable.

Done.

https://codereview.chromium.org/2984843002/diff/40001/appengine/swarming/prot...
appengine/swarming/proto/config.proto:165: // - bot create: create a token to
annonymously fetch the bot code.
On 2017/07/25 17:02:50, Vadim Sh. wrote:
> typo: anonymously

Done.

[Vadim Sh., 2017-07-25 21:06:02]

lgtm

[M-A Ruel, 2017-07-26 14:17:17]

Description was changed from

==========
swarming: switch to a 'capability focused' ACL system

R=vadimsh@chromium.org,kjlubick@chromium.org
BUG=746557
==========

to

==========
swarming: switch to a 'capability focused' ACL system

The main visible ACL change is that privileged_users cannot get the config
anymore.

R=vadimsh@chromium.org,kjlubick@chromium.org
BUG=746557
==========

[M-A Ruel, 2017-07-26 14:19:37]

The CQ bit was checked by maruel@chromium.org

[commit-bot: I haz the power, 2017-07-26 14:19:45]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-07-26 14:23:36]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1501078777254730,
"parent_rev": "6e03ca45d3f6ccd47179283960b8ee9cde5c70d8", "commit_rev":
"35f786195171478c029b17cf1a0841ac2723ec57"}

[commit-bot: I haz the power, 2017-07-26 14:23:40]

Description was changed from

==========
swarming: switch to a 'capability focused' ACL system

The main visible ACL change is that privileged_users cannot get the config
anymore.

R=vadimsh@chromium.org,kjlubick@chromium.org
BUG=746557
==========

to

==========
swarming: switch to a 'capability focused' ACL system

The main visible ACL change is that privileged_users cannot get the config
anymore.

R=vadimsh@chromium.org,kjlubick@chromium.org
BUG=746557

Review-Url: https://codereview.chromium.org/2984843002
Committed:
https://github.com/luci/luci-py/commit/35f786195171478c029b17cf1a0841ac2723ec57
==========

[commit-bot: I haz the power, 2017-07-26 14:23:41]

Committed patchset #4 (id:60001) as
https://github.com/luci/luci-py/commit/35f786195171478c029b17cf1a0841ac2723ec57