[illumos-Developer] No LOGIN method in libsasl? Why not? (with webrev...)

[Roland Mainz]

On Mon, Feb 14, 2011 at 3:21 PM, Dan McDonald <danmcd at nexenta.com> wrote:
> On Mon, Feb 14, 2011 at 03:05:07PM +0100, Roland Mainz wrote:
>> On Mon, Feb 14, 2011 at 2:11 PM, Dan McDonald <danmcd at nexenta.com> wrote:
>> > Here's the webrev:
>> >
>> >       http://www.kebe.com/~danmcd/webrevs/sasl-login/
>>
>> Some comments+questions after a 5min race through the code (and
>> spending much more time trying to find "LOGIN" in the standard
>> documentation... ;-/ ):
>>
>> 1. What's the difference between |_INTEGRATED_SOLARIS_| and |_SUN_SDK_| ?
>
> It's explained in usr/src/lib/libsasl/README.

Ok...

>> 2. usr/src/lib/sasl_plugins/login/login.c, function
>> |login_server_mech_step()| uses |strlen()| on a string constant:
>> -- snip --
>> +         *serveroutlen = strlen(USERNAME_CHALLENGE);
>> -- snip --
>> IMO something like |sizeof()| or any other static expression would be
>> nice (some compilers are able to reduce this kind of expression to a
>> const integer value but not all can do that).
>> The issue is repeated in the code for |strlen(PASSWORD_CHALLENGE);| ...
>
> Understood -- this is an issue in the original sasl code.  I'm loathe to
> change stuff like this only because it'll make an eventual upgrade to a more
> recent libsasl version (e.g. the current 2.1.23) harder w.r.t. merge hell.

Agreed...
... does the new version use this kind of code, too ? If "yes" point
me to the upstream sources that I can hack-up a small patch for this
and the other sources...

>> 3. Then there is this code:
>> +     /* get password */
>> +     password =
>> +         params->utils->malloc(sizeof(sasl_secret_t) + clientinlen + 1);
>> +     if (!password) {
>> +         MEMERROR(params->utils);
>> +         return SASL_NOMEM;
>> +     }
>> +
>> +     strncpy((char *)password->data, clientin, clientinlen);
>> +     password->data[clientinlen] = '\0';
>> +     password->len = clientinlen;
>>
>> Why are you allocating |clientinlen + 1| instead of |clientinlen| in
>> this case ? /usr/include/sasl/sasl.h defines member |data| of
>> |sasl_secret_t| as |unsigned char data[1];|, e.g. it has at least one
>> byte allocated in the structure itself... which AFAIK makes the need
>> for an extra |+1| for the '\0'-character unneccesary...
>
> I'm not, libsasl was.

Ok...

>> 4. Maybe it would be nice to have a comment in the source at the top
>> that "LOGIN" is non-standard...
>
> This I can probably handle.

Thanks...
... is there any URL which describes "LOGIN" ? If "yes" it may be nice
to have the reference link in a comment...

> It's supposed to be obsolete by "plain", but
> there's a lot of old crufty legacy code out there (<cough> Exchange </cough>)
> that needs LOGIN.

xx@@@!!!-M$...
... Ok...

> I'm attaching a diff of login.c vs. the original 2.1.15 libsasl code (the
> base for all Illumos libsasl code) so you can see the minimal intrusions.

Thanks... that looks good...

----

Bye,
Roland

-- 
  __ .  . __
 (o.\ \/ /.o) roland.mainz at nrubsig.org
  \__\/\/__/  MPEG specialist, C&&JAVA&&Sun&&Unix programmer
  /O /==\ O\  TEL +49 641 3992797
 (;O/ \/ \O;)