[Freeipa-devel] [PATCH] 117 extdom: replace winbind calls with POSIX/SSSD calls
Alexander Bokovoy
abokovoy at redhat.com
Wed Jul 10 16:15:44 UTC 2013
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.
--
/ Alexander Bokovoy
-------------- next part --------------
>From 10eef7991c879fc0f7860d8b6ef6abc45f0465dd Mon Sep 17 00:00:00 2001
From: Alexander Bokovoy <abokovoy at redhat.com>
Date: Tue, 9 Jul 2013 10:26:22 +0300
Subject: [PATCH 14/15] 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 | 33 ++++++++++++++++++++--
1 file changed, 30 insertions(+), 3 deletions(-)
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 823745e..26262e4 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
@@ -48,6 +48,7 @@
#include "util.h"
#define MAX(a,b) (((a)>(b))?(a):(b))
+#define SSSD_DOMAIN_SEPARATOR '@'
int parse_request_data(struct berval *req_val, struct extdom_req **_req)
{
@@ -242,7 +243,7 @@ int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req,
goto done;
}
- sep = strrchr(fq_name, '@');
+ sep = strrchr(fq_name, SSSD_DOMAIN_SEPARATOR);
if (sep == NULL) {
ret = LDAP_OPERATIONS_ERROR;
goto done;
@@ -274,8 +275,9 @@ int handle_request(struct ipa_extdom_ctx *ctx, struct extdom_req *req,
domain_name = strdup(req->data.name.domain_name);
break;
case INP_NAME:
- ret = asprintf(&fq_name, "%s@%s", req->data.name.object_name,
- req->data.name.domain_name);
+ ret = asprintf(&fq_name, "%s%c%s", req->data.name.object_name,
+ SSSD_DOMAIN_SEPARATOR,
+ req->data.name.domain_name);
if (ret == -1) {
ret = LDAP_OPERATIONS_ERROR;
fq_name = NULL; /* content is undefined according to
@@ -339,6 +341,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));
@@ -355,10 +358,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, SSSD_DOMAIN_SEPARATOR)) != NULL) {
+ if (strcasecmp(locat+1, domain_name) == 0 ) {
+ locat[0] = 0;
+ }
+ }
res->data.name.object_name =
strdup(pg_data->data.pwd.pw_name);
break;
case SSS_ID_TYPE_GID:
+ if ((locat = strchr(pg_data->data.grp.gr_name, SSSD_DOMAIN_SEPARATOR)) != NULL) {
+ if (strcasecmp(locat+1, domain_name) == 0) {
+ locat[0] = 0;
+ }
+ }
res->data.name.object_name =
strdup(pg_data->data.grp.gr_name);
break;
@@ -394,6 +407,11 @@ int create_response(struct extdom_req *req, struct pwd_grp *pg_data,
case SSS_ID_TYPE_BOTH:
res->response_type = RESP_USER;
res->data.user.domain_name = strdup(domain_name);
+ if ((locat = strchr(pg_data->data.pwd.pw_name, SSSD_DOMAIN_SEPARATOR)) != NULL) {
+ if (strcasecmp(locat+1, domain_name) == 0) {
+ locat[0] = 0;
+ }
+ }
res->data.user.user_name =
strdup(pg_data->data.pwd.pw_name);
@@ -409,6 +427,11 @@ int create_response(struct extdom_req *req, struct pwd_grp *pg_data,
case SSS_ID_TYPE_GID:
res->response_type = RESP_GROUP;
res->data.group.domain_name = strdup(domain_name);
+ if ((locat = strchr(pg_data->data.grp.gr_name, SSSD_DOMAIN_SEPARATOR)) != NULL) {
+ if (strcasecmp(locat+1, domain_name) == 0) {
+ locat[0] = 0;
+ }
+ }
res->data.group.group_name =
strdup(pg_data->data.grp.gr_name);
@@ -439,6 +462,10 @@ done:
free_resp_data(res);
}
+ if (locat != NULL) {
+ locat[0] = SSSD_DOMAIN_SEPARATOR;
+ }
+
return ret;
}
--
1.8.3.1
More information about the Freeipa-devel
mailing list