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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
