[Freeipa-devel] [PATCH] 117 extdom: replace winbind calls with POSIX/SSSD calls
Alexander Bokovoy
abokovoy at redhat.com
Thu Jul 11 11:02:54 UTC 2013
On Wed, 10 Jul 2013, Simo Sorce wrote:
>On Wed, 2013-07-10 at 19:15 +0300, Alexander Bokovoy wrote:
>> On Tue, 09 Jul 2013, Jakub Hrozek wrote:
>> >On Tue, Jul 09, 2013 at 11:42:00AM +0200, Jakub Hrozek wrote:
>> >> On Tue, Jul 09, 2013 at 10:33:19AM +0300, Alexander Bokovoy wrote:
>> >> > On Mon, 08 Jul 2013, Jakub Hrozek wrote:
>> >> > >On Mon, Jul 08, 2013 at 07:32:41PM +0300, Alexander Bokovoy wrote:
>> >> > >>On Mon, 08 Jul 2013, Jakub Hrozek wrote:
>> >> > >>>On Mon, Jul 08, 2013 at 04:15:39PM +0300, Alexander Bokovoy wrote:
>> >> > >>>>On Mon, 08 Jul 2013, Alexander Bokovoy wrote:
>> >> > >>>>>On Wed, 03 Jul 2013, Sumit Bose wrote:
>> >> > >>>>>>Hi,
>> >> > >>>>>>
>> >> > >>>>>>with this patch the extdom plugin, the LDAP extended operation that
>> >> > >>>>>>allows IPA clients with recent SSSD to lookup AD users and groups, will
>> >> > >>>>>>not use winbind for the lookup anymore but will use SSSD running in
>> >> > >>>>>>ipa_server_mode.
>> >> > >>>>>>
>> >> > >>>>>>Since now no plugin uses the winbind client libraries anymore, the
>> >> > >>>>>>second patch removes the related configures checks.
>> >> > >>>>>>
>> >> > >>>>>>I think for the time being we cannot remove winbind completely because
>> >> > >>>>>>it might be needed for msbd to work properly in a trusted environment.
>> >> > >>>>>s/msbd/smbd/
>> >> > >>>>>
>> >> > >>>>>ACK. I need to add 'ipa_server_mode = True' support to
>> >> > >>>>>the installer code and then these patches can go in.
>> >> > >>>>Actually, the code still doesn't work due to some bug in sssd which
>> >> > >>>>fails to respond properly to getsidbyname() request in libsss_nss_idmap.
>> >> > >>>>
>> >> > >>>>Additionally I've found one missing treatment of domain_name for
>> >> > >>>>INP_NAME requests.
>> >> > >>>>
>> >> > >>>>We are working with Jakub on tracking down what's wrong on SSSD side.
>> >> > >>>
>> >> > >>>Indeed, there was a casing issue in sysdb. You can continue testing with
>> >> > >>>lowercase user names in the meantime. A patch is already on the SSSD
>> >> > >>>list.
>> >> > >>In addition, we need to disqualify user name when returning back a
>> >> > >>packet from extdom operation as this is what SSSD expects.
>> >> > >>
>> >> > >>Attached patch does it for all types of requests.
>> >> > >>
>> >> > >>--
>> >> > >>/ Alexander Bokovoy
>> >> > >
>> >> > >>>From 3659059c646f7b584ee07fb9e780759bcc0bb08e Mon Sep 17 00:00:00 2001
>> >> > >>From: Alexander Bokovoy <abokovoy at redhat.com>
>> >> > >>Date: Mon, 8 Jul 2013 19:19:56 +0300
>> >> > >>Subject: [PATCH] Fix extdom plugin to provide unqualified name in response as
>> >> > >> sssd expects
>> >> > >>
>> >> > >>extdom plugin handles external operation over which SSSD asks IPA server about
>> >> > >>trusted domain users not found through normal paths but detected to belong
>> >> > >>to the trusted domains associated with IPA realm.
>> >> > >>
>> >> > >>SSSD expects that user or group name in the response will be unqualified
>> >> > >>because domain name for the user or group is also included in the response.
>> >> > >>Strip domain name from the name if getgrnam_r/getpwnam_r calls returned fully
>> >> > >>qualified name which includes the domain name we are asked to handle.
>> >> > >>
>> >> > >>The code already expects that fully-qualified names are following user at domain
>> >> > >>convention so we are simply tracking whether '@' symbol is present and is followed
>> >> > >>by the domain name.
>> >> > >>---
>> >> > >> .../ipa-extdom-extop/ipa_extdom_common.c | 26 ++++++++++++++++++++++
>> >> > >> 1 file changed, 26 insertions(+)
>> >> > >>
>> >> > >>diff --git a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
>> >> > >>index 8aa22e1..290da4e 100644
>> >> > >>--- a/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
>> >> > >>+++ b/daemons/ipa-slapi-plugins/ipa-extdom-extop/ipa_extdom_common.c
>> >> > >>@@ -295,6 +295,7 @@ int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req,
>> >> > >> &grp_result);
>> >> > >> }
>> >> > >> }
>> >> > >>+ domain_name = strdup(req->data.name.domain_name);
>> >> > >
>> >> > >I would prefer if this was a separate patch. But this is a correct
>> >> > >change.
>> >> > Separated.
>> >> >
>> >>
>> >> Ack to this patch.
>> >>
>> >> > >
>> >> > >> break;
>> >> > >> default:
>> >> > >> ret = LDAP_PROTOCOL_ERROR;
>> >> > >>@@ -338,6 +339,7 @@ int create_response(struct extdom_req *req, struct pwd_grp *pg_data,
>> >> > >> const char *domain_name, struct extdom_res **_res)
>> >> > >> {
>> >> > >> int ret = EFAULT;
>> >> > >>+ char *locat = NULL;
>> >> > >> struct extdom_res *res;
>> >> > >>
>> >> > >> res = calloc(1, sizeof(struct extdom_res));
>> >> > >>@@ -354,10 +356,20 @@ int create_response(struct extdom_req *req, struct pwd_grp *pg_data,
>> >> > >> switch(id_type) {
>> >> > >> case SSS_ID_TYPE_UID:
>> >> > >> case SSS_ID_TYPE_BOTH:
>> >> > >>+ if ((locat = strchr(pg_data->data.pwd.pw_name, '@')) != NULL) {
>> >> > >>+ if (strstr(locat, domain_name) != NULL ) {
>> >> > >
>> >> > >strstr doesn't work correctly in my case. In SSSD, the domain names are
>> >> > >case-insensitive, so you can use strcasestr here. In my case, the
>> >> > >condition is never hit as the domain case differs:
>> >> > >
>> >> > >407 res->data.user.domain_name = strdup(domain_name);
>> >> > >(gdb)
>> >> > >408 if ((locat = strchr(pg_data->data.pwd.pw_name, '@')) != NULL) {
>> >> > >(gdb) n
>> >> > >409 if (strstr(locat, domain_name) != NULL) {
>> >> > >(gdb)
>> >> > >414 strdup(pg_data->data.pwd.pw_name);
>> >> > >(gdb) p domain_name
>> >> > >$1 = 0x7f2e800f0690 "AD.EXAMPLE.COM"
>> >> > >(gdb) p locat
>> >> > >$2 = 0x7f2e8006500d "@ad.example.com"
>> >> > Replaced by strcasestr.
>> >> >
>> >> >
>> >> > >
>> >> > >Also, this is good enough for the beta or when the default values are used, but
>> >> > >in theory, the admin could configure the fully qualified name format to not
>> >> > >include the @-sign (see full_name_format in sssd.conf) at all.
>> >> > >
>> >> > >It's not very likely, but I think we should account for that case
>> >> > >eventually. I'm not exactly sure how yet as the full_name_format is pretty
>> >> > >free-form, maybe we could simply disallow setting it if server_mode was
>> >> > >set to True.
>> >> > I'd prefer the latter indeed. Given that you can have full_name_format
>> >> > varying between servers and clients, this is simply asking for disaster.
>> >> >
>> >> > I'd preper concentrating our effort on making default configuration
>> >> > working so well that you never need to modify these parameters. After
>> >> > all, we are talking about SSSD use as FreeIPA client with trusts
>> >> > enabled. SSSD configuration is dictated by ipa-client-install in this
>> >> > case for all clients.
>> >>
>> >> And Ack to the second patch as well.
>> >>
>> >> I opened https://fedorahosted.org/sssd/ticket/2009 on the SSSD side to
>> >> make the behaviour more predictable and robust.
>> >
>> >btw of course I tested with Sumit's patches applied before these two.
>> >They are quite large and I don't think I can review them, but from
>> >purely functional point of view, ack.
>> Simo asked to replace strcasestr() with strcasecmp() and I also replaced
>> '@' with SSSD_DOMAIN_SEPARATOR to make it clear what we are dealing
>> with.
>>
>> We'll need to be able to read separator value out of /etc/sssd.conf. I
>> wonder if this could actually be done by libsss_nss_idmap so that we
>> don't need to reimplement the code to read config and link directly with
>> libini_config.
>
>ack from me
Committed to master.
--
/ Alexander Bokovoy
More information about the Freeipa-devel
mailing list