[SSSD] [PATCH] CIFS idmap Plugin using SSSD

Benjamin Franzke benjaminfranzke at googlemail.com
Thu Oct 10 12:41:54 UTC 2013


Thanks for your review!

2013/10/10 Sumit Bose <sbose at redhat.com>

> On Wed, Oct 09, 2013 at 03:50:11PM +0200, Benjamin Franzke wrote:
> > Hi Sumit,
> >
> > Sorry for the delay, got sidetracked by trying to find an appropriate way
> > to handle the well known sids etc..
> > But thanks to your reminder I got over to do the integration into sssd's
> > autoconf/make now :)
> > So here is the patch as an attachment.
> > (as recommended by the contrib page - but if you like it better for
> review
> > l'll git send-email it as well..)
>
> Thanks a lot for the patch. It works well in my test but I have some
> comments, see below.
> >
> > This patch does not include an update for the rpm specs.
> > Could you do that integration? For me as a gentoo user that is currently
> an
> > unknown-area :)
>
> Sure no problem.
>
> >
> > Regards, Ben
> >
> > 2013/10/9 Sumit Bose <sbose at redhat.com>
> >
> > > On Mon, Oct 07, 2013 at 12:22:13PM +0200, Benjamin Franzke wrote:
> > > > 2013/10/7 Sumit Bose <sbose at redhat.com>
> > > >
> > > > > On Thu, Oct 03, 2013 at 11:57:07PM +0200, Benjamin Franzke wrote:
> > > > > > >
> > > > > > > Hi Ben,
> > > > > > >
> > > > > > > this is really great, thank you very much for the
> contribution! As
> > > a
> > > > > > > matter of fact, we've been planning on delivering the CIFS
> plugin
> > > as
> > > > > > > part of the next release:
> > > > > > > https://fedorahosted.org/sssd/ticket/1534
> > > > > > >
> > > > > > > Have you found the design page we created?
> > > > > > >
> > > > >
> > >
> https://fedorahosted.org/sssd/wiki/DesignDocs/IntegrateSSSDWithCIFSClient
> > > > > >
> > > > > >
> > > > > > Yeah, I've read those pages before writing the plugin..
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > Mostly the work on our side was done by Sumit Bose (I've CC-ed
> > > him). I
> > > > > > > would propose to get in touch with him to avoid duplicating
> some
> > > > > effort.
> > > > > > >
> > > > > > Unfortunately Sumit is in Germany and there is a holiday so he
> won't
> > > be
> > > > > > > available until Monday.
> > > > > > >
> > > > > >
> > > > > > > In general, I would suggest to merge this code into the SSSD
> tree
> > > if
> > > > > you
> > > > > > > wouldn't mind. I think it would makes sense to have the code
> in the
> > > > > same
> > > > > > > repo as the ID mapping library.
> > > > > > >
> > > > > >
> > > > > > I'd really appreciate having it integrated in sssd tree,
> > > > > > to have centralized maintainance and adoption by distributions..
> > > > >
> > > > > Thanks a lot for the patch. I would suggest to put your code into
> > > > > src/lib/cifs_idmap_sss/ and modify SSSD's Makefile.am to build the
> > > > > plugin. Would you like to do the integration and send the result
> for
> > > > > review to this list again?
> > > > >
> > > >
> > > > I'd like to do that integration, will send a patch..
> > >
> > > I'd like to add the plugin to the next Fedora release which
> > > unfortunately has its deadline next week. I don't want to rush you, but
> > > do you have a chance to send the patch today or tomorrow? Otherwise
> > > would you mind if I prepare the patch to integrate your code into the
> > > SSSD tree?
> > >
> > > Thanks for your help.
> > >
> > > bye,
> > > Sumit
> > > >
> > > >
> > > > > About the well-know SIDs. They should not be handled inside the
> plugin
> > > > > but by SSSD itself. If you are interested in adding them I'd be
> happy
> > > to
> > > > > help you how to start (if my help is needed at all :-).
> > > > >
> > > >
> > > > Yup, sounds better to handle the SIDs by SSSD core. I'd like to try..
> > > > I'll see whether time permits me to do that soon.. (doing this as a
> part
> > > of
> > > > my thesis)
> > > >
> > > >
> > > >
> > > > >
> > > > > bye,
> > > > > Sumit
> > > > >
> > > > > >
> > > > > >
> > > > > > > Thanks again for the contribution, much appreciated!
> > > > > > > _______________________________________________
> > > > > > > sssd-devel mailing list
> > > > > > > sssd-devel at lists.fedorahosted.org
> > > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > > >
> > > > >
> > > > > > _______________________________________________
> > > > > > sssd-devel mailing list
> > > > > > sssd-devel at lists.fedorahosted.org
> > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > sssd-devel at lists.fedorahosted.org
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > >
> > >
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > sssd-devel at lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > >
> > >
>
> ...
>
> > 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..f0599cb6b943145a685dbfd57bf5f8f48bfbfde6
> > --- /dev/null
> > +++ b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c
> > @@ -0,0 +1,338 @@
> > +/*
> > +    Authors:
> > +        Benjamin Franzke <benjaminfranzke at googlemail.com>
> > +
> > +    Copyright (C) 2013 Benjamin Franzke
> > +
> > +    This program is free software; you can redistribute it and/or modify
> > +    it under the terms of the GNU General Public License as published by
> > +    the Free Software Foundation; either version 3 of the License, or
> > +    (at your option) any later version.
> > +
> > +    This program is distributed in the hope that it will be useful,
> > +    but WITHOUT ANY WARRANTY; without even the implied warranty of
> > +    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > +    GNU General Public License for more details.
> > +
> > +    You should have received a copy of the GNU General Public License
> > +    along with this program.  If not, see <http://www.gnu.org/licenses/
> >.
> > +*/
> > +
> > +/* TODO: Support well known SIDs as in samba's
> > + *        - librpc/idl/security.idl or
> > + *        - source4/rpc_server/lsa/lsa_lookup.c?
> > + */
> > +
> > +/* TODO: Support of [all] samba's Unix SIDs:
> > + *         Users:  S-1-22-1-%UID
> > + *         Groups: S-1-22-2-%GID
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <errno.h>
> > +#include <string.h>
> > +#include <limits.h>
> > +#include <stdarg.h>
> > +
> > +#include <cifsidmap.h>
> > +
> > +#include "lib/idmap/sss_idmap.h"
> > +#include "sss_client/idmap/sss_nss_idmap.h"
> > +
> > +#define WORLD_SID "S-1-1-0"
> > +
> > +#ifdef DEBUG
> > +#include <syslog.h>
> > +#define debug(str, ...) \
> > +    syslog(0, "%s: " str "\n", \
> > +           __FUNCTION__, ##__VA_ARGS__)
> > +#else
> > +#define debug(...) do { } while(0)
> > +#endif
> > +
> > +struct sssd_ctx {
> > +    struct sss_idmap_ctx *idmap;
> > +    const char **errmsg;
> > +};
> > +
> > +#define ctx_set_error(ctx, error) \
> > +    do { \
> > +        *ctx->errmsg = error; \
> > +        debug("%s", error ? error : ""); \
> > +    } while (0);
> > +
> > +int cifs_idmap_init_plugin(void **handle, const char **errmsg)
> > +{
> > +    struct sssd_ctx *ctx;
> > +    enum idmap_error_code err;
>
> it should be checked if handle or errmsg are NULL.


> > +
> > +    ctx = malloc(sizeof *ctx);
> > +    if (!ctx) {
> > +        *errmsg = "Failed to allocate context";
> > +        return -1;
> > +    }
> > +    ctx->errmsg = errmsg;
> > +    ctx_set_error(ctx, NULL);
> > +
> > +    err = sss_idmap_init(NULL, NULL, NULL, &ctx->idmap);
> > +    if (err != IDMAP_SUCCESS) {
> > +        ctx_set_error(ctx, idmap_error_string(err));
>
> ctx should be freed to not leak memory.
>
> > +        return -1;
> > +    }
> > +
> > +    *handle = ctx;
> > +    return 0;
> > +}
> > +
> > +void cifs_idmap_exit_plugin(void *handle)
> > +{
> > +    struct sssd_ctx *ctx = handle;
> > +
> > +    debug("exit");
> > +
>
> it should be check if ctx is NULL before using it.
>
> > +    sss_idmap_free(ctx->idmap);
> > +
> > +    free(ctx);
> > +}
> > +
> > +
> > +/* Test with `getcifsacl file` on client. */
> > +int cifs_idmap_sid_to_str(void *handle, const struct cifs_sid *csid,
> > +                          char **name)
> > +{
> > +    struct sssd_ctx *ctx = handle;
> > +    enum idmap_error_code iderr;
> > +    char *sid;
> > +    enum sss_id_type id_type;
> > +    int err;
> > +
> > +    iderr = sss_idmap_bin_sid_to_sid(ctx->idmap,
> > +                                     (uint8_t *) csid, sizeof(*csid),
> &sid);
>
> please use const uint8_t. Using sss_idmap_bin_sid_to_sid() works as
> expected
> because struct cifs_sid is defined in the right order and is packed.
> Nevertheless I think in the long run we should add something like
> sss_idmap_cifs_sid_to_sid() to make the code more robust. But this does not
> need to happen now.

Ah, that missing const annoyes me..
Left that out while trying different formats (smb_sid etc..), and then I
forgot to add it back :\

I checked the bin_sid format in the sssd tests and found they match
the alignment and packing specifications of cifs_sid, so its the same
format..
What is bin_sid for, if not for comaptible binary formats?
Yeah, it could be replaced with a cifs_sid wrapper,
but maybe just remove the bin_sid stuff then as well?
(BTW:  removed that const when trying other formats, like smb_sid, which
were


> > +    if (iderr != IDMAP_SUCCESS) {
> > +        ctx_set_error(ctx, idmap_error_string(iderr));
> > +        *name = NULL;
> > +        return -1;
> > +    }
> > +
> > +    debug("sid: %s", sid);
> > +
> > +    if (strcmp(sid, WORLD_SID) == 0) {
> > +        *name = strdup("\\Everyone");
> > +        if (!*name) {
> > +            ctx_set_error(ctx, strerror(ENOMEM));
> > +            return -ENOMEM;
> > +        }
> > +        return 0;
> > +    }
> > +
> > +    err = sss_nss_getnamebysid(sid, name, &id_type);
> > +    if (err != 0)  {
> > +        ctx_set_error(ctx, strerror(err));
> > +        *name = NULL;
> > +        return -err;
> > +    }
> > +
> > +    /* FIXME: Map Samba Unix SIDs? (sid->id and use getpwuid)? */
> > +
> > +    debug("name: %s", *name);
> > +
> > +    return 0;
> > +}
> > +
> > +static int sid_to_cifs_sid(struct sssd_ctx *ctx, const char *sid,
> > +                           struct cifs_sid *csid)
> > +{
> > +    uint8_t *bsid = NULL;
> > +    enum idmap_error_code err;
> > +    size_t length;
> > +
> > +    err = sss_idmap_sid_to_bin_sid(ctx->idmap,
> > +                                   sid, &bsid, &length);
> > +    if (err != IDMAP_SUCCESS) {
> > +        ctx_set_error(ctx, idmap_error_string(err));
> > +        return -1;
> > +    }
> > +    if (length > sizeof(struct cifs_sid)) {
> > +        ctx_set_error(ctx, "too large sid length");
> > +        free(bsid);
> > +        return -1;
> > +    }
> > +
> > +    memcpy(csid, bsid, length);
> > +    free(bsid);
> > +
> > +    return 0;
> > +}
> > +
> > +/* Test with setcifsacl -a */
> > +int cifs_idmap_str_to_sid(void *handle, const char *name,
> > +                          struct cifs_sid *csid)
> > +{
> > +    struct sssd_ctx *ctx = handle;
> > +    int err;
> > +    enum sss_id_type id_type;
> > +    const char *sid;
> > +    char *sss_sid = NULL;
> > +    int success = 0;
> > +
> > +    debug("%s", name);
> > +
> > +    if (strncmp("S-", name, 2) == 0) {
>
> I think this check is not needed and puts an unnecessary limitation to the
> user
> names.
>

With that implementation it was a limitation to the user names.
I changed the code to use string-sid to cifs-sid conversion only if name
lookup fails.

>
> > +        debug("%s: name is sid string representation", __FUNCTION__);
> > +        sid = name;
> > +        if (!sid) {
> > +        }
> > +    } else {
> > +        err = sss_nss_getsidbyname(name, &sss_sid, &id_type);
> > +        if (err != 0)  {
> > +            ctx_set_error(ctx, strerror(err));
> > +            /* TODO: Map name==Everyone to WOLD_SID? */
> > +            return -err;
> > +        }
> > +        sid = sss_sid;
> > +    }
> > +
> > +    if (sid_to_cifs_sid(ctx, sid, csid) != 0)
> > +        success = -1;
> > +
> > +    if (sss_sid)
>
> the if here is not needed, free() can handle NULL.
>
> > +        free(sss_sid);
> > +
> > +    return success;
> > +}
> > +
> > +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;
> > +    default:
> > +        cuxid->type = CIFS_UXID_TYPE_UNKNOWN;
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +static int sss_sid_to_id(struct sssd_ctx *ctx, const char *sid,
> > +                         struct cifs_uxid *cuxid)
> > +{
> > +    int err;
> > +    enum sss_id_type id_type;
> > +
> > +    err = sss_nss_getidbysid(sid, (uint32_t *)&cuxid->id.uid, &id_type);
> > +    if (err != 0)  {
> > +        ctx_set_error(ctx, strerror(err));
> > +        return -1;
> > +    }
> > +
> > +    switch (id_type) {
> > +    case SSS_ID_TYPE_UID:
> > +        cuxid->type = CIFS_UXID_TYPE_UID;
> > +        break;
> > +    case SSS_ID_TYPE_GID:
> > +        cuxid->type = CIFS_UXID_TYPE_GID;
> > +        break;
> > +    case SSS_ID_TYPE_BOTH:
> > +        cuxid->type = CIFS_UXID_TYPE_BOTH;
> > +        break;
> > +    case SSS_ID_TYPE_NOT_SPECIFIED:
> > +    default:
> > +        return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> > +/**
> > + * cifs_idmap_sids_to_ids - convert struct cifs_sids to struct
> cifs_uxids
> > + * usecase: mount.cifs -o sec=krb5,multiuser,cifsacl,nounix
>
> there is a trailing whitespace                                ^
>
> > + * test: ls -n on mounted share
> > + */
> > +int cifs_idmap_sids_to_ids(void *handle, const struct cifs_sid *csid,
> > +                           const size_t num, struct cifs_uxid *cuxid)
> > +{
> > +    struct sssd_ctx *ctx = handle;
> > +    enum idmap_error_code err;
> > +    int success = -1;
> > +    size_t i;
> > +    char *sid;
> > +
> > +    debug("num: %zd", num);
> > +
> > +    if (num > UINT_MAX) {
> > +        ctx_set_error(ctx, "num is too large.");
> > +        return -EINVAL;
> > +    }
> > +
> > +    for (i = 0; i < num; ++i) {
> > +        err = sss_idmap_bin_sid_to_sid(ctx->idmap, (uint8_t *) &csid[i],
> > +                                       sizeof(csid[i]), &sid);
>
> please use const uint8_t here as well.
>
> > +        if (err != IDMAP_SUCCESS) {
> > +            ctx_set_error(ctx, idmap_error_string(err));
> > +            continue;
> > +        }
> > +
> > +        cuxid[i].type = CIFS_UXID_TYPE_UNKNOWN;
> > +
> > +        if (sss_sid_to_id(ctx, sid, &cuxid[i]) == 0 ||
>
> there is a trailing whitespace                           ^
>
> > +            samba_unix_sid_to_id(sid, &cuxid[i]) == 0) {
> > +
> > +            debug("setting uid of %s to %d", sid, cuxid[i].id.uid);
> > +            success = 0;
> > +        }
> > +
> > +        free(sid);
> > +    }
> > +
> > +    return success;
> > +}
> > +
> > +
> > +int cifs_idmap_ids_to_sids(void *handle, const struct cifs_uxid *cuxid,
> > +                           const size_t num, struct cifs_sid *csid)
> > +{
> > +    struct sssd_ctx *ctx = handle;
> > +    int err, success = -1;
> > +    char *sid;
> > +    enum sss_id_type id_type;
> > +    size_t i;
> > +
> > +    debug("num ids: %zd", num);
> > +
> > +    if (num > UINT_MAX) {
> > +        ctx_set_error(ctx, "num is too large.");
> > +        return -EINVAL;
> > +    }
> > +
> > +    for (i = 0; i < num; ++i) {
> > +        err = sss_nss_getsidbyid((uint32_t)cuxid[i].id.uid, &sid,
> &id_type);
> > +        if (err != 0)  {
> > +            ctx_set_error(ctx, strerror(err));
> > +            csid[i].revision = 0;
> > +            /* FIXME: would it be safe to map *any* uid/gids unknown by
> sssd to
> > +             * SAMBA's UNIX SIDs? */
> > +            continue;
> > +        }
> > +
> > +        if (sid_to_cifs_sid(ctx, sid, csid) == 0)
> > +            success = 0;
> > +        else
> > +            csid[i].revision = 0;
> > +        free(sid);
> > +    }
> > +
> > +    return success;
> > +}
> > --
> > 1.8.1.5
> >
>

The patch with your concers fixed is attached.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131010/cadc86f0/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Add-CIFS-idmap-plugin.patch
Type: application/octet-stream
Size: 13036 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20131010/cadc86f0/attachment.obj>


More information about the sssd-devel mailing list