mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Adam B <a...@mesosphere.io>
Subject Re: Review Request 47891: Added RUN_TASK authorization action.
Date Fri, 27 May 2016 08:52:37 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/47891/#review135182
-----------------------------------------------------------



Please update `Master::authorizeTask` to call your new/aliased action with additional FwkInfo/ExecInfo,
plus value=user for those authorization modules that didn't want to modify any code yet. Please
also update the user extraction code to include taskInfo.executorInfo, and order them as suggested.
Thanks!


include/mesos/authorizer/authorizer.proto (line 56)
<https://reviews.apache.org/r/47891/#comment200204>

    Is this deprecated or unused now?



include/mesos/authorizer/authorizer.proto (line 85)
<https://reviews.apache.org/r/47891/#comment200203>

    s/or TaskInfp/and TaskInfo/



include/mesos/authorizer/authorizer.proto (line 86)
<https://reviews.apache.org/r/47891/#comment200206>

    I wonder if we should just alias RUN_TASK to the same enum value as RUN_TASK_WITH_USER..
There shouldn't be any backwards compatibility issues since these are only used in-memory,
and modules have to recompile anyway.



src/authorizer/local/authorizer.cpp (line 128)
<https://reviews.apache.org/r/47891/#comment200205>

    This case will currently never be called. See Master::authorizeTask for where we call
the authorizer with RUN_TASK_WITH_USER. We'll need to set the new action, and fill in the
new Object metadata (FwkInfo, TaskInfo)



src/authorizer/local/authorizer.cpp (line 304)
<https://reviews.apache.org/r/47891/#comment200208>

    I'd rather we didn't put all this RunTask/user-specific logic in the general LocalAuthorizer::authorized()
method.
    See how `authorization::ACCESS_SANDBOX` handles this with its `realRequest` (although
maybe `specificRequest` would be a better name), in https://reviews.apache.org/r/47795/ (soon
to be committed).



src/authorizer/local/authorizer.cpp (lines 307 - 311)
<https://reviews.apache.org/r/47891/#comment200207>

    For RUN_TASK, I would actually reverse the order of precedence (and add executorInfo.commandInfo.user).
    Ideally, I find a user in the taskInfo/executorInfo (only one can have a CommandInfo).
If the task/executor doesn't have a user set, then default to the framework info.
    FrameworkInfo must have a user, but if the FrameworkInfo (and TaskInfo) weren't set, then
I'd rely on the value string.
    task_info.executor_info.command.user
    task_info.command.user
    framework_info.user
    value



src/tests/authorization_tests.cpp (lines 117 - 144)
<https://reviews.apache.org/r/47891/#comment200210>

    Redundant. Please remove.



src/tests/authorization_tests.cpp (line 203)
<https://reviews.apache.org/r/47891/#comment200214>

    But principal "foo" could run as any other user, e.g. "bar", right? That'd be worth testing.



src/tests/authorization_tests.cpp (line 529)
<https://reviews.apache.org/r/47891/#comment200216>

    Would be better to test that "bar" cannot run a "user1", since we've shown previously
that somebody else ("foo") can.


- Adam B


On May 26, 2016, 2:17 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47891/
> -----------------------------------------------------------
> 
> (Updated May 26, 2016, 2:17 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Michael Park.
> 
> 
> Bugs: MESOS-5459
>     https://issues.apache.org/jira/browse/MESOS-5459
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Authorization requests for RUN_TASK actions can pass `SOME`
> authorization object either in a `FrameworkInfo` holding a user, or a
> `TaskInfo` with optionally a `CommandInfo` which can optionally hold a
> user. If either of these fields is set it will be used as the object;
> otherwise an `ANY` type authorization object will be created.
> 
> 
> Diffs
> -----
> 
>   include/mesos/authorizer/authorizer.proto 02d1a01d57cf34b38524f4368187878b03343537

>   src/authorizer/local/authorizer.cpp 3c7c791bde65cfcbcc4e319c9ccc487ab37d8029 
>   src/tests/authorization_tests.cpp 54bfb46a807677f4a4a2bb88dcb78a358cf5121a 
> 
> Diff: https://reviews.apache.org/r/47891/diff/
> 
> 
> Testing
> -------
> 
> Tested on a range of Linux configurations on internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message