mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Quinn Leng <quinn.leng....@gmail.com>
Subject Re: Review Request 61189: Added authorization for V1 events.
Date Fri, 18 Aug 2017 18:18:24 GMT


> On Aug. 8, 2017, 9:36 a.m., Alexander Rojas wrote:
> > src/master/master.cpp
> > Lines 9547-9569 (patched)
> > <https://reviews.apache.org/r/61189/diff/2/?file=1785310#file1785310line9547>
> >
> >     I don't think this part should be done as it is. Consider the case when you
have an `Acceptor` which uses a security module which connects to LDAP for ACLs.
> >     
> >     There is a delay for each request you make. It would be wasteful to create an
Acceptor which you will later not use.

After discussion in the #ship-mesos-api-filter channel, we decide to keep the logic. Thus
the authorizer is called for every events emitted by the stream.


> On Aug. 8, 2017, 9:36 a.m., Alexander Rojas wrote:
> > src/master/master.cpp
> > Lines 9547-9569 (patched)
> > <https://reviews.apache.org/r/61189/diff/2/?file=1785310#file1785310line9547>
> >
> >     In fact, why not having one acceptor for each event, something like:
> >     
> >     ```c++
> >     // This code doesn't necesarily compile.
> >     
> >     class EventTaskAddedAcceptor
> >     {
> >     public:
> >       static Owned<EventTaskAddedAcceptor> create(Principal, Authorizer);
> >       
> >       bool accept(Task, FrameworkInfo);
> >     
> >     private:
> >       EventTaskAddedAcceptor(
> >         Owned<AuthorizationAcceptor> authorizeTask,
> >         Owned<AuthorizationAcceptor> authorizeFramework);
> >      
> >       Owned<AuthorizationAcceptor> authorizeTask_;
> >       Owned<AuthorizationAcceptor> authorizeFramework_;
> >     };
> >     
> >     Owned<EventTaskAddedAcceptor> EventTaskAddedAcceptor::create(
> >         Principal principal, 
> >         Authorizer authorizer)
> >     {
> >       Future<Owned<AuthorizationAcceptor>> authorizeRole =
> >         AuthorizationAcceptor::create(
> >             subscriber->principal,
> >             subscriber->master->authorizer,
> >             authorization::VIEW_ROLE);
> >     
> >       Future<Owned<AuthorizationAcceptor>> authorizeFramework =
> >         AuthorizationAcceptor::create(
> >             subscriber->principal,
> >             subscriber->authorizer,
> >             authorization::VIEW_FRAMEWORK);
> >             
> >       return collect(authorizeRole, authorizeFramework)
> >         .then([](tuple<Owned<AuthorizationAcceptor>,
> >                        Owned<AuthorizationAcceptor>> acceptors) {
> >           return new EventTaskAddedAcceptor(acceptors[0], acceptors[1]);
> >         });
> >     }
> >     
> >     EventTaskAddedAcceptor::EventTaskAddedAcceptor(
> >         Owned<AuthorizationAcceptor> authorizeTask,
> >         Owned<AuthorizationAcceptor> authorizeFramework)
> >       : authorizeTask_(authorizeTask),
> >         authorizeFramework_(authorizeFramework)
> >     {
> >     }
> >     
> >     bool EventTaskAddedAcceptor::accept(Task task, FrameworkInfo info)
> >     {
> >       return authorizeTask->accept(task, info) &&
> >              authorizeFramework->accept(info);
> >     }
> >     ```
> >     
> >     This way you only need one acceptor per event and you hid the details on how
the authorization is made inside the acceptor API and remove that code from the caller.

Given our decision to call authorizer for every event, this combination doesn't bring in much
benefit.


- Quinn


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


On Aug. 17, 2017, 10:59 p.m., Quinn Leng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
> 
> (Updated Aug. 17, 2017, 10:59 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Alexander Rojas, and Greg Mann.
> 
> 
> Bugs: MESOS-7785
>     https://issues.apache.org/jira/browse/MESOS-7785
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Added authorization filtering for the V1 operator events on the
> master, the subscriber should only receive events that are
> authorized based on their principal and ACLs. Since the authorizer
> is called for every event emitted on the stream, the change of
> authorization rule will affect events that the subscribers can
> receive.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 959091c8ec03b6ac7bcb5d21b04d2f7d5aff7d54 
>   src/master/master.hpp b802fd153a10f6012cea381f153c28cc78cae995 
>   src/master/master.cpp 7f38a5e21884546d4b4c866ca5918db779af8f99 
>   src/tests/api_tests.cpp 3ab4740bcac29ecb89585da6adb1f563d6fc1f5f 
> 
> 
> Diff: https://reviews.apache.org/r/61189/diff/3/
> 
> 
> Testing
> -------
> 
> make check
> GLOG_v=2 ./bin/mesos-tests.sh --gtest_filter="ContentType/MasterAPITest.EventsAuthorizationFiltering*"
--verbose --gtest_repeat=100 --gtest_break_on_failure
> 
> 
> Thanks,
> 
> Quinn Leng
> 
>


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