On 08/05/2013 02:45 PM, Alexander Bokovoy wrote: > On Sun, 04 Aug 2013, Nalin Dahyabhai wrote: >>> >>* The help text still refers to SSSD specifically, when the code doesn't >>> >>enforce or guarantee that SSSD's involved when performing nsswitch >>> >>lookups or PAM authentication. >>> > >>> >The whole setup really makes sense only when SSSD is in use. Aside from >>> >that, the whole setup is triggered only if we find libsss_nss_idmap >>> >library which is provided by SSSD. Yes, it is used for an optional >>> >adding of the SID but linking is explicit. >> >> Yeah, but why is all of that required? If we want to be able to gateway >> whatever's going on at the server, the ipaNTSecurityIdentifier lookup is >> already optional at runtime. Making that part optional at compile-time >> would make the rest of the code (at least the nsswitch parts) easier to >> self-test. > 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 also moved src/back-sch-sssd.c to src/back-sch-nss.c to reflect that > and renamed SSSD references to NSSWITCH. > >>> >>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. > >>> >>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. > >>> >>* backend_retrieve_user_entry_from_sssd(): extra whitespace when >>> >>constructing the new entry's DN. >> >> Both of these still have an extra space after the assignment operator. > Done > > >>> >>* backend_retrieve_group_list_from_sssd() needs to handle ERANGE errors. >>> >>* backend_retrieve_group_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_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. > >>> >>HEAD~5: >>> >>* free_pam_response() uses slapi_ch_free() to free memory that was >>> >>probably allocated with malloc(). >>> >>* pam_conv_func(): remove extra whitespace in call to slapi_pblock_get() >>> >>* pam_conv_func(): use malloc() instead of slapi_ch_calloc() to allocate >>> >>replies, because plugins tend to be use free() to free them. >>> >>* pam_conv_func(): replace if/else-if/else-if/else-if stacks with a >>> >>switch() statement. >>> >>* do_pam_auth(): fix line wrapping in function signature. >>> >>* do_pam_auth(): comments suggest that it's just entered or left a >>> >>critical section, when the function's been called in one. >>> >>* do_pam_auth(): replace if/else-if/else-if/else-if stacks with a >>> >>switch() statement. >>> >>* do_pam_auth(): fix line wrapping when calling PR_smprintf(). >> >> Missed one, right after the done: label. > Done. > >>> >>* 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. > > I moved freeing the closest_match after we print the debug message. > >> >>> >>HEAD~2: >>> >>* backend_bind_cb() should avoid having to cast away a "const" by using >>> >>non-const variables for the saved copies of the group and set names. >>> >all done >> >> I meant to suggest that attempting to use an array to store both a const >> and a non-const string still forces a cast somewhere. Use two >> variables. > Done. > >> >>> ... 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. > > I don't think there is a need to abstract it more -- when we stage the request > it is easier to store name (or uid) in a string form anyway. Otherwise > I'd need to go with a union to store either string, an uid, or a gid... > > New code is in my repository at fedorapeople.org. Additionally, a patch > to FreeIPA to change configuration variables names is attached.
ACK. Pushed to master. Martin _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel