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.
