ACK: [Utopic][PATCH] KEYS: request_key() should reget expired keys rather than give EKEYEXPIRED

[Chris J Arges]

This backport matches what debian has, and had to be backported because
of the KEYRING_SEARCH_* flag that is removed. The bug now has the SRU
template and I've got verification from Dariusz that he's built/tested
this patch.

Acked-by: Chris J Arges <chris.j.arges at canonical.com>

On 03/26/2015 10:16 AM, Dariusz Gadomski wrote:
> From: David Howells <dhowells at redhat.com>
> 
> BugLink: https://bugs.launchpad.net/bugs/1124250
> 
> Since the keyring facility can be viewed as a cache (at least in some
> applications), the local expiration time on the key should probably be viewed
> as a 'needs updating after this time' property rather than an absolute 'anyone
> now wanting to use this object is out of luck' property.
> 
> Since request_key() is the main interface for the usage of keys, this should
> update or replace an expired key rather than issuing EKEYEXPIRED if the local
> expiration has been reached (ie. it should refresh the cache).
> 
> For absolute conditions where refreshing the cache probably doesn't help, the
> key can be negatively instantiated using KEYCTL_REJECT_KEY with EKEYEXPIRED
> given as the error to issue.  This will still cause request_key() to return
> EKEYEXPIRED as that was explicitly set.
> 
> In the future, if the key type has an update op available, we might want to
> upcall with the expired key and allow the upcall to update it.  We would pass
> a different operation name (the first column in /etc/request-key.conf) to the
> request-key program.
> 
> request_key() returning EKEYEXPIRED is causing an NFS problem which Chuck
> Lever describes thusly:
> 
> 	After about 10 minutes, my NFSv4 functional tests fail because the
> 	ownership of the test files goes to "-2". Looking at /proc/keys
> 	shows that the id_resolv keys that map to my test user ID have
> 	expired. The ownership problem persists until the expired keys are
> 	purged from the keyring, and fresh keys are obtained.
> 
> 	I bisected the problem to 3.13 commit b2a4df200d57 ("KEYS: Expand
> 	the capacity of a keyring"). This commit inadvertantly changes the
> 	API contract of the internal function keyring_search_aux().
> 
> 	The root cause appears to be that b2a4df200d57 made "no state check"
> 	the default behavior. "No state check" means the keyring search
> 	iterator function skips checking the key's expiry timeout, and
> 	returns expired keys.  request_key_and_link() depends on getting
> 	an -EAGAIN result code to know when to perform an upcall to refresh
> 	an expired key.
> 
> This patch can be tested directly by:
> 
> 	keyctl request2 user debug:fred a @s
> 	keyctl timeout %user:debug:fred 3
> 	sleep 4
> 	keyctl request2 user debug:fred a @s
> 
> Without the patch, the last command gives error EKEYEXPIRED, but with the
> command it gives a new key.
> 
> Reported-by: Carl Hetherington <cth at carlh.net>
> Reported-by: Chuck Lever <chuck.lever at oracle.com>
> Signed-off-by: David Howells <dhowells at redhat.com>
> Tested-by: Chuck Lever <chuck.lever at oracle.com>
> (backported from commit 0b0a84154eff56913e91df29de5c3a03a0029e38 upstream)
> Signed-off-by: Dariusz Gadomski <dariusz.gadomski at canonical.com>
> 
> Conflicts:
> 	security/keys/internal.h
> 	security/keys/request_key.c
> ---
>  security/keys/internal.h    | 1 +
>  security/keys/keyring.c     | 3 ++-
>  security/keys/request_key.c | 3 ++-
>  3 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/security/keys/internal.h b/security/keys/internal.h
> index 5f20da0..6786b51 100644
> --- a/security/keys/internal.h
> +++ b/security/keys/internal.h
> @@ -121,6 +121,7 @@ struct keyring_search_context {
>  #define KEYRING_SEARCH_NO_UPDATE_TIME	0x0008	/* Don't update times */
>  #define KEYRING_SEARCH_NO_CHECK_PERM	0x0010	/* Don't check permissions */
>  #define KEYRING_SEARCH_DETECT_TOO_DEEP	0x0020	/* Give an error on excessive depth */
> +#define KEYRING_SEARCH_SKIP_EXPIRED	0x0040	/* Ignore expired keys (intention to replace) */
>  
>  	int (*iterator)(const void *object, void *iterator_data);
>  
> diff --git a/security/keys/keyring.c b/security/keys/keyring.c
> index 9cf2575..d78ce06 100644
> --- a/security/keys/keyring.c
> +++ b/security/keys/keyring.c
> @@ -526,7 +526,8 @@ static int keyring_search_iterator(const void *object, void *iterator_data)
>  		}
>  
>  		if (key->expiry && ctx->now.tv_sec >= key->expiry) {
> -			ctx->result = ERR_PTR(-EKEYEXPIRED);
> +			if (!(ctx->flags & KEYRING_SEARCH_SKIP_EXPIRED))
> +				ctx->result = ERR_PTR(-EKEYEXPIRED);
>  			kleave(" = %d [expire]", ctx->skipped_ret);
>  			goto skipped;
>  		}
> diff --git a/security/keys/request_key.c b/security/keys/request_key.c
> index 3814119..239896b 100644
> --- a/security/keys/request_key.c
> +++ b/security/keys/request_key.c
> @@ -533,7 +533,8 @@ struct key *request_key_and_link(struct key_type *type,
>  		.cred			= current_cred(),
>  		.match			= type->match,
>  		.match_data		= description,
> -		.flags			= KEYRING_SEARCH_LOOKUP_DIRECT,
> +		.flags			= (KEYRING_SEARCH_LOOKUP_DIRECT |
> +					   KEYRING_SEARCH_SKIP_EXPIRED),
>  	};
>  	struct key *key;
>  	key_ref_t key_ref;
>