Message-ID: <5267DD77.2090904@RedHat.com>
Date: Wed, 23 Oct 2013 10:30:15 -0400
From: Steve Dickson <SteveD@redhat.com>
MIME-Version: 1.0
To: andros@netapp.com
CC: linux-nfs@vger.kernel.org, Weston Andros Adamson <dros@netapp.com>
Subject: Re: [PATCH Version 2 2/3] WIP: Add gsskeyd
References: <1382451757-3032-1-git-send-email-andros@netapp.com> <1382451757-3032-3-git-send-email-andros@netapp.com>
In-Reply-To: <1382451757-3032-3-git-send-email-andros@netapp.com>
Content-Type: text/plain; charset=ISO-8859-1
Sender: linux-nfs-owner@vger.kernel.org

Hello.

After read this these recent threads about this, we may or may not end up with this
type of daemon, personally I hope we don't... ;-) But if we do... 

On 22/10/13 10:22, andros@netapp.com wrote:
> From: Weston Andros Adamson <dros@netapp.com>
> 
> this is an experimental daemon that watches for krb credential cache files
> being created and deleted and creates or removes the mapping in the user
> keyring.
> 
> this replaces the wrappers for kinit and kdestroy as they are no longer needed -
> users can go ahead using kinit/kdestroy and existing pam modules.
> 
> if this is the way we want to go, this should probably end up as a processes
> forked from gssd and not its own daemon.
> 
> Signed-off-by: Weston Andros Adamson <dros@netapp.com>
> ---
>  configure.ac            |   1 +
>  utils/Makefile.am       |   2 +-
>  utils/gsskeyd/gsskeyd.c | 328 ++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 330 insertions(+), 1 deletion(-)
>  create mode 100644 utils/gsskeyd/gsskeyd.c
> 
> diff --git a/configure.ac b/configure.ac
> index d3ad854..a8e75ac 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -497,6 +497,7 @@ AC_CONFIG_FILES([
>  	utils/nfsdcltrack/Makefile
>  	utils/exportfs/Makefile
>  	utils/gssd/Makefile
> +	utils/gsskeyd/Makefile
>  	utils/idmapd/Makefile
>  	utils/mount/Makefile
>  	utils/mountd/Makefile
> diff --git a/utils/Makefile.am b/utils/Makefile.am
> index b892dc8..943ae2d 100644
> --- a/utils/Makefile.am
> +++ b/utils/Makefile.am
> @@ -14,7 +14,7 @@ OPTDIRS += blkmapd
>  endif
>  
>  if CONFIG_GSS
> -OPTDIRS += gssd
> +OPTDIRS += gssd gsskeyd
>  endif
>  
>  if CONFIG_MOUNT
> diff --git a/utils/gsskeyd/gsskeyd.c b/utils/gsskeyd/gsskeyd.c
> new file mode 100644
> index 0000000..6d598fa
> --- /dev/null
> +++ b/utils/gsskeyd/gsskeyd.c
> @@ -0,0 +1,328 @@
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <assert.h>
> +#include <keyutils.h>
> +#include <err.h>
> +#include <errno.h>
> +#include <string.h>
> +
> +#include <sys/inotify.h>
> +#include <sys/errno.h>
> +#include <sys/select.h>
> +#include <sys/param.h>
> +#include <sys/time.h>
> +#include <sys/types.h>
> +#include <sys/wait.h>
> +#include <sys/queue.h>
> +#include <sys/uio.h>
> +#include <unistd.h>
> +
> +#define MAX_EVENT_SIZE (sizeof(struct inotify_event) + NAME_MAX + 1)
> +
> +//#define DEBUG_TRACE
> +
> +#ifdef DEBUG_TRACE
> +#define TRACE(X...) do { fprintf(stderr, "_trace_: " X); } while (0)
> +#else
> +#define TRACE(X...)
> +#endif
Please do not make debugging a compile decision. Go ahead and 
use a '-d' or '-vvvv' (which each 'v' add more logging) flags to
make it a run time decision... Having to recompile to debug is
something that never made much sense to me... 

Secondly, you please use the existing xlog() interface to do your 
error/warning/debugging logging... Having all logging going through
on interface is a good thing... IMHO... 

steved.
> +
> +/* .25 second timeouts to select */
> +#define SELECT_INTERVAL_TV { 0, 250000 }
> +
> +/* .5 seconds to collect list of unique file names that had events */
> +#define COLLECT_INTERVAL_TV { 0, 500000 }
> +
> +struct watchdir {
> +	char dir[MAXPATHLEN];
> +	int wd;
> +	LIST_ENTRY(watchdir) entry;
> +};
> +
> +LIST_HEAD(watchdir_list, watchdir);
> +
> +struct update {
> +	char dir[MAXPATHLEN];
> +	char name[MAXPATHLEN];
> +	int is_remove;
> +	LIST_ENTRY(update) entry;
> +};
> +
> +LIST_HEAD(update_list, update);
> +
> +struct updates {
> +	struct update_list list;
> +	struct timeval first;
> +};
> +
> +void
> +watchdir_add(struct watchdir_list *watchdirs, int notify, char *dir)
> +{
> +	struct watchdir *w;
> +
> +	w = calloc(1, sizeof(struct watchdir));
> +	assert(w != NULL);
> +
> +	strncpy(w->dir, dir, MAXPATHLEN);
> +
> +	w->wd = inotify_add_watch(notify, dir,
> +	    IN_CREATE | IN_DELETE | IN_MOVED_FROM | IN_MOVED_TO );
> +	    //| IN_MODIFY
> +
> +	if (w->wd < 0)
> +		perror("inotify_add_watch");
> +
> +	LIST_INSERT_HEAD(watchdirs, w, entry);
> +
> +	TRACE("WATCH(%d): %s\n", w->wd, w->dir);
> +}
> +
> +/*
> + * XXX maybe use hash when there is more than one watchdir?
> + */
> +struct watchdir *
> +watchdir_lookup(struct watchdir_list *watchdirs, int wd)
> +{
> +	struct watchdir *w;
> +
> +	LIST_FOREACH(w, watchdirs, entry) {
> +		if (wd == w->wd)
> +			return w;
> +	}
> +
> +	return NULL;
> +}
> +
> +static void
> +update_set_type(struct update *u, struct inotify_event *e)
> +{
> +	if (e->mask & (IN_DELETE | IN_MOVED_FROM))
> +		u->is_remove = 1;
> +	else
> +		u->is_remove = 0;
> +}
> +
> +void
> +updates_add(struct updates *updates, struct watchdir_list *watchdirs, struct inotify_event *e)
> +{
> +	struct update *u;
> +	struct watchdir *w;
> +
> +
> +	w = watchdir_lookup(watchdirs, e->wd);
> +	assert(w != NULL);
> +
> +	TRACE("updates_add: %s/%s\n", w->dir, e->name);
> +
> +	LIST_FOREACH(u, &updates->list, entry) {
> +		if (strncmp(w->dir, u->dir, MAXPATHLEN) == 0 &&
> +		    strncmp(e->name, u->name, MAXPATHLEN) == 0) {
> +			update_set_type(u, e);
> +			return;
> +		}
> +	}
> +
> +	u = calloc(1, sizeof(struct update));
> +	strncpy(u->dir, w->dir, MAXPATHLEN);
> +	strncpy(u->name, e->name, MAXPATHLEN);
> +	update_set_type(u, e);
> +
> +	LIST_INSERT_HEAD(&updates->list, u, entry);
> +
> +	if (!timerisset(&updates->first)) {
> +		if (gettimeofday(&updates->first, NULL) < 0)
> +			perror("gettimeofday");
> +	}
> +}
> +
> +void
> +print_event_mask(FILE *fp, uint32_t mask)
> +{
> +#define _print_if_found(X, S) do {	\
> +		if (mask & (X)) { 		\
> +			fprintf(fp, S);		\
> +		} \
> +	} while(0)
> +
> +	_print_if_found(IN_ACCESS,		"access");
> +	_print_if_found(IN_ATTRIB,		"attrib");
> +	_print_if_found(IN_CLOSE_WRITE,		"close write");
> +	_print_if_found(IN_CLOSE_NOWRITE,	"close nowrite");
> +	_print_if_found(IN_CREATE,		"create");
> +	_print_if_found(IN_DELETE,		"delete");
> +	_print_if_found(IN_DELETE_SELF,		"delete self");
> +	_print_if_found(IN_MODIFY,		"modify");
> +	_print_if_found(IN_MOVE_SELF,		"move self");
> +	_print_if_found(IN_MOVED_FROM,		"moved from");
> +	_print_if_found(IN_MOVED_TO,		"moved to");
> +	_print_if_found(IN_OPEN,		"open");
> +}
> +
> +static int
> +manage_gss_ctx_keyring_mapping(struct update *u)
> +{
> +	char buf[MAXPATHLEN];
> +	int status;
> +	uid_t uid;
> +	pid_t pid;
> +	key_serial_t key;
> +	long res;
> +
> +	snprintf(buf, MAXPATHLEN, "%s/%s", u->dir, u->name);
> +
> +	if (sscanf(u->name, "krb5cc_%u", &uid) <= 0)
> +		perror("parsing krb5cc uid");
> +
> +	if ((pid = fork()) < 0)
> +		perror("fork");
> +
> +	if (pid == 0) {
> +		if (setuid(uid) < 0)
> +			perror("setuid");
> +
> +		if (u->is_remove) {
> +			fprintf(stderr, "remove gss ctx mapping for uid %u\n",
> +				uid);
> +			key = request_key("gss-ctx", "_nfstgt_", NULL,
> +				KEY_SPEC_USER_KEYRING);
> +			if (key > 0) {
> +				res = keyctl_unlink(key, KEY_SPEC_USER_KEYRING);
> +				if (res < 0)
> +					warn("keyctl_unlink failed");
> +			} else
> +				warn("request_key failed");
> +		} else {
> +			fprintf(stderr, "add gss ctx mapping for uid=%u\n",
> +				uid);
> +			snprintf(buf, MAXPATHLEN, "FILE:%s/%s",
> +				u->dir, u->name);
> +			key = add_key("gss-ctx", "_nfstgt_", buf,
> +				MAXPATHLEN, KEY_SPEC_USER_KEYRING);
> +
> +			if (key < 0)
> +				warn("add_key failed");
> +		}
> +
> +		exit(0);
> +	} else {
> +		waitpid(pid, &status, 0);
> +
> +		if (!WIFEXITED(status) || WEXITSTATUS(status) != 0) {
> +			return WEXITSTATUS(status);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +void
> +parse_events(int notify, struct watchdir_list *watchdirs, struct updates *updates)
> +{
> +	ssize_t nread, pos;
> +	unsigned char buf[MAX_EVENT_SIZE * 100];
> +	struct inotify_event *e;
> +
> +	for (;;) {
> +		nread = read(notify, buf, sizeof(buf));
> +
> +		if (nread <= 0) {
> +			TRACE("read returns %d, errno %u\n", nread, errno);
> +			break;
> +		}
> +
> +		TRACE("Read %d bytes\n", nread);
> +
> +		pos = 0;
> +		while (pos < nread) {
> +			assert((nread - pos) >=
> +			       (ssize_t)sizeof(struct inotify_event));
> +			e = (struct inotify_event *)(buf + pos);
> +			pos += sizeof(struct inotify_event) + e->len;
> +
> +#ifdef DEBUG_TRACE
> +			TRACE("EVENT on %s: ", e->name);
> +			print_event_mask(stderr, e->mask);
> +			fprintf(stderr, "\n");
> +#endif
> +
> +			if (strlen(e->name) >= strlen("krb5cc_") &&
> +			    strncmp("krb5cc_", e->name, strlen("krb5cc_")) != 0)
> +				TRACE("skip file: %s\n", e->name);
> +			else
> +				updates_add(updates, watchdirs, e);
> +		}
> +		assert(pos == nread);
> +	}
> +}
> +
> +void
> +handle_events(struct updates *updates)
> +{
> +	struct update *u;
> +	struct timeval now, diff, collection_interval = COLLECT_INTERVAL_TV;
> +
> +	if (!timerisset(&updates->first))
> +		return;
> +
> +	if (gettimeofday(&now, NULL) < 0)
> +		perror("gettimeofday");
> +
> +	timersub(&now, &updates->first, &diff);
> +
> +	if (!(diff.tv_sec >= collection_interval.tv_sec &&
> +	      diff.tv_usec >= collection_interval.tv_usec)) {
> +		TRACE("it's been %u s %u ms - waiting longer...\n",
> +			diff.tv_sec, diff.tv_usec);
> +		return;
> +	}
> +
> +	while (!LIST_EMPTY(&updates->list)) {
> +		u = updates->list.lh_first;
> +
> +		TRACE("handle krb5 cc file: %s\n", u->name);
> +		manage_gss_ctx_keyring_mapping(u);
> +		LIST_REMOVE(u, entry);
> +	}
> +
> +	timerclear(&updates->first);
> +}
> +
> +
> +int
> +main(int argc, char **argv)
> +{
> +	fd_set readfds;
> +	int notify;
> +	int res;
> +	struct timeval timeo, select_interval = SELECT_INTERVAL_TV;
> +	struct watchdir_list watchdirs;
> +	struct updates updates;
> +
> +	LIST_INIT(&watchdirs);
> +
> +	notify = inotify_init1(IN_NONBLOCK);
> +	if (notify < 0)
> +		perror("inotify_init1");
> +
> +	watchdir_add(&watchdirs, notify, "/tmp");
> +
> +	LIST_INIT(&updates.list);
> +	timerclear(&updates.first);
> +
> +	for (;;) {
> +		FD_ZERO(&readfds);
> +		FD_SET(notify, &readfds);
> +		memcpy(&timeo, &select_interval, sizeof(struct timeval));
> +
> +		res = select(notify + 1, &readfds, NULL, NULL, &timeo);
> +
> +		if (res < 0)
> +			perror("error calling select");
> +
> +		if (FD_ISSET(notify, &readfds))
> +			parse_events(notify, &watchdirs, &updates);
> +
> +		handle_events(&updates);
> +	}
> +}
>