[SSSD] [PATCH] CIFS idmap Plugin using SSSD

Lukas Slebodnik lslebodn at redhat.com
Fri Oct 11 14:38:37 UTC 2013


On (11/10/13 16:23), Benjamin Franzke wrote:
>Hi, Lukas
>
>
>2013/10/11 Lukas Slebodnik <lslebodn at redhat.com>
>
>> 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.
>>
>
>As Sumit said, he'll do that part..
>
No problem.
I read this thread very quickly :-)

I've just wanted to point out, that it can break nightly built on RHEL6.
So it would be better to do changes at one, but I don't have a problem with two
patches.

>>
>> 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.
>>
>> +
>> +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?
>>
>
>That was a mistake, thanks.
>
Thank you.

>>
>> +    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?
>>
>
>Done. New patch attached.
>
Thank you for updating 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

It looks good to me.

LS



More information about the sssd-devel mailing list