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 <[email protected]>
>> > >>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.
