[Freeipa-devel] [PATCH] slapi-nis support for trusted domains
Alexander Bokovoy
abokovoy at redhat.com
Mon Aug 5 12:45:06 UTC 2013
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.
--
/ Alexander Bokovoy
-------------- next part --------------
>From 85ccd4f5290ad9ac05c5b534b9ce64b078f6e5c1 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at redhat.com>
Date: Mon, 5 Aug 2013 15:42:29 +0300
Subject: [PATCH] Rename slapi-nis configuration variable
---
ipaserver/install/adtrustinstance.py | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/ipaserver/install/adtrustinstance.py b/ipaserver/install/adtrustinstance.py
index f072a6a..5839b2f 100644
--- a/ipaserver/install/adtrustinstance.py
+++ b/ipaserver/install/adtrustinstance.py
@@ -667,16 +667,16 @@ class ADTRUSTInstance(service.Service):
def __enable_compat_tree(self):
try:
compat_plugin_dn = DN("cn=Schema Compatibility,cn=plugins,cn=config")
- lookup_sssd_name = "schema-compat-lookup-sssd"
+ lookup_nsswitch_name = "schema-compat-lookup-nsswitch"
for config in (("cn=users", "user"), ("cn=groups", "group")):
entry_dn = DN(config[0], compat_plugin_dn)
current = self.admin_conn.get_entry(entry_dn)
- lookup_sssd = current.get(lookup_sssd_name, [])
- if not(config[1] in lookup_sssd):
- current[lookup_sssd_name] = [config[1]]
+ lookup_nsswitch = current.get(lookup_nsswitch_name, [])
+ if not(config[1] in lookup_nsswitch):
+ current[lookup_nsswitch_name] = [config[1]]
self.admin_conn.update_entry(entry_dn, current)
except Exception, e:
- root_logger.critical("Enabling SSSD support in slapi-nis failed with error '%s'" % e)
+ root_logger.critical("Enabling nsswitch support in slapi-nis failed with error '%s'" % e)
def __start(self):
try:
--
1.8.3.1
More information about the Freeipa-devel
mailing list