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. > 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. > >>>>HEAD~7: > >>>>* Fix formatting of wrapped lines when calling > >>>>backend_shr_get_vattr_str(). > >>>>* Check for errors when converting the configured sssd_min_id to a number > >>>>by switching from atol() to strtol() or strtoul(). > >>>done. > > > >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. > >>>>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. > >>>>* backend_retrieve_group_list_from_sssd(): check for NULL result from > >>>>realloc(). > > > >Leaks are possible if realloc() fails for grouplist or entries. > 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'. > >>>>* backend_search_cb() now appears to be freeing the closest-match > >>>>as part of a conditional clause, right before it would unconditionally > >>>>do so. > >>>removed the duplicate. My original intent in moving that code was to > >>>avoid freeing closest-match right before we are printing it with > >>>slapi_log_error() and then sending the result with send_ldap_result(): > >>>https://git.fedorahosted.org/cgit/slapi-nis.git/tree/src/back-sch.c#n1219 > >>> > >>>I can revert this part back if you wish but I think there is an error in > >>>the original code. > > > >The current intent there is to avoid sending a closest-match DN when > >we're returning entries as part of the result, to only send such a value > >as part of the result if we're sending back an LDAP_NO_SUCH_OBJECT > >error. > My main issue with this is that for the case when we found the entry we > still show the debug message with 'closest match = (null)'. I think it > is misleading at least. Oh, that's the error you were referring to. Yeah, we should fix that. > >>... and I did fix couple more comments received from Sumit: > >> > >>1. switched to use usigned long for sssd_min_id > >>2. added initializer for 'dn' in > >>backend_retrieve_user_entry_from_sssd()/backend_retrieve_group_entry_from_sssd > >>3. switched to atoll() with cast of the result to (uid_t)/(gid_t) because > >> we already know at staging phase that the id in as a string will convert > >> without errors to an integer and it is > min id. > > > >Okay. Though, formatting the uid/gid as a string to pass it into a > >local function that converts it right back to an integer seems > >excessive. Anyhow, I guess we're down to minor stuff now. > Following this suggestion, I made two entry points to a larger helper, > one is a general one and another accepting the gid directly. The second one is > used by the grouplist search. Looks reasonable. HTH, Nalin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel