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
>From 85ccd4f5290ad9ac05c5b534b9ce64b078f6e5c1 Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <aboko...@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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel
