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 <noam(a)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