> On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/master/flags.hpp, lines 109-119 > > <https://reviews.apache.org/r/18381/diff/2/?file=511055#file511055line109> > > > > have defaults as "false"? > > > > simply do a validation in master that they don't conflict.
Got rid of the Option<>s and put --authenticate as it was, but renamed Flags::authenticate to Flags::authenticate_frameworks for clarity. Added a TODO for future deprecation and transition to --authenticate_frameworks. > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 293-305 > > <https://reviews.apache.org/r/18381/diff/2/?file=511057#file511057line293> > > > > Hmm. This is getting complicated. > > > > Can you actually not do the flag deprecation stuff in this review. Lets > > do that in a subsequent review. IOW, keep "authenticate" flag as is and > > simply add "authenticate_slaves" flag. You can add a TODO for the rename. Understood. Left --authenticate as it was, but renamed Flags::authenticate to Flags::authenticate_frameworks for clarity. > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/master/master.cpp, line 297 > > <https://reviews.apache.org/r/18381/diff/2/?file=511057#file511057line297> > > > > s/frameworks but not slaves/ ? "Master only allowing authenticated frameworks but not slaves to register" sounds awkward to me, and inaccurate since any slave could register, regardless of whether it wants to authenticate. So I made frameworks and slaves separate log lines, e.g.: "Master only allowing authenticated frameworks to register Master allowing unauthenticated slaves to register" > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/sasl/authenticatee.hpp, lines 41-66 > > <https://reviews.apache.org/r/18381/diff/2/?file=511059#file511059line41> > > > > Why this refactoring? When I tried to include authenticatee.hpp in slave.cpp, I got "multiple definition" linker errors like: ./.libs/libmesos_no_3rdparty.a(libmesos_no_3rdparty_la-slave.o): In function `mesos::internal::sasl::Authenticatee::~Authenticatee()': /home/me/dev/mesos/build/src/../../src/sasl/authenticatee.hpp:390: multiple definition of `mesos::internal::sasl::Authenticatee::~Authenticatee()' ./.libs/libmesos_no_3rdparty.a(libmesos_no_3rdparty_la-sched.o):/home/me/dev/mesos/build/src/../../src/sasl/authenticatee.hpp:390: first defined here There are a few ways to fix this: 1) Move the implementations of Authenticatee's methods into the class definition (as I had done). 2) Move the implementations into an authenticatee.cpp. 3) Mark these implementation methods as 'inline'. I have now marked the implementations as inline, which is much less disruptive. > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/master/master.cpp, lines 2563-2604 > > <https://reviews.apache.org/r/18381/diff/2/?file=511057#file511057line2563> > > > > There's a lot of code duplication here. I'm not convinced that > > differentiating framework/slave authentication has any benefits. > > > > Either way we need to figure out to factor out some of this common code. After merging *.frameworks/slaves back into authenticating/authenticated/authenticators, most of the duplication was removed. There is now just the single authenticate() and _authenticate, as before. The only distinct code for frameworks vs. slaves is looping through master.frameworks and master.slaves to call deactivate(Framework) or deactivate(Slave) on each, plus a little logging. If we wanted to iterate through both frameworks and slaves to look for a matching pid to deactivate, we wouldn't even need the authenticatee type in AuthenticateMessage. However, I feel that it is important for the master to know what it is trying to authenticate the pid as, especially as we get to implementing role authorization. The other alternative is that we could have a separate AuthenticateSlaveMessage and leave AuthenticateMessage for frameworks; then it would be easier to add new fields (FrameworkID/SlaveID?) that might not apply to all kinds of AuthenticateMessages. Thoughts? > On March 6, 2014, 4:07 p.m., Vinod Kone wrote: > > src/slave/slave.cpp, lines 86-116 > > <https://reviews.apache.org/r/18381/diff/2/?file=511063#file511063line86> > > > > Both master and slave use this. We should really refactor this into a > > common function that both can use. We could probably stick it in > > sasl/common.hpp? Made a first pass at refactoring this. There's still the difference that master expects multiple credential lines whereas slave only expects a single credential line, so I have separate readCredential/readCredentials functions. That combined with the different errors/logging makes the current sasl/common.hpp not as neat as I'd like it to be. Ideas for improvement? - Adam ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/18381/#review36419 ----------------------------------------------------------- On March 7, 2014, 1:11 a.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/18381/ > ----------------------------------------------------------- > > (Updated March 7, 2014, 1:11 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/cluster.hpp d1bf680 > 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 > >
