[PATCH 0/6] nfsidmap enhancements

[PATCH 0/6] nfsidmap enhancements

[Chuck Lever]

Add "-d" and "-l" options, and clean up some error handling
problems.

These need testing. I'm still thinking about how to set the NFSv4
domain name.

---

Chuck Lever (6):
      nfsidmap: Display the effective NFSv4 domain name
      nfsidmap: Use find_key_by_type_and_desc() if available
      nfsidmap: List cached ID mapping results
      nfsidmap: Fix error handling in id_lookup()
      nfsidmap: Fix error handling in name_lookup()
      nfsidmap: Clean up other exit status cases


 utils/nfsidmap/nfsidmap.c   |  298 ++++++++++++++++++++++++++++++-------------
 utils/nfsidmap/nfsidmap.man |   74 +++++++++--
 2 files changed, 271 insertions(+), 101 deletions(-)

--
Chuck Lever

[PATCH 1/6] nfsidmap: Display the effective NFSv4 domain name

[Chuck Lever]

Sorry for the extensive man page changes. I added the description
for the new "-d" option, then realized there was no explanation
about what an "NFSv4 domain name" is.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 utils/nfsidmap/nfsidmap.c   |   24 ++++++++++++++++-
 utils/nfsidmap/nfsidmap.man |   59 +++++++++++++++++++++++++++++++++++--------
 2 files changed, 70 insertions(+), 13 deletions(-)

diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index 1f5ba67..85177bf 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -37,6 +37,21 @@ static int keyring_clear(char *keyring);
 #define UIDKEYS 0x1
 #define GIDKEYS 0x2
 
+static int display_default_domain(void)
+{
+	char domain[NFS4_MAX_DOMAIN_LEN];
+	int rc;
+
+	rc = nfs4_get_default_domain(NULL, domain, NFS4_MAX_DOMAIN_LEN);
+	if (rc) {
+		xlog_errno(rc, "nfs4_get_default_domain failed: %m");
+		return EXIT_FAILURE;
+	}
+
+	printf("%s\n", domain);
+	return EXIT_SUCCESS;
+}
+
 /*
  * Find either a user or group id based on the name@domain string
  */
@@ -248,7 +263,7 @@ int main(int argc, char **argv)
 	int timeout = 600;
 	key_serial_t key;
 	char *progname, *keystr = NULL;
-	int clearing = 0, keymask = 0;
+	int clearing = 0, keymask = 0, display = 0;
 
 	/* Set the basename */
 	if ((progname = strrchr(argv[0], '/')) != NULL)
@@ -258,8 +273,11 @@ int main(int argc, char **argv)
 
 	xlog_open(progname);
 
