<div dir="ltr">Thanks for your review!<div><br><div class="gmail_extra"><div class="gmail_quote">2013/10/10 Sumit Bose <span dir="ltr"><<a href="mailto:sbose@redhat.com" target="_blank">sbose@redhat.com</a>></span><br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div>On Wed, Oct 09, 2013 at 03:50:11PM +0200, Benjamin Franzke wrote:<br>
> Hi Sumit,<br>
><br>
> Sorry for the delay, got sidetracked by trying to find an appropriate way<br>
> to handle the well known sids etc..<br>
> But thanks to your reminder I got over to do the integration into sssd's<br>
> autoconf/make now :)<br>
> So here is the patch as an attachment.<br>
> (as recommended by the contrib page - but if you like it better for review<br>
> l'll git send-email it as well..)<br>
<br>
</div>Thanks a lot for the patch. It works well in my test but I have some<br>
comments, see below.<br>
<div>><br>
> This patch does not include an update for the rpm specs.<br>
> Could you do that integration? For me as a gentoo user that is currently an<br>
> unknown-area :)<br>
<br>
</div>Sure no problem.<br>
<div><div><br>
><br>
> Regards, Ben<br>
><br>
> 2013/10/9 Sumit Bose <<a href="mailto:sbose@redhat.com" target="_blank">sbose@redhat.com</a>><br>
><br>
> > On Mon, Oct 07, 2013 at 12:22:13PM +0200, Benjamin Franzke wrote:<br>
> > > 2013/10/7 Sumit Bose <<a href="mailto:sbose@redhat.com" target="_blank">sbose@redhat.com</a>><br>
> > ><br>
> > > > On Thu, Oct 03, 2013 at 11:57:07PM +0200, Benjamin Franzke wrote:<br>
> > > > > ><br>
> > > > > > Hi Ben,<br>
> > > > > ><br>
> > > > > > this is really great, thank you very much for the contribution! As<br>
> > a<br>
> > > > > > matter of fact, we've been planning on delivering the CIFS plugin<br>
> > as<br>
> > > > > > part of the next release:<br>
> > > > > > <a href="https://fedorahosted.org/sssd/ticket/1534" target="_blank">https://fedorahosted.org/sssd/ticket/1534</a><br>
> > > > > ><br>
> > > > > > Have you found the design page we created?<br>
> > > > > ><br>
> > > ><br>
> > <a href="https://fedorahosted.org/sssd/wiki/DesignDocs/IntegrateSSSDWithCIFSClient" target="_blank">https://fedorahosted.org/sssd/wiki/DesignDocs/IntegrateSSSDWithCIFSClient</a><br>
> > > > ><br>
> > > > ><br>
> > > > > Yeah, I've read those pages before writing the plugin..<br>
> > > > ><br>
> > > > ><br>
> > > > > ><br>
> > > > > > Mostly the work on our side was done by Sumit Bose (I've CC-ed<br>
> > him). I<br>
> > > > > > would propose to get in touch with him to avoid duplicating some<br>
> > > > effort.<br>
> > > > > ><br>
> > > > > Unfortunately Sumit is in Germany and there is a holiday so he won't<br>
> > be<br>
> > > > > > available until Monday.<br>
> > > > > ><br>
> > > > ><br>
> > > > > > In general, I would suggest to merge this code into the SSSD tree<br>
> > if<br>
> > > > you<br>
> > > > > > wouldn't mind. I think it would makes sense to have the code in the<br>
> > > > same<br>
> > > > > > repo as the ID mapping library.<br>
> > > > > ><br>
> > > > ><br>
> > > > > I'd really appreciate having it integrated in sssd tree,<br>
> > > > > to have centralized maintainance and adoption by distributions..<br>
> > > ><br>
> > > > Thanks a lot for the patch. I would suggest to put your code into<br>
> > > > src/lib/cifs_idmap_sss/ and modify SSSD's Makefile.am to build the<br>
> > > > plugin. Would you like to do the integration and send the result for<br>
> > > > review to this list again?<br>
> > > ><br>
> > ><br>
> > > I'd like to do that integration, will send a patch..<br>
> ><br>
> > I'd like to add the plugin to the next Fedora release which<br>
> > unfortunately has its deadline next week. I don't want to rush you, but<br>
> > do you have a chance to send the patch today or tomorrow? Otherwise<br>
> > would you mind if I prepare the patch to integrate your code into the<br>
> > SSSD tree?<br>
> ><br>
> > Thanks for your help.<br>
> ><br>
> > bye,<br>
> > Sumit<br>
> > ><br>
> > ><br>
> > > > About the well-know SIDs. They should not be handled inside the plugin<br>
> > > > but by SSSD itself. If you are interested in adding them I'd be happy<br>
> > to<br>
> > > > help you how to start (if my help is needed at all :-).<br>
> > > ><br>
> > ><br>
> > > Yup, sounds better to handle the SIDs by SSSD core. I'd like to try..<br>
> > > I'll see whether time permits me to do that soon.. (doing this as a part<br>
> > of<br>
> > > my thesis)<br>
> > ><br>
> > ><br>
> > ><br>
> > > ><br>
> > > > bye,<br>
> > > > Sumit<br>
> > > ><br>
> > > > ><br>
> > > > ><br>
> > > > > > Thanks again for the contribution, much appreciated!<br>
> > > > > > _______________________________________________<br>
> > > > > > sssd-devel mailing list<br>
> > > > > > <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
> > > > > > <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
> > > > > ><br>
> > > ><br>
> > > > > _______________________________________________<br>
> > > > > sssd-devel mailing list<br>
> > > > > <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
> > > > > <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
> > > ><br>
> > > > _______________________________________________<br>
> > > > sssd-devel mailing list<br>
> > > > <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
> > > > <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
> > > ><br>
> ><br>
> > > _______________________________________________<br>
> > > sssd-devel mailing list<br>
> > > <a href="mailto:sssd-devel@lists.fedorahosted.org" target="_blank">sssd-devel@lists.fedorahosted.org</a><br>
> > > <a href="https://lists.fedorahosted.org/mailman/listinfo/sssd-devel" target="_blank">https://lists.fedorahosted.org/mailman/listinfo/sssd-devel</a><br>
> ><br>
> ><br>
<br>
</div></div>...<br>
<br>
> diff --git a/src/lib/cifs_idmap_sss/cifs_idmap_sss.c b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c<br>
> new file mode 100644<br>
> index 0000000000000000000000000000000000000000..f0599cb6b943145a685dbfd57bf5f8f48bfbfde6<br>
> --- /dev/null<br>
> +++ b/src/lib/cifs_idmap_sss/cifs_idmap_sss.c<br>
> @@ -0,0 +1,338 @@<br>
> +/*<br>
> + Authors:<br>
> + Benjamin Franzke <<a href="mailto:benjaminfranzke@googlemail.com" target="_blank">benjaminfranzke@googlemail.com</a>><br>
> +<br>
> + Copyright (C) 2013 Benjamin Franzke<br>
> +<br>
> + This program is free software; you can redistribute it and/or modify<br>
> + it under the terms of the GNU General Public License as published by<br>
> + the Free Software Foundation; either version 3 of the License, or<br>
> + (at your option) any later version.<br>
> +<br>
> + This program is distributed in the hope that it will be useful,<br>
> + but WITHOUT ANY WARRANTY; without even the implied warranty of<br>
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the<br>
> + GNU General Public License for more details.<br>
> +<br>
> + You should have received a copy of the GNU General Public License<br>
> + along with this program. If not, see <<a href="http://www.gnu.org/licenses/" target="_blank">http://www.gnu.org/licenses/</a>>.<br>
> +*/<br>
> +<br>
> +/* TODO: Support well known SIDs as in samba's<br>
> + * - librpc/idl/security.idl or<br>
> + * - source4/rpc_server/lsa/lsa_lookup.c?<br>
> + */<br>
> +<br>
> +/* TODO: Support of [all] samba's Unix SIDs:<br>
> + * Users: S-1-22-1-%UID<br>
> + * Groups: S-1-22-2-%GID<br>
> + */<br>
> +<br>
> +#include <stdio.h><br>
> +#include <stdlib.h><br>
> +#include <errno.h><br>
> +#include <string.h><br>
> +#include <limits.h><br>
> +#include <stdarg.h><br>
> +<br>
> +#include <cifsidmap.h><br>
> +<br>
> +#include "lib/idmap/sss_idmap.h"<br>
> +#include "sss_client/idmap/sss_nss_idmap.h"<br>
> +<br>
> +#define WORLD_SID "S-1-1-0"<br>
> +<br>
> +#ifdef DEBUG<br>
> +#include <syslog.h><br>
> +#define debug(str, ...) \<br>
> + syslog(0, "%s: " str "\n", \<br>
> + __FUNCTION__, ##__VA_ARGS__)<br>
> +#else<br>
> +#define debug(...) do { } while(0)<br>
> +#endif<br>
> +<br>
> +struct sssd_ctx {<br>
> + struct sss_idmap_ctx *idmap;<br>
> + const char **errmsg;<br>
> +};<br>
> +<br>
> +#define ctx_set_error(ctx, error) \<br>
> + do { \<br>
> + *ctx->errmsg = error; \<br>
> + debug("%s", error ? error : ""); \<br>
> + } while (0);<br>
> +<br>
> +int cifs_idmap_init_plugin(void **handle, const char **errmsg)<br>
> +{<br>
> + struct sssd_ctx *ctx;<br>
> + enum idmap_error_code err;<br>
<br>
it should be checked if handle or errmsg are NULL.</blockquote><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> +<br>
> + ctx = malloc(sizeof *ctx);<br>
> + if (!ctx) {<br>
> + *errmsg = "Failed to allocate context";<br>
> + return -1;<br>
> + }<br>
> + ctx->errmsg = errmsg;<br>
> + ctx_set_error(ctx, NULL);<br>
> +<br>
> + err = sss_idmap_init(NULL, NULL, NULL, &ctx->idmap);<br>
> + if (err != IDMAP_SUCCESS) {<br>
> + ctx_set_error(ctx, idmap_error_string(err));<br>
<br>
ctx should be freed to not leak memory.<br>
<br>
> + return -1;<br>
> + }<br>
> +<br>
> + *handle = ctx;<br>
> + return 0;<br>
> +}<br>
> +<br>
> +void cifs_idmap_exit_plugin(void *handle)<br>
> +{<br>
> + struct sssd_ctx *ctx = handle;<br>
> +<br>
> + debug("exit");<br>
> +<br>
<br>
it should be check if ctx is NULL before using it.<br>
<br>
> + sss_idmap_free(ctx->idmap);<br>
> +<br>
> + free(ctx);<br>
> +}<br>
> +<br>
> +<br>
> +/* Test with `getcifsacl file` on client. */<br>
> +int cifs_idmap_sid_to_str(void *handle, const struct cifs_sid *csid,<br>
> + char **name)<br>
> +{<br>
> + struct sssd_ctx *ctx = handle;<br>
> + enum idmap_error_code iderr;<br>
> + char *sid;<br>
> + enum sss_id_type id_type;<br>
> + int err;<br>
> +<br>
> + iderr = sss_idmap_bin_sid_to_sid(ctx->idmap,<br>
> + (uint8_t *) csid, sizeof(*csid), &sid);<br>
<br>
please use const uint8_t. Using sss_idmap_bin_sid_to_sid() works as expected<br>
because struct cifs_sid is defined in the right order and is packed.<br>
Nevertheless I think in the long run we should add something like<br>
sss_idmap_cifs_sid_to_sid() to make the code more robust. But this does not<br>
need to happen now. </blockquote><div>Ah, that missing const annoyes me..</div><div>Left that out while trying different formats (smb_sid etc..), and then I forgot to add it back :\</div><div><br></div><div>I checked the bin_sid format in the sssd tests and found they match</div>
<div>the alignment and packing specifications of cifs_sid, so its the same format..<br></div><div>What is bin_sid for, if not for comaptible binary formats?</div><div>Yeah, it could be replaced with a cifs_sid wrapper,</div>
<div>but maybe just remove the bin_sid stuff then as well?</div><div>(BTW: removed that const when trying other formats, like smb_sid, which were</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + if (iderr != IDMAP_SUCCESS) {<br>
> + ctx_set_error(ctx, idmap_error_string(iderr));<br>
> + *name = NULL;<br>
> + return -1;<br>
> + }<br>
> +<br>
> + debug("sid: %s", sid);<br>
> +<br>
> + if (strcmp(sid, WORLD_SID) == 0) {<br>
> + *name = strdup("\\Everyone");<br>
> + if (!*name) {<br>
> + ctx_set_error(ctx, strerror(ENOMEM));<br>
> + return -ENOMEM;<br>
> + }<br>
> + return 0;<br>
> + }<br>
> +<br>
> + err = sss_nss_getnamebysid(sid, name, &id_type);<br>
> + if (err != 0) {<br>
> + ctx_set_error(ctx, strerror(err));<br>
> + *name = NULL;<br>
> + return -err;<br>
> + }<br>
> +<br>
> + /* FIXME: Map Samba Unix SIDs? (sid->id and use getpwuid)? */<br>
> +<br>
> + debug("name: %s", *name);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int sid_to_cifs_sid(struct sssd_ctx *ctx, const char *sid,<br>
> + struct cifs_sid *csid)<br>
> +{<br>
> + uint8_t *bsid = NULL;<br>
> + enum idmap_error_code err;<br>
> + size_t length;<br>
> +<br>
> + err = sss_idmap_sid_to_bin_sid(ctx->idmap,<br>
> + sid, &bsid, &length);<br>
> + if (err != IDMAP_SUCCESS) {<br>
> + ctx_set_error(ctx, idmap_error_string(err));<br>
> + return -1;<br>
> + }<br>
> + if (length > sizeof(struct cifs_sid)) {<br>
> + ctx_set_error(ctx, "too large sid length");<br>
> + free(bsid);<br>
> + return -1;<br>
> + }<br>
> +<br>
> + memcpy(csid, bsid, length);<br>
> + free(bsid);<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/* Test with setcifsacl -a */<br>
> +int cifs_idmap_str_to_sid(void *handle, const char *name,<br>
> + struct cifs_sid *csid)<br>
> +{<br>
> + struct sssd_ctx *ctx = handle;<br>
> + int err;<br>
> + enum sss_id_type id_type;<br>
> + const char *sid;<br>
> + char *sss_sid = NULL;<br>
> + int success = 0;<br>
> +<br>
> + debug("%s", name);<br>
> +<br>
> + if (strncmp("S-", name, 2) == 0) {<br>
<br>
I think this check is not needed and puts an unnecessary limitation to the user<br>
names.<br></blockquote><div><br></div><div>With that implementation it was a limitation to the user names.</div><div>I changed the code to use string-sid to cifs-sid conversion only if name lookup fails.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
> + debug("%s: name is sid string representation", __FUNCTION__);<br>
> + sid = name;<br>
> + if (!sid) {<br>
> + }<br>
> + } else {<br>
> + err = sss_nss_getsidbyname(name, &sss_sid, &id_type);<br>
> + if (err != 0) {<br>
> + ctx_set_error(ctx, strerror(err));<br>
> + /* TODO: Map name==Everyone to WOLD_SID? */<br>
> + return -err;<br>
> + }<br>
> + sid = sss_sid;<br>
> + }<br>
> +<br>
> + if (sid_to_cifs_sid(ctx, sid, csid) != 0)<br>
> + success = -1;<br>
> +<br>
> + if (sss_sid)<br>
<br>
the if here is not needed, free() can handle NULL.<br>
<br>
> + free(sss_sid);<br>
> +<br>
> + return success;<br>
> +}<br>
> +<br>
> +static int samba_unix_sid_to_id(const char *sid, struct cifs_uxid *cuxid)<br>
> +{<br>
> + id_t id;<br>
> + uint8_t type;<br>
> +<br>
> + if (sscanf(sid, "S-1-22-%hhu-%u", &type, &id) != 2)<br>
> + return -1;<br>
> +<br>
> + switch (type) {<br>
> + case 1:<br>
> + cuxid->type = CIFS_UXID_TYPE_UID;<br>
> + cuxid->id.uid = id;<br>
> + break;<br>
> + case 2:<br>
> + cuxid->type = CIFS_UXID_TYPE_GID;<br>
> + cuxid->id.gid = id;<br>
> + default:<br>
> + cuxid->type = CIFS_UXID_TYPE_UNKNOWN;<br>
> + return -1;<br>
> + }<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +static int sss_sid_to_id(struct sssd_ctx *ctx, const char *sid,<br>
> + struct cifs_uxid *cuxid)<br>
> +{<br>
> + int err;<br>
> + enum sss_id_type id_type;<br>
> +<br>
> + err = sss_nss_getidbysid(sid, (uint32_t *)&cuxid->id.uid, &id_type);<br>
> + if (err != 0) {<br>
> + ctx_set_error(ctx, strerror(err));<br>
> + return -1;<br>
> + }<br>
> +<br>
> + switch (id_type) {<br>
> + case SSS_ID_TYPE_UID:<br>
> + cuxid->type = CIFS_UXID_TYPE_UID;<br>
> + break;<br>
> + case SSS_ID_TYPE_GID:<br>
> + cuxid->type = CIFS_UXID_TYPE_GID;<br>
> + break;<br>
> + case SSS_ID_TYPE_BOTH:<br>
> + cuxid->type = CIFS_UXID_TYPE_BOTH;<br>
> + break;<br>
> + case SSS_ID_TYPE_NOT_SPECIFIED:<br>
> + default:<br>
> + return -1;<br>
> + }<br>
> +<br>
> + return 0;<br>
> +}<br>
> +<br>
> +/**<br>
> + * cifs_idmap_sids_to_ids - convert struct cifs_sids to struct cifs_uxids<br>
> + * usecase: mount.cifs -o sec=krb5,multiuser,cifsacl,nounix<br>
<br>
there is a trailing whitespace ^<br>
<br>
> + * test: ls -n on mounted share<br>
> + */<br>
> +int cifs_idmap_sids_to_ids(void *handle, const struct cifs_sid *csid,<br>
> + const size_t num, struct cifs_uxid *cuxid)<br>
> +{<br>
> + struct sssd_ctx *ctx = handle;<br>
> + enum idmap_error_code err;<br>
> + int success = -1;<br>
> + size_t i;<br>
> + char *sid;<br>
> +<br>
> + debug("num: %zd", num);<br>
> +<br>
> + if (num > UINT_MAX) {<br>
> + ctx_set_error(ctx, "num is too large.");<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + for (i = 0; i < num; ++i) {<br>
> + err = sss_idmap_bin_sid_to_sid(ctx->idmap, (uint8_t *) &csid[i],<br>
> + sizeof(csid[i]), &sid);<br>
<br>
please use const uint8_t here as well.<br>
<br>
> + if (err != IDMAP_SUCCESS) {<br>
> + ctx_set_error(ctx, idmap_error_string(err));<br>
> + continue;<br>
> + }<br>
> +<br>
> + cuxid[i].type = CIFS_UXID_TYPE_UNKNOWN;<br>
> +<br>
> + if (sss_sid_to_id(ctx, sid, &cuxid[i]) == 0 ||<br>
<br>
there is a trailing whitespace ^<br>
<br>
> + samba_unix_sid_to_id(sid, &cuxid[i]) == 0) {<br>
> +<br>
> + debug("setting uid of %s to %d", sid, cuxid[i].id.uid);<br>
> + success = 0;<br>
> + }<br>
> +<br>
> + free(sid);<br>
> + }<br>
> +<br>
> + return success;<br>
> +}<br>
> +<br>
> +<br>
> +int cifs_idmap_ids_to_sids(void *handle, const struct cifs_uxid *cuxid,<br>
> + const size_t num, struct cifs_sid *csid)<br>
> +{<br>
> + struct sssd_ctx *ctx = handle;<br>
> + int err, success = -1;<br>
> + char *sid;<br>
> + enum sss_id_type id_type;<br>
> + size_t i;<br>
> +<br>
> + debug("num ids: %zd", num);<br>
> +<br>
> + if (num > UINT_MAX) {<br>
> + ctx_set_error(ctx, "num is too large.");<br>
> + return -EINVAL;<br>
> + }<br>
> +<br>
> + for (i = 0; i < num; ++i) {<br>
> + err = sss_nss_getsidbyid((uint32_t)cuxid[i].id.uid, &sid, &id_type);<br>
> + if (err != 0) {<br>
> + ctx_set_error(ctx, strerror(err));<br>
> + csid[i].revision = 0;<br>
> + /* FIXME: would it be safe to map *any* uid/gids unknown by sssd to<br>
> + * SAMBA's UNIX SIDs? */<br>
> + continue;<br>
> + }<br>
> +<br>
> + if (sid_to_cifs_sid(ctx, sid, csid) == 0)<br>
> + success = 0;<br>
> + else<br>
> + csid[i].revision = 0;<br>
> + free(sid);<br>
> + }<br>
> +<br>
> + return success;<br>
> +}<br>
<span><font color="#888888">> --<br>
> 1.8.1.5<br>
><br></font></span></blockquote><div><br></div><div>The patch with your concers fixed is attached.</div></div><br></div></div></div>