On Fri, Jun 20, 2014 at 5:09 PM, Jakub Hrozek <jhrozek@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@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.