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

[Nalin Dahyabhai]

On Mon, Aug 05, 2013 at 03:45:06PM +0300, Alexander Bokovoy wrote:
> 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 like where this is going, but the configure logic looks a bit off.

In HEAD~9, if configure is passed --without-nsswitch, $use_nsswitch will
be set to "no", so the conditional USE_NSSWITCH will be enabled.  The
USE_PAM conditional's based on $use_nsswitch instead of $use_pam, which
looks like a copy/paste error.

The subsequent call to check the contents of $USE_NSSWITCH should
probably be changed to check $use_nsswitch, as automake conditionals
don't set variables in configure that are named after the conditionals.
Likewise for $USE_PAM and $use_pam.

> I also moved src/back-sch-sssd.c to src/back-sch-nss.c to reflect that
> and renamed SSSD references to NSSWITCH.

Ok.  Some line wrapping adjustments appear to still be needed.

> >>>>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.

That's fine by me.  Please correct the comment to reflect the intent.

> >>>>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.

Good.  One thing I just noticed: in backend_search_filter_has_cn_uid(),
is there a guarantee that bval->bv_val is NUL-terminated when it's being
passed to strcasecmp()?  If not, it could cause problems later.

> >>>>* 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.

I see it being handled for 'entries' now, but not where it's being done
for 'grouplist'.

> >>>>* 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.

Oh, that's the error you were referring to.  Yeah, we should fix that.  

> >>... 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.

Looks reasonable.

HTH,

Nalin