[PATCH v2 00/12] cifscreds: cleanup and overhaul of cifscreds utility

[PATCH v2 00/12] cifscreds: cleanup and overhaul of cifscreds utility

[Jeff Layton]

This patchset is a cleanup and overhaul of the cifscreds utility that
lives in the cifs-utils tree today. Igor Druzhinin did a wonderful job
on this when he did the original code a couple of years ago, but I did a
rather poor job at the time of communicating what we actually need for
this tool to do. Mea culpa...

This patch is a second pass at morphing it into a tool that's more like
what we need. I believe with this, I'll be able to roll some kernel
patches that can use the stashed key for establishing sessions.

I've made a few changes since the last set:

- combine some of the earlier patches so it's a smaller set

- I've dropped the patch to make key_search use keyctl_search. I'd still
  like to do this differently, but for now it's not possible to do so
  and protect the key payload

The idea here is that we want to be able to allow users to stash their
NTLM credentials in the kernel, so that it's possible to establish a
session on the fly when that user walks into a multiuser mount.

To that end, there are a number of changes that I'm proposing:

- a number of structural cleanups that may make this code more amenable
  to conversion to a library later and that make it easier to maintain

- hang these off of the session keyring instead of the uid keyring. I
  believe this will make this more friendly for use in containers and
  may make it harder to compromise the user's password.

- instead of having the domain as an optional parameter, allow the user
  to specify it in lieu of the hostname. During session setup, the kernel can
  first look for a host-specific key, and then fall back to looking for
  one that matches the domain if a host key isn't found.

There are still some things that need to be done to make this really
usable:

- a manpage

- kernel patches that can make these keys usable

Comments and suggestions welcome...

Jeff Layton (12):
  util: move getusername to util.c
  cifscreds: add unused attribute to argv parm in cifscreds_clearall
  cifscreds: eliminate domain parm from most functions
  cifscreds: remove user parameter from create_description
  cifscreds: make username part of value instead of description
  cifscreds: make usage use "return" and have callers return
  cifscreds: move option parsing into main()
  cifscreds: make username parameter optional
  cifscreds: add --domain flag
  cifscreds: loosen allowed characters in domain names
  cifscreds: use the session keyring
  cifscreds: further restrict permissions on keys

 cifscreds.c  |  257 +++++++++++++++++++++++++++++-----------------------------
 mount.cifs.c |   11 ---
 util.c       |   13 +++
 util.h       |    1 +
 4 files changed, 141 insertions(+), 141 deletions(-)

[PATCH v2 01/12] util: move getusername to util.c

[Jeff Layton]

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 mount.cifs.c |   11 -----------
 util.c       |   13 +++++++++++++
 util.h       |    1 +
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/mount.cifs.c b/mount.cifs.c
index 147f7fc..ced9f4e 100644
--- a/mount.cifs.c
+++ b/mount.cifs.c
@@ -361,17 +361,6 @@ static int set_password(struct parsed_mount_info *parsed_info, const char *src)
 	return 0;
 }
 
-/* caller frees username if necessary */
-static char *getusername(uid_t uid)
-{
-	char *username = NULL;
-	struct passwd *password = getpwuid(uid);
-
-	if (password)
-		username = password->pw_name;
-	return username;
-}
-
 /*
  * Parse a username string into parsed_mount_info fields. The format is:
  *
diff --git a/util.c b/util.c
index 41c6784..80d5a80 100644
--- a/util.c
+++ b/util.c
@@ -24,6 +24,7 @@
 
 #include <sys/types.h>
 #include <string.h>
+#include <pwd.h>
 
 /* glibc doesn't have strlcpy, strlcat. Ensure we do. JRA. We
  * don't link to libreplace so need them here. */
@@ -69,3 +70,15 @@ size_t strlcat(char *d, const char *s, size_t bufsize)
 }
 #endif
 
+/* caller frees username if necessary */
+char *
+getusername(uid_t uid)
+{
+	char *username = NULL;
+	struct passwd *password = getpwuid(uid);
+
+	if (password)
+		username = password->pw_name;
+	return username;
+}
+
diff --git a/util.h b/util.h
index 7a44faf..d3ca015 100644
--- a/util.h
+++ b/util.h
@@ -28,5 +28,6 @@
 size_t strlcpy(char *d, const char *s, size_t bufsize);
 size_t strlcat(char *d, const char *s, size_t bufsize);
 
+char *getusername(uid_t uid);
 #endif /* _LIBUTIL_H */
 
-- 
1.7.1

[PATCH v2 02/12] cifscreds: add unused attribute to argv parm in cifscreds_clearall

[Jeff Layton]

...to eliminate this warning:

cifscreds.c: In function ‘cifscreds_clearall’:
cifscreds.c:422:47: warning: unused parameter ‘argv’

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index f21a47f..d771056 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -419,7 +419,7 @@ static int cifscreds_clear(int argc, char *argv[])
 }
 
 /* clearall command handler */
-static int cifscreds_clearall(int argc, char *argv[])
+static int cifscreds_clearall(int argc, char *argv[] __attribute__ ((unused)))
 {
 	key_serial_t key;
 	int count = 0, errors = 0;
-- 
1.7.1

[PATCH v2 03/12] cifscreds: eliminate domain parm from most functions

[Jeff Layton]

Eventually we'll add this back in a different way. The domain and
address should be exclusive of one another. IOW, we want the kernel to
be able to find credentials for a specific address or for the domain of
which the server is a member.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |   85 ++++++++++++----------------------------------------------
 1 files changed, 18 insertions(+), 67 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index d771056..a9181ef 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -62,10 +62,10 @@ static int cifscreds_update(int argc, char *argv[]);
 const char *thisprogram;
 
 struct command commands[] = {
-	{ cifscreds_add,	"add",		"<host> <user> [domain]" },
-	{ cifscreds_clear,	"clear",	"<host> <user> [domain]" },
+	{ cifscreds_add,	"add",		"<host> <user>" },
+	{ cifscreds_clear,	"clear",	"<host> <user>" },
 	{ cifscreds_clearall,	"clearall",	"" },
-	{ cifscreds_update,	"update",	"<host> <user> [domain]" },
+	{ cifscreds_update,	"update",	"<host> <user>" },
 	{ NULL, "", NULL }
 };
 
@@ -85,41 +85,28 @@ static void usage(void)
 
 /* create key's description string from given credentials */
 static char *
-create_description(const char *addr, const char *user,
-		   const char *domain, char *desc)
+create_description(const char *addr, const char *user, char *desc)
 {
 	char *str_end;
 	int str_len;
 
 	sprintf(desc, "%s:%s:%s:", THIS_PROGRAM_NAME, addr, user);
 
-	if (domain != NULL) {
-		str_end = desc + strnlen(desc, INET6_ADDRSTRLEN + \
-					+ MAX_USERNAME_SIZE + \
-					+ sizeof(THIS_PROGRAM_NAME) + 3);
-		str_len = strnlen(domain, MAX_DOMAIN_SIZE);
-		while (str_len--) {
-			*str_end = tolower(*domain++);
-			str_end++;
-		}
-		*str_end = '\0';
-	}
-
 	return desc;
 }
 
 /* search a specific key in keyring */
 static key_serial_t
-key_search(const char *addr, const char *user, const char *domain)
+key_search(const char *addr, const char *user)
 {
-	char desc[INET6_ADDRSTRLEN + MAX_USERNAME_SIZE + MAX_DOMAIN_SIZE + \
+	char desc[INET6_ADDRSTRLEN + MAX_USERNAME_SIZE + \
 		+ sizeof(THIS_PROGRAM_NAME) + 3];
 	key_serial_t key, *pk;
 	void *keylist;
 	char *buffer;
 	int count, dpos, n, ret;
 
-	create_description(addr, user, domain, desc);
+	create_description(addr, user, desc);
 
 	/* read the key payload data */
 	count = keyctl_read_alloc(DEST_KEYRING, &keylist);
@@ -219,13 +206,11 @@ key_search_all_out:
 
 /* add or update a specific key to keyring */
 static key_serial_t
-key_add(const char *addr, const char *user,
-	const char *domain, const char *pass)
+key_add(const char *addr, const char *user, const char *pass)
 {
-	char desc[INET6_ADDRSTRLEN + MAX_USERNAME_SIZE + MAX_DOMAIN_SIZE + \
-		+ sizeof(THIS_PROGRAM_NAME) + 3];
+	char desc[INET6_ADDRSTRLEN + MAX_USERNAME_SIZE + sizeof(THIS_PROGRAM_NAME) + 3];
 
-	create_description(addr, user, domain, desc);
+	create_description(addr, user, desc);
 
 	return add_key("user", desc, pass, strnlen(pass, MOUNT_PASSWD_SIZE) + 1,
 		DEST_KEYRING);
@@ -239,7 +224,7 @@ static int cifscreds_add(int argc, char *argv[])
 	char *pass;
 	int ret;
 
-	if (argc != 4 && argc != 5)
+	if (argc != 4)
 		usage();
 
 	ret = resolve_host(argv[2], addrstr);
@@ -259,15 +244,6 @@ static int cifscreds_add(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
-	if (argc == 5) {
-		if (strspn(argv[4], DOMAIN_ALLOWED_CHARS) !=
-			strnlen(argv[4], MAX_DOMAIN_SIZE)
-		) {
-			fprintf(stderr, "error: Incorrect domain name\n");
-			return EXIT_FAILURE;
-		}
-	}
-
 	/* search for same credentials stashed for current host */
 	currentaddress = addrstr;
 	nextaddress = strchr(currentaddress, ',');
@@ -275,9 +251,7 @@ static int cifscreds_add(int argc, char *argv[])
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		if (key_search(currentaddress, argv[3],
-			argc == 5 ? argv[4] : NULL) > 0
-		) {
+		if (key_search(currentaddress, argv[3]) > 0) {
 			printf("You already have stashed credentials "
 				"for %s (%s)\n", currentaddress, argv[2]);
 			printf("If you want to update them use:\n");
@@ -307,8 +281,7 @@ static int cifscreds_add(int argc, char *argv[])
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		key_serial_t key = key_add(currentaddress, argv[3],
-					   argc == 5 ? argv[4] : NULL, pass);
+		key_serial_t key = key_add(currentaddress, argv[3], pass);
 		if (key <= 0) {
 			fprintf(stderr, "error: Add credential key for %s\n",
 				currentaddress);
@@ -346,7 +319,7 @@ static int cifscreds_clear(int argc, char *argv[])
 	char *currentaddress, *nextaddress;
 	int ret, count = 0, errors = 0;
 
-	if (argc != 4 && argc != 5)
+	if (argc != 4)
 		usage();
 
 	ret = resolve_host(argv[2], addrstr);
@@ -366,15 +339,6 @@ static int cifscreds_clear(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
-	if (argc == 5) {
-		if (strspn(argv[4], DOMAIN_ALLOWED_CHARS) !=
-			strnlen(argv[4], MAX_DOMAIN_SIZE)
-		) {
-			fprintf(stderr, "error: Incorrect domain name\n");
-			return EXIT_FAILURE;
-		}
-	}
-
 	/*
 	 * search for same credentials stashed for current host
 	 * and unlink them from session keyring
@@ -385,8 +349,7 @@ static int cifscreds_clear(int argc, char *argv[])
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		key_serial_t key = key_search(currentaddress, argv[3],
-						argc == 5 ? argv[4] : NULL);
+		key_serial_t key = key_search(currentaddress, argv[3]);
 		if (key > 0) {
 			if (keyctl(KEYCTL_UNLINK, key, DEST_KEYRING) < 0) {
 				fprintf(stderr, "error: Removing key from "
@@ -464,7 +427,7 @@ static int cifscreds_update(int argc, char *argv[])
 	char *addrs[16];
 	int ret, id, count = 0;
 
-	if (argc != 4 && argc != 5)
+	if (argc != 4)
 		usage();
 
 	ret = resolve_host(argv[2], addrstr);
@@ -484,15 +447,6 @@ static int cifscreds_update(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
-	if (argc == 5) {
-		if (strspn(argv[4], DOMAIN_ALLOWED_CHARS) !=
-			strnlen(argv[4], MAX_DOMAIN_SIZE)
-		) {
-			fprintf(stderr, "error: Incorrect domain name\n");
-			return EXIT_FAILURE;
-		}
-	}
-
 	/* search for necessary credentials stashed in session keyring */
 	currentaddress = addrstr;
 	nextaddress = strchr(currentaddress, ',');
@@ -500,9 +454,7 @@ static int cifscreds_update(int argc, char *argv[])
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		if (key_search(currentaddress, argv[3],
-			argc == 5 ? argv[4] : NULL) > 0
-		) {
+		if (key_search(currentaddress, argv[3]) > 0) {
 			addrs[count] = currentaddress;
 			count++;
 		}
@@ -528,8 +480,7 @@ static int cifscreds_update(int argc, char *argv[])
 	pass = getpass("Password: ");
 
 	for (id = 0; id < count; id++) {
-		key_serial_t key = key_add(addrs[id], argv[3],
-					argc == 5 ? argv[4] : NULL, pass);
+		key_serial_t key = key_add(addrs[id], argv[3], pass);
 		if (key <= 0)
 			fprintf(stderr, "error: Update credential key "
 				"for %s\n", addrs[id]);
-- 
1.7.1

[PATCH v2 04/12] cifscreds: remove user parameter from create_description

[Jeff Layton]

The username should be part of the key payload and not part of
the description. Also, prefix the address with an "a:" in the
description. Eventually we'll also need a "domain" key variant.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |   16 ++++++++--------
 1 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index a9181ef..a6cec98 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -85,19 +85,19 @@ static void usage(void)
 
 /* create key's description string from given credentials */
 static char *
-create_description(const char *addr, const char *user, char *desc)
+create_description(const char *addr, char *desc)
 {
 	char *str_end;
 	int str_len;
 
-	sprintf(desc, "%s:%s:%s:", THIS_PROGRAM_NAME, addr, user);
+	sprintf(desc, "%s:a:%s", THIS_PROGRAM_NAME, addr);
 
 	return desc;
 }
 
 /* search a specific key in keyring */
 static key_serial_t
-key_search(const char *addr, const char *user)
+key_search(const char *addr)
 {
 	char desc[INET6_ADDRSTRLEN + MAX_USERNAME_SIZE + \
 		+ sizeof(THIS_PROGRAM_NAME) + 3];
@@ -106,7 +106,7 @@ key_search(const char *addr, const char *user)
 	char *buffer;
 	int count, dpos, n, ret;
 
-	create_description(addr, user, desc);
+	create_description(addr, desc);
 
 	/* read the key payload data */
 	count = keyctl_read_alloc(DEST_KEYRING, &keylist);
@@ -210,7 +210,7 @@ key_add(const char *addr, const char *user, const char *pass)
 {
 	char desc[INET6_ADDRSTRLEN + MAX_USERNAME_SIZE + sizeof(THIS_PROGRAM_NAME) + 3];
 
-	create_description(addr, user, desc);
+	create_description(addr, desc);
 
 	return add_key("user", desc, pass, strnlen(pass, MOUNT_PASSWD_SIZE) + 1,
 		DEST_KEYRING);
@@ -251,7 +251,7 @@ static int cifscreds_add(int argc, char *argv[])
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		if (key_search(currentaddress, argv[3]) > 0) {
+		if (key_search(currentaddress) > 0) {
 			printf("You already have stashed credentials "
 				"for %s (%s)\n", currentaddress, argv[2]);
 			printf("If you want to update them use:\n");
@@ -349,7 +349,7 @@ static int cifscreds_clear(int argc, char *argv[])
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		key_serial_t key = key_search(currentaddress, argv[3]);
+		key_serial_t key = key_search(currentaddress);
 		if (key > 0) {
 			if (keyctl(KEYCTL_UNLINK, key, DEST_KEYRING) < 0) {
 				fprintf(stderr, "error: Removing key from "
@@ -454,7 +454,7 @@ static int cifscreds_update(int argc, char *argv[])
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		if (key_search(currentaddress, argv[3]) > 0) {
+		if (key_search(currentaddress) > 0) {
 			addrs[count] = currentaddress;
 			count++;
 		}
-- 
1.7.1

[PATCH v2 05/12] cifscreds: make username part of value instead of description

[Jeff Layton]

Change the payload to be "username:password". Since usernames can't
contain ':', this is suitable delimiter. Also, create_description
is just a sprintf now, so eliminate it.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |   30 +++++++++++-------------------
 1 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index a6cec98..b6c1fb6 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -83,30 +83,17 @@ static void usage(void)
 	exit(EXIT_FAILURE);
 }
 
-/* create key's description string from given credentials */
-static char *
-create_description(const char *addr, char *desc)
-{
-	char *str_end;
-	int str_len;
-
-	sprintf(desc, "%s:a:%s", THIS_PROGRAM_NAME, addr);
-
-	return desc;
-}
-
 /* search a specific key in keyring */
 static key_serial_t
 key_search(const char *addr)
 {
-	char desc[INET6_ADDRSTRLEN + MAX_USERNAME_SIZE + \
-		+ sizeof(THIS_PROGRAM_NAME) + 3];
+	char desc[INET6_ADDRSTRLEN + sizeof(THIS_PROGRAM_NAME) + 4];
 	key_serial_t key, *pk;
 	void *keylist;
 	char *buffer;
 	int count, dpos, n, ret;
 
-	create_description(addr, desc);
+	sprintf(desc, "%s:a:%s", THIS_PROGRAM_NAME, addr);
 
 	/* read the key payload data */
 	count = keyctl_read_alloc(DEST_KEYRING, &keylist);
@@ -208,12 +195,17 @@ key_search_all_out:
 static key_serial_t
 key_add(const char *addr, const char *user, const char *pass)
 {
-	char desc[INET6_ADDRSTRLEN + MAX_USERNAME_SIZE + sizeof(THIS_PROGRAM_NAME) + 3];
+	int len;
+	char desc[INET6_ADDRSTRLEN + sizeof(THIS_PROGRAM_NAME) + 4];
+	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
+
+	/* set key description */
+	sprintf(desc, "%s:a:%s", THIS_PROGRAM_NAME, addr);
 
-	create_description(addr, desc);
+	/* set payload contents */
+	len = sprintf(val, "%s:%s", user, pass);
 
-	return add_key("user", desc, pass, strnlen(pass, MOUNT_PASSWD_SIZE) + 1,
-		DEST_KEYRING);
+	return add_key("user", desc, val, len + 1, DEST_KEYRING);
 }
 
 /* add command handler */
-- 
1.7.1

[PATCH v2 06/12] cifscreds: make usage use "return" and have callers return

[Jeff Layton]

...banish the use of exit(), which may be helpful in the future in
the event that we eventually move some of this code into a library.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |   21 +++++++++++----------
 1 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index b6c1fb6..3ed15f2 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -70,7 +70,8 @@ struct command commands[] = {
 };
 
 /* display usage information */
-static void usage(void)
+static int
+usage(void)
 {
 	struct command *cmd;
 
@@ -80,7 +81,7 @@ static void usage(void)
 			cmd->name, cmd->format);
 	fprintf(stderr, "\n");
 
-	exit(EXIT_FAILURE);
+	return EXIT_FAILURE;
 }
 
 /* search a specific key in keyring */
@@ -217,7 +218,7 @@ static int cifscreds_add(int argc, char *argv[])
 	int ret;
 
 	if (argc != 4)
-		usage();
+		return usage();
 
 	ret = resolve_host(argv[2], addrstr);
 	switch (ret) {
@@ -312,7 +313,7 @@ static int cifscreds_clear(int argc, char *argv[])
 	int ret, count = 0, errors = 0;
 
 	if (argc != 4)
-		usage();
+		return usage();
 
 	ret = resolve_host(argv[2], addrstr);
 	switch (ret) {
@@ -380,7 +381,7 @@ static int cifscreds_clearall(int argc, char *argv[] __attribute__ ((unused)))
 	int count = 0, errors = 0;
 
 	if (argc != 2)
-		usage();
+		return usage();
 
 	/*
 	 * search for all program's credentials stashed in session keyring
@@ -420,7 +421,7 @@ static int cifscreds_update(int argc, char *argv[])
 	int ret, id, count = 0;
 
 	if (argc != 4)
-		usage();
+		return usage();
 
 	ret = resolve_host(argv[2], addrstr);
 	switch (ret) {
@@ -491,7 +492,7 @@ int main(int argc, char **argv)
 		thisprogram = THIS_PROGRAM_NAME;
 
 	if (argc == 1)
-		usage();
+		return usage();
 
 	/* find the best fit command */
 	best = NULL;
@@ -510,7 +511,7 @@ int main(int argc, char **argv)
 		/* partial match */
 		if (best) {
 			fprintf(stderr, "Ambiguous command\n");
-			exit(EXIT_FAILURE);
+			return EXIT_FAILURE;
 		}
 
 		best = cmd;
@@ -518,8 +519,8 @@ int main(int argc, char **argv)
 
 	if (!best) {
 		fprintf(stderr, "Unknown command\n");
-		exit(EXIT_FAILURE);
+		return EXIT_FAILURE;
 	}
 
-	exit(best->action(argc, argv));
+	return best->action(argc, argv);
 }
-- 
1.7.1

[PATCH v2 07/12] cifscreds: move option parsing into main()

[Jeff Layton]

Having to parse options in every command routine is cumbersome and
restrictive. Declare a struct to hold arguments, and then have the
functions take that struct as an argument.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |   76 ++++++++++++++++++++++++++++++++++------------------------
 1 files changed, 44 insertions(+), 32 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index 3ed15f2..79ab426 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -48,16 +48,21 @@
 /* destination keyring */
 #define DEST_KEYRING KEY_SPEC_USER_KEYRING
 
+struct cmdarg {
+	char	*host;
+	char	*user;
+};
+
 struct command {
-	int (*action)(int argc, char *argv[]);
+	int (*action)(struct cmdarg *arg);
 	const char	name[MAX_COMMAND_SIZE];
 	const char	*format;
 };
 
-static int cifscreds_add(int argc, char *argv[]);
-static int cifscreds_clear(int argc, char *argv[]);
-static int cifscreds_clearall(int argc, char *argv[]);
-static int cifscreds_update(int argc, char *argv[]);
+static int cifscreds_add(struct cmdarg *arg);
+static int cifscreds_clear(struct cmdarg *arg);
+static int cifscreds_clearall(struct cmdarg *arg);
+static int cifscreds_update(struct cmdarg *arg);
 
 const char *thisprogram;
 
@@ -210,21 +215,21 @@ key_add(const char *addr, const char *user, const char *pass)
 }
 
 /* add command handler */
-static int cifscreds_add(int argc, char *argv[])
+static int cifscreds_add(struct cmdarg *arg)
 {
 	char addrstr[MAX_ADDR_LIST_LEN];
 	char *currentaddress, *nextaddress;
 	char *pass;
 	int ret;
 
-	if (argc != 4)
+	if (arg->host == NULL || arg->user == NULL)
 		return usage();
 
-	ret = resolve_host(argv[2], addrstr);
+	ret = resolve_host(arg->host, addrstr);
 	switch (ret) {
 	case EX_USAGE:
 		fprintf(stderr, "error: Could not resolve address "
-			"for %s\n", argv[2]);
+			"for %s\n", arg->host);
 		return EXIT_FAILURE;
 
 	case EX_SYSERR:
@@ -232,7 +237,7 @@ static int cifscreds_add(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
-	if (strpbrk(argv[3], USER_DISALLOWED_CHARS)) {
+	if (strpbrk(arg->user, USER_DISALLOWED_CHARS)) {
 		fprintf(stderr, "error: Incorrect username\n");
 		return EXIT_FAILURE;
 	}
@@ -246,7 +251,7 @@ static int cifscreds_add(int argc, char *argv[])
 	while (currentaddress) {
 		if (key_search(currentaddress) > 0) {
 			printf("You already have stashed credentials "
-				"for %s (%s)\n", currentaddress, argv[2]);
+				"for %s (%s)\n", currentaddress, arg->host);
 			printf("If you want to update them use:\n");
 			printf("\t%s update\n", thisprogram);
 
@@ -274,7 +279,7 @@ static int cifscreds_add(int argc, char *argv[])
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		key_serial_t key = key_add(currentaddress, argv[3], pass);
+		key_serial_t key = key_add(currentaddress, arg->user, pass);
 		if (key <= 0) {
 			fprintf(stderr, "error: Add credential key for %s\n",
 				currentaddress);
@@ -289,7 +294,7 @@ static int cifscreds_add(int argc, char *argv[])
 				if (keyctl(KEYCTL_UNLINK, key, DEST_KEYRING) < 0) {
 					fprintf(stderr, "error: Deleting key from "
 						"keyring for %s (%s)\n",
-						currentaddress, argv[2]);
+						currentaddress, arg->host);
 				}
 			}
 		}
@@ -306,20 +311,20 @@ static int cifscreds_add(int argc, char *argv[])
 }
 
 /* clear command handler */
-static int cifscreds_clear(int argc, char *argv[])
+static int cifscreds_clear(struct cmdarg *arg)
 {
 	char addrstr[MAX_ADDR_LIST_LEN];
 	char *currentaddress, *nextaddress;
 	int ret, count = 0, errors = 0;
 
-	if (argc != 4)
+	if (arg->host == NULL || arg->user == NULL)
 		return usage();
 
-	ret = resolve_host(argv[2], addrstr);
+	ret = resolve_host(arg->host, addrstr);
 	switch (ret) {
 	case EX_USAGE:
 		fprintf(stderr, "error: Could not resolve address "
-			"for %s\n", argv[2]);
+			"for %s\n", arg->host);
 		return EXIT_FAILURE;
 
 	case EX_SYSERR:
@@ -327,7 +332,7 @@ static int cifscreds_clear(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
-	if (strpbrk(argv[3], USER_DISALLOWED_CHARS)) {
+	if (strpbrk(arg->user, USER_DISALLOWED_CHARS)) {
 		fprintf(stderr, "error: Incorrect username\n");
 		return EXIT_FAILURE;
 	}
@@ -347,7 +352,7 @@ static int cifscreds_clear(int argc, char *argv[])
 			if (keyctl(KEYCTL_UNLINK, key, DEST_KEYRING) < 0) {
 				fprintf(stderr, "error: Removing key from "
 					"keyring for %s (%s)\n",
-					currentaddress, argv[2]);
+					currentaddress, arg->host);
 				errors++;
 			} else {
 				count++;
@@ -364,7 +369,7 @@ static int cifscreds_clear(int argc, char *argv[])
 
 	if (!count && !errors) {
 		printf("You have no same stashed credentials "
-			" for %s\n", argv[2]);
+			" for %s\n", arg->host);
 		printf("If you want to add them use:\n");
 		printf("\t%s add\n", thisprogram);
 
@@ -375,14 +380,11 @@ static int cifscreds_clear(int argc, char *argv[])
 }
 
 /* clearall command handler */
-static int cifscreds_clearall(int argc, char *argv[] __attribute__ ((unused)))
+static int cifscreds_clearall(struct cmdarg *arg __attribute__ ((unused)))
 {
 	key_serial_t key;
 	int count = 0, errors = 0;
 
-	if (argc != 2)
-		return usage();
-
 	/*
 	 * search for all program's credentials stashed in session keyring
 	 * and then unlink them
@@ -413,21 +415,21 @@ static int cifscreds_clearall(int argc, char *argv[] __attribute__ ((unused)))
 }
 
 /* update command handler */
-static int cifscreds_update(int argc, char *argv[])
+static int cifscreds_update(struct cmdarg *arg)
 {
 	char addrstr[MAX_ADDR_LIST_LEN];
 	char *currentaddress, *nextaddress, *pass;
 	char *addrs[16];
 	int ret, id, count = 0;
 
-	if (argc != 4)
+	if (arg->host == NULL || arg->user == NULL)
 		return usage();
 
-	ret = resolve_host(argv[2], addrstr);
+	ret = resolve_host(arg->host, addrstr);
 	switch (ret) {
 	case EX_USAGE:
 		fprintf(stderr, "error: Could not resolve address "
-			"for %s\n", argv[2]);
+			"for %s\n", arg->host);
 		return EXIT_FAILURE;
 
 	case EX_SYSERR:
@@ -435,7 +437,7 @@ static int cifscreds_update(int argc, char *argv[])
 		return EXIT_FAILURE;
 	}
 
-	if (strpbrk(argv[3], USER_DISALLOWED_CHARS)) {
+	if (strpbrk(arg->user, USER_DISALLOWED_CHARS)) {
 		fprintf(stderr, "error: Incorrect username\n");
 		return EXIT_FAILURE;
 	}
@@ -462,7 +464,7 @@ static int cifscreds_update(int argc, char *argv[])
 
 	if (!count) {
 		printf("You have no same stashed credentials "
-			"for %s\n", argv[2]);
+			"for %s\n", arg->host);
 		printf("If you want to add them use:\n");
 		printf("\t%s add\n", thisprogram);
 
@@ -473,7 +475,7 @@ static int cifscreds_update(int argc, char *argv[])
 	pass = getpass("Password: ");
 
 	for (id = 0; id < count; id++) {
-		key_serial_t key = key_add(addrs[id], argv[3], pass);
+		key_serial_t key = key_add(addrs[id], arg->user, pass);
 		if (key <= 0)
 			fprintf(stderr, "error: Update credential key "
 				"for %s\n", addrs[id]);
@@ -485,8 +487,11 @@ static int cifscreds_update(int argc, char *argv[])
 int main(int argc, char **argv)
 {
 	struct command *cmd, *best;
+	struct cmdarg arg;
 	int n;
 
+	memset(&arg, 0, sizeof(arg));
+
 	thisprogram = (char *)basename(argv[0]);
 	if (thisprogram == NULL)
 		thisprogram = THIS_PROGRAM_NAME;
@@ -522,5 +527,12 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	return best->action(argc, argv);
+	/* first argument should be host */
+	if (argc >= 3)
+		arg.host = argv[2];
+
+	if (argc >= 4)
+		arg.user = argv[3];
+
+	return best->action(&arg);
 }
-- 
1.7.1

[PATCH v2 08/12] cifscreds: make username parameter optional

[Jeff Layton]

...and use getopt_long to get it. If someone doesn't specify the username,
use getusername() to get it.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |   39 ++++++++++++++++++++++++++++-----------
 1 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index 79ab426..f45497a 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -27,8 +27,10 @@
 #include <string.h>
 #include <ctype.h>
 #include <keyutils.h>
+#include <getopt.h>
 #include "mount.h"
 #include "resolve_host.h"
+#include "util.h"
 
 #define THIS_PROGRAM_NAME "cifscreds"
 
@@ -49,8 +51,8 @@
 #define DEST_KEYRING KEY_SPEC_USER_KEYRING
 
 struct cmdarg {
-	char	*host;
-	char	*user;
+	char		*host;
+	char		*user;
 };
 
 struct command {
@@ -67,13 +69,18 @@ static int cifscreds_update(struct cmdarg *arg);
 const char *thisprogram;
 
 struct command commands[] = {
-	{ cifscreds_add,	"add",		"<host> <user>" },
-	{ cifscreds_clear,	"clear",	"<host> <user>" },
+	{ cifscreds_add,	"add",		"[-u username] <host>" },
+	{ cifscreds_clear,	"clear",	"[-u username] <host>" },
 	{ cifscreds_clearall,	"clearall",	"" },
-	{ cifscreds_update,	"update",	"<host> <user>" },
+	{ cifscreds_update,	"update",	"[-u username] <host>" },
 	{ NULL, "", NULL }
 };
 
+struct option longopts[] = {
+	{"username", 1, NULL, 'u'},
+	{NULL, 0, NULL, 0}
+};
+
 /* display usage information */
 static int
 usage(void)
@@ -499,12 +506,22 @@ int main(int argc, char **argv)
 	if (argc == 1)
 		return usage();
 
+	while((n = getopt_long(argc, argv, "u:", longopts, NULL)) != -1) {
+		switch (n) {
+		case 'u':
+			arg.user = optarg;
+			break;
+		default:
+			return usage();
+		}
+	}
+
 	/* find the best fit command */
 	best = NULL;
-	n = strnlen(argv[1], MAX_COMMAND_SIZE);
+	n = strnlen(argv[optind], MAX_COMMAND_SIZE);
 
 	for (cmd = commands; cmd->action; cmd++) {
-		if (memcmp(cmd->name, argv[1], n) != 0)
+		if (memcmp(cmd->name, argv[optind], n) != 0)
 			continue;
 
 		if (cmd->name[n] == 0) {
@@ -527,12 +544,12 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	/* first argument should be host */
+	/* second argument should be host */
 	if (argc >= 3)
-		arg.host = argv[2];
+		arg.host = argv[optind + 1];
 
-	if (argc >= 4)
-		arg.user = argv[3];
+	if (arg.user == NULL)
+		arg.user = getusername(getuid());
 
 	return best->action(&arg);
 }
-- 
1.7.1

[PATCH v2 09/12] cifscreds: add --domain flag

[Jeff Layton]

...to indicate that the first argument is not a hostname but an
NT domain name. If it's set, then treat the argument as a
string literal.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |   64 ++++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 44 insertions(+), 20 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index f45497a..279517a 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -53,6 +53,7 @@
 struct cmdarg {
 	char		*host;
 	char		*user;
+	char		keytype;
 };
 
 struct command {
@@ -69,15 +70,16 @@ static int cifscreds_update(struct cmdarg *arg);
 const char *thisprogram;
 
 struct command commands[] = {
-	{ cifscreds_add,	"add",		"[-u username] <host>" },
-	{ cifscreds_clear,	"clear",	"[-u username] <host>" },
+	{ cifscreds_add,	"add",		"[-u username] [-d] <host|domain>" },
+	{ cifscreds_clear,	"clear",	"[-u username] [-d] <host|domain>" },
 	{ cifscreds_clearall,	"clearall",	"" },
-	{ cifscreds_update,	"update",	"[-u username] <host>" },
+	{ cifscreds_update,	"update",	"[-u username] [-d] <host|domain>" },
 	{ NULL, "", NULL }
 };
 
 struct option longopts[] = {
 	{"username", 1, NULL, 'u'},
+	{"domain", 0, NULL, 'd' },
 	{NULL, 0, NULL, 0}
 };
 
@@ -98,7 +100,7 @@ usage(void)
 
 /* search a specific key in keyring */
 static key_serial_t
-key_search(const char *addr)
+key_search(const char *addr, char keytype)
 {
 	char desc[INET6_ADDRSTRLEN + sizeof(THIS_PROGRAM_NAME) + 4];
 	key_serial_t key, *pk;
@@ -106,7 +108,7 @@ key_search(const char *addr)
 	char *buffer;
 	int count, dpos, n, ret;
 
-	sprintf(desc, "%s:a:%s", THIS_PROGRAM_NAME, addr);
+	sprintf(desc, "%s:%c:%s", THIS_PROGRAM_NAME, keytype, addr);
 
 	/* read the key payload data */
 	count = keyctl_read_alloc(DEST_KEYRING, &keylist);
@@ -206,14 +208,14 @@ key_search_all_out:
 
 /* add or update a specific key to keyring */
 static key_serial_t
-key_add(const char *addr, const char *user, const char *pass)
+key_add(const char *addr, const char *user, const char *pass, char keytype)
 {
 	int len;
 	char desc[INET6_ADDRSTRLEN + sizeof(THIS_PROGRAM_NAME) + 4];
 	char val[MOUNT_PASSWD_SIZE +  MAX_USERNAME_SIZE + 2];
 
 	/* set key description */
-	sprintf(desc, "%s:a:%s", THIS_PROGRAM_NAME, addr);
+	sprintf(desc, "%s:%c:%s", THIS_PROGRAM_NAME, keytype, addr);
 
 	/* set payload contents */
 	len = sprintf(val, "%s:%s", user, pass);
@@ -227,12 +229,16 @@ static int cifscreds_add(struct cmdarg *arg)
 	char addrstr[MAX_ADDR_LIST_LEN];
 	char *currentaddress, *nextaddress;
 	char *pass;
-	int ret;
+	int ret = 0;
 
 	if (arg->host == NULL || arg->user == NULL)
 		return usage();
 
-	ret = resolve_host(arg->host, addrstr);
+	if (arg->keytype == 'd')
+		strlcpy(addrstr, arg->host, MAX_ADDR_LIST_LEN);
+	else
+		ret = resolve_host(arg->host, addrstr);
+
 	switch (ret) {
 	case EX_USAGE:
 		fprintf(stderr, "error: Could not resolve address "
@@ -256,7 +262,7 @@ static int cifscreds_add(struct cmdarg *arg)
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		if (key_search(currentaddress) > 0) {
+		if (key_search(currentaddress, arg->keytype) > 0) {
 			printf("You already have stashed credentials "
 				"for %s (%s)\n", currentaddress, arg->host);
 			printf("If you want to update them use:\n");
@@ -286,7 +292,7 @@ static int cifscreds_add(struct cmdarg *arg)
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		key_serial_t key = key_add(currentaddress, arg->user, pass);
+		key_serial_t key = key_add(currentaddress, arg->user, pass, arg->keytype);
 		if (key <= 0) {
 			fprintf(stderr, "error: Add credential key for %s\n",
 				currentaddress);
@@ -322,12 +328,16 @@ static int cifscreds_clear(struct cmdarg *arg)
 {
 	char addrstr[MAX_ADDR_LIST_LEN];
 	char *currentaddress, *nextaddress;
-	int ret, count = 0, errors = 0;
+	int ret = 0, count = 0, errors = 0;
 
 	if (arg->host == NULL || arg->user == NULL)
 		return usage();
 
-	ret = resolve_host(arg->host, addrstr);
+	if (arg->keytype == 'd')
+		strlcpy(addrstr, arg->host, MAX_ADDR_LIST_LEN);
+	else
+		ret = resolve_host(arg->host, addrstr);
+
 	switch (ret) {
 	case EX_USAGE:
 		fprintf(stderr, "error: Could not resolve address "
@@ -354,7 +364,7 @@ static int cifscreds_clear(struct cmdarg *arg)
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		key_serial_t key = key_search(currentaddress);
+		key_serial_t key = key_search(currentaddress, arg->keytype);
 		if (key > 0) {
 			if (keyctl(KEYCTL_UNLINK, key, DEST_KEYRING) < 0) {
 				fprintf(stderr, "error: Removing key from "
@@ -427,12 +437,16 @@ static int cifscreds_update(struct cmdarg *arg)
 	char addrstr[MAX_ADDR_LIST_LEN];
 	char *currentaddress, *nextaddress, *pass;
 	char *addrs[16];
-	int ret, id, count = 0;
+	int ret = 0, id, count = 0;
 
 	if (arg->host == NULL || arg->user == NULL)
 		return usage();
 
-	ret = resolve_host(arg->host, addrstr);
+	if (arg->keytype == 'd')
+		strlcpy(addrstr, arg->host, MAX_ADDR_LIST_LEN);
+	else
+		ret = resolve_host(arg->host, addrstr);
+
 	switch (ret) {
 	case EX_USAGE:
 		fprintf(stderr, "error: Could not resolve address "
@@ -456,7 +470,7 @@ static int cifscreds_update(struct cmdarg *arg)
 		*nextaddress++ = '\0';
 
 	while (currentaddress) {
-		if (key_search(currentaddress) > 0) {
+		if (key_search(currentaddress, arg->keytype) > 0) {
 			addrs[count] = currentaddress;
 			count++;
 		}
@@ -482,7 +496,7 @@ static int cifscreds_update(struct cmdarg *arg)
 	pass = getpass("Password: ");
 
 	for (id = 0; id < count; id++) {
-		key_serial_t key = key_add(addrs[id], arg->user, pass);
+		key_serial_t key = key_add(addrs[id], arg->user, pass, arg->keytype);
 		if (key <= 0)
 			fprintf(stderr, "error: Update credential key "
 				"for %s\n", addrs[id]);
@@ -498,6 +512,7 @@ int main(int argc, char **argv)
 	int n;
 
 	memset(&arg, 0, sizeof(arg));
+	arg.keytype = 'a';
 
 	thisprogram = (char *)basename(argv[0]);
 	if (thisprogram == NULL)
@@ -506,8 +521,11 @@ int main(int argc, char **argv)
 	if (argc == 1)
 		return usage();
 
-	while((n = getopt_long(argc, argv, "u:", longopts, NULL)) != -1) {
+	while((n = getopt_long(argc, argv, "du:", longopts, NULL)) != -1) {
 		switch (n) {
+		case 'd':
+			arg.keytype = (char) n;
+			break;
 		case 'u':
 			arg.user = optarg;
 			break;
@@ -544,10 +562,16 @@ int main(int argc, char **argv)
 		return EXIT_FAILURE;
 	}
 
-	/* second argument should be host */
+	/* second argument should be host or domain */
 	if (argc >= 3)
 		arg.host = argv[optind + 1];
 
+	if (arg.host && arg.keytype == 'd' &&
+	    strspn(arg.host, DOMAIN_ALLOWED_CHARS) != strnlen(arg.host, MAX_DOMAIN_SIZE)) {
+		fprintf(stderr, "error: Domain name contains invalid characters\n");
+		return EXIT_FAILURE;
+	}
+
 	if (arg.user == NULL)
 		arg.user = getusername(getuid());
 
-- 
1.7.1

[PATCH v2 10/12] cifscreds: loosen allowed characters in domain names

[Jeff Layton]

As Donald points out, NetBIOS domains are allowed more characters than
the code currently allows. Change the test to one that checks for
disallowed characters instead.

Also, I can't find anything that says that '@' is not allowed in a
username. Might as well allow that too. Worst case, the server will
reject the username.

Reported-by: Donald R. Gray Jr <donald.r.gray-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index 279517a..cbd431e 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -42,10 +42,13 @@
 #define MOUNT_PASSWD_SIZE 128
 #define MAX_DOMAIN_SIZE 64
 
-/* allowed and disallowed characters for user and domain name */
-#define USER_DISALLOWED_CHARS "\\/\"[]:|<>+=;,?*@"
-#define DOMAIN_ALLOWED_CHARS "abcdefghijklmnopqrstuvwxyz" \
-			     "ABCDEFGHIJKLMNOPQRSTUVWXYZ-."
+/*
+ * disallowed characters for user and domain names. See:
+ * http://technet.microsoft.com/en-us/library/bb726984.aspx
+ * http://support.microsoft.com/kb/909264
+ */
+#define USER_DISALLOWED_CHARS "\\/\"[]:|<>+=;,?*"
+#define DOMAIN_DISALLOWED_CHARS "\\/:*?\"<>|"
 
 /* destination keyring */
 #define DEST_KEYRING KEY_SPEC_USER_KEYRING
@@ -567,7 +570,7 @@ int main(int argc, char **argv)
 		arg.host = argv[optind + 1];
 
 	if (arg.host && arg.keytype == 'd' &&
-	    strspn(arg.host, DOMAIN_ALLOWED_CHARS) != strnlen(arg.host, MAX_DOMAIN_SIZE)) {
+	    strpbrk(arg.host, DOMAIN_DISALLOWED_CHARS)) {
 		fprintf(stderr, "error: Domain name contains invalid characters\n");
 		return EXIT_FAILURE;
 	}
-- 
1.7.1

[PATCH v2 11/12] cifscreds: use the session keyring

[Jeff Layton]

This seems like a reasonable change, but I'm willing to listen to
arguments to the contrary...

cifscreds currently hangs the keys off of the uid keyring. It seems
more appropriate though that we require that each session have its
own set.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index cbd431e..6079b38 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -51,7 +51,7 @@
 #define DOMAIN_DISALLOWED_CHARS "\\/:*?\"<>|"
 
 /* destination keyring */
-#define DEST_KEYRING KEY_SPEC_USER_KEYRING
+#define DEST_KEYRING KEY_SPEC_SESSION_KEYRING
 
 struct cmdarg {
 	char		*host;
-- 
1.7.1

[PATCH v2 12/12] cifscreds: further restrict permissions on keys

[Jeff Layton]

The idea with this tool is to put the credentials in the kernel so that
the kernel can use them. Restrict write permissions to the possessor only,
but allow the user to view them. That seems to be the minimum permissions
that allow the use cases we expect.

Signed-off-by: Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
---
 cifscreds.c |    5 +----
 1 files changed, 1 insertions(+), 4 deletions(-)

diff --git a/cifscreds.c b/cifscreds.c
index 6079b38..8f548e9 100644
--- a/cifscreds.c
+++ b/cifscreds.c
@@ -300,10 +300,7 @@ static int cifscreds_add(struct cmdarg *arg)
 			fprintf(stderr, "error: Add credential key for %s\n",
 				currentaddress);
 		} else {
-			if (keyctl(KEYCTL_SETPERM, key, KEY_POS_VIEW | \
-				KEY_POS_WRITE | KEY_USR_VIEW | \
-				KEY_USR_WRITE) < 0
-			) {
+			if (keyctl(KEYCTL_SETPERM, key, KEY_POS_WRITE | KEY_USR_VIEW) < 0) {
 				fprintf(stderr, "error: Setting permissons "
 					"on key, attempt to delete...\n");
 
-- 
1.7.1

Re: [PATCH v2 00/12] cifscreds: cleanup and overhaul of cifscreds utility

[Shirish Pargaonkar]

On Thu, Dec 15, 2011 at 12:12 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> This patchset is a cleanup and overhaul of the cifscreds utility that
> lives in the cifs-utils tree today. Igor Druzhinin did a wonderful job
> on this when he did the original code a couple of years ago, but I did a
> rather poor job at the time of communicating what we actually need for
> this tool to do. Mea culpa...
>
> This patch is a second pass at morphing it into a tool that's more like
> what we need. I believe with this, I'll be able to roll some kernel
> patches that can use the stashed key for establishing sessions.
>
> I've made a few changes since the last set:
>
> - combine some of the earlier patches so it's a smaller set
>
> - I've dropped the patch to make key_search use keyctl_search. I'd still
>  like to do this differently, but for now it's not possible to do so
>  and protect the key payload
>
> The idea here is that we want to be able to allow users to stash their
> NTLM credentials in the kernel, so that it's possible to establish a
> session on the fly when that user walks into a multiuser mount.

Jeff, is there an initial/earlier document that states exactly how
a user can stash NTLM credentials?

By NTLM credentials, it is meant that server/domain name/address
and corrosponding password etc.?

>
> To that end, there are a number of changes that I'm proposing:
>
> - a number of structural cleanups that may make this code more amenable
>  to conversion to a library later and that make it easier to maintain
>
> - hang these off of the session keyring instead of the uid keyring. I
>  believe this will make this more friendly for use in containers and
>  may make it harder to compromise the user's password.
>
> - instead of having the domain as an optional parameter, allow the user
>  to specify it in lieu of the hostname. During session setup, the kernel can
>  first look for a host-specific key, and then fall back to looking for
>  one that matches the domain if a host key isn't found.
>
> There are still some things that need to be done to make this really
> usable:
>
> - a manpage
>
> - kernel patches that can make these keys usable
>
> Comments and suggestions welcome...
>
> Jeff Layton (12):
>  util: move getusername to util.c
>  cifscreds: add unused attribute to argv parm in cifscreds_clearall
>  cifscreds: eliminate domain parm from most functions
>  cifscreds: remove user parameter from create_description
>  cifscreds: make username part of value instead of description
>  cifscreds: make usage use "return" and have callers return
>  cifscreds: move option parsing into main()
>  cifscreds: make username parameter optional
>  cifscreds: add --domain flag
>  cifscreds: loosen allowed characters in domain names
>  cifscreds: use the session keyring
>  cifscreds: further restrict permissions on keys
>
>  cifscreds.c  |  257 +++++++++++++++++++++++++++++-----------------------------
>  mount.cifs.c |   11 ---
>  util.c       |   13 +++
>  util.h       |    1 +
>  4 files changed, 141 insertions(+), 141 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/12] cifscreds: cleanup and overhaul of cifscreds utility

[Shirish Pargaonkar]

On Thu, Dec 15, 2011 at 12:12 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> This patchset is a cleanup and overhaul of the cifscreds utility that
> lives in the cifs-utils tree today. Igor Druzhinin did a wonderful job
> on this when he did the original code a couple of years ago, but I did a
> rather poor job at the time of communicating what we actually need for
> this tool to do. Mea culpa...
>
> This patch is a second pass at morphing it into a tool that's more like
> what we need. I believe with this, I'll be able to roll some kernel
> patches that can use the stashed key for establishing sessions.
>
> I've made a few changes since the last set:
>
> - combine some of the earlier patches so it's a smaller set
>
> - I've dropped the patch to make key_search use keyctl_search. I'd still
>  like to do this differently, but for now it's not possible to do so
>  and protect the key payload
>
> The idea here is that we want to be able to allow users to stash their
> NTLM credentials in the kernel, so that it's possible to establish a
> session on the fly when that user walks into a multiuser mount.
>
> To that end, there are a number of changes that I'm proposing:
>
> - a number of structural cleanups that may make this code more amenable
>  to conversion to a library later and that make it easier to maintain
>
> - hang these off of the session keyring instead of the uid keyring. I
>  believe this will make this more friendly for use in containers and
>  may make it harder to compromise the user's password.
>
> - instead of having the domain as an optional parameter, allow the user
>  to specify it in lieu of the hostname. During session setup, the kernel can
>  first look for a host-specific key, and then fall back to looking for
>  one that matches the domain if a host key isn't found.
>
> There are still some things that need to be done to make this really
> usable:
>
> - a manpage
>
> - kernel patches that can make these keys usable
>
> Comments and suggestions welcome...
>
> Jeff Layton (12):
>  util: move getusername to util.c
>  cifscreds: add unused attribute to argv parm in cifscreds_clearall
>  cifscreds: eliminate domain parm from most functions
>  cifscreds: remove user parameter from create_description
>  cifscreds: make username part of value instead of description
>  cifscreds: make usage use "return" and have callers return
>  cifscreds: move option parsing into main()
>  cifscreds: make username parameter optional
>  cifscreds: add --domain flag
>  cifscreds: loosen allowed characters in domain names
>  cifscreds: use the session keyring
>  cifscreds: further restrict permissions on keys
>
>  cifscreds.c  |  257 +++++++++++++++++++++++++++++-----------------------------
>  mount.cifs.c |   11 ---
>  util.c       |   13 +++
>  util.h       |    1 +
>  4 files changed, 141 insertions(+), 141 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Will there be a utility like smbpasswd as part of cifs-utils package?

Re: [PATCH v2 00/12] cifscreds: cleanup and overhaul of cifscreds utility

[Jeff Layton]

On Thu, 15 Dec 2011 12:20:15 -0600
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, Dec 15, 2011 at 12:12 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > This patchset is a cleanup and overhaul of the cifscreds utility that
> > lives in the cifs-utils tree today. Igor Druzhinin did a wonderful job
> > on this when he did the original code a couple of years ago, but I did a
> > rather poor job at the time of communicating what we actually need for
> > this tool to do. Mea culpa...
> >
> > This patch is a second pass at morphing it into a tool that's more like
> > what we need. I believe with this, I'll be able to roll some kernel
> > patches that can use the stashed key for establishing sessions.
> >
> > I've made a few changes since the last set:
> >
> > - combine some of the earlier patches so it's a smaller set
> >
> > - I've dropped the patch to make key_search use keyctl_search. I'd still
> >  like to do this differently, but for now it's not possible to do so
> >  and protect the key payload
> >
> > The idea here is that we want to be able to allow users to stash their
> > NTLM credentials in the kernel, so that it's possible to establish a
> > session on the fly when that user walks into a multiuser mount.
> 
> Jeff, is there an initial/earlier document that states exactly how
> a user can stash NTLM credentials?
> 
> By NTLM credentials, it is meant that server/domain name/address
> and corrosponding password etc.?
> 

Correct. The idea here is that this tool stashes a key (or keys) in the
session keyring with a name like:

    cifscreds:a:<IP Address>

...or:

    cifscreds:d:<NT Domain Name>

...that key has a payload that looks like this:

   <username>:<password>

For a multiuser mount, when a user walks into the mount for the first
time, the kernel can look for a key in the keyring for the right server,
or failing that, the right NT domain. It can then get username and
password info out of the payload to establish a session.

We could also use this to get credentials for the initial mount too,
but that use-case is less compelling.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Re: [PATCH v2 00/12] cifscreds: cleanup and overhaul of cifscreds utility

[Shirish Pargaonkar]

On Thu, Dec 15, 2011 at 12:59 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> On Thu, 15 Dec 2011 12:20:15 -0600
> Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>> On Thu, Dec 15, 2011 at 12:12 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
>> > This patchset is a cleanup and overhaul of the cifscreds utility that
>> > lives in the cifs-utils tree today. Igor Druzhinin did a wonderful job
>> > on this when he did the original code a couple of years ago, but I did a
>> > rather poor job at the time of communicating what we actually need for
>> > this tool to do. Mea culpa...
>> >
>> > This patch is a second pass at morphing it into a tool that's more like
>> > what we need. I believe with this, I'll be able to roll some kernel
>> > patches that can use the stashed key for establishing sessions.
>> >
>> > I've made a few changes since the last set:
>> >
>> > - combine some of the earlier patches so it's a smaller set
>> >
>> > - I've dropped the patch to make key_search use keyctl_search. I'd still
>> >  like to do this differently, but for now it's not possible to do so
>> >  and protect the key payload
>> >
>> > The idea here is that we want to be able to allow users to stash their
>> > NTLM credentials in the kernel, so that it's possible to establish a
>> > session on the fly when that user walks into a multiuser mount.
>>
>> Jeff, is there an initial/earlier document that states exactly how
>> a user can stash NTLM credentials?
>>
>> By NTLM credentials, it is meant that server/domain name/address
>> and corrosponding password etc.?
>>
>
> Correct. The idea here is that this tool stashes a key (or keys) in the
> session keyring with a name like:
>
>    cifscreds:a:<IP Address>
>
> ...or:
>
>    cifscreds:d:<NT Domain Name>
>
> ...that key has a payload that looks like this:
>
>   <username>:<password>
>
> For a multiuser mount, when a user walks into the mount for the first
> time, the kernel can look for a key in the keyring for the right server,
> or failing that, the right NT domain. It can then get username and
> password info out of the payload to establish a session.
>
> We could also use this to get credentials for the initial mount too,
> but that use-case is less compelling.
>
> --
> Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>

Jeff, one of the concerns would be, how to sync up the pasword at the
server and domain.  If the user password at the server changes,
user must make corrosponding changes at the client machine?
Is there a provision to delete or update NTLM credential?
Also, if the same user can use multiple client machines to authenticate to the
same server or domain, should every client machine have a copy of
(the same) NTLM credentials stashed?  And then if password changes,
user must remember to update NTLM credentials on each client machine!

Re: [PATCH v2 00/12] cifscreds: cleanup and overhaul of cifscreds utility

[Jeff Layton]

On Fri, 16 Dec 2011 09:48:30 -0600
Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On Thu, Dec 15, 2011 at 12:59 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> > On Thu, 15 Dec 2011 12:20:15 -0600
> > Shirish Pargaonkar <shirishpargaonkar-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> >
> >> On Thu, Dec 15, 2011 at 12:12 PM, Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org> wrote:
> >> > This patchset is a cleanup and overhaul of the cifscreds utility that
> >> > lives in the cifs-utils tree today. Igor Druzhinin did a wonderful job
> >> > on this when he did the original code a couple of years ago, but I did a
> >> > rather poor job at the time of communicating what we actually need for
> >> > this tool to do. Mea culpa...
> >> >
> >> > This patch is a second pass at morphing it into a tool that's more like
> >> > what we need. I believe with this, I'll be able to roll some kernel
> >> > patches that can use the stashed key for establishing sessions.
> >> >
> >> > I've made a few changes since the last set:
> >> >
> >> > - combine some of the earlier patches so it's a smaller set
> >> >
> >> > - I've dropped the patch to make key_search use keyctl_search. I'd still
> >> >  like to do this differently, but for now it's not possible to do so
> >> >  and protect the key payload
> >> >
> >> > The idea here is that we want to be able to allow users to stash their
> >> > NTLM credentials in the kernel, so that it's possible to establish a
> >> > session on the fly when that user walks into a multiuser mount.
> >>
> >> Jeff, is there an initial/earlier document that states exactly how
> >> a user can stash NTLM credentials?
> >>
> >> By NTLM credentials, it is meant that server/domain name/address
> >> and corrosponding password etc.?
> >>
> >
> > Correct. The idea here is that this tool stashes a key (or keys) in the
> > session keyring with a name like:
> >
> >    cifscreds:a:<IP Address>
> >
> > ...or:
> >
> >    cifscreds:d:<NT Domain Name>
> >
> > ...that key has a payload that looks like this:
> >
> >   <username>:<password>
> >
> > For a multiuser mount, when a user walks into the mount for the first
> > time, the kernel can look for a key in the keyring for the right server,
> > or failing that, the right NT domain. It can then get username and
> > password info out of the payload to establish a session.
> >
> > We could also use this to get credentials for the initial mount too,
> > but that use-case is less compelling.
> >
> > --
> > Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
> 
> Jeff, one of the concerns would be, how to sync up the pasword at the
> server and domain.  If the user password at the server changes,
> user must make corrosponding changes at the client machine?

Yes, at least prior to the next reconnect. Those are the perils of
password-based auth.

> Is there a provision to delete or update NTLM credential?

Yes, the cifscreds command has delete and update functionality.

> Also, if the same user can use multiple client machines to authenticate to the
> same server or domain, should every client machine have a copy of
> (the same) NTLM credentials stashed?  And then if password changes,
> user must remember to update NTLM credentials on each client machine!

Yes. It's somewhat similar to the situation with kerberos tickets. This
is just a way for users to securely store their username and password
so we can use it for multiuser mounts.

-- 
Jeff Layton <jlayton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>