[SSSD] [PATCH] CIFS idmap Plugin using SSSD

Sumit Bose sbose at redhat.com
Fri Oct 11 14:12:51 UTC 2013


On Fri, Oct 11, 2013 at 03:36:13PM +0200, Lukas Slebodnik wrote:
> On (11/10/13 12:53), Benjamin Franzke wrote:
> >2013/10/11 Sumit Bose <sbose at redhat.com>
> >
> >> On Thu, Oct 10, 2013 at 02:41:54PM +0200, Benjamin Franzke wrote:
> >> > Thanks for your review!
> >> >
> >> <snip>
> >> >
> >> > The patch with your concers fixed is attached.
> >>
> >> Thank you for the new version. I only have two comments left:
> >>
> >> +    if (handle == NULL || errmsg == NULL) {
> >> +        *errmsg = strerror(EINVAL);
> >>
> >> you cannot return an error message if errmsg == NULL.
> >>
> >
> >yes, of course not :)
> >had to leave house and wanted to get the patch out before, that dumb
> >mistake was the result :\
> >
> >>
> >> +        return -EINVAL;
> >> +    }
> >>
> >>
> >> +    err = sss_nss_getsidbyname(name, &sid, &id_type);
> >> +    if (err != 0)  {
> >> +        /* Might be a raw string representation of SID,
> >> +         * try converting that before returning an error. */
> >> +        if (sid_to_cifs_sid(ctx, name, csid) == 0)
> >> +            return 0;
> >> +
> >>
> >>  ^^^^^^^^ trailing whitespace
> >>
> >> Oops, again.. have them highlighted in my editor now ;)
> >
> >> +        ctx_set_error(ctx, strerror(err));
> >> +        return -err;
> >> +    }
> >>
> >>
> >> bye,
> >> Sumit
> >>
> >
> >New patch attached, with these two things fixed.
> 
> Inline.
> 
> From c548048ad7f7db530de2194d9a71ba7a265a34c4 Mon Sep 17 00:00:00 2001
> From: Benjamin Franzke <benjaminfranzke at googlemail.com>
> Date: Thu, 26 Sep 2013 10:27:33 +0200
> Subject: [PATCH] Add CIFS idmap plugin
> 
> ---
>  Makefile.am                             |  21 ++
>  configure.ac                            |   2 +
>  src/conf_macros.m4                      |  14 ++
>  src/external/cifsidmap.m4               |  13 ++
>  src/lib/cifs_idmap_sss/cifs_idmap_sss.c | 339 ++++++++++++++++++++++++++++++++
>  5 files changed, 389 insertions(+)
>  create mode 100644 src/external/cifsidmap.m4
>  create mode 100644 src/lib/cifs_idmap_sss/cifs_idmap_sss.c
> 
> diff --git a/src/external/cifsidmap.m4 b/src/external/cifsidmap.m4
> new file mode 100644
> index 0000000000000000000000000000000000000000..546c373b0e66dd4136bca8d92cf220102b2f7830
> --- /dev/null
> +++ b/src/external/cifsidmap.m4
> @@ -0,0 +1,13 @@
> +AC_ARG_ENABLE([cifs-idmap-plugin],
> +              [AS_HELP_STRING([--disable-cifs-idmap-plugin],
> +                              [do not build CIFS idmap plugin])],
> +              [build_cifs_idmap_plugin=$enableval],
> +              [build_cifs_idmap_plugin=yes])
>                                       ^^^^^^
> This line mean that cifs_idmap_plugin will be build by default,
> but sssd.spec.in file is not updated with appropriate devel package.
> 
> Something like
>   %if (0%{?fedora} >= 17)
>   BuildRequires: cifs-utils-devel
>   %endif
> 
> BTW: cifs-utils-devel is not available on RHEL6, do we want to build
> cifs_idmap_plugin by default?
> 
> And library cifs_idmap_sss.so should be added to some (or new) subpackage
> in spec file. If you are not familiar with rpm packaging, we can help you.

Hi Lukas,

thank you for an additional pair of eyes. Ealier in the thread I said
that I'll take care of enhancing the spec file. I think
cifs_idmap_plugin should be build by default and disabled on platforms
like RHEL6 where cifs-utils do not uspport the plugin interface.

I've planned to add the plugin to the sssd-client package.

> 
> +
> +AS_IF([test x$build_cifs_idmap_plugin = xyes],
> +      [AC_CHECK_HEADER([cifsidmap.h], [],
> +                       [AC_MSG_ERROR([you must have the cifsidmap header installed to build the idmap plugin])])
> +      ])
> +
> +AM_CONDITIONAL([BUILD_CIFS_IDMAP_PLUGIN],
> +               [test x$build_cifs_idmap_plugin = xyes])
> 
> 
> diff --git a/src/lib/cifs_idmap_sss/cifs_idmap_sss.c b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..bcea6d6d84dbd6184c2f719d0c1b4a2ac3c014ca
> --- /dev/null
> +++ b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c
> 
> ... snip ...
> 
> +static int samba_unix_sid_to_id(const char *sid, struct cifs_uxid *cuxid)
> +{
> +    id_t id;
> +    uint8_t type;
> +
> +    if (sscanf(sid, "S-1-22-%hhu-%u", &type, &id) != 2)
> +        return -1;
> +
> +    switch (type) {
> +    case 1:
> +        cuxid->type = CIFS_UXID_TYPE_UID;
> +        cuxid->id.uid = id;
> +        break;
> +    case 2:
> +        cuxid->type = CIFS_UXID_TYPE_GID;
> +        cuxid->id.gid = id;
>         ^^^^^^^
> Missing brake statement.
> Is it aim or mistake? 

good catch

> 
> +    default:
> +        cuxid->type = CIFS_UXID_TYPE_UNKNOWN;
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> 
> And the last note.
> You created new dynamic library. Could you add this library to dlopen test?
> 
> +++ b/src/tests/dlopen-tests.c
> @@ -40,6 +40,7 @@ struct so {
>      { "libsss_debug.so", { LIBPFX"libsss_debug.so", NULL } },
>      { "libipa_hbac.so", { LIBPFX"libipa_hbac.so", NULL } },
>      { "libsss_idmap.so", { LIBPFX"libsss_idmap.so", NULL } },
> +    { "cifs_idmap_sss.so", { LIBPFX"cifs_idmap_sss.so", NULL } },
>      { "libsss_nss_idmap.so", { LIBPFX"libsss_nss_idmap.so", NULL } },
>      { "libnss_sss.so", { LIBPFX"libnss_sss.so", NULL } },
>      { "pam_sss.so", { LIBPFX"pam_sss.so", NULL } },
> 
> Thank you very much .
> 
> LS
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel



More information about the sssd-devel mailing list