[Freeipa-devel] [PATCH] 117 extdom: replace winbind calls with POSIX/SSSD calls
Simo Sorce
simo at redhat.com
Wed Jul 10 16:33:42 UTC 2013
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
Simo.
--
Simo Sorce * Red Hat, Inc * New York
More information about the Freeipa-devel
mailing list