----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/47891/#review135182 -----------------------------------------------------------
Please update `Master::authorizeTask` to call your new/aliased action with additional FwkInfo/ExecInfo, plus value=user for those authorization modules that didn't want to modify any code yet. Please also update the user extraction code to include taskInfo.executorInfo, and order them as suggested. Thanks! include/mesos/authorizer/authorizer.proto (line 56) <https://reviews.apache.org/r/47891/#comment200204> Is this deprecated or unused now? include/mesos/authorizer/authorizer.proto (line 85) <https://reviews.apache.org/r/47891/#comment200203> s/or TaskInfp/and TaskInfo/ include/mesos/authorizer/authorizer.proto (line 86) <https://reviews.apache.org/r/47891/#comment200206> I wonder if we should just alias RUN_TASK to the same enum value as RUN_TASK_WITH_USER.. There shouldn't be any backwards compatibility issues since these are only used in-memory, and modules have to recompile anyway. src/authorizer/local/authorizer.cpp (line 128) <https://reviews.apache.org/r/47891/#comment200205> This case will currently never be called. See Master::authorizeTask for where we call the authorizer with RUN_TASK_WITH_USER. We'll need to set the new action, and fill in the new Object metadata (FwkInfo, TaskInfo) src/authorizer/local/authorizer.cpp (line 304) <https://reviews.apache.org/r/47891/#comment200208> I'd rather we didn't put all this RunTask/user-specific logic in the general LocalAuthorizer::authorized() method. See how `authorization::ACCESS_SANDBOX` handles this with its `realRequest` (although maybe `specificRequest` would be a better name), in https://reviews.apache.org/r/47795/ (soon to be committed). src/authorizer/local/authorizer.cpp (lines 307 - 311) <https://reviews.apache.org/r/47891/#comment200207> For RUN_TASK, I would actually reverse the order of precedence (and add executorInfo.commandInfo.user). Ideally, I find a user in the taskInfo/executorInfo (only one can have a CommandInfo). If the task/executor doesn't have a user set, then default to the framework info. FrameworkInfo must have a user, but if the FrameworkInfo (and TaskInfo) weren't set, then I'd rely on the value string. task_info.executor_info.command.user task_info.command.user framework_info.user value src/tests/authorization_tests.cpp (lines 117 - 144) <https://reviews.apache.org/r/47891/#comment200210> Redundant. Please remove. src/tests/authorization_tests.cpp (line 203) <https://reviews.apache.org/r/47891/#comment200214> But principal "foo" could run as any other user, e.g. "bar", right? That'd be worth testing. src/tests/authorization_tests.cpp (line 529) <https://reviews.apache.org/r/47891/#comment200216> Would be better to test that "bar" cannot run a "user1", since we've shown previously that somebody else ("foo") can. - Adam B On May 26, 2016, 2:17 p.m., Benjamin Bannier wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/47891/ > ----------------------------------------------------------- > > (Updated May 26, 2016, 2:17 p.m.) > > > Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Michael > Park. > > > Bugs: MESOS-5459 > https://issues.apache.org/jira/browse/MESOS-5459 > > > Repository: mesos > > > Description > ------- > > Authorization requests for RUN_TASK actions can pass `SOME` > authorization object either in a `FrameworkInfo` holding a user, or a > `TaskInfo` with optionally a `CommandInfo` which can optionally hold a > user. If either of these fields is set it will be used as the object; > otherwise an `ANY` type authorization object will be created. > > > Diffs > ----- > > include/mesos/authorizer/authorizer.proto > 02d1a01d57cf34b38524f4368187878b03343537 > src/authorizer/local/authorizer.cpp > 3c7c791bde65cfcbcc4e319c9ccc487ab37d8029 > src/tests/authorization_tests.cpp 54bfb46a807677f4a4a2bb88dcb78a358cf5121a > > Diff: https://reviews.apache.org/r/47891/diff/ > > > Testing > ------- > > Tested on a range of Linux configurations on internal CI. > > > Thanks, > > Benjamin Bannier > >
