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 <aboko...@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@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 Simo. -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel