[Freeipa-devel] [PATCH] slapi-nis support for trusted domains

[Alexander Bokovoy]

On Mon, 05 Aug 2013, Nalin Dahyabhai wrote:
>On Mon, Aug 05, 2013 at 03:45:06PM +0300, Alexander Bokovoy wrote:
>> OK, fair enough. I did use of libsss_nss_idmap optional. For tests I
>> think we need to involve nsswrapper here to make sure of a predictable
>> testing.
>>
>> I've added:
>>
>>   --with-nsswitch         use nsswitch API to look up users and groupsnot
>>                           found in the LDAP
>>   --with-sss-nss-idmap    use libsss_nss_idmap to discover SIDs. Requires
>>                           --with-nsswitch as well
>>   --with-pam              use PAM API to authenticate users not found in the
>>                           LDAP. Requires --with-nsswitch as well
>>
>> And also split PAM use -- if --with-nsswitch is provided, you may
>> optionally disable use of PAM.
>
>I like where this is going, but the configure logic looks a bit off.
>
>In HEAD~9, if configure is passed --without-nsswitch, $use_nsswitch will
>be set to "no", so the conditional USE_NSSWITCH will be enabled.  The
>USE_PAM conditional's based on $use_nsswitch instead of $use_pam, which
>looks like a copy/paste error.
>
>The subsequent call to check the contents of $USE_NSSWITCH should
>probably be changed to check $use_nsswitch, as automake conditionals
>don't set variables in configure that are named after the conditionals.
>Likewise for $USE_PAM and $use_pam.
Thanks, fixed.


>> I also moved src/back-sch-sssd.c to src/back-sch-nss.c to reflect that
>> and renamed SSSD references to NSSWITCH.
>
>Ok.  Some line wrapping adjustments appear to still be needed.
Yes, fixed.

>> >Looks good, except that the comment in the checking block implies that
>> >it's imposing a lower limit on ret.sssd_min_id, when it's reassigning
>> >the default in case of a parsing error.  If the value parses as valid,
>> >it appears that a value lower than 1000 would be accepted.
>> That's OK because it is a configuration option. If you want to have it set
>> to something like 500, so be it -- not all distributions enforce 1000
>> as their defaults.
>
>That's fine by me.  Please correct the comment to reflect the intent.
I've added one more sentence. Below is the full comment, reformatted by
my mail client:
    /* Make sure we don't return system users/groups by limiting lower bound on searches.
     * If config value cannot be parsed or not specified, default to 1000.
     * It is OK to specify something lower in the config as some
     * Linux distributions force lower limit to 500 */


>> >>>>HEAD~6:
>> >>>>* Drop extra whitespace after the type of the "name" member when
>> >>>>defining struct backend_search_filter_config.
>> >>>>* backend_search_filter_has_cn_uid() allocates config->name using
>> >>>>slapi_ch_malloc(), but it is later freed by free().
>> >>>>* backend_retrieve_user_entry_from_sssd(): fix line wrapping in
>> >>>>function signature.
>> >>>>* backend_retrieve_user_entry_from_sssd() needs to handle ERANGE errors.
>> >>>>* backend_retrieve_user_entry_from_sssd(): avoid using
>> >>>>slapi_entry_attr_set_int(), which will treat an unsigned 32-bit
>> >>>>with the high bit set as a negative value
>> >>>>* backend_retrieve_user_entry_from_sssd(): check for zero-length
>> >>>>pw_shell value, and avoid adding it as a loginShell value, since
>> >>>>that'd be invalid
>> >>>all done.
>> >
>> >That last one's just a NULL check now.  Please add a check for a
>> >zero-length value, which would also be skipped.  Otherwise, looks good.
>> Done.
>
>Good.  One thing I just noticed: in backend_search_filter_has_cn_uid(),
>is there a guarantee that bval->bv_val is NUL-terminated when it's being
>passed to strcasecmp()?  If not, it could cause problems later.
I've switched to use strncasecmp() with bval->bv_len but in reality
slapd manages it as a NUL-terminated string:
$ git grep f_avvalue
filter.c:    ptr = slapi_filter_sprintf(fmt, f->f_avtype, ESC_NEXT_VAL, f->f_avvalue.bv_val );
filter.c:                               if (strchr (f->f_avvalue.bv_val, '.')) {
filter.c:                                       char    *ocname = oc_find_name( f->f_avvalue.bv_val );
filter.c:                                               slapi_ch_free((void**)&f->f_avvalue.bv_val );
filter.c:                                               f->f_avvalue.bv_val = ocname;
filter.c:                                               f->f_avvalue.bv_len = strlen ( f->f_avvalue.bv_val );
filter.c:       *bval = &f->f_avvalue;
filter.c:       if ( 0 == strcasecmp ( f->f_avvalue.bv_val, SLAPI_ATTR_VALUE_TOMBSTONE)) {
filter.c:       if ( 0 == strcasecmp ( f->f_avvalue.bv_val, "ffffffff-ffffffff-ffffffff-ffffffff")) {
slap.h:#define f_avvalue        f_un.f_un_ava.ava_value
str2filter.c:           f->f_avvalue.bv_val = unqstr;
str2filter.c:           f->f_avvalue.bv_len = len2;
str2filter.c:           f->f_avvalue.bv_val = slapi_ch_strdup ( value );
str2filter.c:           f->f_avvalue.bv_len = strlen ( f->f_avvalue.bv_val );
subentry.c:     if ( 0 == strcasecmp ( f->f_avvalue.bv_val, "ldapsubentry")) {

Using strncasecmp() doesn't really hurt.

>> I've used a temporary variable to realloc() into and then only change
>> 'entries' if its value is not NULL. This should handle the case.
>
>I see it being handled for 'entries' now, but not where it's being done
>for 'grouplist'.
Added handling to all realloc()s.

Updated my tree with the new set of patches.
-- 
/ Alexander Bokovoy