mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rojas <alexan...@mesosphere.io>
Subject Re: Review Request 61189: Added authorization for V1 events.
Date Tue, 08 Aug 2017 09:36:08 GMT

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




src/master/master.hpp
Lines 899 (patched)
<https://reviews.apache.org/r/61189/#comment258263>

    Why not making the principal part of the `HttpConnection`? any reason in particular



src/master/master.hpp
Line 1831 (original), 1834-1835 (patched)
<https://reviews.apache.org/r/61189/#comment258265>

    Why passing the `master` and the `authorizer` if one can extract the latter from the former.
    
    In fact `master.cpp` line 9550 you do `subscriber->master->authorizer` but all the
other ones use `subcriber->authorizer`.
    
    Let's get consistent there or add a comment.



src/master/master.cpp
Lines 9547-9569 (patched)
<https://reviews.apache.org/r/61189/#comment258266>

    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.



src/master/master.cpp
Lines 9547-9569 (patched)
<https://reviews.apache.org/r/61189/#comment258268>

    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.


- Alexander Rojas


On July 27, 2017, 8:25 p.m., Quinn Leng wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61189/
> -----------------------------------------------------------
> 
> (Updated July 27, 2017, 8:25 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 V1 streaming events, the
> subscriber should only receive events that are authorized
> based on their principal and ACLs.
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
>   src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 
>   src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32 
>   src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f 
> 
> 
> Diff: https://reviews.apache.org/r/61189/diff/2/
> 
> 
> 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