[PATCH] nvme: add 'static' suffix for user-specified TLS keys

[PATCH] nvme: add 'static' suffix for user-specified TLS keys

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.de>

If the user specifies a specific TLS key via the '--tls_key'
option to nvme-cli we should be adding a ';static' suffix
to the 'tls_key' sysfs attribute to signal that this is a
user-specific key, and not one auto-selected during TLS
protocol negotiation.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/nvme/host/sysfs.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index d099218e494a..79f25029c2fb 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -674,10 +674,14 @@ static ssize_t tls_key_show(struct device *dev,
 			    struct device_attribute *attr, char *buf)
 {
 	struct nvme_ctrl *ctrl = dev_get_drvdata(dev);
+	char *suffix = "";
 
 	if (!ctrl->tls_key)
 		return 0;
-	return sysfs_emit(buf, "%08x", key_serial(ctrl->tls_key));
+	if (ctrl->opts->tls_key)
+		suffix = ";static";
+	return sysfs_emit(buf, "%08x%s",
+			  key_serial(ctrl->tls_key), suffix);
 }
 static DEVICE_ATTR_RO(tls_key);
 #endif
-- 
2.35.3

Re: [PATCH] nvme: add 'static' suffix for user-specified TLS keys

[Daniel Wagner]

On Mon, Feb 26, 2024 at 01:24:49PM +0100, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> If the user specifies a specific TLS key via the '--tls_key'
> option to nvme-cli we should be adding a ';static' suffix
> to the 'tls_key' sysfs attribute to signal that this is a
> user-specific key, and not one auto-selected during TLS
> protocol negotiation.

Isn't this conflicting the one value per sysfs entry rule?

Re: [PATCH] nvme: add 'static' suffix for user-specified TLS keys

[Hannes Reinecke]

On 2/26/24 17:35, Daniel Wagner wrote:
> On Mon, Feb 26, 2024 at 01:24:49PM +0100, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> If the user specifies a specific TLS key via the '--tls_key'
>> option to nvme-cli we should be adding a ';static' suffix
>> to the 'tls_key' sysfs attribute to signal that this is a
>> user-specific key, and not one auto-selected during TLS
>> protocol negotiation.
> 
> Isn't this conflicting the one value per sysfs entry rule?
Yeah, you are right.

I'll be sending a new patch introducing a separate attribute.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                  Kernel Storage Architect
hare@suse.de                                +49 911 74053 688
SUSE Software Solutions GmbH, Frankenstr. 146, 90461 Nürnberg
HRB 36809 (AG Nürnberg), GF: I. Totev, A. McDonald, W. Knoblich

Re: [PATCH] nvme: add 'static' suffix for user-specified TLS keys

[Sagi Grimberg]

On 26/02/2024 14:24, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.de>
>
> If the user specifies a specific TLS key via the '--tls_key'
> option to nvme-cli we should be adding a ';static' suffix
> to the 'tls_key' sysfs attribute to signal that this is a
> user-specific key, and not one auto-selected during TLS
> protocol negotiation.

Static is not the best wording. Doesn't the user already
know what it provided? How is this indication useful?

Re: [PATCH] nvme: add 'static' suffix for user-specified TLS keys

[Hannes Reinecke]

On 3/7/24 10:45, Sagi Grimberg wrote:
> 
> 
> On 26/02/2024 14:24, Hannes Reinecke wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> If the user specifies a specific TLS key via the '--tls_key'
>> option to nvme-cli we should be adding a ';static' suffix
>> to the 'tls_key' sysfs attribute to signal that this is a
>> user-specific key, and not one auto-selected during TLS
>> protocol negotiation.
> 
> Static is not the best wording. Doesn't the user already
> know what it provided? How is this indication useful?

What I'm trying to achieve here is to differentiate between

nvme connect --tls

and

nvme connect --tls_key=<key_id>

both calls will result in key serial number in the
'tls_key' attribute, so if we try to generate the json
configuration file from an existing connection we
have no idea which of the arguments the admin has
used. Problem is that first invocation will auto-select
the TLS key, and hence we can do a key rotation.
The second invocation will always refer to a specific
key, and hence we cannot do a key rotation.

So we'll need an additional setting allowing us
to differentate here.

I do agree that adding a field to an existing
attribute is probably suboptimal, so I guess
I'll retract this patch and send another introducing
a new sysfs attribute ('tls_provisioning' ?)
The alternative would be to add a new attribute for
the keyring (ie introduce a 'tls_keyring' attribute).
That would be empty for a pre-configured key (as we
are looking up the key by serial, and don't care on
which keyring the key is in), and contain the keyring
name otherwise.

