[SSSD] [PATCH v2 1/5] NEW CLIENT: plugin for NFSv4 rpc.idmapd

Noam Meltzer tsnoam at gmail.com
Fri Jun 20 20:53:46 UTC 2014


On Fri, Jun 20, 2014 at 5:09 PM, Jakub Hrozek <jhrozek at redhat.com> wrote:

> On Tue, Mar 04, 2014 at 09:37:52AM +0200, Noam Meltzer wrote:
> > Implementation of design document:
> > https://fedorahosted.org/sssd/wiki/DesignDocs/rpc.idmapd%20plugin
> > ---
> >  src/sss_client/common.c             |   5 +
> >  src/sss_client/nfs/sss_nfs_client.c | 569
> ++++++++++++++++++++++++++++++++++++
> >  src/sss_client/sss_cli.h            |   2 +
> >  3 files changed, 576 insertions(+)
> >  create mode 100644 src/sss_client/nfs/sss_nfs_client.c
> >
> > diff --git a/src/sss_client/common.c b/src/sss_client/common.c
> > index 6044af0..58a9eca 100644
> > --- a/src/sss_client/common.c
> > +++ b/src/sss_client/common.c
> > @@ -936,6 +936,11 @@ int sss_ssh_make_request(enum sss_cli_command cmd,
> >      return ret;
> >  }
> >
> > +int sss_nfs_make_request(enum sss_cli_command cmd, struct
> sss_cli_req_data *rd,
> > +                         uint8_t **rep, size_t *replen, int *errnop)
> > +{
> > +    return sss_nss_make_request(cmd, rd, rep, replen, errnop);
> > +}
>
> I wonder if we need to add this call at all. Since the pipe we are
> talking to is the NSS pipe and we are locking the NSS mutex as well,
> maybe it's more readable to drop this call altogether and use
> sss_nss_make_request directly?
>