-	while ((opt = getopt(argc, argv, "u:g:r:ct:v")) != -1) {
+	while ((opt = getopt(argc, argv, "du:g:r:ct:v")) != -1) {
 		switch (opt) {
+		case 'd':
+			display++;
+			break;
 		case 'u':
 			keymask = UIDKEYS;
 			keystr = strdup(optarg);
@@ -294,6 +312,8 @@ int main(int argc, char **argv)
 	if (!verbose)
 		verbose = conf_get_num("General", "Verbosity", 0);
 
+	if (display)
+		return display_default_domain();
 	if (keystr) {
 		rc = key_invalidate(keystr, keymask);
 		return rc;		
diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
index 3a3a523..04ddff6 100644
--- a/utils/nfsidmap/nfsidmap.man
+++ b/utils/nfsidmap/nfsidmap.man
@@ -11,27 +11,54 @@ nfsidmap \- The NFS idmapper upcall program
 .B "nfsidmap [-v] [-c]"
 .br
 .B "nfsidmap [-v] [-u|-g|-r user]"
+.br
+.B "nfsidmap -d"
 .SH DESCRIPTION
-The file
+The NFSv4 protocol represents the local system's UID and GID values
+on the wire as strings of the form
+.IR user@domain .
+The process of translating from UID to string and string to UID is
+referred to as "ID mapping."
+.PP
+The system derives the
+.I user
+part of the string by performing a password or group lookup.
+The lookup mechanism is configured in
+.IR /etc/idmapd.conf .
+.PP
+By default, the
+.I domain
+part of the string is the system's DNS domain name.
+It can also be specified in
+.I /etc/idmapd.conf
+if the system is multi-homed,
+or if the system's DNS domain name does
+not match the name of the system's Kerberos realm.
+.PP
+The
 .I /usr/sbin/nfsidmap
-is used by the NFS idmapper to translate user and group ids into names, and to
-translate user and group names into ids. Idmapper uses request-key to perform
-the upcall and cache the result.
+program performs translations on behalf of the kernel.
+The kernel uses the request-key mechanism to perform
+an upcall.
 .I /usr/sbin/nfsidmap
-is called by /sbin/request-key, and will perform the translation and
-initialize a key with the resulting information.
+is invoked by /sbin/request-key, performs the translation,
+and initializes a key with the resulting information.
+The kernel then caches the translation results in the key.
 .PP
 .I nfsidmap
-can also used to clear the keyring of all the keys or 
-revoke one particular key.  
-This is useful when the id mappings have failed to due 
-to a lookup error resulting in all the cached uids/gids to be set 
-to the user id nobody.
+can also clear cached ID map results in the kernel,
+or revoke one particular key.
+An incorrect cached key can result in file and directory ownership
+reverting to "nobody" on NFSv4 mount points.
 .SH OPTIONS
 .TP
 .B -c 
 Clear the keyring of all the keys.
 .TP
+.B -d
+Display the system's effective NFSv4 domain name on
+.IR stdout .
+.TP
 .B -g user
 Revoke the gid key of the given user.
 .TP
@@ -89,5 +116,15 @@ Notice that the new line was added above the line for the generic program.
 request-key will find the first matching line and run the corresponding program.
 In this case, /some/other/program will handle all uid lookups, and
 /usr/sbin/nfsidmap will handle gid, user, and group lookups.
+.SH FILES
+.TP
+.I /etc/idmapd.conf
+ID mapping configuration file
+.TP
+.I /etc/request-key.conf
+Request key configuration file
+.SH "SEE ALSO"
+.BR idmapd.conf (5),
+.BR request-key (8)
 .SH AUTHOR
 Bryan Schumaker, <bjschuma@netapp.com>

[PATCH 2/6] nfsidmap: Use find_key_by_type_and_desc() if available

[Chuck Lever]

Recent versions of libkeyutils have find_key_by_type_and_desc()
which replaces the open-coded keyring search in keyring_clear().

I don't quite understand what's going on in key_invalidate(),
so I didn't touch it.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 utils/nfsidmap/nfsidmap.c |  106 +++++++++++++++++++++++++--------------------
 1 file changed, 60 insertions(+), 46 deletions(-)

diff --git a/aclocal/keyutils.m4 b/aclocal/keyutils.m4
index a392c0e..16b225d 100644
--- a/aclocal/keyutils.m4
+++ b/aclocal/keyutils.m4
@@ -8,4 +8,8 @@ AC_DEFUN([AC_KEYUTILS], [
 
   AC_CHECK_HEADERS([keyutils.h])
 
+  AC_CHECK_LIB([keyutils], [find_key_by_type_and_desc],
+		[AC_DEFINE([HAVE_FIND_KEY_BY_TYPE_AND_DESC], [1],
+			[Define to 1 if you have the `find_key_by_type_and_desc' function.])],)
+
 ])dnl
diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index 85177bf..44b8b4b 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -1,3 +1,4 @@
+#include "config.h"
 
 #include <stdarg.h>
 #include <stdio.h>
@@ -32,11 +33,66 @@ char *usage="Usage: %s [-v] [-c || [-u|-g|-r key] || [-t timeout] key desc]";
 #define PATH_IDMAPDCONF "/etc/idmapd.conf"
 #endif
 
-static int keyring_clear(char *keyring);
-
 #define UIDKEYS 0x1
 #define GIDKEYS 0x2
 
+#ifndef HAVE_FIND_KEY_BY_TYPE_AND_DESC
+static key_serial_t find_key_by_type_and_desc(const char *type,
+		const char *desc, key_serial_t destringid)
+{
+	char buf[BUFSIZ];
+	key_serial_t key;
+	FILE *fp;
+
+	if ((fp = fopen(PROCKEYS, "r")) == NULL) {
+		xlog_err("fopen(%s) failed: %m", PROCKEYS);
+		return -1;
+	}
+
+	key = -1;
+	while(fgets(buf, BUFSIZ, fp) != NULL) {
+		unsigned int id;
+
+		if (strstr(buf, type) == NULL)
+			continue;
+		if (strstr(buf, desc) == NULL)
+			continue;
+		if (sscanf(buf, "%x %*s", &id) != 1) {
+			xlog_err("Unparsable keyring entry in %s", PROCKEYS);
+			continue;
+		}
+
+		key = (key_serial_t)id;
+		break;
+	}
+
+	fclose(fp);
+	return key;
+}
+#endif
+
+/*
+ * Clear all the keys on the given keyring
+ */
+static int keyring_clear(const char *keyring)
+{
+	key_serial_t key;
+
+	key = find_key_by_type_and_desc("keyring", keyring, 0);
+	if (key == -1) {
+		xlog_err("'%s' keyring was not found.", keyring);
+		return EXIT_FAILURE;
+	}
+
+	if (keyctl_clear(key) < 0) {
+		xlog_err("keyctl_clear(0x%x) failed: %m",
+				(unsigned int)key);
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
+
 static int display_default_domain(void)
 {
 	char domain[NFS4_MAX_DOMAIN_LEN];
@@ -55,7 +111,7 @@ static int display_default_domain(void)
 /*
  * Find either a user or group id based on the name@domain string
  */
-int id_lookup(char *name_at_domain, key_serial_t key, int type)
+static int id_lookup(char *name_at_domain, key_serial_t key, int type)
 {
 	char id[MAX_ID_LEN];
 	uid_t uid = 0;
@@ -101,7 +157,7 @@ int id_lookup(char *name_at_domain, key_serial_t key, int type)
 /*
  * Find the name@domain string from either a user or group id
  */
-int name_lookup(char *id, key_serial_t key, int type)
+static int name_lookup(char *id, key_serial_t key, int type)
 {
 	char name[IDMAP_NAMESZ];
 	char domain[NFS4_MAX_DOMAIN_LEN];
@@ -136,49 +192,7 @@ int name_lookup(char *id, key_serial_t key, int type)
 out:
 	return rc;
 }
-/*
- * Clear all the keys on the given keyring
- */
-static int keyring_clear(char *keyring)
-{
-	FILE *fp;
-	char buf[BUFSIZ];
-	key_serial_t key;
-
-	if (keyring == NULL)
-		keyring = DEFAULT_KEYRING;
-
-	if ((fp = fopen(PROCKEYS, "r")) == NULL) {
-		xlog_err("fopen(%s) failed: %m", PROCKEYS);
-		return 1;
-	}
 
-	while(fgets(buf, BUFSIZ, fp) != NULL) {
-		if (strstr(buf, "keyring") == NULL)
-			continue;
-		if (strstr(buf, keyring) == NULL)
-			continue;
-		if (verbose) {
-			*(strchr(buf, '\n')) = '\0';
-			xlog_warn("clearing '%s'", buf);
-		}
-		/*
-		 * The key is the first arugment in the string
-		 */
-		*(strchr(buf, ' ')) = '\0';
-		sscanf(buf, "%x", &key);
-		if (keyctl_clear(key) < 0) {
-			xlog_err("keyctl_clear(0x%x) failed: %m", key);
-			fclose(fp);
-			return 1;
-		}
-		fclose(fp);
-		return 0;
-	}
-	xlog_err("'%s' keyring was not found.", keyring);
-	fclose(fp);
-	return 1;
-}
 /*
  * Revoke a key 
  */

[PATCH 3/6] nfsidmap: List cached ID mapping results

[Chuck Lever]

User space can see the keys, but not their contents.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 utils/nfsidmap/nfsidmap.c   |   85 ++++++++++++++++++++++++++++++++++++++++++-
 utils/nfsidmap/nfsidmap.man |   15 ++++++++
 2 files changed, 98 insertions(+), 2 deletions(-)

diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index 44b8b4b..ed397c6 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -108,6 +108,81 @@ static int display_default_domain(void)
 	return EXIT_SUCCESS;
 }
 
+static void list_key(key_serial_t key)
+{
+	char *buffer, *c;
+	int rc;
+
+	rc = keyctl_describe_alloc(key, &buffer);
+	if (rc < 0) {
+		switch (errno) {
+		case EKEYEXPIRED:
+			printf("Expired key not displayed\n");
+			break;
+		default:
+			xlog_err("Failed to describe key: %m");
+		}
+		return;
+	}
+
+	c = strrchr(buffer, ';');
+	if (!c) {
+		xlog_err("Unparsable key not displayed\n");
+		goto out_free;
+	}
+	printf("  %s\n", ++c);
+
+out_free:
+	free(buffer);
+}
+
+static void list_keys(const char *ring_name, key_serial_t ring_id)
+{
+	key_serial_t *key;
+	void *keylist;
+	int count;
+
+	count = keyctl_read_alloc(ring_id, &keylist);
+	if (count < 0) {
+		xlog_err("Failed to read keyring %s: %m", ring_name);
+		return;
+	}
+	count /= (int)sizeof(*key);
+
+	switch (count) {
+	case 0:
+		printf("No %s keys found.\n", ring_name);
+		break;
+	case 1:
+		printf("1 %s key found:\n", ring_name);
+		break;
+	default:
+		printf("%u %s keys found:\n", count, ring_name);
+	}
+
+	for (key = keylist; count--; key++)
+		list_key(*key);
+
+	free(keylist);
+}
+
+/*
+ * List all keys on a keyring
+ */
+static int list_keyring(const char *keyring)
+{
+	key_serial_t key;
+
+	key = find_key_by_type_and_desc("keyring", keyring, 0);
+	if (key == -1) {
+		xlog_err("'%s' keyring was not found.", keyring);
+		return EXIT_FAILURE;
+	}
+
+	list_keys(keyring, key);
+	return EXIT_SUCCESS;
+}
+
 /*
  * Find either a user or group id based on the name@domain string
  */
@@ -277,7 +352,7 @@ int main(int argc, char **argv)
 	int timeout = 600;
 	key_serial_t key;
 	char *progname, *keystr = NULL;
-	int clearing = 0, keymask = 0, display = 0;
+	int clearing = 0, keymask = 0, display = 0, list = 0;
 
 	/* Set the basename */
 	if ((progname = strrchr(argv[0], '/')) != NULL)
@@ -287,11 +362,14 @@ int main(int argc, char **argv)
 
 	xlog_open(progname);
 
-	while ((opt = getopt(argc, argv, "du:g:r:ct:v")) != -1) {
+	while ((opt = getopt(argc, argv, "du:g:r:ct:vl")) != -1) {
 		switch (opt) {
 		case 'd':
 			display++;
 			break;
+		case 'l':
+			list++;
+			break;
 		case 'u':
 			keymask = UIDKEYS;
 			keystr = strdup(optarg);
@@ -328,6 +406,9 @@ int main(int argc, char **argv)
 
 	if (display)
 		return display_default_domain();
+	else if (list)
+		return list_keyring(DEFAULT_KEYRING);
+
 	if (keystr) {
 		rc = key_invalidate(keystr, keymask);
 		return rc;		
diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
index 04ddff6..0275bdf 100644
--- a/utils/nfsidmap/nfsidmap.man
+++ b/utils/nfsidmap/nfsidmap.man
@@ -13,6 +13,8 @@ nfsidmap \- The NFS idmapper upcall program
 .B "nfsidmap [-v] [-u|-g|-r user]"
 .br
 .B "nfsidmap -d"
+.br
+.B "nfsidmap -l"
 .SH DESCRIPTION
 The NFSv4 protocol represents the local system's UID and GID values
 on the wire as strings of the form
@@ -50,6 +52,13 @@ can also clear cached ID map results in the kernel,
 or revoke one particular key.
 An incorrect cached key can result in file and directory ownership
 reverting to "nobody" on NFSv4 mount points.
+.PP
+In addition, the
+.B -d
+and
+.B -l
+options are available to help diagnose misconfigurations.
+They have no effect on the keyring containing ID mapping results.
 .SH OPTIONS
 .TP
 .B -c 
@@ -62,6 +71,12 @@ Display the system's effective NFSv4 domain name on
 .B -g user
 Revoke the gid key of the given user.
 .TP
+.B -l
+Display on
+.I stdout
+all keys currently in the keyring used to cache ID mapping results.
+These keys are visible only to the superuser.
+.TP
 .B -r user
 Revoke both the uid and gid key of the given user.
 .TP

Re: [PATCH 3/6] nfsidmap: List cached ID mapping results

[Steve Dickson]

Hello,

On 07/31/2015 06:13 PM, Chuck Lever wrote:
> User space can see the keys, but not their contents.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  utils/nfsidmap/nfsidmap.c   |   85 ++++++++++++++++++++++++++++++++++++++++++-
>  utils/nfsidmap/nfsidmap.man |   15 ++++++++
>  2 files changed, 98 insertions(+), 2 deletions(-)
> 
> diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
> index 44b8b4b..ed397c6 100644
> --- a/utils/nfsidmap/nfsidmap.c
> +++ b/utils/nfsidmap/nfsidmap.c
> @@ -108,6 +108,81 @@ static int display_default_domain(void)
>  	return EXIT_SUCCESS;
>  }
>  
> +static void list_key(key_serial_t key)
> +{
> +	char *buffer, *c;
> +	int rc;
> +
> +	rc = keyctl_describe_alloc(key, &buffer);
> +	if (rc < 0) {
> +		switch (errno) {
> +		case EKEYEXPIRED:
> +			printf("Expired key not displayed\n");
> +			break;
> +		default:
> +			xlog_err("Failed to describe key: %m");
> +		}
> +		return;
> +	}
> +
> +	c = strrchr(buffer, ';');
> +	if (!c) {
> +		xlog_err("Unparsable key not displayed\n");
> +		goto out_free;
> +	}
> +	printf("  %s\n", ++c);
> +
> +out_free:
> +	free(buffer);
> +}
> +
> +static void list_keys(const char *ring_name, key_serial_t ring_id)
> +{
> +	key_serial_t *key;
> +	void *keylist;
> +	int count;
> +
> +	count = keyctl_read_alloc(ring_id, &keylist);
> +	if (count < 0) {
> +		xlog_err("Failed to read keyring %s: %m", ring_name);
> +		return;
> +	}
> +	count /= (int)sizeof(*key);
> +
> +	switch (count) {
> +	case 0:
> +		printf("No %s keys found.\n", ring_name);
> +		break;
> +	case 1:
> +		printf("1 %s key found:\n", ring_name);
> +		break;
> +	default:
> +		printf("%u %s keys found:\n", count, ring_name);
> +	}
> +
> +	for (key = keylist; count--; key++)
> +		list_key(*key);
> +
> +	free(keylist);
> +}
> +
> +/*
> + * List all keys on a keyring
> + */
> +static int list_keyring(const char *keyring)
> +{
> +	key_serial_t key;
> +
> +	key = find_key_by_type_and_desc("keyring", keyring, 0);
> +	if (key == -1) {
> +		xlog_err("'%s' keyring was not found.", keyring);
> +		return EXIT_FAILURE;
> +	}
> +
> +	list_keys(keyring, key);
> +	return EXIT_SUCCESS;
> +}
> +
>  /*
>   * Find either a user or group id based on the name@domain string
>   */
> @@ -277,7 +352,7 @@ int main(int argc, char **argv)
>  	int timeout = 600;
>  	key_serial_t key;
>  	char *progname, *keystr = NULL;
> -	int clearing = 0, keymask = 0, display = 0;
> +	int clearing = 0, keymask = 0, display = 0, list = 0;
>  
>  	/* Set the basename */
>  	if ((progname = strrchr(argv[0], '/')) != NULL)
> @@ -287,11 +362,14 @@ int main(int argc, char **argv)
>  
>  	xlog_open(progname);
>  
> -	while ((opt = getopt(argc, argv, "du:g:r:ct:v")) != -1) {
> +	while ((opt = getopt(argc, argv, "du:g:r:ct:vl")) != -1) {
>  		switch (opt) {
>  		case 'd':
>  			display++;
>  			break;
> +		case 'l':
> +			list++;
> +			break;
>  		case 'u':
>  			keymask = UIDKEYS;
>  			keystr = strdup(optarg);
> @@ -328,6 +406,9 @@ int main(int argc, char **argv)
>  
>  	if (display)
>  		return display_default_domain();
> +	else if (list)
> +		return list_keyring(DEFAULT_KEYRING);
> +
Just curious as to why the else clause... Are you
thinking it does not make sense to display the 
domain and list out the key ring?

steved.

>  	if (keystr) {
>  		rc = key_invalidate(keystr, keymask);
>  		return rc;		
> diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
> index 04ddff6..0275bdf 100644
> --- a/utils/nfsidmap/nfsidmap.man
> +++ b/utils/nfsidmap/nfsidmap.man
> @@ -13,6 +13,8 @@ nfsidmap \- The NFS idmapper upcall program
>  .B "nfsidmap [-v] [-u|-g|-r user]"
>  .br
>  .B "nfsidmap -d"
> +.br
> +.B "nfsidmap -l"
>  .SH DESCRIPTION
>  The NFSv4 protocol represents the local system's UID and GID values
>  on the wire as strings of the form
> @@ -50,6 +52,13 @@ can also clear cached ID map results in the kernel,
>  or revoke one particular key.
>  An incorrect cached key can result in file and directory ownership
>  reverting to "nobody" on NFSv4 mount points.
> +.PP
> +In addition, the
> +.B -d
> +and
> +.B -l
> +options are available to help diagnose misconfigurations.
> +They have no effect on the keyring containing ID mapping results.
>  .SH OPTIONS
>  .TP
>  .B -c 
> @@ -62,6 +71,12 @@ Display the system's effective NFSv4 domain name on
>  .B -g user
>  Revoke the gid key of the given user.
>  .TP
> +.B -l
> +Display on
> +.I stdout
> +all keys currently in the keyring used to cache ID mapping results.
> +These keys are visible only to the superuser.
> +.TP
>  .B -r user
>  Revoke both the uid and gid key of the given user.
>  .TP
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/6] nfsidmap: List cached ID mapping results

[Chuck Lever]

On Aug 4, 2015, at 9:29 AM, Steve Dickson <SteveD@redhat.com> wrote:

> Hello,
> 
> On 07/31/2015 06:13 PM, Chuck Lever wrote:
>> User space can see the keys, but not their contents.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> utils/nfsidmap/nfsidmap.c   |   85 ++++++++++++++++++++++++++++++++++++++++++-
>> utils/nfsidmap/nfsidmap.man |   15 ++++++++
>> 2 files changed, 98 insertions(+), 2 deletions(-)
>> 
>> diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
>> index 44b8b4b..ed397c6 100644
>> --- a/utils/nfsidmap/nfsidmap.c
>> +++ b/utils/nfsidmap/nfsidmap.c
>> @@ -108,6 +108,81 @@ static int display_default_domain(void)
>> 	return EXIT_SUCCESS;
>> }
>> 
>> +static void list_key(key_serial_t key)
>> +{
>> +	char *buffer, *c;
>> +	int rc;
>> +
>> +	rc = keyctl_describe_alloc(key, &buffer);
>> +	if (rc < 0) {
>> +		switch (errno) {
>> +		case EKEYEXPIRED:
>> +			printf("Expired key not displayed\n");
>> +			break;
>> +		default:
>> +			xlog_err("Failed to describe key: %m");
>> +		}
>> +		return;
>> +	}
>> +
>> +	c = strrchr(buffer, ';');
>> +	if (!c) {
>> +		xlog_err("Unparsable key not displayed\n");
>> +		goto out_free;
>> +	}
>> +	printf("  %s\n", ++c);
>> +
>> +out_free:
>> +	free(buffer);
>> +}
>> +
>> +static void list_keys(const char *ring_name, key_serial_t ring_id)
>> +{
>> +	key_serial_t *key;
>> +	void *keylist;
>> +	int count;
>> +
>> +	count = keyctl_read_alloc(ring_id, &keylist);
>> +	if (count < 0) {
>> +		xlog_err("Failed to read keyring %s: %m", ring_name);
>> +		return;
>> +	}
>> +	count /= (int)sizeof(*key);
>> +
>> +	switch (count) {
>> +	case 0:
>> +		printf("No %s keys found.\n", ring_name);
>> +		break;
>> +	case 1:
>> +		printf("1 %s key found:\n", ring_name);
>> +		break;
>> +	default:
>> +		printf("%u %s keys found:\n", count, ring_name);
>> +	}
>> +
>> +	for (key = keylist; count--; key++)
>> +		list_key(*key);
>> +
>> +	free(keylist);
>> +}
>> +
>> +/*
>> + * List all keys on a keyring
>> + */
>> +static int list_keyring(const char *keyring)
>> +{
>> +	key_serial_t key;
>> +
>> +	key = find_key_by_type_and_desc("keyring", keyring, 0);
>> +	if (key == -1) {
>> +		xlog_err("'%s' keyring was not found.", keyring);
>> +		return EXIT_FAILURE;
>> +	}
>> +
>> +	list_keys(keyring, key);
>> +	return EXIT_SUCCESS;
>> +}
>> +
>> /*
>>  * Find either a user or group id based on the name@domain string
>>  */
>> @@ -277,7 +352,7 @@ int main(int argc, char **argv)
>> 	int timeout = 600;
>> 	key_serial_t key;
>> 	char *progname, *keystr = NULL;
>> -	int clearing = 0, keymask = 0, display = 0;
>> +	int clearing = 0, keymask = 0, display = 0, list = 0;
>> 
>> 	/* Set the basename */
>> 	if ((progname = strrchr(argv[0], '/')) != NULL)
>> @@ -287,11 +362,14 @@ int main(int argc, char **argv)
>> 
>> 	xlog_open(progname);
>> 
>> -	while ((opt = getopt(argc, argv, "du:g:r:ct:v")) != -1) {
>> +	while ((opt = getopt(argc, argv, "du:g:r:ct:vl")) != -1) {
>> 		switch (opt) {
>> 		case 'd':
>> 			display++;
>> 			break;
>> +		case 'l':
>> +			list++;
>> +			break;
>> 		case 'u':
>> 			keymask = UIDKEYS;
>> 			keystr = strdup(optarg);
>> @@ -328,6 +406,9 @@ int main(int argc, char **argv)
>> 
>> 	if (display)
>> 		return display_default_domain();
>> +	else if (list)
>> +		return list_keyring(DEFAULT_KEYRING);
>> +
> Just curious as to why the else clause... Are you
> thinking it does not make sense to display the 
> domain and list out the key ring?

Well, display already has a hard "return display_default_domain()".
If you specify "-l" you're not getting anything else. The "else"
can probably be removed.

The idea is that "nfsidmap -l" or "nfsidmap -d" can be used
in scripts too. In that context, specifying both would make
parsing the output more difficult. And I think you would use
these in different situations. "-l" might be used a lot. "-d"
might be used once to check the configuration.

And IMO these new options should be careful to avoid updating
or revoking any key, so the logic exits right there, as a
defensive precaution.

We're overloading a kernel API here, after all. Maybe this kind
of debugging belongs in a separate command. But this seems safe
enough.


> steved.
> 
>> 	if (keystr) {
>> 		rc = key_invalidate(keystr, keymask);
>> 		return rc;		
>> diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
>> index 04ddff6..0275bdf 100644
>> --- a/utils/nfsidmap/nfsidmap.man
>> +++ b/utils/nfsidmap/nfsidmap.man
>> @@ -13,6 +13,8 @@ nfsidmap \- The NFS idmapper upcall program
>> .B "nfsidmap [-v] [-u|-g|-r user]"
>> .br
>> .B "nfsidmap -d"
>> +.br
>> +.B "nfsidmap -l"
>> .SH DESCRIPTION
>> The NFSv4 protocol represents the local system's UID and GID values
>> on the wire as strings of the form
>> @@ -50,6 +52,13 @@ can also clear cached ID map results in the kernel,
>> or revoke one particular key.
>> An incorrect cached key can result in file and directory ownership
>> reverting to "nobody" on NFSv4 mount points.
>> +.PP
>> +In addition, the
>> +.B -d
>> +and
>> +.B -l
>> +options are available to help diagnose misconfigurations.
>> +They have no effect on the keyring containing ID mapping results.
>> .SH OPTIONS
>> .TP
>> .B -c 
>> @@ -62,6 +71,12 @@ Display the system's effective NFSv4 domain name on
>> .B -g user
>> Revoke the gid key of the given user.
>> .TP
>> +.B -l
>> +Display on
>> +.I stdout
>> +all keys currently in the keyring used to cache ID mapping results.
>> +These keys are visible only to the superuser.
>> +.TP
>> .B -r user
>> Revoke both the uid and gid key of the given user.
>> .TP
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever

Re: [PATCH 3/6] nfsidmap: List cached ID mapping results

[Chuck Lever]

On Aug 4, 2015, at 10:15 AM, Chuck Lever <chuck.lever@oracle.com> wrote:

> 
> On Aug 4, 2015, at 9:29 AM, Steve Dickson <SteveD@redhat.com> wrote:
> 
>> Hello,
>> 
>> On 07/31/2015 06:13 PM, Chuck Lever wrote:
>>> User space can see the keys, but not their contents.
>>> 
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>> utils/nfsidmap/nfsidmap.c   |   85 ++++++++++++++++++++++++++++++++++++++++++-
>>> utils/nfsidmap/nfsidmap.man |   15 ++++++++
>>> 2 files changed, 98 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
>>> index 44b8b4b..ed397c6 100644
>>> --- a/utils/nfsidmap/nfsidmap.c
>>> +++ b/utils/nfsidmap/nfsidmap.c
>>> @@ -108,6 +108,81 @@ static int display_default_domain(void)
>>> 	return EXIT_SUCCESS;
>>> }
>>> 
>>> +static void list_key(key_serial_t key)
>>> +{
>>> +	char *buffer, *c;
>>> +	int rc;
>>> +
>>> +	rc = keyctl_describe_alloc(key, &buffer);
>>> +	if (rc < 0) {
>>> +		switch (errno) {
>>> +		case EKEYEXPIRED:
>>> +			printf("Expired key not displayed\n");
>>> +			break;
>>> +		default:
>>> +			xlog_err("Failed to describe key: %m");
>>> +		}
>>> +		return;
>>> +	}
>>> +
>>> +	c = strrchr(buffer, ';');
>>> +	if (!c) {
>>> +		xlog_err("Unparsable key not displayed\n");
>>> +		goto out_free;
>>> +	}
>>> +	printf("  %s\n", ++c);
>>> +
>>> +out_free:
>>> +	free(buffer);
>>> +}
>>> +
>>> +static void list_keys(const char *ring_name, key_serial_t ring_id)
>>> +{
>>> +	key_serial_t *key;
>>> +	void *keylist;
>>> +	int count;
>>> +
>>> +	count = keyctl_read_alloc(ring_id, &keylist);
>>> +	if (count < 0) {
>>> +		xlog_err("Failed to read keyring %s: %m", ring_name);
>>> +		return;
>>> +	}
>>> +	count /= (int)sizeof(*key);
>>> +
>>> +	switch (count) {
>>> +	case 0:
>>> +		printf("No %s keys found.\n", ring_name);
>>> +		break;
>>> +	case 1:
>>> +		printf("1 %s key found:\n", ring_name);
>>> +		break;
>>> +	default:
>>> +		printf("%u %s keys found:\n", count, ring_name);
>>> +	}
>>> +
>>> +	for (key = keylist; count--; key++)
>>> +		list_key(*key);
>>> +
>>> +	free(keylist);
>>> +}
>>> +
>>> +/*
>>> + * List all keys on a keyring
>>> + */
>>> +static int list_keyring(const char *keyring)
>>> +{
>>> +	key_serial_t key;
>>> +
>>> +	key = find_key_by_type_and_desc("keyring", keyring, 0);
>>> +	if (key == -1) {
>>> +		xlog_err("'%s' keyring was not found.", keyring);
>>> +		return EXIT_FAILURE;
>>> +	}
>>> +
>>> +	list_keys(keyring, key);
>>> +	return EXIT_SUCCESS;
>>> +}
>>> +
>>> /*
>>> * Find either a user or group id based on the name@domain string
>>> */
>>> @@ -277,7 +352,7 @@ int main(int argc, char **argv)
>>> 	int timeout = 600;
>>> 	key_serial_t key;
>>> 	char *progname, *keystr = NULL;
>>> -	int clearing = 0, keymask = 0, display = 0;
>>> +	int clearing = 0, keymask = 0, display = 0, list = 0;
>>> 
>>> 	/* Set the basename */
>>> 	if ((progname = strrchr(argv[0], '/')) != NULL)
>>> @@ -287,11 +362,14 @@ int main(int argc, char **argv)
>>> 
>>> 	xlog_open(progname);
>>> 
>>> -	while ((opt = getopt(argc, argv, "du:g:r:ct:v")) != -1) {
>>> +	while ((opt = getopt(argc, argv, "du:g:r:ct:vl")) != -1) {
>>> 		switch (opt) {
>>> 		case 'd':
>>> 			display++;
>>> 			break;
>>> +		case 'l':
>>> +			list++;
>>> +			break;
>>> 		case 'u':
>>> 			keymask = UIDKEYS;
>>> 			keystr = strdup(optarg);
>>> @@ -328,6 +406,9 @@ int main(int argc, char **argv)
>>> 
>>> 	if (display)
>>> 		return display_default_domain();
>>> +	else if (list)
>>> +		return list_keyring(DEFAULT_KEYRING);
>>> +
>> Just curious as to why the else clause... Are you
>> thinking it does not make sense to display the 
>> domain and list out the key ring?
> 
> Well, display already has a hard "return display_default_domain()".
> If you specify "-l" you're not getting anything else. The "else"

Sorry, I meant "if you specify "-d" you're not getting anything else."

> can probably be removed.
> 
> The idea is that "nfsidmap -l" or "nfsidmap -d" can be used
> in scripts too. In that context, specifying both would make
> parsing the output more difficult. And I think you would use
> these in different situations. "-l" might be used a lot. "-d"
> might be used once to check the configuration.
> 
> And IMO these new options should be careful to avoid updating
> or revoking any key, so the logic exits right there, as a
> defensive precaution.
> 
> We're overloading a kernel API here, after all. Maybe this kind
> of debugging belongs in a separate command. But this seems safe
> enough.
> 
> 
>> steved.
>> 
>>> 	if (keystr) {
>>> 		rc = key_invalidate(keystr, keymask);
>>> 		return rc;		
>>> diff --git a/utils/nfsidmap/nfsidmap.man b/utils/nfsidmap/nfsidmap.man
>>> index 04ddff6..0275bdf 100644
>>> --- a/utils/nfsidmap/nfsidmap.man
>>> +++ b/utils/nfsidmap/nfsidmap.man
>>> @@ -13,6 +13,8 @@ nfsidmap \- The NFS idmapper upcall program
>>> .B "nfsidmap [-v] [-u|-g|-r user]"
>>> .br
>>> .B "nfsidmap -d"
>>> +.br
>>> +.B "nfsidmap -l"
>>> .SH DESCRIPTION
>>> The NFSv4 protocol represents the local system's UID and GID values
>>> on the wire as strings of the form
>>> @@ -50,6 +52,13 @@ can also clear cached ID map results in the kernel,
>>> or revoke one particular key.
>>> An incorrect cached key can result in file and directory ownership
>>> reverting to "nobody" on NFSv4 mount points.
>>> +.PP
>>> +In addition, the
>>> +.B -d
>>> +and
>>> +.B -l
>>> +options are available to help diagnose misconfigurations.
>>> +They have no effect on the keyring containing ID mapping results.
>>> .SH OPTIONS
>>> .TP
>>> .B -c 
>>> @@ -62,6 +71,12 @@ Display the system's effective NFSv4 domain name on
>>> .B -g user
>>> Revoke the gid key of the given user.
>>> .TP
>>> +.B -l
>>> +Display on
>>> +.I stdout
>>> +all keys currently in the keyring used to cache ID mapping results.
>>> +These keys are visible only to the superuser.
>>> +.TP
>>> .B -r user
>>> Revoke both the uid and gid key of the given user.
>>> .TP
>>> 
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> --
> Chuck Lever
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
Chuck Lever

[PATCH 4/6] nfsidmap: Fix error handling in id_lookup()

[Chuck Lever]

As near as I can tell, the exit status of nfsidmap is supposed to be
zero (success) or one (failure).

The return value of id_lookup() becomes the exit status, so it
should return only zero or one.

The libnfsidmap calls return a signed integer, either 0 or negative
errno values. These have to be translated to an exit status.

libkeyutils calls return a signed long, either 0 or -1. These also
have to be translated to an exit status.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 utils/nfsidmap/nfsidmap.c |   39 +++++++++++++++++++++------------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index ed397c6..9881694 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -200,30 +200,33 @@ static int id_lookup(char *name_at_domain, key_serial_t key, int type)
 		rc = nfs4_group_owner_to_gid(name_at_domain, &gid);
 		sprintf(id, "%u", gid);
 	}
-	if (rc < 0)
+	if (rc < 0) {
 		xlog_errno(rc, "id_lookup: %s: failed: %m",
 			(type == USER ? "nfs4_owner_to_uid" : "nfs4_group_owner_to_gid"));
+		return EXIT_FAILURE;
+	}
 
-	if (rc == 0) {
-		rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
-		if (rc < 0) {
-			switch(rc) {
-			case -EDQUOT:
-			case -ENFILE:
-			case -ENOMEM:
-				/*
-			 	 * The keyring is full. Clear the keyring and try again
-			 	 */
-				rc = keyring_clear(DEFAULT_KEYRING);
-				if (rc == 0)
-					rc = keyctl_instantiate(key, id, strlen(id) + 1, 0);
-				break;
-			default:
+	rc = EXIT_SUCCESS;
+	if (keyctl_instantiate(key, id, strlen(id) + 1, 0)) {
+		switch (errno) {
+		case EDQUOT:
+		case ENFILE:
+		case ENOMEM:
+			/*
+			 * The keyring is full. Clear the keyring and try again
+			 */
+			rc = keyring_clear(DEFAULT_KEYRING);
+			if (rc)
 				break;
+			if (keyctl_instantiate(key, id, strlen(id) + 1, 0)) {
+				rc = EXIT_FAILURE;
+				xlog_err("id_lookup: keyctl_instantiate failed: %m");
 			}
+			break;
+		default:
+			rc = EXIT_FAILURE;
+			break;
 		}
-		if (rc < 0)
-			xlog_err("id_lookup: keyctl_instantiate failed: %m");
 	}
 
 	return rc;

[PATCH 5/6] nfsidmap: Fix error handling in name_lookup()

[Chuck Lever]

As near as I can tell, the exit status of nfsidmap is supposed to be
zero (success) or one (failure).

The return value of name_lookup() becomes the exit status, so it
should return only zero or one.

The libnfsidmap calls return a signed integer, either 0 or negative
errno values. These have to be translated to an exit status.

libkeyutils calls return a signed long, either 0 or -1. These also
have to be translated to an exit status.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 utils/nfsidmap/nfsidmap.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index 9881694..e9941b4 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -244,11 +244,10 @@ static int name_lookup(char *id, key_serial_t key, int type)
 	int rc;
 
 	rc = nfs4_get_default_domain(NULL, domain, NFS4_MAX_DOMAIN_LEN);
-	if (rc != 0) {
+	if (rc) {
 		xlog_errno(rc,
 			"name_lookup: nfs4_get_default_domain failed: %m");
-		rc = -1;
-		goto out;
+		return EXIT_FAILURE;
 	}
 
 	if (type == USER) {
@@ -258,16 +257,18 @@ static int name_lookup(char *id, key_serial_t key, int type)
 		gid = atoi(id);
 		rc = nfs4_gid_to_name(gid, domain, name, IDMAP_NAMESZ);
 	}
-	if (rc < 0)
+	if (rc) {
 		xlog_errno(rc, "name_lookup: %s: failed: %m",
 			(type == USER ? "nfs4_uid_to_name" : "nfs4_gid_to_name"));
+		return EXIT_FAILURE;
+	}
 
-	if (rc == 0) {
-		rc = keyctl_instantiate(key, &name, strlen(name), 0);
-		if (rc < 0)
-			xlog_err("name_lookup: keyctl_instantiate failed: %m");
+	rc = EXIT_SUCCESS;
+	if (keyctl_instantiate(key, &name, strlen(name), 0)) {
+		rc = EXIT_FAILURE;
+		xlog_err("name_lookup: keyctl_instantiate failed: %m");
 	}
-out:
+
 	return rc;
 }
 

[PATCH 6/6] nfsidmap: Clean up other exit status cases

[Chuck Lever]

Make it unambiguous where 0 or 1 represent an exit status.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 utils/nfsidmap/nfsidmap.c |   29 ++++++++++++++---------------
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/utils/nfsidmap/nfsidmap.c b/utils/nfsidmap/nfsidmap.c
index e9941b4..88e5c1e 100644
--- a/utils/nfsidmap/nfsidmap.c
+++ b/utils/nfsidmap/nfsidmap.c
@@ -286,7 +286,7 @@ static int key_invalidate(char *keystr, int keymask)
 
 	if ((fp = fopen(PROCKEYS, "r")) == NULL) {
 		xlog_err("fopen(%s) failed: %m", PROCKEYS);
-		return 1;
+		return EXIT_FAILURE;
 	}
 
 	while(fgets(buf, BUFSIZ, fp) != NULL) {
@@ -325,13 +325,13 @@ static int key_invalidate(char *keystr, int keymask)
 			if (errno != EOPNOTSUPP) {
 				xlog_err("keyctl_invalidate(0x%x) failed: %m", key);
 				fclose(fp);
-				return 1;
+				return EXIT_FAILURE;
 			} else {
 				/* older kernel compatibility attempt: */
 				if (keyctl_revoke(key) < 0) {
 					xlog_err("keyctl_revoke(0x%x) failed: %m", key);
 					fclose(fp);
-					return 1;
+					return EXIT_FAILURE;
 				}
 			}
 		}
@@ -339,12 +339,12 @@ static int key_invalidate(char *keystr, int keymask)
 		keymask &= ~mask;
 		if (keymask == 0) {
 			fclose(fp);
-			return 0;
+			return EXIT_SUCCESS;
 		}
 	}
 	xlog_err("'%s' key was not found.", keystr);
 	fclose(fp);
-	return 1;
+	return EXIT_FAILURE;
 }
 
 int main(int argc, char **argv)
@@ -403,7 +403,7 @@ int main(int argc, char **argv)
 
 	if ((rc = nfs4_init_name_mapping(PATH_IDMAPDCONF)))  {
 		xlog_errno(rc, "Unable to create name to user id mappings.");
-		return 1;
+		return EXIT_FAILURE;
 	}
 	if (!verbose)
 		verbose = conf_get_num("General", "Verbosity", 0);
@@ -414,20 +414,18 @@ int main(int argc, char **argv)
 		return list_keyring(DEFAULT_KEYRING);
 
 	if (keystr) {
-		rc = key_invalidate(keystr, keymask);
-		return rc;		
+		return key_invalidate(keystr, keymask);
 	}
 	if (clearing) {
 		xlog_syslog(0);
-		rc = keyring_clear(DEFAULT_KEYRING);
-		return rc;		
+		return keyring_clear(DEFAULT_KEYRING);
 	}
 
 	xlog_stderr(0);
 	if ((argc - optind) != 2) {
 		xlog_err("Bad arg count. Check /etc/request-key.conf");
 		xlog_warn(usage, progname);
-		return 1;
+		return EXIT_FAILURE;
 	}
 
 	if (verbose)
@@ -438,13 +436,14 @@ int main(int argc, char **argv)
 	arg = strdup(argv[optind]);
 	if (arg == NULL) {
 		xlog_err("strdup failed: %m");
-		return 1;
+		return EXIT_FAILURE;
 	}
 	type = strtok(arg, ":");
 	value = strtok(NULL, ":");
-    if (value == NULL) {
+	if (value == NULL) {
+		free(arg);
 		xlog_err("Error: Null uid/gid value.");
-		return 1;
+		return EXIT_FAILURE;
 	}
 	if (verbose) {
 		xlog_warn("key: 0x%lx type: %s value: %s timeout %ld",
@@ -464,7 +463,7 @@ int main(int argc, char **argv)
 		rc = name_lookup(value, key, GROUP);
 
 	/* Set timeout to 10 (600 seconds) minutes */
-	if (rc == 0)
+	if (rc == EXIT_SUCCESS)
 		keyctl_set_timeout(key, timeout);
 
 	free(arg);