mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Ben Mahler" <benjamin.mah...@gmail.com>
Subject Re: Review Request 14310: Added SASL authentication support.
Date Wed, 25 Sep 2013 07:09:53 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message