I have to Nack this one.
I will check the following patches in the next few days, but I am
traveling so it may take a while.
Comments inline.
What does the s_ stand for ?
On Thu, 2014-01-16 at 11:02 +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 | 490 ++++++++++++++++++++++++++++++++++++
> src/sss_client/sss_cli.h | 2 +
> 3 files changed, 497 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);
> +}
>
> 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..9f6e047
> --- /dev/null
> +++ b/src/sss_client/nfs/sss_nfs_client.c
> @@ -0,0 +1,490 @@
> +/*
> + SSSD
> +
> + NFS Client
> +
> + Copyright (C) Noam Meltzer <noam@primarydata.com> 2013
> +
> + 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/>.
> +*/
> +
> +#include <stddef.h>
> +#include <stdlib.h>
> +#include <sys/types.h>
> +#include <errno.h>
> +#include <string.h>
> +
> +#include <nfsidmap.h>
> +#include "nfsidmap_internal.h"
> +#include "cfg.h"
> +
> +#include "sss_client/sss_cli.h"
> +#include "sss_client/nss_mc.h"
> +
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +#define SSS_NFS_PLUGIN_NAME "sss_nfs"
> +#define SSS_NFS_CONF_SECTION "sss_nfs"
> +#define SSS_NFS_CONF_USE_MC "memcache"
> +#define SSS_NFS_REPLY_ID_OFFSET (8)
> +#define SSS_NFS_REPLY_NAME_OFFSET (SSS_NFS_REPLY_ID_OFFSET + 8)
> +#define SSS_NFS_MCBUF_LEN (4096)
> +#define SSS_NFS_USE_MC_DEFAULT (1)
> +
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +static char s_sss_nfs_plugin_name[] = SSS_NFS_PLUGIN_NAME;
> +static char s_sss_nfs_conf_section[] = SSS_NFS_CONF_SECTION;
> +static char s_sss_nfs_conf_use_mc[] = SSS_NFS_CONF_USE_MC;
We use sss_ as a namespacing measure, but it is strictly necessary only
for non static functions or variables.
We do not use s_sss_ for anything though, and we do not use hungarian
notations or similar.
Anyway it is unclear to me what is the point of defining a static string
that contains a const string ?
You are defining s_mcbuf as a global, then using it liberally in
> +static char *s_mcbuf = NULL;
multiple calls.
This is not reentrant nor thread safe, the client code MUST be thread
safe.
Please rename to get_id_from_reply() or reply_to_id()
> +static int s_use_mc = SSS_NFS_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 get_id_from_rep(id_t *idp, uint8_t *rep, size_t rep_len);
Please rename to get_name_from_reply() or reply_to_name()
> +static int get_name_from_rep(char *name, size_t len, uint8_t *rep,
> + size_t rep_len);
Use char buf[SSS_NFS_MCBUF_LEN]; here instead of a globally allocate
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +/* get from memcache functions */
> +static int get_uid_from_mc(id_t *uid, const char *name)
> +{
> + int rc;
> + struct passwd pwd;
> + char *buf;
buffer.
It's unlikely a passwd structure will be bigger than 4K but to be
absolutely accurate you may want to do what I suggest later for the
group entries (for those it is mandatory).
Missing error checking here, see _nss_sss_getpwnam_r()
> + size_t len = 0;
> +
> + if (!s_use_mc) {
> + return -1;
> + }
> +
> + buf = s_mcbuf;
> + sss_strnlen(name, SSS_NAME_MAX, &len);
No error checking as above here ^
> + rc = sss_nss_mc_getpwnam(name, len, &pwd, buf, SSS_NFS_MCBUF_LEN);
> + 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));
> + }
> +
> + return rc;
> +}
> +
> +static int get_gid_from_mc(id_t *gid, const char *name) {
> + int rc;
> + struct group grp;
> + char *buf;
> + size_t len;
> +
> + if (!s_use_mc) {
> + return -1;
> + }
> +
> + buf = s_mcbuf;
> + sss_strnlen(name, SSS_NAME_MAX, &len);
For the following call using a fixed size buffer will not do, because
group memberships may be quite large in some environments.
You can use it for a first call, but if ERANGE is returned you must
retry with a bigger, allocated buffer.
You are not doing the retry at all now, which is a bug.
Pleas rename to pw_name_len
> + rc = sss_nss_mc_getgrnam(name, len, &grp, buf, SSS_NFS_MCBUF_LEN);
> + 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));
> + }
> +
> + return rc;
> +}
> +
> +static int get_user_from_mc(char *name, size_t len, uid_t uid)
> +{
> + int rc;
> + struct passwd pwd;
> + char *buf;
> + size_t cache_len;
Same comments as per get_uid_from_mc()
> + if (!s_use_mc) {
> + return -1;
> + }
> +
> + buf = s_mcbuf;
Same comments as per get_gid_from_mc()
> + rc = sss_nss_mc_getpwuid(uid, &pwd, buf, SSS_NFS_MCBUF_LEN);
> + if (rc == 0) {
> + cache_len = strlen(pwd.pw_name) + 1;
> + if (cache_len > len) {
> + IDMAP_LOG(0, ("%s: reply too long; cache_len=%lu, len=%lu",
> + __func__, cache_len, len));
> + rc = -ENOBUFS;
> + }
> + memcpy(name, pwd.pw_name, cache_len);
> + }
> +
> + return rc;
> +}
> +
> +static int get_group_from_mc(char *name, size_t len, id_t gid)
> +{
> + int rc;
> + struct group grp;
> + char *buf;
> + size_t cache_len;
> +
> + buf = s_mcbuf;
missing error checking
> + rc = sss_nss_mc_getgrgid(gid, &grp, buf, SSS_NFS_MCBUF_LEN);
> + if (rc == 0) {
> + cache_len = strlen(grp.gr_name) + 1;
> + if (cache_len > len) {
> + IDMAP_LOG(0, ("%s: reply too long; cache_len=%lu, len=%lu",
> + __func__, cache_len, len));
> + rc = -ENOBUFS;
> + }
> + memcpy(name, grp.gr_name, cache_len);
> + }
> +
> + return rc;
> +}
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +static int name_to_id(const char *name, id_t *id, enum sss_cli_command cmd)
> +{
> + int rc;
> + uint8_t *rep = NULL;
> + size_t rep_len = 0;
> + size_t name_len;
> +
> + sss_strnlen(name, SSS_NAME_MAX, &name_len);
Don't test for rep, just call free() straight, glibc knows to ignore
> + rc = send_recv(&rep, &rep_len, cmd, name, name_len + 1);
> + if (rc == 0) {
> + rc = get_id_from_rep(id, rep, rep_len);
> + }
> +
> + if (rep != NULL) {
NULLs.
This is not safe on some architectures (like ARM).
> + free(rep);
> + }
> +
> + return rc;
> +}
> +
> +static int id_to_name(char *name, size_t len, id_t id,
> + enum sss_cli_command cmd)
> +{
> + int rc;
> + const void *req = NULL;
> + uint8_t *rep = NULL;
> + size_t req_len;
> + size_t rep_len = 0;
> +
> + req = (void *)(&id);
> + req_len = sizeof(id);
do this instead:
size_t req_len = sizeof(id_t);
uint8_t req[req_len];
memcpy(req, &id, req_len)
Just call free() w/o checks.
> + rc = send_recv(&rep, &rep_len, cmd, req, req_len);
> + if (rc == 0) {
> + rc = get_name_from_rep(name, len, rep, rep_len);
> + }
> +
> + if (rep != NULL) {
> + free(rep);
> + }
Why send_recv instead of just calling it sss_nfs_make_request() and
> +
> + return rc;
> +}
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +static int send_recv(uint8_t **repp, size_t *rep_lenp, enum sss_cli_command cmd,
avoid the wrapper by the same name that does nothing in common.c ?
If the wrapper needs to stay for some reason, then rename send_recv() to
sss_nfs_request()
Also use rep and rep_len, without the p, we do not change names based on
whether they are pointers or not, the compiler will warn you anyway if
you use them incorrectly.
Is -EPIPE really what rpc.idmap expects if a function fails ?
> + const void *req, size_t req_len)
> +{
> + int err = 0;
> + enum nss_status req_rc;
> + struct sss_cli_req_data rd;
> + uint8_t *rep;
> + size_t rep_len;
> +
> + rd.data = req;
> + rd.len = req_len;
> +
> + sss_nss_lock();
> + req_rc = sss_nfs_make_request(cmd, &rd, &rep, &rep_len, &err);
> + sss_nss_unlock();
> +
> + if (req_rc == NSS_STATUS_NOTFOUND) {
> + return -ENOENT;
> + }
> + if (req_rc != NSS_STATUS_SUCCESS) {
> + IDMAP_LOG(0, ("no-make-request; err=%i (%s)", err, strerror(err)));
> + return -EPIPE;
> + }
Why the double assignment here ? Why don't you just pass the function
> + *repp = rep;
> + *rep_lenp = rep_len;
arguments directly to sss_nfs_make_request() ?
The internal function and it will not assign buffers in case of error,
so you can pass in the arguments directly w/o worry.
You need to check that rep_len is >= sizeof(uint32_t) +
> + return 0;
> +}
> +
> +static int get_id_from_rep(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 return_rc;
> + }
> +
> + 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 return_rc;
> + }
> + if (num_results == 0) {
> + rc = -ENOENT;
> + goto return_rc;
> + }
SSS_NFS_REPLY_ID_OFFSET before trying to copy or you could segafault if
for some reason it is smaller.
Please use the label 'done' when you want to exit early.
> + SAFEALIGN_COPY_UINT32(&id, rep + SSS_NFS_REPLY_ID_OFFSET, NULL);
> + *idp = id;
> +
> +return_rc:
As above, please do boundary checks here too.
> + return rc;
> +}
> +
> +static int get_name_from_rep(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 return_rc;
> + }
> +
> + 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 return_rc;
> + }
> + if (num_results == 0) {
> + rc = -ENOENT;
> + goto return_rc;
> + }
Use 'done'
> + buf = (const char *)(rep + SSS_NFS_REPLY_NAME_OFFSET);
> + buf_len = rep_len - SSS_NFS_REPLY_NAME_OFFSET;
> + offset = 0;
> + rc = sss_readrep_copy_string(buf, &offset, &buf_len, &len, &name, NULL);
> + if (rc != 0) {
> + rc = -rc;
> + }
> +
> +return_rc:
You are changing the semantics of the original function here and
> + return rc;
> +}
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +/* configuration parsing aids */
> +static int nfs_strcasecmp(const char *s1, const char *s2)
returning a boolean that is true if the strings are equal and false if
they are not, the origin stncasecmp instead sets -1 0 or 1 depending on
the comparison, by keeping a similar name you provides false
expectations.
Please use instead the following prototype:
static bool str_equal(const char *s1, const char *s2)
> +{
> + int res = 0;
> + 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)you are getting a boolean, then please return a 'bool'
> +{
In what case would you want to disable using the mmap_cache ?
> + int res;
> + char *val;
> +
> + res = def;
> + val = conf_get_str(sect, attr);
> + if (val) {
> + res = (nfs_strcasecmp("1", val) ||
> + nfs_strcasecmp("yes", val) ||
> + nfs_strcasecmp("true", val) ||
> + nfs_strcasecmp("on", val));
> + }
> +
> + return res;
> +}
> +
> +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .*/
> +/* The external interface */
> +static int sss_nfs_init(void)
> +{
> + s_mcbuf = malloc(SSS_NFS_MCBUF_LEN);
> + if (s_mcbuf == NULL) {
> + return -ENOMEM;
> + }
> +
> + s_use_mc = nfs_conf_get_bool(s_sss_nfs_conf_section, s_sss_nfs_conf_use_mc,
> + SSS_NFS_USE_MC_DEFAULT);
Why these (void)s ?
> + return 0;
> +}
> +
> +static int sss_nfs_princ_to_ids(char *secname, char *princ, uid_t *uid,
> + gid_t *gid, extra_mapping_params **ex)
> +{
> + (void)secname;
> + (void)princ;
> + (void)uid;
> + (void)gid;
> + (void)ex;
Simo Sorce * Red Hat, Inc * New York
> + IDMAP_LOG(0, ("%s: not implemented", __func__));
> + return -ENOSYS;
> +}
> +
> +static int sss_nfs_name_to_uid(char *name, uid_t *uid)
> +{
> + int rc;
> + size_t name_len = 0;
> +
> + if (name == NULL) {
> + IDMAP_LOG(0, ("%s: name is null", __func__));
> + return -EINVAL;
> + }
> + if (uid == NULL) {
> + IDMAP_LOG(0, ("%s: uid is null", __func__));
> + return -EINVAL;
> + }
> +
> + rc = sss_strnlen(name, SSS_NAME_MAX, &name_len);
> + if (rc != 0) {
> + IDMAP_LOG(0, ("%s: no-strnlen; rc=%i (%s)", __func__, rc,
> + strerror(rc)));
> + return -rc;
> + }
> +
> + rc = get_uid_from_mc(uid, name);
> + if (rc != 0) {
> + rc = name_to_id(name, uid, SSS_NSS_GETPWNAM);
> + }
> +
> + return rc;
> +}
> +
> +static int sss_nfs_name_to_gid(char *name, gid_t *gid)
> +{
> + int rc;
> + size_t name_len = 0;
> +
> + if (name == NULL) {
> + IDMAP_LOG(0, ("%s: name is null", __func__));
> + return -EINVAL;
> + }
> + if (gid == NULL) {
> + IDMAP_LOG(0, ("%s: gid is null", __func__));
> + return -EINVAL;
> + }
> +
> + rc = sss_strnlen(name, SSS_NAME_MAX, &name_len);
> + if (rc != 0) {
> + IDMAP_LOG(0, ("%s: no-strnlen; rc=%i (%s)", __func__, rc,
> + strerror(rc)));
> + return -rc;
> + }
> +
> + rc = get_gid_from_mc(gid, name);
> + if (rc != 0) {
> + rc = name_to_id(name, gid, SSS_NSS_GETGRNAM);
> + }
> +
> + return rc;
> +}
> +
> +static int sss_nfs_uid_to_name(uid_t uid, char *domain, char *name, size_t len)
> +{
> + int rc;
> +
> + if (name == NULL) {
> + IDMAP_LOG(0, ("%s: name is null", __func__));
> + return -EINVAL;
> + }
> +
> + rc = get_user_from_mc(name, len, uid);
> + if (rc != 0) {
> + rc = id_to_name(name, len, uid, SSS_NSS_GETPWUID);
> + }
> +
> + return rc;
> +}
> +
> +static int sss_nfs_gid_to_name(gid_t gid, char *domain, char *name, size_t len)
> +{
> + int rc;
> +
> + if (name == NULL) {
> + IDMAP_LOG(0, ("%s: name is null", __func__));
> + return -EINVAL;
> + }
> +
> + rc = get_group_from_mc(name, len, gid);
> + if (rc != 0) {
> + rc = id_to_name(name, len, gid, SSS_NSS_GETGRGID);
> + }
> +
> + return rc;
> +}
> +
> +static int sss_nfs_gss_princ_to_grouplist(
> + char *secname, char *princ, gid_t *groups, int *ngroups,
> + extra_mapping_params **ex)
> +{
> + (void)secname;
> + (void)princ;
> + (void)groups;
> + (void)ngroups;
> + (void)ex;
> + IDMAP_LOG(0, ("%s: not implemented", __func__));
> + return -ENOSYS;
> +}
> +
> +static struct trans_func s_sss_nfs_trans = {
> + .name = s_sss_nfs_plugin_name,
> + .init = sss_nfs_init,
> + .princ_to_ids = sss_nfs_princ_to_ids,
> + .name_to_uid = sss_nfs_name_to_uid,
> + .name_to_gid = sss_nfs_name_to_gid,
> + .uid_to_name = sss_nfs_uid_to_name,
> + .gid_to_name = sss_nfs_gid_to_name,
> + .gss_princ_to_grouplist = sss_nfs_gss_princ_to_grouplist,
> +};
> +
> +struct trans_func *libnfsidmap_plugin_init(void)
> +{
> + return (&s_sss_nfs_trans);
> +}
> diff --git a/src/sss_client/sss_cli.h b/src/sss_client/sss_cli.h
> index 285a297..7e488e4 100644
> --- a/src/sss_client/sss_cli.h
> +++ b/src/sss_client/sss_cli.h
> @@ -527,6 +527,8 @@ int sss_ssh_make_request(enum sss_cli_command cmd,
> struct sss_cli_req_data *rd,
> uint8_t **repbuf, size_t *replen,
> int *errnop);
> +int sss_nfs_make_request(enum sss_cli_command cmd, struct sss_cli_req_data *rd,
> + uint8_t **rep, size_t *replen, int *errnop);
>
> #if 0
>
--
Noam Meltzer Linux Software Engineer PRIMARY DATA P.O. Box 12650, Herzliya Pituach 4673300 9 Hamenofim St. Akerstein Towers, Tower A, 5th fl. Herzliya Office: +972-77-8981888 | Fax: +972-3-7617140 | Mobile: +972-54-5873843 Email: noam@primarydata.com |