Hmm. I guess I like this better, as then we would
also be able to display the 'keyring' setting which
currently is also not present in sysfs.

Cheers,

Hannes

Re: [PATCH] nvme: add 'static' suffix for user-specified TLS keys

[Sagi Grimberg]

On 07/03/2024 12:47, Hannes Reinecke wrote:
> On 3/7/24 10:45, Sagi Grimberg wrote:
>>
>>
>> On 26/02/2024 14:24, Hannes Reinecke wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> If the user specifies a specific TLS key via the '--tls_key'
>>> option to nvme-cli we should be adding a ';static' suffix
>>> to the 'tls_key' sysfs attribute to signal that this is a
>>> user-specific key, and not one auto-selected during TLS
>>> protocol negotiation.
>>
>> Static is not the best wording. Doesn't the user already
>> know what it provided? How is this indication useful?
>
> What I'm trying to achieve here is to differentiate between
>
> nvme connect --tls
>
> and
>
> nvme connect --tls_key=<key_id>
>
> both calls will result in key serial number in the
> 'tls_key' attribute, so if we try to generate the json
> configuration file from an existing connection we
> have no idea which of the arguments the admin has
> used. Problem is that first invocation will auto-select
> the TLS key, and hence we can do a key rotation.
> The second invocation will always refer to a specific
> key, and hence we cannot do a key rotation.
>
> So we'll need an additional setting allowing us
> to differentate here.
>
> I do agree that adding a field to an existing
> attribute is probably suboptimal, so I guess
> I'll retract this patch and send another introducing
> a new sysfs attribute ('tls_provisioning' ?)

What are the outputs for this new attributes?

>
> The alternative would be to add a new attribute for
> the keyring (ie introduce a 'tls_keyring' attribute).
> That would be empty for a pre-configured key (as we
> are looking up the key by serial, and don't care on
> which keyring the key is in), and contain the keyring
> name otherwise.
>
> Hmm. I guess I like this better, as then we would
> also be able to display the 'keyring' setting which
> currently is also not present in sysfs.

Agree that displaying the keyring in sysfs is useful to
have.

Re: [PATCH] nvme: add 'static' suffix for user-specified TLS keys

[Hannes Reinecke]

On 3/8/24 11:42, Sagi Grimberg wrote:
> 
> 
> On 07/03/2024 12:47, Hannes Reinecke wrote:
>> On 3/7/24 10:45, Sagi Grimberg wrote:
>>>
>>>
>>> On 26/02/2024 14:24, Hannes Reinecke wrote:
>>>> From: Hannes Reinecke <hare@suse.de>
>>>>
>>>> If the user specifies a specific TLS key via the '--tls_key'
>>>> option to nvme-cli we should be adding a ';static' suffix
>>>> to the 'tls_key' sysfs attribute to signal that this is a
>>>> user-specific key, and not one auto-selected during TLS
>>>> protocol negotiation.
>>>
>>> Static is not the best wording. Doesn't the user already
>>> know what it provided? How is this indication useful?
>>
>> What I'm trying to achieve here is to differentiate between
>>
>> nvme connect --tls
>>
>> and
>>
>> nvme connect --tls_key=<key_id>
>>
>> both calls will result in key serial number in the
>> 'tls_key' attribute, so if we try to generate the json
>> configuration file from an existing connection we
>> have no idea which of the arguments the admin has
>> used. Problem is that first invocation will auto-select
>> the TLS key, and hence we can do a key rotation.
>> The second invocation will always refer to a specific
>> key, and hence we cannot do a key rotation.
>>
>> So we'll need an additional setting allowing us
>> to differentate here.
>>
>> I do agree that adding a field to an existing
>> attribute is probably suboptimal, so I guess
>> I'll retract this patch and send another introducing
>> a new sysfs attribute ('tls_provisioning' ?)
> 
> What are the outputs for this new attributes?
> 
I've just sent a patch for this.

>>
>> The alternative would be to add a new attribute for
>> the keyring (ie introduce a 'tls_keyring' attribute).
>> That would be empty for a pre-configured key (as we
>> are looking up the key by serial, and don't care on
>> which keyring the key is in), and contain the keyring
>> name otherwise.
>>
>> Hmm. I guess I like this better, as then we would
>> also be able to display the 'keyring' setting which
>> currently is also not present in sysfs.
> 
> Agree that displaying the keyring in sysfs is useful to
> have.

Indeed.

Cheers,

Hannes