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

[Martin Kosek]

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