mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 55887: Check task user before allowing a task to be launched on the agent.
Date Mon, 20 Feb 2017 07:07:28 GMT

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




src/slave/slave.cpp (line 1662)
<https://reviews.apache.org/r/55887/#comment237880>

    Although I can tell what `transaction` means as an analogy here, perhaps just saying "because
they are either a single task or tasks in a single task group" is more direct?



src/slave/slave.cpp (line 1663)
<https://reviews.apache.org/r/55887/#comment237920>

    We can also comment that 
    
    ```
    Note that we only report the first error we encounter for the task group.
    ```



src/slave/slave.cpp (lines 1669 - 1670)
<https://reviews.apache.org/r/55887/#comment237899>

    We have already said "If any of the authorizations fails, we fail all the tasks" above.



src/slave/slave.cpp (line 1671)
<https://reviews.apache.org/r/55887/#comment237895>

    In this case I think we don't need an `await`, we can use a `collect` because of the "failing
all tasks when one authorization fails" semantics.



src/slave/slave.cpp (line 1689)
<https://reviews.apache.org/r/55887/#comment237913>

    s/taskUser/user/ would be shorter but still sufficient?



src/slave/slave.cpp (line 1706)
<https://reviews.apache.org/r/55887/#comment237919>

    s/Status Update/status update/



src/slave/slave.cpp (lines 1710 - 1711)
<https://reviews.apache.org/r/55887/#comment237917>

    Two-space indentation.



src/slave/slave.cpp (line 1713)
<https://reviews.apache.org/r/55887/#comment237918>

    s/tasks(s) // 
    because it's already in `taskOrTaskGroup`?
    
    Also s/WARNING/ERROR/ because this is actually an error, if we do log it.



src/slave/slave.cpp (line 1722)
<https://reviews.apache.org/r/55887/#comment237938>

    Why not TASK_ERROR?



src/slave/slave.cpp (lines 1735 - 1739)
<https://reviews.apache.org/r/55887/#comment237897>

    The way it is written, `Slave::run()` (and the slave actor) is going to be blocked by
`authorized.get()` which is not good. We should introduce a continuation method `_run` to
put the rest of the method and contents in the lambda above and rename the current `_run`
as `__run`.
    
    Then at the end of run():
    
    ```
      collect(authorizations)
        .onAny(defer(self(),
                     &Self::_run,
                     ...));
    ```



src/slave/slave.cpp (line 6161)
<https://reviews.apache.org/r/55887/#comment237898>

    This method is identitcal to the one on the master and at first glance people would probably
wonder why this is done again on the agent. We could use the space to explain why we do the
identitcal thing here for run tasks while not for other things.
    
    The reason as I see is that
    
    1. In general we shouldn't need to re-authorize actions that have already been authorized
by the master. To prevent a client from spoofing as the master and exploiting the agent, the
agent should authenticate the client as the master.
    
    2. We specially re-authorize the `RUN_TASK` action even though the same ACL is on the
master because a) in cases where hosts have heterogeneous user-account configurations, it
makes sense to set the ACL on the agent instead of on the master; 2) compared to other actions
such as killing a task and shutting down a framework, it's a greater security risk if malicious
tasks are launched as a superuser on the agent.


- Jiang Yan Xu


On Feb. 7, 2017, 7:53 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55887/
> -----------------------------------------------------------
> 
> (Updated Feb. 7, 2017, 7:53 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6953
>     https://issues.apache.org/jira/browse/MESOS-6953
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added support for action `run_tasks` on the agent's flag `acl`. Based on
> the ACL configured for `run_tasks`, a task to be launched on the agent
> will be (dis)allowed to launch on the agent.
> If a task or task group cannot be launched due to failed authorization,
> a `TASK_FAILED` Status Update shall be sent with a reason code of
> `REASON_TASK_UNAUTHORIZED` or `REASON_TASK_GROUP_UNAUTHORIZED` as
> applicable.
> Note that in case of a task group, all tasks fail if any of the tasks
> within the task group encounter the authorization error.
> 
> 
> Diffs
> -----
> 
>   src/slave/slave.hpp 5049eb783b8ad7b9599f20c3701f7d3d654b4491 
>   src/slave/slave.cpp 92564ff8fff06d1cb17192d374d355b4bb7d39d8 
> 
> Diff: https://reviews.apache.org/r/55887/diff/
> 
> 
> Testing
> -------
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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