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

Fri Aug 2 13:44:33 UTC 2013

On Fri, 02 Aug 2013, Alexander Bokovoy wrote:
>Hi Nalin!
>
>Thanks for the review.
>
>On Thu, 01 Aug 2013, Nalin Dahyabhai wrote:
>>On Wed, Jul 31, 2013 at 03:53:21PM +0300, Alexander Bokovoy wrote:
>>>Authentication is handled for both IPA and trusted domain users. The
>>>former case requires some specific handling of the SLAPI_BIND_TARGET_SDN
>>>to rewrite it to the original entry's DN. As result successful bind
>>>looks like this in the dirsrv logs:
>>>[31/Jul/2013:15:49:03 +0300] conn=15 fd=79 slot=79 SSL connection from 192.168.111.216 to 192.168.111.216
>>>[31/Jul/2013:15:49:03 +0300] conn=15 SSL 256-bit AES
>>>[31/Jul/2013:15:49:03 +0300] conn=15 op=0 BIND dn="uid=admin,cn=users,cn=compat,dc=example,dc=com" method=128 version=3
>>>[31/Jul/2013:15:49:03 +0300] conn=15 op=1 SRCH base="dc=example,dc=com" scope=2 filter="(uid=foobar)" attrs=ALL
>>>[31/Jul/2013:15:49:03 +0300] conn=15 op=0 RESULT err=0 tag=97 nentries=0 etime=0 dn="uid=admin,cn=users,cn=accounts,dc=example,dc=com"
>>
>>The RESULT dn being different from the BIND dn is easy to miss, but it
>>should prevent possible problems where a given user can be bound to more
>>than one DN, depending on where they started, so I guess it's fine.
>Yes. Frankly, I don't see any other way to handle this authentication
>correctly.
>
>>On to specific patches:
>>HEAD~11 looks fine.
>>HEAD~10:
>>* Add internal whitespace when computing the value to pass to
>> slapi_ch_malloc().
>>* Break the declaration and initialization of "str" into two lines.
>>* Use '\0' to terminate str instead of 0.
>>* In the new comment in format.h, NULL should be NUL.
>done
>
>>HEAD~9 looks fine.
>>HEAD~8:
>>* In configure.in, drop the hunk that changes the version number that we
>> pass to AC_INIT.
>>* In configure.in, one test compares x$use_sss_nss_idmap != xno, while
>> one shortly after compares $use_sss_nss_idmap = yes.  If
>> $use_sss_nss_idmap can be empty, then the second test needs to be
>> ready for that.
>>* 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.
>
>Due to these reasons I has left the text there and added small
>mentioning of nsswitch interface.
>
>>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.
>
>>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.
>
>>* backend_retrieve_user_entry_from_sssd() adds "ipaNTUserAttrs" as an
>> objectClass in the temporary entry, but the value isn't copied into
>> the cache.  What purpose does it serve?
>I removed this part since we are anyway using extensibleObject for the
>generated entry.
>
>>* backend_retrieve_user_entry_from_sssd(): extra whitespace when
>> constructing the new entry's DN.
>>* backend_retrieve_user_entry_from_sssd(): memory leak from not freeing
>> the result of slapi_escape_filter_value().
>>* backend_retrieve_group_entry_from_sssd(): fix line wrapping in
>> function signature.
>>* backend_retrieve_group_entry_from_sssd() needs to handle ERANGE errors.
>>* backend_retrieve_group_entry_from_sssd(): extra whitespace when
>> constructing the new entry's DN.
>>* backend_retrieve_group_entry_from_sssd(): memory leak from not freeing
>> the result of slapi_escape_filter_value().
>all done
>
>>* backend_retrieve_group_entry_from_sssd() adds "ipaNTGroupAttrs" as an
>> objectClass in the temporary entry, but the value isn't copied into
>> the cache.  What purpose does it serve?
>I removed this part since we are anyway using extensibleObject for the
>generated entry
>
>>* 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().
>>* backend_search_sssd(): fix line wrapping for call to
>> slapi_filter_apply().
>>* backend_search_sssd(): use strtol() or strtoul() to convert a value to
>> a number, and check for errors.
>>* backend_search_sssd(): staged->container_sdn/map_group/map_set are
>> allocated with slapi_ch_strdup() but freed with free() later.
>>* backend_retrieve_from_sssd(): check for NULL result from malloc().
>>* backend_retrieve_from_sssd(): fix line wrapping for calls to
>> backend_retrieve_user/group_entry_from_sssd.
>all done
>
>>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().
>all done
>
>>HEAD~4:
>>* Also needs to add back-sch.h to nisserver_plugin_la_SOURCES.
>not done. I see no reason to add back-sch.h to
>nisserver_plugin_la_SOURCES -- it is not used or referenced in any code
>included into the nisserver plugin.
>
>>HEAD~3:
>>* backend_search_find_set_data_in_group_cb(): fix line wrapping in
>> function signature.
>>* backend_search_cb() looks like it'll leak staged->entries[i] if
>> another thread has added a similarly-named entry to the cache.
>all 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.
>
>>* backend_search_cb() doesn't need to cast cbdata.sssd_buffer_len before
>> comparing it to -1.
>done
>
>>HEAD~2:
>>* backend_bind_cb() allocates strings with slapi_ch_strdup() and
>> slapi_entry_attr_get_charptr() but frees them with free().
>>* 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
>
>>HEAD~1:
>>* Drop the version number change.
>done. Instead, I have 'WIP' topmost patch handling version bump for
>development purposes that you can simply ignore.
>
>>HEAD:
>>* Doesn't mention the PAM service used for authentication, which it
>> probably should.  Still, a good start.
>Added a paragraph about PAM service, on par with man page for
>ipa-adtrust-install utility.
>
>My fedorapeople tree has new version.
... 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.
4. fixed copyright header in src/back-sch-sssd.c to mention 2013 only as this is new code.


-- 
/ Alexander Bokovoy