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. 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 <n...@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; What does the s_ stand for ? 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 ? > +static char *s_mcbuf = NULL; You are defining s_mcbuf as a global, then using it liberally in multiple calls. This is not reentrant nor thread safe, the client code MUST be thread safe. > +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_id_from_reply() or reply_to_id() > +static int get_name_from_rep(char *name, size_t len, uint8_t *rep, > + size_t rep_len); Please rename to get_name_from_reply() or reply_to_name() > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . > .*/ > +/* get from memcache functions */ > +static int get_uid_from_mc(id_t *uid, const char *name) > +{ > + int rc; > + struct passwd pwd; > + char *buf; Use char buf[SSS_NFS_MCBUF_LEN]; here instead of a globally allocate 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). > + size_t len = 0; > + > + if (!s_use_mc) { > + return -1; > + } > + > + buf = s_mcbuf; > + sss_strnlen(name, SSS_NAME_MAX, &len); Missing error checking here, see _nss_sss_getpwnam_r() > + 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); No error checking as above here ^ 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. > + 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; Pleas rename to pw_name_len > + if (!s_use_mc) { > + return -1; > + } > + > + buf = s_mcbuf; Same comments as per get_uid_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; Same comments as per get_gid_from_mc() > + 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); missing error checking > + 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) { Don't test for rep, just call free() straight, glibc knows to ignore NULLs. > + 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); This is not safe on some architectures (like ARM). do this instead: size_t req_len = sizeof(id_t); uint8_t req[req_len]; memcpy(req, &id, req_len) > + 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); > + } Just call free() w/o checks. > + > + return rc; > +} > + > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . > .*/ > +static int send_recv(uint8_t **repp, size_t *rep_lenp, enum sss_cli_command > cmd, Why send_recv instead of just calling it sss_nfs_make_request() and 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. > + 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; > + } Is -EPIPE really what rpc.idmap expects if a function fails ? > + *repp = rep; > + *rep_lenp = rep_len; Why the double assignment here ? Why don't you just pass the function 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. > + 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; > + } You need to check that rep_len is >= sizeof(uint32_t) + SSS_NFS_REPLY_ID_OFFSET before trying to copy or you could segafault if for some reason it is smaller. > + SAFEALIGN_COPY_UINT32(&id, rep + SSS_NFS_REPLY_ID_OFFSET, NULL); > + *idp = id; > + > +return_rc: Please use the label 'done' when you want to exit early. > + 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; > + } As above, please do boundary checks here too. > + 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: Use 'done' > + return rc; > +} > + > +/*. . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . > .*/ > +/* configuration parsing aids */ > +static int nfs_strcasecmp(const char *s1, const char *s2) You are changing the semantics of the original function here and 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' > + 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); In what case would you want to disable using the mmap_cache ? > + 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; Why these (void)s ? > + 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 > -- Simo Sorce * Red Hat, Inc * New York _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
