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