----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/14310/#review26367 -----------------------------------------------------------
Ship it! src/sasl/authenticatee.hpp <https://reviews.apache.org/r/14310/#comment51480> Do you want to hide this in a namespace? Ditto for AuthenticatorProcess. src/sasl/authenticatee.hpp <https://reviews.apache.org/r/14310/#comment51482> Line this up with the comment on the previous line. src/sasl/authenticatee.hpp <https://reviews.apache.org/r/14310/#comment51484> s/THe/The/ src/sasl/authenticatee.hpp <https://reviews.apache.org/r/14310/#comment51483> s/choosen/chosen src/sasl/authenticatee.hpp <https://reviews.apache.org/r/14310/#comment51485> Reading the manpage for sasl_client_start, I only see SASL_CONTINUE as a posible return code. What made you check for SASL_OK? src/sasl/authenticatee.hpp <https://reviews.apache.org/r/14310/#comment51495> Should we follow the SASL example here? We should not be sending when data/length is NULL/0 and SASL_OK is returned, looking at their sample client: https://github.com/winlibs/cyrus-sasl/blob/master/sample/sample-client.c#L818 src/sasl/authenticatee.hpp <https://reviews.apache.org/r/14310/#comment51487> This change mixes casts and static casts, stick to one? src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51499> Append sasl_errstring as done elsewhere? src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51500> Would be nice to comment on what NULL means for these as you've done above, ditto for the other calls if it's succinct :) src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51501> What do you mean by this comment? src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51502> Exclamation warranted? ;) Ditto on these other log lines. src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51503> Can you add the sasl_errstring to the log message? src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51504> Seems the sasl_errstring would also be useful in this log message? src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51505> Ditto on including sasl_errstring on these last two log messages. src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51506> Why hard-code length when you could do: if (...) { *result = "in-memory-auxprop"; } else if (...) { *result = "CRAM-MD5"; } if (length != NULL) { *length = strlen(*result); } I couldn't find good documentation on this, can length be NULL? Should we add an else block to guard against unexpected option strings? src/sasl/authenticator.hpp <https://reviews.apache.org/r/14310/#comment51507> Is there a sasl constant for this? I see the following in sasl.h: #define SASL_AUX_PASSWORD "*userPassword" /* User Password (of authid) */ The leading * is odd, and I see conversation around it. For example, they skip over it here: http://marc.info/?l=cyrus-sasl&m=104072288229946&w=2 Not sure how your I see you've used the newer SASL_AUX_PASSWORD_PROP below, should you use that constant here and skip the *? src/sasl/auxprop.hpp <https://reviews.apache.org/r/14310/#comment51508> What made you name the second argument as api? I see: /* default name for auxprop plug-in entry point is "sasl_auxprop_init" * similar to sasl_server_plug_init model, except only returns one * sasl_auxprop_plug_t structure; */ typedef int sasl_auxprop_init_t(const sasl_utils_t *utils, int max_version, int *out_version, sasl_auxprop_plug_t **plug, const char *plugname); Referring to: /* plug-in entry point: * utils -- utility callback functions * plugname -- name of plug-in (may be NULL) * max_version -- highest server plug version supported * returns: * out_version -- server plug-in version of result * pluglist -- list of mechanism plug-ins * plugcount -- number of mechanism plug-ins * results: * SASL_OK -- success * SASL_NOMEM -- failure * SASL_BADVERS -- max_version too small * SASL_BADPARAM -- bad config string * ... */ typedef int sasl_server_plug_init_t(const sasl_utils_t *utils, int max_version, int *out_version, sasl_server_plug_t **pluglist, int *plugcount); I can see how max_version is effectively the api version, is that why you renamed it? src/sasl/auxprop.cpp <https://reviews.apache.org/r/14310/#comment51509> Sasl didn't make this const?! src/sasl/auxprop.cpp <https://reviews.apache.org/r/14310/#comment51479> You wanted me to remind you to explain this line. src/sasl/auxprop.cpp <https://reviews.apache.org/r/14310/#comment51510> How about: CHECK(properties != NULL) << ... src/sasl/auxprop.cpp <https://reviews.apache.org/r/14310/#comment51511> From your formatting here, it looks as though you wanted newlines? If not, then maybe we should add quotes around the values to make this clearer on a single line: e.g. << " user: '" << user << "'" src/sasl/auxprop.cpp <https://reviews.apache.org/r/14310/#comment51512> It looks like you can keep property in the for loop scope: for (const propval* property = properties; property->name != NULL; ++property) { Did you avoid this because you wanted the for loop to fit on a single line? src/sasl/auxprop.cpp <https://reviews.apache.org/r/14310/#comment51514> s/for subsequent/to the same name as previous/ src/sasl/auxprop.cpp <https://reviews.apache.org/r/14310/#comment51515> Did you mean -1? From the documentation: * vallen -- length of value, if < 0 then strlen(value) will be used - Ben Mahler On Sept. 24, 2013, 8:26 p.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/14310/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2013, 8:26 p.m.) > > > Review request for mesos, Ben Mahler and Vinod Kone. > > > Repository: mesos-git > > > Description > ------- > > Initial SASL based authentication support. (I'll likely also be cleaning some > stuff up as I deal with compilation issues with different libsasl2* versions.) > > > Diffs > ----- > > configure.ac 96ac7a81311e9f3765bd54e078368566a3cc33ca > include/mesos/mesos.proto 8f845cc9b7cd491622cb29a69f6909170510a664 > src/Makefile.am 3eae9642b639e78f101e7fe580aa803beadbebda > src/messages/messages.proto 4d400c2d8f6ecb9f7cf8635b2a98341206bbb6c5 > src/sasl/authenticatee.hpp PRE-CREATION > src/sasl/authenticator.hpp PRE-CREATION > src/sasl/auxprop.hpp PRE-CREATION > src/sasl/auxprop.cpp PRE-CREATION > src/tests/sasl_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/14310/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
