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

[Alexander Bokovoy]

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.

-- 
/ Alexander Bokovoy