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/11/28 13:18:06 UTC
Re: Review Request 53541: Added authorization actions for debug API.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/
-----------------------------------------------------------
(Updated Nov. 28, 2016, 2:18 p.m.)
Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
Bugs: MESOS-6474
https://issues.apache.org/jira/browse/MESOS-6474
Repository: mesos
Description
-------
Added authorization actions for debug API.
Diffs
-----
include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
Diff: https://reviews.apache.org/r/53541/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/#review157047
-----------------------------------------------------------
Patch looks great!
Reviews applied: [53851, 53541]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Nov. 28, 2016, 4:42 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53541/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2016, 4:42 p.m.)
>
>
> Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
>
>
> Bugs: MESOS-6474
> https://issues.apache.org/jira/browse/MESOS-6474
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Creates new authorization action for all the API's related to
> nested containers. This patch does not add the code necesary to
> call use those actions, this is done in a latter patch.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
> include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
> include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
> src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
> src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
>
> Diff: https://reviews.apache.org/r/53541/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/
-----------------------------------------------------------
(Updated Nov. 30, 2016, 11:48 a.m.)
Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
Bugs: MESOS-6474
https://issues.apache.org/jira/browse/MESOS-6474
Repository: mesos
Description
-------
Creates new authorization action for all the API's related to
nested containers. This patch does not add the code necesary to
call use those actions, this is done in a latter patch.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
Diff: https://reviews.apache.org/r/53541/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/#review157387
-----------------------------------------------------------
Fix it, then Ship it!
Looks great! Just a few remaining nits.
src/tests/authorization_tests.cpp (line 2558)
<https://reviews.apache.org/r/53541/#comment227983>
s/execturos/executors/
src/tests/authorization_tests.cpp (lines 3144 - 3145)
<https://reviews.apache.org/r/53541/#comment227989>
"bar" can launch nested containers under executors running as linux user "bar"
src/tests/authorization_tests.cpp (line 3163)
<https://reviews.apache.org/r/53541/#comment227990>
s/execturos/executors/
src/tests/authorization_tests.cpp (lines 3233 - 3234)
<https://reviews.apache.org/r/53541/#comment227991>
s/no user in command/no command/
src/tests/authorization_tests.cpp (line 3376)
<https://reviews.apache.org/r/53541/#comment227992>
s/cannot/can/
- Adam B
On Nov. 29, 2016, 7:18 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53541/
> -----------------------------------------------------------
>
> (Updated Nov. 29, 2016, 7:18 a.m.)
>
>
> Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
>
>
> Bugs: MESOS-6474
> https://issues.apache.org/jira/browse/MESOS-6474
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Creates new authorization action for all the API's related to
> nested containers. This patch does not add the code necesary to
> call use those actions, this is done in a latter patch.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
> include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
> include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
> src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
> src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
>
> Diff: https://reviews.apache.org/r/53541/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/
-----------------------------------------------------------
(Updated Nov. 29, 2016, 4:18 p.m.)
Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
Bugs: MESOS-6474
https://issues.apache.org/jira/browse/MESOS-6474
Repository: mesos
Description
-------
Creates new authorization action for all the API's related to
nested containers. This patch does not add the code necesary to
call use those actions, this is done in a latter patch.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
Diff: https://reviews.apache.org/r/53541/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Alexander Rojas <al...@mesosphere.io>.
> On Nov. 29, 2016, 10:47 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, line 471
> > <https://reviews.apache.org/r/53541/diff/4-7/?file=1568604#file1568604line471>
> >
> > Why check the command_info? It's optional either way, so leave it out. Your current logic would allow an executor_info without a framework_info (or vice versa) as long as there's a command_info. I don't think that's what we want. I assumed you were trying to check that if there's an object present, then it should have both executor_info and framework_info.
I will just remove the whole check here
> On Nov. 29, 2016, 10:47 a.m., Adam B wrote:
> > src/authorizer/local/authorizer.cpp, lines 468-471
> > <https://reviews.apache.org/r/53541/diff/7/?file=1571191#file1571191line468>
> >
> > When will object ever be None?
Not at this point.
> On Nov. 29, 2016, 10:47 a.m., Adam B wrote:
> > src/tests/authorization_tests.cpp, line 2554
> > <https://reviews.apache.org/r/53541/diff/7/?file=1571192#file1571192line2554>
> >
> > `"foo" principal cannot launch nested container sessions as any user.`?
> > Can foo still launch container_info's instead of command_info's?
that really depends of the other ACL (the one related to executor), since in that case we don't verify this ACL.
> On Nov. 29, 2016, 10:47 a.m., Adam B wrote:
> > src/tests/authorization_tests.cpp, line 2546
> > <https://reviews.apache.org/r/53541/diff/7/?file=1571192#file1571192line2546>
> >
> > What's the comment here? I can't tell if this is equivalent to a permissive=false for this ACL.
pretty much so, but `permissive=false` affects all the ACLs.
- Alexander
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/#review157211
-----------------------------------------------------------
On Nov. 29, 2016, 4:18 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53541/
> -----------------------------------------------------------
>
> (Updated Nov. 29, 2016, 4:18 p.m.)
>
>
> Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
>
>
> Bugs: MESOS-6474
> https://issues.apache.org/jira/browse/MESOS-6474
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Creates new authorization action for all the API's related to
> nested containers. This patch does not add the code necesary to
> call use those actions, this is done in a latter patch.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
> include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
> include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
> src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
> src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
>
> Diff: https://reviews.apache.org/r/53541/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Adam B <ad...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/#review157211
-----------------------------------------------------------
Looks great! Nothing too major. I think we can ship this once you've addressed these comments.
- In a follow-up patch, please update authorization.md with the new actions/ACLs.
include/mesos/authorizer/acls.proto (line 327)
<https://reviews.apache.org/r/53541/#comment227705>
s/containers/container/
include/mesos/authorizer/acls.proto (line 334)
<https://reviews.apache.org/r/53541/#comment227707>
s/containers/nested containers/
... whose nested containers can be killed.
include/mesos/authorizer/acls.proto (line 338)
<https://reviews.apache.org/r/53541/#comment227706>
s/containers/container/
include/mesos/authorizer/authorizer.proto (line 162)
<https://reviews.apache.org/r/53541/#comment227708>
s/an object/objects/
src/authorizer/local/authorizer.cpp (line 471)
<https://reviews.apache.org/r/53541/#comment227709>
Why check the command_info? It's optional either way, so leave it out. Your current logic would allow an executor_info without a framework_info (or vice versa) as long as there's a command_info. I don't think that's what we want. I assumed you were trying to check that if there's an object present, then it should have both executor_info and framework_info.
include/mesos/authorizer/acls.proto (line 282)
<https://reviews.apache.org/r/53541/#comment227711>
s/containers/container/
include/mesos/authorizer/acls.proto (line 293)
<https://reviews.apache.org/r/53541/#comment227712>
s/containers/container/
src/authorizer/local/authorizer.cpp (lines 468 - 471)
<https://reviews.apache.org/r/53541/#comment227714>
When will object ever be None?
src/authorizer/local/authorizer.cpp (line 491)
<https://reviews.apache.org/r/53541/#comment227715>
Don't you mean `framework-info->user()`?
src/authorizer/local/authorizer.cpp (line 535)
<https://reviews.apache.org/r/53541/#comment227716>
s/user/child/ or s/user/command/?
src/authorizer/local/authorizer.cpp (lines 565 - 568)
<https://reviews.apache.org/r/53541/#comment227710>
Maybe let's call it the `commandApprover` or `childApprover`, since we could technically be approving anything about the child command, not just its user. We're more generally concerned about approving the subject to perform the action a) for a child container with the given command_info, and b) under the given parent executor.
src/tests/authorization_tests.cpp (line 2517)
<https://reviews.apache.org/r/53541/#comment227739>
No test for `LaunchNestedContainer`? Should be identical except for comments, but probably worth including.
src/tests/authorization_tests.cpp (line 2523)
<https://reviews.apache.org/r/53541/#comment227717>
s/nested/parent/
src/tests/authorization_tests.cpp (line 2539)
<https://reviews.apache.org/r/53541/#comment227718>
"bar" can launch sessions nested under a container running as linux user "bar"
src/tests/authorization_tests.cpp (line 2546)
<https://reviews.apache.org/r/53541/#comment227719>
What's the comment here? I can't tell if this is equivalent to a permissive=false for this ACL.
src/tests/authorization_tests.cpp (line 2554)
<https://reviews.apache.org/r/53541/#comment227720>
`"foo" principal cannot launch nested container sessions as any user.`?
Can foo still launch container_info's instead of command_info's?
src/tests/authorization_tests.cpp (line 2562)
<https://reviews.apache.org/r/53541/#comment227721>
s/in containers //
src/tests/authorization_tests.cpp (line 2570)
<https://reviews.apache.org/r/53541/#comment227722>
s/in all containers/as any linux user/
src/tests/authorization_tests.cpp (line 2578)
<https://reviews.apache.org/r/53541/#comment227723>
s/in nested containers/as any user/
src/tests/authorization_tests.cpp (lines 2613 - 2614)
<https://reviews.apache.org/r/53541/#comment227724>
And an ExecutorInfo with a ContainerInfo instead of a CommandInfo? We ought to test `command_info.isNone()
src/tests/authorization_tests.cpp (line 2618)
<https://reviews.apache.org/r/53541/#comment227725>
Want to give it a unique executorId, maybe `n`?
src/tests/authorization_tests.cpp (line 2662)
<https://reviews.apache.org/r/53541/#comment227726>
... running with the default frameworkInfo.user "user"
src/tests/authorization_tests.cpp (lines 2709 - 2711)
<https://reviews.apache.org/r/53541/#comment227727>
`commandInfoNoUser.set_value("echo hello");`
No `executorInfoNoUser` needed.
src/tests/authorization_tests.cpp (lines 2744 - 2745)
<https://reviews.apache.org/r/53541/#comment227728>
Principal "bar" can launch a session as user "bar" under parent containers running as user "bar"
src/tests/authorization_tests.cpp (line 2759)
<https://reviews.apache.org/r/53541/#comment227729>
s/cannot/can/
src/tests/authorization_tests.cpp (lines 2772 - 2775)
<https://reviews.apache.org/r/53541/#comment227731>
Could you please also test:
- executorInfoNoUser (defaults to frameworkInfo's "user") + commandInfo[Bar]
- executorInfoNoUser + commandInfoNoUser
src/tests/authorization_tests.cpp (line 2934)
<https://reviews.apache.org/r/53541/#comment227738>
s/input/output/ throughout this test
src/tests/authorization_tests.cpp (line 3206)
<https://reviews.apache.org/r/53541/#comment227737>
s/bar/ops/ here and in other tests
src/tests/authorization_tests.cpp (line 3222)
<https://reviews.apache.org/r/53541/#comment227732>
s/killing for/killing/
src/tests/authorization_tests.cpp (line 3298)
<https://reviews.apache.org/r/53541/#comment227733>
s/wait for/kill/
src/tests/authorization_tests.cpp (line 3311)
<https://reviews.apache.org/r/53541/#comment227734>
s/wait for/kill/
src/tests/authorization_tests.cpp (line 3324)
<https://reviews.apache.org/r/53541/#comment227735>
s/wait for/kill/ in the rest of these comments
src/tests/authorization_tests.cpp (line 3351)
<https://reviews.apache.org/r/53541/#comment227736>
s/bar/ops/
- Adam B
On Nov. 28, 2016, 9:32 a.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53541/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2016, 9:32 a.m.)
>
>
> Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
>
>
> Bugs: MESOS-6474
> https://issues.apache.org/jira/browse/MESOS-6474
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Creates new authorization action for all the API's related to
> nested containers. This patch does not add the code necesary to
> call use those actions, this is done in a latter patch.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
> include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
> include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
> src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
> src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
>
> Diff: https://reviews.apache.org/r/53541/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/
-----------------------------------------------------------
(Updated Nov. 28, 2016, 6:32 p.m.)
Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
Bugs: MESOS-6474
https://issues.apache.org/jira/browse/MESOS-6474
Repository: mesos
Description
-------
Creates new authorization action for all the API's related to
nested containers. This patch does not add the code necesary to
call use those actions, this is done in a latter patch.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
Diff: https://reviews.apache.org/r/53541/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On Nov. 28, 2016, 6:08 p.m., Benjamin Bannier wrote:
> > src/authorizer/local/authorizer.cpp, lines 479-480
> > <https://reviews.apache.org/r/53541/diff/6/?file=1571122#file1571122line479>
> >
> > You could remove one level of nesting here, e.g.,
> >
> > if (object->command_info != nullptr &&
> > object->command_info->has_user()) { ...
Actually this control flow is needed so we can `break` in all cases where `command_info != nullptr`, dropping.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/#review157028
-----------------------------------------------------------
On Nov. 28, 2016, 5:42 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53541/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2016, 5:42 p.m.)
>
>
> Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
>
>
> Bugs: MESOS-6474
> https://issues.apache.org/jira/browse/MESOS-6474
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Creates new authorization action for all the API's related to
> nested containers. This patch does not add the code necesary to
> call use those actions, this is done in a latter patch.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
> include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
> include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
> src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
> src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
>
> Diff: https://reviews.apache.org/r/53541/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Benjamin Bannier <be...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/#review157028
-----------------------------------------------------------
src/authorizer/local/authorizer.cpp (line 502)
<https://reviews.apache.org/r/53541/#comment227411>
Fits on one line.
src/authorizer/local/authorizer.cpp (line 511)
<https://reviews.apache.org/r/53541/#comment227412>
The `const` is redundant here, mind cleaning it up here and above in a case?
src/authorizer/local/authorizer.cpp (lines 479 - 480)
<https://reviews.apache.org/r/53541/#comment227434>
You could remove one level of nesting here, e.g.,
if (object->command_info != nullptr &&
object->command_info->has_user()) { ...
src/authorizer/local/authorizer.cpp (line 508)
<https://reviews.apache.org/r/53541/#comment227429>
This can go now, right?
src/authorizer/local/authorizer.cpp (line 545)
<https://reviews.apache.org/r/53541/#comment227438>
Mind adding a small comment here explicitly mentioning the approval policy?
src/authorizer/local/authorizer.cpp (lines 554 - 555)
<https://reviews.apache.org/r/53541/#comment227430>
Move this down to the first use.
src/authorizer/local/authorizer.cpp (line 569)
<https://reviews.apache.org/r/53541/#comment227461>
I think this behaves incorrectly for some cases when `permissive==true` and the presence of an explicit deny rule only for `userApprover_`. We might here fallback to `permissive` in `parentApprover_` before ever examing the rule in `userApprover_`.
Instead I think you should create the subapprovers with `permissive=false` and handle permissive mode here explicitly.
- Benjamin Bannier
On Nov. 28, 2016, 5:42 p.m., Alexander Rojas wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53541/
> -----------------------------------------------------------
>
> (Updated Nov. 28, 2016, 5:42 p.m.)
>
>
> Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
>
>
> Bugs: MESOS-6474
> https://issues.apache.org/jira/browse/MESOS-6474
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Creates new authorization action for all the API's related to
> nested containers. This patch does not add the code necesary to
> call use those actions, this is done in a latter patch.
>
>
> Diffs
> -----
>
> include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
> include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
> include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
> src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
> src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
>
> Diff: https://reviews.apache.org/r/53541/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Alexander Rojas
>
>
Re: Review Request 53541: Added authorization actions for Nested
Container and Debug API.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/
-----------------------------------------------------------
(Updated Nov. 28, 2016, 5:42 p.m.)
Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
Changes
-------
Updates `LocalNestedContainerObjectApprover` adter discussion with bbanier.
Summary (updated)
-----------------
Added authorization actions for Nested Container and Debug API.
Bugs: MESOS-6474
https://issues.apache.org/jira/browse/MESOS-6474
Repository: mesos
Description (updated)
-------
Creates new authorization action for all the API's related to
nested containers. This patch does not add the code necesary to
call use those actions, this is done in a latter patch.
Diffs (updated)
-----
include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
Diff: https://reviews.apache.org/r/53541/diff/
Testing
-------
make check
Thanks,
Alexander Rojas
Re: Review Request 53541: Added authorization actions for debug API.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/53541/
-----------------------------------------------------------
(Updated Nov. 28, 2016, 5:34 p.m.)
Review request for mesos, Adam B, Kapil Arya, Kevin Klues, and Till Toenshoff.
Bugs: MESOS-6474
https://issues.apache.org/jira/browse/MESOS-6474
Repository: mesos
Description
-------
Added authorization actions for debug API.
Diffs
-----
include/mesos/authorizer/acls.proto e3fd6a4a1b617a75714ebd6e08ab10cffa1a7d1b
include/mesos/authorizer/authorizer.hpp 3cd0f84e25c0ea91a6f20eba26341a1b830d8cf2
include/mesos/authorizer/authorizer.proto 0696a629ac2d2b9950e20708f0c3666b58ff7ca0
src/authorizer/local/authorizer.cpp 77e05dd2475d6e7511e7c7eeea578ec31ff3d198
src/tests/authorization_tests.cpp d23f551c2caa454da0c0f6cb7d77a8c2bd75a474
Diff: https://reviews.apache.org/r/53541/diff/
Testing
-------
make check
Thanks,
Alexander Rojas