Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.
--- 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.
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 https://reviews.apache.org/r/36927/diff/1/?file=1024921#file1024921line1574 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.
--- 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.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36927/#review93532 --- src/master/master.cpp (lines 1574 - 1576) https://reviews.apache.org/r/36927/#comment147918 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) https://reviews.apache.org/r/36927/#comment147917 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) https://reviews.apache.org/r/36927/#comment147916 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) https://reviews.apache.org/r/36927/#comment147919 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
Re: Review Request 36927: Pulled apart authorization and authentication validation for frameworks in the master.
--- 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.
On July 30, 2015, 12:49 a.m., Vinod Kone wrote: src/master/master.cpp, lines 1757-1779 https://reviews.apache.org/r/36927/diff/1/?file=1024921#file1024921line1757 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