You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@mesos.apache.org by Till Toenshoff <to...@me.com> on 2015/03/03 15:51:46 UTC
Re: Review Request 27760: Revised authenticator interface to allow
for two fold implementations.
> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
> > src/authentication/cram_md5/authenticator.cpp, line 449
> > <https://reviews.apache.org/r/27760/diff/17/?file=869278#file869278line449>
> >
> > Shouldn't this be protected by once() to avoid 2 different threads loading secrets at the same time?
>
> Till Toenshoff wrote:
> That would render our tests broken (those that reset the credentials). The concurrency is covered by a Lock within that SASL aux plugin code.
>
> Till Toenshoff wrote:
> Let me know if this is ok, please.
>
> Vinod Kone wrote:
> Sorry, I don't think I follow. Can you elaborate on what specific tests fail and why?
Our tests all run within the same OS-process. A ONCE-gatekeeper on credential loading within this process would indeed allow this only a single time. The CRAM-MD5 tests do attempt to re/load the credentials; see cram_md5_authentication.cpp #95, #140, ...
These tests make sense given that we want to test the behavior of the authenticator/authenticatee when using non matching credentials.
> On Feb. 20, 2015, 10:19 p.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 466-467
> > <https://reviews.apache.org/r/27760/diff/17/?file=869282#file869282line466>
> >
> > why this if condition for logging?
>
> Till Toenshoff wrote:
> Not sure I understand. Did you see the comment directly above those lines?
> ```
> // A failure to initialize the authenticator does lead to
> // unusable authentication but still allows non authenticating
> // frameworks and slaves to connect.
> ```
>
> Adam B wrote:
> I think (a few revisions back), the thought was that the default parameters for a master are authenticate_frameworks/slaves=false && credentials=none && authenticator=default(cram), at which point it seems unnecessary to warn somebody that we didn't load the authenticator. Upon returning to this decision, I see no reason not to log this message anytime authentication is disabled, even if it's the default setting. Maybe it would seem less harsh as an INFO in the default case, but a single WARN message on master startup could be a gentle nudge to try out authentication.
>
> Till Toenshoff wrote:
> Let's try to get this sorted out as well. Any actions needed?
>
> Vinod Kone wrote:
> I just meant, that we should probably always log that warning irrespective of the 'if(credentials.isSome() || (authenticatorNames[0] != DEFAULT_AUTHENTICATOR)) ' condition. IIUC, the fact that we are initializing an authenticator, irrespective of whether 'credentials' is provided is because there could be authenticators which don't depend on 'credentials'. So, if there are initialization errors it is better to log them sooner than later when they do enable 'authenticate_frameworks' or 'authenticate_slaves'.
>
> Alternatively, I would just simplify this as follows:
>
> ```
> if (intialize.isError()) {
>
> EXIT(1) << "Failed to initialize authenticator '" << authenticatorNames[0]
> << ": " << initialize.error();
> }
> ```
The default authenticator (CRAM MD5) does rely upon receiving credentials, hence its initializing fails without credentials. By default, users are not required to provide credentials, hence mesos would exit by default even though the user did not even activate authentication -- not a nice user experience.
I think the only possible simplification here is indeed removing the limitation on `if(credentials.isSome() || (authenticatorNames[0] != DEFAULT_AUTHENTICATOR))` and that is what I am going to propose in an update now :)
- Till
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/27760/#review73310
-----------------------------------------------------------
On Feb. 27, 2015, 2:28 a.m., Till Toenshoff wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/27760/
> -----------------------------------------------------------
>
> (Updated Feb. 27, 2015, 2:28 a.m.)
>
>
> Review request for mesos, Adam B, Kapil Arya, Niklas Nielsen, and Vinod Kone.
>
>
> Bugs: MESOS-2050
> https://issues.apache.org/jira/browse/MESOS-2050
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The initial design and implementation of the authenticator module interface caused issues and was not optimal for heavy lifting setup of external dependencies. By introducing a two fold design, this has been decoupled from the authentication message processing. The new design also gets us back on track to the goal of makeing SASL a soft dependency of mesos.
>
>
> Diffs
> -----
>
> include/mesos/authentication/authenticator.hpp f66217a
> src/Makefile.am 17d0d7a
> src/authentication/cram_md5/authenticator.hpp c6f465f
> src/authentication/cram_md5/authenticator.cpp PRE-CREATION
> src/authentication/cram_md5/auxprop.hpp b894386
> src/authentication/cram_md5/auxprop.cpp cf503a2
> src/master/master.hpp e288cdb
> src/master/master.cpp 76e217d
> src/tests/cram_md5_authentication_tests.cpp 92a89c5
>
> Diff: https://reviews.apache.org/r/27760/diff/
>
>
> Testing
> -------
>
> make check
>
>
> Thanks,
>
> Till Toenshoff
>
>