> On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 259-262 > > <https://reviews.apache.org/r/18381/diff/2/?file=511057#file511057line259> > > > > New line. > > > > Also, this file doesn't use multi-line style comments. Can yo just use > > "//"?
Any thoughts on using Doxygen-style comments (http://www.doxygen.nl/docblocks.html#cppblock), at least on public methods? Better (and more accessible) internal documentation would help the rest of us get up to speed on the code internals. > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 2494 > > <https://reviews.apache.org/r/18381/diff/2/?file=511057#file511057line2494> > > > > Why did you kill the "erase" here? Because deactivate(framework) which is called right above this calls authenticated.erase(framework->pid); Although, if the pid isn't found in frameworks for some reason, then we wouldn't call deactivate(framework) and the pid wouldn't be removed from authenticated. This scenario could arise if the framework successfully authenticates once without actually registering, then tries to authenticate again. I'm putting the explicit erase call back in (also for slaves), since it doesn't hurt to try to erase it twice. > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/tests/slave_recovery_tests.cpp, line 1741 > > <https://reviews.apache.org/r/18381/diff/2/?file=511068#file511068line1741> > > > > Why is this disabled? Updated the comment to: // Disable authentication so the spoofed re-registration below works. This test starts a slave and framework, waits for a running task, then stops the slave/executor. It then expects the framework/scheduler to receive a TASK_LOST message after it spoofs a slave registration, but the master won't accept the registration because that slave pid is no longer authenticated. Disabling authenticate_slaves makes the spoof work again. Otherwise, we would also need to spoof the whole authentication process, or disable checkpointing(?). // Spoof the registration attempt of a checkpointing slave // that failed recovery. We do this because simply restarting // the slave will result in a slave with a different pid than // the previous one. post(slave.get(), master.get(), registerSlaveMessage.get()); > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/master/master.hpp, lines 386-401 > > <https://reviews.apache.org/r/18381/diff/2/?file=511056#file511056line386> > > > > Since slaves and frameworks have different pids why do we want > > different hashmaps for frameworks and slaves? Is there anything in the master that prevents a malicious user from authenticating/registering a framework using a known authenticated slave's PID or v.v.? I guess the upcoming Authorization ACL/roles would take care of that, so I'm comfortable merging these and expecting authorization to handle separating the roles. > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, lines 588-592 > > <https://reviews.apache.org/r/18381/diff/2/?file=511063#file511063line588> > > > > Instead of a new message how about adding an "optional string message" > > to the shutdown method? Great idea. Making it "optional string error" so the slave can confidently "LOG(ERROR) << error;" without worrying that ShutdownMessage.error may have just been informational(INFO). > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 1934-1935 > > <https://reviews.apache.org/r/18381/diff/2/?file=511057#file511057line1934> > > > > indentation. > > > > More importantly can we just leverage the SlaveShutdown message? Now using the ShutdownMessage for slave, with optional 'error' string. > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/tests/examples_tests.cpp, line 35 > > <https://reviews.apache.org/r/18381/diff/2/?file=511065#file511065line35> > > > > Why is this disabled? Point to a JIRA. Looks like it was just some cruft leftover from pulling down the containerizer changes. Re-enabling. > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/messages/messages.proto, line 349 > > <https://reviews.apache.org/r/18381/diff/2/?file=511058#file511058line349> > > > > Can we make this optional? It will decouple scheduler and master > > deploys. Sure. Just trying to think what will happen during cluster upgrade, depending on the upgrade order: 1) Framework upgrades before master: Framework auth will set the 'type', (old) master will ignore and authenticate as a framework. Success. 2) Master upgrades before framework: Framework won't set type, master will interpret none as 'framework' and authenticate the framework. Success. 3) Master upgrades before slave: Slave won't try to authenticate, master shouldn't require it to. Slave will register as before. Success. 4) Slave upgrades before Master: Slave sets 'type=slave', master ignores type and authenticates slave as a framework. Slave credential probably isn't in the master yet, so master will fail the authentication, and the slave will shutdown because "Master refused authentication". Good enough? - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18381/#review36419 ----------------------------------------------------------- On March 5, 2014, 11:12 a.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18381/ > ----------------------------------------------------------- > > (Updated March 5, 2014, 11:12 a.m.) > > > Review request for mesos and Vinod Kone. > > > Bugs: MESOS-804 > https://issues.apache.org/jira/browse/MESOS-804 > > > Repository: mesos-git > > > Description > ------- > > Added authentication support for slaves. > Fixes MESOS-804. > > Open Issues: > - Should AuthenticateMessage be replaced with AuthenticateFrameworkMessage, > or specify an Authenticatee type as coded here? > - removeSlave vs. deactivate(Slave): Some uses of removeSlave might benefit > from just deactivating if checkpointing is enabled. > - We currently deactivate a registered slave/framework when a new > authenticate message comes in, even if the new authentication message is a > failure/fake. Will file a new JIRA for this security hole. > - When multiple entries for the same principal exist in the credentials file, > only the last entry is used. Acceptable behavior, but shouldn't this be > documented? > > > Diffs > ----- > > src/master/flags.hpp 159b2de > src/master/master.hpp 49a3e15 > src/master/master.cpp f7ba9aa > src/messages/messages.proto c26a3d0 > src/sasl/authenticatee.hpp 42a4eba > src/sched/sched.cpp 00f6307 > src/slave/flags.hpp e4d98a5 > src/slave/slave.hpp 01b80df > src/slave/slave.cpp b350df4 > src/tests/authentication_tests.cpp 127c5e6 > src/tests/examples_tests.cpp 34f0233 > src/tests/mesos.cpp 96adeac > src/tests/sasl_tests.cpp 945426d > src/tests/slave_recovery_tests.cpp 40a9599 > > Diff: https://reviews.apache.org/r/18381/diff/ > > > Testing > ------- > > make check; manually tested flatfile slave authentication success/failure. > Added new slave authentication unit tests in authentication_tests.cpp. > > > Thanks, > > Adam B > >
