mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 56178: Enabled the authorizer to work with MULTI_ROLE frameworks.
Date Thu, 09 Feb 2017 08:59:53 GMT


> On Feb. 7, 2017, 10:16 a.m., Adam B wrote:
> > src/master/master.cpp, line 2178
> > <https://reviews.apache.org/r/56178/diff/5/?file=1626265#file1626265line2178>
> >
> >     What does the framework's capability have to do with whether a custom authorizer
can support multi-role authorization?
> >     If I have an old authorizer module, and multi-role (phase 1) capable frameworks,
I will only be able to authorize frameworks with a single role (regardless of capability).
For a multi-role framework, I could authorize a single role from the list, but that essentially
provides a back-door for those frameworks to use other roles. Or I could call the authorizer
multiple times, one for each role. But we wouldn't want to do that for multi-role capable
authorizers. Maybe we need the concept of "module capabilites" now too?
> >     Am I overthinking this?
> 
> Benjamin Bannier wrote:
>     There is only a weak relation between this framework capability and the capabilities
of authorizers. My thinking here was roughly:
>     
>     * The way authorization works ("send at most a single authorization request per authorizable
action") all information required for authorization needs to be packed into a single authorization
request.
>     * There are authorizers relying only on the `value` field to determine the role of
a framework; this approach is deprecated.
>     * In the current authorization approach there is no meaningful value for `value`
for multirole frameworks.
>     * Multirole frameworks are a new feature so not updating deprecated features to work
with them does not break backwards compatibility.
>     
>     With this change we
>     
>     * maintain the current, single-request authorization approach,
>     * avoid breaking authorizers assuming frameworks can only have a single role,
>     * avoid breaking authorizers using `value` and assuming a single-role world, and
>     * enable authorizers using `framework_info` to authorize multirole frameworks.
>     
>     This is very much in line with how e.g., `FrameworkInfo` has both a (deprecated)
`role` and a `roles` field, and the correct action is taken depending on framework capabilities.
>     
>     Would we instead e.g., send an authorization request for each role, we wouldn't just
introduce new code to use a deprecated field (`value`), but we would also introduce a lot
of potential overhead, all while ignoring the existence of `framework_info` which already
now bundles all this information and can be sent as part of an authorization request.
> 
> Adam B wrote:
>     Thanks for the explanation.
>     I agree that sending an authz request for each role is overkill just to support a
deprecated field. Forget about that.
>     My only concern is that authz decisions generally prefer to deny access when they
don't know what to do, and an old authorizer that only looks at the `value` field will try
to authorize a Framework registering with no role. Is that the same as registering with `*`?
If not, they probably already handle that as an error. If so, they would probably authorize
based on `*` (allow), when they should be authorizing based on `foo` and `bar` (deny). If
that's a serious concern, I'd rather remove the `value` field or otherwise break their compilation
so they are forced to review the API changes.
> 
> Benjamin Bannier wrote:
>     > My only concern is that authz decisions generally prefer to deny access when
they don't know what to do, and an old authorizer that only looks at the value field will
try to authorize a Framework registering with no role. Is that the same as registering with
*?
>     
>     Yes, for single role frameworks, if no role is set, we currently and in the future
will explicitly propagate the default `*` -- that's (unfortunately) part of our `FrameworkInfo`
proto API.
>     
>     For multirole frameworks we leave `value` unset, and since there is no default value
for `value` in the `Object` proto, consumers of this value would get the default value for
the string type (empty string). So authorizers relying on `value` and not explicitly checking
if `value` has been set, would see different behavior for new multirole-capable frameworks
(empty string vs. `*`) which I believe is good and should give them some pause. I'd argue
that since we explicitly propagate even an unset single-role framework role as `*` to authorizers,
having rules to match the empty string in an authorizer implementation seems unlikely as the
empty string is not a valid role value.
>     
>     > If not, they probably already handle that as an error. If so, they would probably
authorize based on * (allow), when they should be authorizing based on foo and bar (deny).
If that's a serious concern, I'd rather remove the value field or otherwise break their compilation
so they are forced to review the API changes.
>     
>     We are on the `if not` branch.
> 
> Adam B wrote:
>     Brilliant. Thanks for talking me through that. My concerns are addressed, and I'll
drop this issue and re-review.

Not directly related to `value`, but maybe worth pointing out: authorizers already looking
at `framework_info`, but unaware of `MULTI_ROLE` and not checking if `framework_info.has_role()`
would extract `*` from querying `framework_info.role()` for a `MULTI_ROLE` framework. This
might lead them to match against rules for `*` when they should have matched against rules
for `roles`. This would be caused by an unfortunate combination of a framework performing
incomplete input validation and us making a default of `*` part of the `FrameworkInfo` API.
I believe there is little we can do right here to mitigate this.

Ideally we should deprecate the default value for `role`. I am not sure this makes sense now
as `role` is already deprecated itself.


- Benjamin


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


On Feb. 9, 2017, 9:59 a.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56178/
> -----------------------------------------------------------
> 
> (Updated Feb. 9, 2017, 9:59 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7022
>     https://issues.apache.org/jira/browse/MESOS-7022
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This updates the local authorizer so that MULTI_ROLE frameworks can be
> authorized.
> 
> For non-MULTI_ROLE frameworks we continue to support use of the
> deprecated 'value' field in the authorization request's 'Object';
> however for MULTI_ROLE frameworks the 'value' field will not be set,
> and authorizers still relying on it should be updated to instead use
> the object's 'framework_info' field to extract roles to authorize
> against from.
> 
> 
> Diffs
> -----
> 
>   src/authorizer/local/authorizer.cpp b98e1fcdf2ee5ec1f6ac0be6f8accdefaa390a09 
>   src/master/master.cpp 620919ecfe85367b5c1281afc5216cc20e5e2e3c 
> 
> Diff: https://reviews.apache.org/r/56178/diff/
> 
> 
> Testing
> -------
> 
> Tested on various configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


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