mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 71863: Moved creating authorization Object out of `Master::authorize*Volume`.
Date Thu, 12 Dec 2019 18:13:29 GMT


> On Dec. 4, 2019, 8:07 p.m., Benjamin Mahler wrote:
> > src/master/authorization.cpp
> > Lines 145-147 (patched)
> > <https://reviews.apache.org/r/71863/diff/1/?file=2181776#file2181776line145>
> >
> >     A broader question that relates to all of these reviews with Try values.. are
we essentially changing the way validation works? Will validation no longer be catching cases
(and creating error message for them) because we now catch them during authorization, and
potentially send worse error messages than the validation code generates?
> 
> Andrei Sekretenko wrote:
>     Of the two reviews with Try, the preceding patch does not change the way how authorization
interacts with validation. Will check error messages there.
>     
>     This one does - but I will reconsider this change, as it does not address the underlying
problem fully.
>     
>     -----
>     **The problem** is that currently combination of authorization + validation handles
some invalid operations in a very *interesting* way.
>     
>     If the operation is "valid enough" to build a valid authz object, we build the object,
authorize and, if authorized, validate. That produces appropriate validation/authorization
errors.
>     
>     On the other hand, if we cannot even build a valid authz object, we request authorization
for SOMETHING (usually, to perform the specified action on ANY object), authorize, and, if
authorized, validate. 
>     This means that if permissions setup is restrictive enough, the caller will be **declined
authorization** for a wrong reason instead of **getting a validation error**.
>     
>     I see two options to fix this systematically:
>      - split ACCEPT validation into stateless and stateful part, and move the first one
before authorization
>      - if the synchronous authz makes it (and we manage to avoid having async fallback
for non-compliant authorizers), to move the whole validation before ACCEPT authorization
>     
>     Will file a ticket and add this as TODO.

>  - if the synchronous authz makes it (and we manage to avoid having async fallback for
non-compliant authorizers), to move the whole validation before ACCEPT authorization

That would be great!


- Benjamin


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


On Dec. 3, 2019, 5:54 p.m., Andrei Sekretenko wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71863/
> -----------------------------------------------------------
> 
> (Updated Dec. 3, 2019, 5:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Benjamin Mahler.
> 
> 
> Bugs: MESOS-10023 and MESOS-10056
>     https://issues.apache.org/jira/browse/MESOS-10023
>     https://issues.apache.org/jira/browse/MESOS-10056
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Moved creating authorization Object out of `Master::authorize*Volume`.
> 
> 
> Diffs
> -----
> 
>   src/master/authorization.hpp PRE-CREATION 
>   src/master/authorization.cpp PRE-CREATION 
>   src/master/http.cpp e03655863ea2d4e4464b3d14b359de3d7f059778 
>   src/master/master.hpp 93630421d58e6fd26566e81a23cd910957795665 
>   src/master/master.cpp 14b90a5e276df055bb8a570331f27cab200c9869 
> 
> 
> Diff: https://reviews.apache.org/r/71863/diff/1/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Andrei Sekretenko
> 
>


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