mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Greg Mann <g...@mesosphere.io>
Subject Re: Review Request 58255: Added implicit authorization to the agent executor API.
Date Tue, 18 Apr 2017 03:16:36 GMT


> On April 12, 2017, 3:08 p.m., Alexander Rojas wrote:
> > My concern about this patch is, that unlike all the other security components of
Mesos, it makes implicit authorization mandatory. Moreover, we are breaking here the process
of authz between the module and mesos itself.
> > 
> > I think it would be better `if`'s statements to check first that `authorizer.isSome()`
as we used somewhere else. Although I would prefer to delegate the whole thing to the authorizer
itself, along the lines of:
> > 
> > ```c++
> > if (authorizer) {
> >   authorizer->verifyClaims(principal, ???);
> > }
> > ```

Yea this is a good point. One other place in the Mesos code where we do this is in the scheduler
API: https://github.com/apache/mesos/blob/3ded707cab2c1037fd1a699b075895feceb3ae4a/src/master/http.cpp#L885-L890

This enforces for the V1 API a constraint which is implicit in the V0 scheduler API: a framework
can only perform actions on itself. In V0, since authentication is performed only once when
the persistent TCP connection is established and all subsequent calls are issued over that
connection, this constraint is inherent to the implementation. In V1, we perform a check in
the handler to enforce that constraint.

The case of the executor API is slightly different, since the V0 executor API has no authentication
at all. Consequently, if we were to add the implicit authorization from this patch to the
local authorizer, we would be introducing authorization actions which are only used for the
V1 executor API and not for the V0 executor API, which would be an unfortunate inconsistency.

I think that either approach is not ideal. On one hand, as you said we would have implicit
authorization which is made mandatory in the handler, and on the other hand, we would have
a set of authorization actions which are only invoked for the V0 API, even though those actions
do exist on the V0 API as well.

I would propose that we punt on this for now, and leave a TODO here and in the relevant section
of 'master/http.cpp' to move both the scheduler and the executor implicit authorization into
the local authorizer later on. I could create a ticket for that work, reference it in the
TODO, and add it to the Executor AuthN Improvements epic when we create one. Does that seem
reasonable, or do you think that the executor API authorization should go into the authorizer
now?


- Greg


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


On April 14, 2017, 9:14 p.m., Greg Mann wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58255/
> -----------------------------------------------------------
> 
> (Updated April 14, 2017, 9:14 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7339
>     https://issues.apache.org/jira/browse/MESOS-7339
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This patch updates the agent handler for the executor API to
> verify the FrameworkID and ExecutorID contained within the
> executor's `Principal`, if present. This effectively performs
> implicit authorization of executor calls.
> 
> 
> Diffs
> -----
> 
>   src/slave/http.cpp 468cf332d79ed7315ecf51955235735dec0a2df1 
> 
> 
> Diff: https://reviews.apache.org/r/58255/diff/3/
> 
> 
> Testing
> -------
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>


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