Re: [PATCH 1/2] NFS: support the kernel keyring for TLS

[Jarkko Sakkinen <jarkko@kernel.org>]

On Thu, May 15, 2025 at 01:50:55PM +0200, Christoph Hellwig wrote:
> Allow tlshd to use a per-mount key from the kernel keyring similar
> to NVMe over TCP.
> 
> Note that tlshd expects keys and certificates stored in the kernel
> keyring to be in DER format, not the PEM format used for file based keys
> and certificates, so they need to be converted before they are added
> to the keyring, which is a bit unexpected.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/nfs/fs_context.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/fs/nfs/fs_context.c b/fs/nfs/fs_context.c
> index 13f71ca8c974..9e94d18448ff 100644
> --- a/fs/nfs/fs_context.c
> +++ b/fs/nfs/fs_context.c
> @@ -96,6 +96,8 @@ enum nfs_param {
>  	Opt_wsize,
>  	Opt_write,
>  	Opt_xprtsec,
> +	Opt_cert_serial,
> +	Opt_privkey_serial,
>  };
>  
>  enum {
> @@ -221,6 +223,8 @@ static const struct fs_parameter_spec nfs_fs_parameters[] = {
>  	fsparam_enum  ("write",		Opt_write, nfs_param_enums_write),
>  	fsparam_u32   ("wsize",		Opt_wsize),
>  	fsparam_string("xprtsec",	Opt_xprtsec),
> +	fsparam_s32("cert_serial",	Opt_cert_serial),
> +	fsparam_s32("privkey_serial",	Opt_privkey_serial),
>  	{}
>  };
>  
> @@ -551,6 +555,32 @@ static int nfs_parse_version_string(struct fs_context *fc,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_KEYS
> +static int nfs_tls_key_verify(key_serial_t key_id)
> +{
> +	struct key *key = key_lookup(key_id);
> +	int error = 0;
> +
> +	if (IS_ERR(key)) {
> +		pr_err("key id %08x not found\n", key_id);
> +		return PTR_ERR(key);
> +	}
> +	if (test_bit(KEY_FLAG_REVOKED, &key->flags) ||
> +	    test_bit(KEY_FLAG_INVALIDATED, &key->flags)) {
> +		pr_err("key id %08x revoked\n", key_id);
> +		error = -EKEYREVOKED;
> +	}
> +
> +	key_put(key);
> +	return error;
> +}

This is equivalent nvme_tls_key_lookup() so would it be more senseful
to call it nfs_tls_key_lookup()? I'm also a bit puzzled how the code
will associate nfs_keyring to all this (e.g., with keyring_search as
done in nvme_tls_psk_lookup())?

> +#else
> +static inline int nfs_tls_key_verify(key_serial_t key_id)
> +{
> +	return -ENOENT;
> +}
> +#endif /* CONFIG_KEYS */
> +
>  /*
>   * Parse a single mount parameter.
>   */
> @@ -807,6 +837,18 @@ static int nfs_fs_context_parse_param(struct fs_context *fc,
>  		if (ret < 0)
>  			return ret;
>  		break;
> +	case Opt_cert_serial:
> +		ret = nfs_tls_key_verify(result.int_32);
> +		if (ret < 0)
> +			return ret;
> +		ctx->xprtsec.cert_serial = result.int_32;
> +		break;
> +	case Opt_privkey_serial:
> +		ret = nfs_tls_key_verify(result.int_32);
> +		if (ret < 0)
> +			return ret;
> +		ctx->xprtsec.privkey_serial = result.int_32;
> +		break;
>  
>  	case Opt_proto:
>  		if (!param->string)
> -- 
> 2.47.2
> 

I get the change i.e., keep keys opaque, and it is a reasonable goal.
However, the keyring-key association is where I get lost, so if you
could help me out with that a bit, we could make progress :-)

BR, Jarkko