> On Feb. 7, 2017, 1: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.
Brilliant. Thanks for talking me through that. My concerns are addressed, and I'll drop this issue and re-review. - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/56178/#review164477 ----------------------------------------------------------- On Feb. 7, 2017, 2:26 a.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/56178/ > ----------------------------------------------------------- > > (Updated Feb. 7, 2017, 2:26 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 98c39b279e7b9830d02efc8ec6a4469afc15d62a > > Diff: https://reviews.apache.org/r/56178/diff/ > > > Testing > ------- > > Tested on various configurations in internal CI. > > > Thanks, > > Benjamin Bannier > >
