mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Sekretenko <asekrete...@mesosphere.io>
Subject Re: Review Request 71863: Moved creating authorization Object out of `Master::authorize*Volume`.
Date Wed, 11 Dec 2019 18:38:25 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?

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.


> On Dec. 4, 2019, 8:07 p.m., Benjamin Mahler wrote:
> > src/master/http.cpp
> > Lines 1067 (patched)
> > <https://reviews.apache.org/r/71863/diff/1/?file=2181777#file2181777line1067>
> >
> >     `std::move(*actionObjects)` ?

Checked once again - both ways are correct.

`(*actionObjects)` in this context a non-const lvalue reference, hence `std::move(*actionObject)`
is an rvalue reference to the vector.

`std::move(actionObject)` is an rvalue reference, and then `T&& Try<T> get()&&`
is used.

Will use the first option everywhere, for consistency.


- Andrei


-----------------------------------------------------------
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