Thanks for your review!

2013/10/10 Sumit Bose <sbose@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@redhat.com>
>
> > On Mon, Oct 07, 2013 at 12:22:13PM +0200, Benjamin Franzke wrote:
> > > 2013/10/7 Sumit Bose <sbose@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@lists.fedorahosted.org
> > > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > > > >
> > > >
> > > > > _______________________________________________
> > > > > sssd-devel mailing list
> > > > > sssd-devel@lists.fedorahosted.org
> > > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> > > > _______________________________________________
> > > > sssd-devel mailing list
> > > > sssd-devel@lists.fedorahosted.org
> > > > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
> > > >
> >
> > > _______________________________________________
> > > sssd-devel mailing list
> > > sssd-devel@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@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.