I like this "simple" wrapper for a single reason - code readability.
at the end, I believe the compiler will "optimise out" this function.
(though, I haven't actually tested it)
bottom line - let me know what you prefer, and I'll go with you.


>
> >
> >  const char *ssscli_err2string(int err)
> >  {
> > diff --git a/src/sss_client/nfs/sss_nfs_client.c
> b/src/sss_client/nfs/sss_nfs_client.c
> > new file mode 100644
> > index 0000000..7b23eab
> > --- /dev/null
> > +++ b/src/sss_client/nfs/sss_nfs_client.c
> > @@ -0,0 +1,569 @@
> > +/*
> > +   SSSD
> > +
> > +   NFS Client
> > +
> > +   Copyright (C) Noam Meltzer <noam at primarydata.com>    2013-2014
>
> This, and other places -- I guess you'd like to update the address to
> something that exists :-)
>
PrimaryData deserves their credit. I'll add a 2nd line with my personal
email :)


>
> > +
> > +   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/
> >.
> > +*/
> > +
> > +#define _GNU_SOURCE
> > +
> > +#include <stddef.h>
> > +#include <stdlib.h>
> > +#include <sys/types.h>
> > +#include <errno.h>
> > +#include <string.h>
> > +
> > +#include <nfsidmap.h>
> > +#include "nfsidmap_internal.h"
> > +
> > +#include "sss_client/sss_cli.h"
> > +#include "sss_client/nss_mc.h"
> > +
> > +
> > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
> . . .*/
> > +#define PLUGIN_NAME                 "sss_nfs"
> > +#define CONF_SECTION                "sss_nfs"
> > +#define CONF_USE_MC                 "memcache"
> > +#define REPLY_ID_OFFSET             (8)
> > +#define REPLY_NAME_OFFSET           (REPLY_ID_OFFSET + 8)
> > +#define MCBUF_LEN                   (4096)
>
> I think this constant is a bit misnamed. The MC prefix suggests that
> it's only used for memory cache results, which is not true.
>

Will "BUF_LEN" make more sense?



>
> > +#define USE_MC_DEFAULT              true
> > +
> > +
> > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
> . . .*/
> > +static char sss_nfs_plugin_name[]   = PLUGIN_NAME;
> > +static char nfs_conf_sect[]         = CONF_SECTION;
> > +static char nfs_conf_use_mc[]       = CONF_USE_MC;
> > +
> > +static bool nfs_use_mc              = USE_MC_DEFAULT;
> > +
> > +
> > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
> . . .*/
> > +/* Forward declarations */
> > +static int send_recv(uint8_t **repp, size_t *rep_lenp, enum
> sss_cli_command cmd,
> > +                     const void *req, size_t req_len);
> > +static int reply_to_id(id_t *idp, uint8_t *rep, size_t rep_len);
> > +static int reply_to_name(char *name, size_t len, uint8_t *rep, size_t
> rep_len);
> > +
> > +
> > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
> . . .*/
> > +/* get from memcache functions */
> > +static int get_uid_from_mc(id_t *uid, const char *name)
> > +{
> > +    int rc = 0;
> > +    struct passwd pwd;
> > +    char *buf = NULL;
> > +    char *p = NULL;
> > +    size_t buflen = 0;
> > +    size_t len = 0;
> > +
> > +    if (!nfs_use_mc) {
> > +        return -1;
> > +    }
> > +
> > +    sss_strnlen(name, SSS_NAME_MAX, &len);
> > +
> > +    do {
> > +        buflen += MCBUF_LEN;
> > +        if ((p = realloc(buf, buflen)) == NULL) {
> > +            rc = ENOMEM;
> > +            goto done;
> > +        }
> > +        buf = p;
> > +        rc = sss_nss_mc_getpwnam(name, len, &pwd, buf, buflen);
> > +    } while (rc == ERANGE);
> > +
> > +    if (rc == 0) {
> > +        IDMAP_LOG(1, ("found user %s in memcache", name));
> > +        *uid = pwd.pw_uid;
> > +    } else {
> > +        IDMAP_LOG(1, ("user %s not in memcache", name));
> > +    }
> > +
> > +done:
> > +    free(buf);
> > +    return rc;
> > +}
> > +
> > +static int get_gid_from_mc(id_t *gid, const char *name) {
>                                                       ^^^^^^^^
> We prefer to put the opening "{" after a newline for function
> declarations.
>

so do I. my bad :)



>
> > +    int rc = 0;
> > +    struct group grp;
> > +    char *buf = NULL;
> > +    char *p = NULL;
> > +    size_t buflen = 0;
> > +    size_t len;
> > +
> > +    if (!nfs_use_mc) {
> > +        return -1;
> > +    }
> > +
> > +    sss_strnlen(name, SSS_NAME_MAX, &len);
> > +
> > +    do {
> > +        buflen += MCBUF_LEN;
> > +        if ((p = realloc(buf, buflen)) == NULL) {
> > +            rc = ENOMEM;
> > +            goto done;
> > +        }
> > +        buf = p;
> > +        rc = sss_nss_mc_getgrnam(name, len, &grp, buf, buflen);
> > +    } while (rc == ERANGE);
> > +
> > +    if (rc == 0) {
> > +        IDMAP_LOG(1, ("found group %s in memcache", name));
> > +        *gid = grp.gr_gid;
> > +    } else {
> > +        IDMAP_LOG(1, ("group %s not in memcache", name));
> > +    }
> > +
> > +done:
> > +    free(buf);
> > +    return rc;
> > +}
>
> [snip]
>
> > +static int reply_to_id(id_t *idp, uint8_t *rep, size_t rep_len)
> > +{
> > +    int rc = 0;
> > +    id_t id;
> > +    uint32_t num_results = 0;
> > +
> > +    if (rep_len < sizeof(uint32_t)) {
> > +        IDMAP_LOG(0, ("%s: reply too small; rep_len=%lu", __func__,
> rep_len));
> > +        rc = EBADMSG;
> > +        goto done;
> > +    }
> > +
> > +    SAFEALIGN_COPY_UINT32(&num_results, rep, NULL);
> > +    if (num_results > 1) {
> > +        IDMAP_LOG(0, ("%s: too many results (%lu)", __func__,
> num_results));
> > +        rc = EBADMSG;
> > +        goto done;
> > +    }
> > +    if (num_results == 0) {
> > +        rc = ENOENT;
> > +        goto done;
> > +    }
> > +    if (rep_len < sizeof(uint32_t) + REPLY_ID_OFFSET) {
> > +        IDMAP_LOG(0, ("%s: reply too small(2); rep_len=%lu", __func__,
> > +                      rep_len));
> > +        rc = EBADMSG;
> > +        goto done;
> > +    }
>
> Could we use just this ^ check instead of the one at the beginning of
> the function? It's not like any partial results can be used if the
> buffer is > sizeof(uint32_t) but < sizeof(uint32_t) + REPLY_ID_OFFSET.
>
> the first check ( rep_len < sizeof(uint32_t) ) is mainly defensive
programming: what if rep_len is 0?
if i'm not missing something here, in such a case, SAFEALIGN_COPY_UINT32
will copy garbage into num_results, resulting in unexpected behaviour.




> > +
> > +    SAFEALIGN_COPY_UINT32(&id, rep + REPLY_ID_OFFSET, NULL);
> > +    *idp = id;
> > +
> > +done:
> > +    return rc;
> > +}
> > +
> > +static int reply_to_name(char *name, size_t len, uint8_t *rep, size_t
> rep_len)
> > +{
> > +    int rc = 0;
> > +    uint32_t num_results = 0;
> > +    const char *buf;
> > +    size_t buf_len;
> > +    size_t offset;
> > +
> > +    if (rep_len < sizeof(uint32_t)) {
> > +        IDMAP_LOG(0, ("%s: reply too small; rep_len=%lu", __func__,
> rep_len));
> > +        rc = EBADMSG;
> > +        goto done;
> > +    }
> > +
> > +    SAFEALIGN_COPY_UINT32(&num_results, rep, NULL);
> > +    if (num_results > 1) {
> > +        IDMAP_LOG(0, ("%s: too many results (%lu)", __func__,
> num_results));
> > +        rc = EBADMSG;
> > +        goto done;
> > +    }
> > +    if (num_results == 0) {
> > +        rc = ENOENT;
> > +        goto done;
> > +    }
> > +    if (rep_len < sizeof(uint32_t) + REPLY_ID_OFFSET) {
>
> I think this is a typo, shouldn't the length be checked against
> REPLY_NAME_OFFSET instead?
>
indeed.


>
> > +        IDMAP_LOG(0, ("%s: reply too small(2); rep_len=%lu", __func__,
> > +                      rep_len));
> > +        rc = EBADMSG;
> > +        goto done;
> > +    }
>
> Similar comment about the length check.
>
similar answer :)



>
> > +
> > +    buf = (const char *)(rep + REPLY_NAME_OFFSET);
> > +    buf_len = rep_len - REPLY_NAME_OFFSET;
> > +    offset = 0;
> > +    rc = sss_readrep_copy_string(buf, &offset, &buf_len, &len, &name,
> NULL);
> > +    if (rc != 0) {
> > +        rc = -rc;
> > +    }
> > +
> > +done:
> > +    return rc;
> > +}
> > +
> > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
> . . .*/
> > +/* configuration parsing aids */
> > +static bool str_equal(const char *s1, const char *s2)
> > +{
> > +    bool res = false;
> > +    size_t len1;
> > +    size_t len2;
> > +
> > +    len1 = strlen(s1);
> > +    len2 = strlen(s2);
> > +
> > +    if (len1 == len2) {
> > +        res = (strncasecmp(s1, s2, len1) == 0);
> > +    }
> > +
> > +    return res;
> > +}
> > +
> > +static int nfs_conf_get_bool(char *sect, char *attr, int def)
> > +{
> > +    int res;
> > +    char *val;
> > +
> > +    res = def;
> > +    val = conf_get_str(sect, attr);
> > +    if (val) {
> > +        res = (str_equal("1", val) ||
> > +               str_equal("yes", val) ||
> > +               str_equal("true", val) ||
> > +               str_equal("on", val));
> > +    }
> > +
> > +    return res;
> > +}
> > +
> > +
> > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
> . . .*/
> > +/* libnfsidmap return-code aids */
> > +
> > +/*
> > + * we only want to return 0 or ENOENT; otherwise libnfsidmap will stop
> > + * translation instead of proceeding to the next translation plugin
> > + */
> > +int normalise_rc(int rc) {
>                         ^^^^^^^
>                     Same comment like get_gid_from_mc
> > +    int res;
> > +
> > +    res = rc;
> > +    if (res != 0 && res != ENOENT) {
> > +        res = ENOENT;
> > +    }
> > +
> > +    return res;
> > +}
> > +
> > +/* log the actual rc from our code (to be used before normalising the
> rc) */
> > +void log_actual_rc(const char *trans_name, int rc) {
>                                                 ^^^^^^^^^
>                                         And here.
> > +    char tmp[80];
> > +    IDMAP_LOG(1, ("%s: rc=%i msg=%s", trans_name, rc,
> > +                  strerror_r(rc, tmp, sizeof(tmp))));
> > +}
> > +
> > +
>
> The rest of the file looks good to me.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20140620/b0060221/attachment.html>


More information about the sssd-devel mailing list