Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On July 30, 2015, 10:16 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36927/
> ---
> 
> (Updated July 30, 2015, 10:16 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework authorization is now done through `authorizeFramework` which is 
> consistent with `authorizeTask`.
> Also, `validateFrameworkAuthentication` captures the validation for 
> authentication.
> 
> I also made registration and re-registration consistent:
> 
> * Both perform the check for root submission, rather than just registration.
> * Authentication checks in `_registerFramework` and `_reregisterFramework` 
> are now comprehensive (thanks to `validateFrameworkAuthentication`).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
>   src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32 
> 
> Diff: https://reviews.apache.org/r/36927/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

[Ben Mahler]

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

(Updated July 30, 2015, 10:16 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Avoid double logging.


Repository: mesos


Description
---

Framework authorization is now done through `authorizeFramework` which is 
consistent with `authorizeTask`.
Also, `validateFrameworkAuthentication` captures the validation for 
authentication.

I also made registration and re-registration consistent:

* Both perform the check for root submission, rather than just registration.
* Authentication checks in `_registerFramework` and `_reregisterFramework` are 
now comprehensive (thanks to `validateFrameworkAuthentication`).


Diffs (updated)
-

  src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
  src/master/master.cpp 8162b2b796c9712a7b2d544118fe8de2646a8d32 

Diff: https://reviews.apache.org/r/36927/diff/


Testing
---

make check


Thanks,

Ben Mahler

Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

[Ben Mahler]

> On July 30, 2015, 12:49 a.m., Vinod Kone wrote:
> >

Kept the validation error composition per our offline discussion, returning for 
each case individually led to really verbose code, and we looked at using a 
lambda to leverage 'return', but this seemed to be the simplest route for now.


> On July 30, 2015, 12:49 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1574-1576
> > 
> >
> > so this line will be presented twice if principal is not set (which 
> > will be for most frameworks) because this function is called twice. that is 
> > kinda unfortunate and likely confusing to users.
> > 
> > not sure what we could do here yet.

Hm.. this is unfortunate, I've pulled it out into the caller code for now to 
avoid the double logging.


- Ben


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


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36927/
> ---
> 
> (Updated July 29, 2015, 11:52 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework authorization is now done through `authorizeFramework` which is 
> consistent with `authorizeTask`.
> Also, `validateFrameworkAuthentication` captures the validation for 
> authentication.
> 
> I also made registration and re-registration consistent:
> 
> * Both perform the check for root submission, rather than just registration.
> * Authentication checks in `_registerFramework` and `_reregisterFramework` 
> are now comprehensive (thanks to `validateFrameworkAuthentication`).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
>   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
> 
> Diff: https://reviews.apache.org/r/36927/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

[Vinod Kone]

> On July 30, 2015, 12:49 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1757-1779
> > 
> >
> > checking validationError.isNone() in each if statement looks a bit 
> > weird. how about doing these in an else if instead?
> > 
> > if (frameworkInfo.has_id() && !(frameworkInfo.id() == "")) {
> > 
> > } else if (!roles.contains(frameworkInfo.role()) {
> > 
> > } else if (frameworkInfo.user() == "root" && !flags.root_submissions) {
> > 
> > } else {
> > 
> > }
> > 
> > 
> > i think the above is easier to read?
> 
> Ben Mahler wrote:
> That's originally what I did, but it's harder to read that way. It looks 
> saner in your example because you used empty blocks. Here is what it would 
> actually look like:
> 
> ```
>   // TODO(vinod): Add "!=" operator for FrameworkID.
>   if (frameworkInfo.has_id() && !(frameworkInfo.id() == "")) {
> validationError = Error("Registering with 'id' already set");
>   } else if (!roles.contains(frameworkInfo.role())) {
> // TODO(vinod): Deprecate this in favor of ACLs.
> validationError = Error("Role '" + frameworkInfo.role() + "' is not" +
> " present in the master's --roles");
>   } else if (frameworkInfo.user() == "root" && !flags.root_submissions) {
> // TODO(vinod): Deprecate this in favor of authorization.
> validationError = Error("User 'root' is not allowed to run frameworks"
> " without --root_submissions set");
>   }
> ```
> 
> Also the it doesn't work so nicely for reregister because one of the 
> checks requires looping through framework ids.

It does look dense because of the comments but not that bad IMO. For example, 
you used the same pattern for checking authorization failure.

AFAIK we haven't used the pattern that you are proposing elsewhere and it reads 
weird to me. I would rather you just return with an error right away, even 
though it might be a bit verbose. What do you think?


- Vinod


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


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36927/
> ---
> 
> (Updated July 29, 2015, 11:52 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework authorization is now done through `authorizeFramework` which is 
> consistent with `authorizeTask`.
> Also, `validateFrameworkAuthentication` captures the validation for 
> authentication.
> 
> I also made registration and re-registration consistent:
> 
> * Both perform the check for root submission, rather than just registration.
> * Authentication checks in `_registerFramework` and `_reregisterFramework` 
> are now comprehensive (thanks to `validateFrameworkAuthentication`).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
>   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
> 
> Diff: https://reviews.apache.org/r/36927/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36927]

All tests passed.

- Mesos ReviewBot


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36927/
> ---
> 
> (Updated July 29, 2015, 11:52 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework authorization is now done through `authorizeFramework` which is 
> consistent with `authorizeTask`.
> Also, `validateFrameworkAuthentication` captures the validation for 
> authentication.
> 
> I also made registration and re-registration consistent:
> 
> * Both perform the check for root submission, rather than just registration.
> * Authentication checks in `_registerFramework` and `_reregisterFramework` 
> are now comprehensive (thanks to `validateFrameworkAuthentication`).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
>   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
> 
> Diff: https://reviews.apache.org/r/36927/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

[Ben Mahler]

> On July 30, 2015, 12:49 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 1757-1779
> > 
> >
> > checking validationError.isNone() in each if statement looks a bit 
> > weird. how about doing these in an else if instead?
> > 
> > if (frameworkInfo.has_id() && !(frameworkInfo.id() == "")) {
> > 
> > } else if (!roles.contains(frameworkInfo.role()) {
> > 
> > } else if (frameworkInfo.user() == "root" && !flags.root_submissions) {
> > 
> > } else {
> > 
> > }
> > 
> > 
> > i think the above is easier to read?

That's originally what I did, but it's harder to read that way. It looks saner 
in your example because you used empty blocks. Here is what it would actually 
look like:

```
  // TODO(vinod): Add "!=" operator for FrameworkID.
  if (frameworkInfo.has_id() && !(frameworkInfo.id() == "")) {
validationError = Error("Registering with 'id' already set");
  } else if (!roles.contains(frameworkInfo.role())) {
// TODO(vinod): Deprecate this in favor of ACLs.
validationError = Error("Role '" + frameworkInfo.role() + "' is not" +
" present in the master's --roles");
  } else if (frameworkInfo.user() == "root" && !flags.root_submissions) {
// TODO(vinod): Deprecate this in favor of authorization.
validationError = Error("User 'root' is not allowed to run frameworks"
" without --root_submissions set");
  }
```

Also the it doesn't work so nicely for reregister because one of the checks 
requires looping through framework ids.


- Ben


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


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36927/
> ---
> 
> (Updated July 29, 2015, 11:52 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework authorization is now done through `authorizeFramework` which is 
> consistent with `authorizeTask`.
> Also, `validateFrameworkAuthentication` captures the validation for 
> authentication.
> 
> I also made registration and re-registration consistent:
> 
> * Both perform the check for root submission, rather than just registration.
> * Authentication checks in `_registerFramework` and `_reregisterFramework` 
> are now comprehensive (thanks to `validateFrameworkAuthentication`).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
>   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
> 
> Diff: https://reviews.apache.org/r/36927/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.

[Vinod Kone]

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



src/master/master.cpp (lines 1574 - 1576)


so this line will be presented twice if principal is not set (which will be 
for most frameworks) because this function is called twice. that is kinda 
unfortunate and likely confusing to users.

not sure what we could do here yet.



src/master/master.cpp (lines 1720 - 1742)


checking validationError.isNone() in each if statement looks a bit weird. 
how about doing these in an else if instead?

if (frameworkInfo.has_id() && !(frameworkInfo.id() == "")) {

} else if (!roles.contains(frameworkInfo.role()) {

} else if (frameworkInfo.user() == "root" && !flags.root_submissions) {

} else {

}

i think the above is easier to read?



src/master/master.cpp (lines 1731 - 1732)


why did you use 2 if loops here instead of &&?

anyway, see above comment to make it simpler.



src/master/master.cpp (lines 1866 - 1882)


ditto. please use else if.


- Vinod Kone


On July 29, 2015, 11:52 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36927/
> ---
> 
> (Updated July 29, 2015, 11:52 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Framework authorization is now done through `authorizeFramework` which is 
> consistent with `authorizeTask`.
> Also, `validateFrameworkAuthentication` captures the validation for 
> authentication.
> 
> I also made registration and re-registration consistent:
> 
> * Both perform the check for root submission, rather than just registration.
> * Authentication checks in `_registerFramework` and `_reregisterFramework` 
> are now comprehensive (thanks to `validateFrameworkAuthentication`).
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 1135049d34f8ed4d56c795ad28c9ca1d07237b63 
>   src/master/master.cpp 2bfec2f69375444925252480142ee409b8474761 
> 
> Diff: https://reviews.apache.org/r/36927/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>