You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Alexander Rojas <al...@mesosphere.io> on 2016/12/08 15:38:18 UTC
Review Request 54535: WIP: Added authorization actions
VIEW_CONTAINERS and SET_LOG_LEVEL.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54535/
-----------------------------------------------------------
Review request for mesos and Adam B.
Bugs: MESOS-6670
https://issues.apache.org/jira/browse/MESOS-6670
Repository: mesos
Description
-------
Adds the authorization action `VIEW_CONTAINERS` which takes an object
of type `FrameworkInfo` and `ExecutorInfo` and optionally a
`CommandInfo`.
It also adds the authorization action `SET_LOG_LEVEL` which takes no
object.
Includes testing for the ACLs and interface.
Diffs
-----
include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd
include/mesos/authorizer/authorizer.proto b7371cd580bbe64eb1566876d0f253eedbd0789d
src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9
src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8
Diff: https://reviews.apache.org/r/54535/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 54535: WIP: Added authorization actions
VIEW_CONTAINERS and SET_LOG_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Dec. 9, 2016, 10:52 a.m., Adam B wrote:
> > include/mesos/authorizer/acls.proto, lines 355-356
> > <https://reviews.apache.org/r/54535/diff/1/?file=1579835#file1579835line355>
> >
> > "The list of roles whose container metadata the principal can see."
Not using roles anymore. We are authorizing as `VIEW_TASKS` which uses _users_.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54535/#review158660
-----------------------------------------------------------
On Dec. 9, 2016, 4:32 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54535/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 4:32 p.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds the authorization action VIEW_CONTAINERS which takes an object
> of type `FrameworkInfo` and `ExecutorInfo` and optionally a
> `CommandInfo`.
>
> It also adds the authorization action SET_LOG_LEVEL which takes no
> object.
>
> Includes testing for the ACLs and interface.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd
> include/mesos/authorizer/authorizer.proto b7371cd580bbe64eb1566876d0f253eedbd0789d
> src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9
> src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8
>
> Diff: https://reviews.apache.org/r/54535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 54535: WIP: Added authorization actions
VIEW_CONTAINERS and SET_LOG_LEVEL.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54535/#review158660
-----------------------------------------------------------
I'm a bit confused why we don't just have a simple single `ViewContainer` ACL based on the (Executor) container's user. Why bother with the role here if we don't in `ViewTask`/`ViewExecutor`?
include/mesos/authorizer/acls.proto (line 351)
<https://reviews.apache.org/r/54535/#comment229451>
Let's think for a minute how this will change when we have a multi-role framework.
The framework may be subscribed for multiple roles foo and bar, but only uses one (foo) to launch this container. Should users with access to ViewContainerOfFrameworkWithRole=bar be able to access a container launched using foo resources? I would think not.
In which case, the "role" more specifically describes the container, not the framework.
In which case, this becomes `ViewContainerWithRole`
include/mesos/authorizer/acls.proto (lines 355 - 356)
<https://reviews.apache.org/r/54535/#comment229452>
"The list of roles whose container metadata the principal can see."
include/mesos/authorizer/acls.proto (line 367)
<https://reviews.apache.org/r/54535/#comment229453>
Is this necessarily a "nested container"? Then perhaps `ViewNestedContainer[RunningAsUser]` would be more appropriate.
include/mesos/authorizer/acls.proto (line 371)
<https://reviews.apache.org/r/54535/#comment229454>
s/authorized change/authorized to change/
include/mesos/authorizer/acls.proto (lines 443 - 445)
<https://reviews.apache.org/r/54535/#comment229449>
It seems we didn't include the `AsUser` or `WithRole` in the ACL names for `RunTask` or `RegisterFramework`, so maybe we don't need to here either (or in the nested container ACLs), since we already have the Object Entities named `roles` or `users`.
`repeated ACL.ViewContainerOfFramework view_containers_of_framework = 31;`
`repeated ACL.ViewContainer view_containers = 32;`
Why do we even need both of these ACLs?
Why not just `ViewContainer` with Object: `users`, just like ViewTask/ViewExecutor?
src/authorizer/local/authorizer.cpp (line 1164)
<https://reviews.apache.org/r/54535/#comment229456>
s/view_flags/set_log_level/
src/tests/authorization_tests.cpp (line 4154)
<https://reviews.apache.org/r/54535/#comment229457>
s/see the flags/set log level/g here and in other comments
- Adam B
On Dec. 8, 2016, 7:38 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54535/
> -----------------------------------------------------------
>
> (Updated Dec. 8, 2016, 7:38 a.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds the authorization action `VIEW_CONTAINERS` which takes an object
> of type `FrameworkInfo` and `ExecutorInfo` and optionally a
> `CommandInfo`.
>
> It also adds the authorization action `SET_LOG_LEVEL` which takes no
> object.
>
> Includes testing for the ACLs and interface.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd
> include/mesos/authorizer/authorizer.proto b7371cd580bbe64eb1566876d0f253eedbd0789d
> src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9
> src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8
>
> Diff: https://reviews.apache.org/r/54535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 54535: WIP: Added authorization actions
VIEW_CONTAINERS and SET_LOG_LEVEL.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54535/#review158794
-----------------------------------------------------------
Looks good, just some comment nits, and a recommendation to move the rename into a separate patch.
include/mesos/authorizer/acls.proto (line 356)
<https://reviews.apache.org/r/54535/#comment229599>
s/nested//
s/containers/container/
include/mesos/authorizer/acls.proto (line 360)
<https://reviews.apache.org/r/54535/#comment229600>
s/change/to change/
src/authorizer/local/authorizer.cpp (line 544)
<https://reviews.apache.org/r/54535/#comment229601>
Although I like the rename, it isn't really necessary for this patch, right? Maybe split it out into a followup patch.
src/authorizer/local/authorizer.cpp (line 559)
<https://reviews.apache.org/r/54535/#comment229602>
s/and/`AND`/
src/tests/authorization_tests.cpp (line 1929)
<https://reviews.apache.org/r/54535/#comment229603>
s/frameworks/containers/ here and elsewhere
- Adam B
On Dec. 9, 2016, 7:32 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54535/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 7:32 a.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds the authorization action VIEW_CONTAINERS which takes an object
> of type `FrameworkInfo` and `ExecutorInfo` and optionally a
> `CommandInfo`.
>
> It also adds the authorization action SET_LOG_LEVEL which takes no
> object.
>
> Includes testing for the ACLs and interface.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd
> include/mesos/authorizer/authorizer.proto b7371cd580bbe64eb1566876d0f253eedbd0789d
> src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9
> src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8
>
> Diff: https://reviews.apache.org/r/54535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 54535: Added authorization actions VIEW_CONTAINERS
and SET_LOG_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54535/
-----------------------------------------------------------
(Updated Dec. 13, 2016, 2:44 p.m.)
Review request for mesos and Adam B.
Bugs: MESOS-6670
https://issues.apache.org/jira/browse/MESOS-6670
Repository: mesos
Description
-------
Adds the authorization action `VIEW_CONTAINERS` which takes an object
of type `FrameworkInfo` and `ExecutorInfo` and optionally a
`CommandInfo`.
It also adds the authorization action `SET_LOG_LEVEL` which takes no
object.
Includes testing for the ACLs and interface.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd
include/mesos/authorizer/authorizer.proto b7371cd580bbe64eb1566876d0f253eedbd0789d
src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9
src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8
Diff: https://reviews.apache.org/r/54535/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 54535: Added authorization actions VIEW_CONTAINERS
and SET_LOG_LEVEL.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54535/#review158972
-----------------------------------------------------------
Fix it, then Ship it!
Ship it! Just a couple of nits on the comments.
include/mesos/authorizer/acls.proto (line 350)
<https://reviews.apache.org/r/54535/#comment229866>
More accurately, you're authorizing based on the linux user of the executor process, not necessarily the linux user of a nested container process. As in "... of a container whose executor is running as..."
In theory we could allow nested containers to be run as users different from their parent container processes, but I don't know if we actually support that use case yet.
include/mesos/authorizer/acls.proto (line 360)
<https://reviews.apache.org/r/54535/#comment229865>
s/authorizedto change/authorized to change/
src/tests/authorization_tests.cpp (line 1986)
<https://reviews.apache.org/r/54535/#comment229867>
s/a containers/containers/ here and elsewhere
- Adam B
On Dec. 12, 2016, 2:45 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54535/
> -----------------------------------------------------------
>
> (Updated Dec. 12, 2016, 2:45 a.m.)
>
>
> Review request for mesos and Adam B.
>
>
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Adds the authorization action `VIEW_CONTAINERS` which takes an object
> of type `FrameworkInfo` and `ExecutorInfo` and optionally a
> `CommandInfo`.
>
> It also adds the authorization action `SET_LOG_LEVEL` which takes no
> object.
>
> Includes testing for the ACLs and interface.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd
> include/mesos/authorizer/authorizer.proto b7371cd580bbe64eb1566876d0f253eedbd0789d
> src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9
> src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8
>
> Diff: https://reviews.apache.org/r/54535/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 54535: Added authorization actions VIEW_CONTAINERS
and SET_LOG_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54535/
-----------------------------------------------------------
(Updated Dec. 12, 2016, 11:45 a.m.)
Review request for mesos and Adam B.
Summary (updated)
-----------------
Added authorization actions VIEW_CONTAINERS and SET_LOG_LEVEL.
Bugs: MESOS-6670
https://issues.apache.org/jira/browse/MESOS-6670
Repository: mesos
Description (updated)
-------
Adds the authorization action `VIEW_CONTAINERS` which takes an object
of type `FrameworkInfo` and `ExecutorInfo` and optionally a
`CommandInfo`.
It also adds the authorization action `SET_LOG_LEVEL` which takes no
object.
Includes testing for the ACLs and interface.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd
include/mesos/authorizer/authorizer.proto b7371cd580bbe64eb1566876d0f253eedbd0789d
src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9
src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8
Diff: https://reviews.apache.org/r/54535/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 54535: WIP: Added authorization actions
VIEW_CONTAINERS and SET_LOG_LEVEL.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/54535/
-----------------------------------------------------------
(Updated Dec. 9, 2016, 4:32 p.m.)
Review request for mesos and Adam B.
Bugs: MESOS-6670
https://issues.apache.org/jira/browse/MESOS-6670
Repository: mesos
Description (updated)
-------
Adds the authorization action VIEW_CONTAINERS which takes an object
of type `FrameworkInfo` and `ExecutorInfo` and optionally a
`CommandInfo`.
It also adds the authorization action SET_LOG_LEVEL which takes no
object.
Includes testing for the ACLs and interface.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd
include/mesos/authorizer/authorizer.proto b7371cd580bbe64eb1566876d0f253eedbd0789d
src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9
src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8
Diff: https://reviews.apache.org/r/54535/diff/
Testing
-------
make check
Thanks,
Alexander Rojas