* [PATCH 0/1] NVMe/TLS connection issues to SPDK
@ 2025-07-21 2:17 Chris Leech
2025-07-21 2:17 ` [PATCH 1/1] libnvme: TLS PSK derivation fixes Chris Leech
` (3 more replies)
0 siblings, 4 replies; 22+ messages in thread
From: Chris Leech @ 2025-07-21 2:17 UTC (permalink / raw)
To: linux-nvme
Cc: Hannes Reinecke, Daniel Wagner, Prashanth Nayak, John Meneghini
I was attempting to debug connecting the Linux driver / libnvme /
ktls-utils host stack to the SPDK nvmf_tgt over TLS, and ran into some
issues.
The TLS connection fails to complete a handshake because the TLS PSKs
are different. The NVMe/TCP specified key derivation steps from the
configured interchange format, to a retained PSK and finally the TLS
PSK, is implemented incompatibly in libnvme and SPDK. After some
investigation, I believe the SPDK implementation to be correct and am
providing a libnvme patch to match it. With libnvme modified, I see the
TLS handshake complete in tlshd.
(Note that this was tested using the obsolete "version 0" PSK Identity
and TLS PSK derivation from the TCP transport 1.0 specification, as SPDK
has not been updated with the "version 1" changes)
The NVMe/TCP host driver then quickly fails when SPDK sends a TLS "New
Session Ticket" message before ICResp.
While possibly pointless due to the transport specification prohibition
on session resumption and 0-RTT data, I don't think this is necessarily
wrong and the host driver should be able to safely ignore it and
continue.
I'm working on testing that out, but a more general TLS message demuxing
layer to deal with post-handshake messages other than application data
may be wanted to avoid sprinkling checks around the nvme driver.
Chris Leech (1):
libnvme: TLS PSK derivation fixes
src/nvme/linux.c | 86 ++++++++++++++++++++++++++++++++----------------
1 file changed, 57 insertions(+), 29 deletions(-)
--
2.50.1
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-07-21 2:17 [PATCH 0/1] NVMe/TLS connection issues to SPDK Chris Leech
@ 2025-07-21 2:17 ` Chris Leech
2025-07-21 6:36 ` Hannes Reinecke
2025-07-21 7:11 ` [PATCH 0/1] NVMe/TLS connection issues to SPDK Hannes Reinecke
` (2 subsequent siblings)
3 siblings, 1 reply; 22+ messages in thread
From: Chris Leech @ 2025-07-21 2:17 UTC (permalink / raw)
To: linux-nvme
Cc: Hannes Reinecke, Daniel Wagner, Prashanth Nayak, John Meneghini
There are issues with the Retained and TLS PSK derivations due to the
implementation not adhering to the RFC 8446 definition of the
HKDF-Expand-Label function.
1) The 16-bit HkdfLabel.length value must be converted to network byte
order.
2) The variable length HkdfLabel.label and HkdfLabel.context vectors
must be prefixed with a length byte.
Signed-off-by: Chris Leech <cleech@redhat.com>
---
src/nvme/linux.c | 86 ++++++++++++++++++++++++++++++++----------------
1 file changed, 57 insertions(+), 29 deletions(-)
diff --git a/src/nvme/linux.c b/src/nvme/linux.c
index ae4aa526..674b20b5 100644
--- a/src/nvme/linux.c
+++ b/src/nvme/linux.c
@@ -618,6 +618,46 @@ static DEFINE_CLEANUP_FUNC(
cleanup_evp_pkey_ctx, EVP_PKEY_CTX *, EVP_PKEY_CTX_free)
#define _cleanup_evp_pkey_ctx_ __cleanup__(cleanup_evp_pkey_ctx)
+/*
+ * hkdf_info_printf()
+ *
+ * Helper function to append variable length label and context to an HkdfLabel
+ *
+ * RFC 8446 (TLS 1.3) Section 7.1 defines the HKDF-Expand-Label function as a
+ * specialization of the HKDF-Expand function (RFC 5869), where the info
+ * parameter is structured as an HkdfLabel.
+ *
+ * An HkdfLabel structure includes two variable length vectors (label and
+ * context) which must be preceded by their content length as per RFC 8446
+ * Section 3.4 (and not NUL terminated as per Section 7.1). Additionally,
+ * HkdfLabel.label must begin with "tls13 "
+ *
+ * Returns the number of bytes appended to the HKDF info buffer, or -1 on an
+ * error.
+ */
+__attribute__((format(printf, 2, 3)))
+static int hkdf_info_printf(EVP_PKEY_CTX *ctx, char *fmt, ...)
+{
+ _cleanup_free_ char *str;
+ uint8_t len;
+ int ret;
+
+ va_list myargs;
+ va_start(myargs, fmt);
+ ret = vasprintf(&str, fmt, myargs);
+ va_end(myargs);
+ if (ret < 0)
+ return ret;
+ if (ret > 255)
+ return -1;
+ len = ret;
+ if (EVP_PKEY_CTX_add1_hkdf_info(ctx, (unsigned char *)&len, 1) <= 0)
+ return -1;
+ if (EVP_PKEY_CTX_add1_hkdf_info(ctx, (unsigned char *)str, len) <= 0)
+ return -1;
+ return (ret + 1);
+}
+
/*
* derive_retained_key()
*
@@ -652,7 +692,7 @@ static int derive_retained_key(int hmac, const char *hostnqn,
size_t key_len)
{
_cleanup_evp_pkey_ctx_ EVP_PKEY_CTX *ctx = NULL;
- uint16_t length = key_len & 0xFFFF;
+ uint16_t length = htons(key_len & 0xFFFF);
const EVP_MD *md;
size_t hmac_len;
@@ -690,18 +730,11 @@ static int derive_retained_key(int hmac, const char *hostnqn,
errno = ENOKEY;
return -1;
}
- if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
- (const unsigned char *)"tls13 ", 6) <= 0) {
- errno = ENOKEY;
- return -1;
- }
- if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
- (const unsigned char *)"HostNQN", 7) <= 0) {
+ if (hkdf_info_printf(ctx, "tls13 HostNQN") <= 0) {
errno = ENOKEY;
return -1;
}
- if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
- (const unsigned char *)hostnqn, strlen(hostnqn)) <= 0) {
+ if (hkdf_info_printf(ctx, "%s", hostnqn) <= 0) {
errno = ENOKEY;
return -1;
}
@@ -736,12 +769,13 @@ static int derive_retained_key(int hmac, const char *hostnqn,
*
* and the value '0' is invalid here.
*/
+
static int derive_tls_key(int version, unsigned char cipher,
const char *context, unsigned char *retained,
unsigned char *psk, size_t key_len)
{
_cleanup_evp_pkey_ctx_ EVP_PKEY_CTX *ctx = NULL;
- uint16_t length = key_len & 0xFFFF;
+ uint16_t length = htons(key_len & 0xFFFF);
const EVP_MD *md;
size_t hmac_len;
@@ -774,30 +808,24 @@ static int derive_tls_key(int version, unsigned char cipher,
errno = ENOKEY;
return -1;
}
- if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
- (const unsigned char *)"tls13 ", 6) <= 0) {
- errno = ENOKEY;
- return -1;
- }
- if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
- (const unsigned char *)"nvme-tls-psk", 12) <= 0) {
+ if (hkdf_info_printf(ctx, "tls13 nvme-tls-psk") <= 0) {
errno = ENOKEY;
return -1;
}
- if (version == 1) {
- char hash_str[5];
-
- sprintf(hash_str, "%02d ", cipher);
- if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
- (const unsigned char *)hash_str,
- strlen(hash_str)) <= 0) {
+ switch (version) {
+ case 0:
+ if (hkdf_info_printf(ctx, "%s", context) <= 0) {
errno = ENOKEY;
return -1;
}
- }
- if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
- (const unsigned char *)context,
- strlen(context)) <= 0) {
+ break;
+ case 1:
+ if (hkdf_info_printf(ctx, "%02d %s", cipher, context) <= 0) {
+ errno = ENOKEY;
+ return -1;
+ }
+ break;
+ default:
errno = ENOKEY;
return -1;
}
--
2.50.1
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-07-21 2:17 ` [PATCH 1/1] libnvme: TLS PSK derivation fixes Chris Leech
@ 2025-07-21 6:36 ` Hannes Reinecke
2025-07-21 15:31 ` Chris Leech
0 siblings, 1 reply; 22+ messages in thread
From: Hannes Reinecke @ 2025-07-21 6:36 UTC (permalink / raw)
To: Chris Leech, linux-nvme; +Cc: Daniel Wagner, Prashanth Nayak, John Meneghini
On 7/21/25 04:17, Chris Leech wrote:
> There are issues with the Retained and TLS PSK derivations due to the
> implementation not adhering to the RFC 8446 definition of the
> HKDF-Expand-Label function.
>
> 1) The 16-bit HkdfLabel.length value must be converted to network byte
> order.
>
> 2) The variable length HkdfLabel.label and HkdfLabel.context vectors
> must be prefixed with a length byte.
>
> Signed-off-by: Chris Leech <cleech@redhat.com>
> ---
> src/nvme/linux.c | 86 ++++++++++++++++++++++++++++++++----------------
> 1 file changed, 57 insertions(+), 29 deletions(-)
>
> diff --git a/src/nvme/linux.c b/src/nvme/linux.c
> index ae4aa526..674b20b5 100644
> --- a/src/nvme/linux.c
> +++ b/src/nvme/linux.c
> @@ -618,6 +618,46 @@ static DEFINE_CLEANUP_FUNC(
> cleanup_evp_pkey_ctx, EVP_PKEY_CTX *, EVP_PKEY_CTX_free)
> #define _cleanup_evp_pkey_ctx_ __cleanup__(cleanup_evp_pkey_ctx)
>
> +/*
> + * hkdf_info_printf()
> + *
> + * Helper function to append variable length label and context to an HkdfLabel
> + *
> + * RFC 8446 (TLS 1.3) Section 7.1 defines the HKDF-Expand-Label function as a
> + * specialization of the HKDF-Expand function (RFC 5869), where the info
> + * parameter is structured as an HkdfLabel.
> + *
> + * An HkdfLabel structure includes two variable length vectors (label and
> + * context) which must be preceded by their content length as per RFC 8446
> + * Section 3.4 (and not NUL terminated as per Section 7.1). Additionally,
> + * HkdfLabel.label must begin with "tls13 "
> + *
> + * Returns the number of bytes appended to the HKDF info buffer, or -1 on an
> + * error.
> + */
> +__attribute__((format(printf, 2, 3)))
> +static int hkdf_info_printf(EVP_PKEY_CTX *ctx, char *fmt, ...)
> +{
> + _cleanup_free_ char *str;
> + uint8_t len;
> + int ret;
> +
> + va_list myargs;
> + va_start(myargs, fmt);
> + ret = vasprintf(&str, fmt, myargs);
> + va_end(myargs);
> + if (ret < 0)
> + return ret;
> + if (ret > 255)
> + return -1;
> + len = ret;
> + if (EVP_PKEY_CTX_add1_hkdf_info(ctx, (unsigned char *)&len, 1) <= 0)
> + return -1;
> + if (EVP_PKEY_CTX_add1_hkdf_info(ctx, (unsigned char *)str, len) <= 0)
> + return -1;
> + return (ret + 1);
> +}
> +
> /*
> * derive_retained_key()
> *
> @@ -652,7 +692,7 @@ static int derive_retained_key(int hmac, const char *hostnqn,
> size_t key_len)
> {
> _cleanup_evp_pkey_ctx_ EVP_PKEY_CTX *ctx = NULL;
> - uint16_t length = key_len & 0xFFFF;
> + uint16_t length = htons(key_len & 0xFFFF);
> const EVP_MD *md;
> size_t hmac_len;
>
> @@ -690,18 +730,11 @@ static int derive_retained_key(int hmac, const char *hostnqn,
> errno = ENOKEY;
> return -1;
> }
> - if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
> - (const unsigned char *)"tls13 ", 6) <= 0) {
> - errno = ENOKEY;
> - return -1;
> - }
> - if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
> - (const unsigned char *)"HostNQN", 7) <= 0) {
> + if (hkdf_info_printf(ctx, "tls13 HostNQN") <= 0) {
> errno = ENOKEY;
> return -1;
> }
> - if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
> - (const unsigned char *)hostnqn, strlen(hostnqn)) <= 0) {
> + if (hkdf_info_printf(ctx, "%s", hostnqn) <= 0) {
> errno = ENOKEY;
> return -1;
> }
> @@ -736,12 +769,13 @@ static int derive_retained_key(int hmac, const char *hostnqn,
> *
> * and the value '0' is invalid here.
> */
> +
> static int derive_tls_key(int version, unsigned char cipher,
> const char *context, unsigned char *retained,
> unsigned char *psk, size_t key_len)
> {
> _cleanup_evp_pkey_ctx_ EVP_PKEY_CTX *ctx = NULL;
> - uint16_t length = key_len & 0xFFFF;
> + uint16_t length = htons(key_len & 0xFFFF);
> const EVP_MD *md;
> size_t hmac_len;
>
> @@ -774,30 +808,24 @@ static int derive_tls_key(int version, unsigned char cipher,
> errno = ENOKEY;
> return -1;
> }
> - if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
> - (const unsigned char *)"tls13 ", 6) <= 0) {
> - errno = ENOKEY;
> - return -1;
> - }
> - if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
> - (const unsigned char *)"nvme-tls-psk", 12) <= 0) {
> + if (hkdf_info_printf(ctx, "tls13 nvme-tls-psk") <= 0) {
> errno = ENOKEY;
> return -1;
> }
> - if (version == 1) {
> - char hash_str[5];
> -
> - sprintf(hash_str, "%02d ", cipher);
> - if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
> - (const unsigned char *)hash_str,
> - strlen(hash_str)) <= 0) {
> + switch (version) {
> + case 0:
> + if (hkdf_info_printf(ctx, "%s", context) <= 0) {
> errno = ENOKEY;
> return -1;
> }
> - }
> - if (EVP_PKEY_CTX_add1_hkdf_info(ctx,
> - (const unsigned char *)context,
> - strlen(context)) <= 0) {
> + break;
> + case 1:
> + if (hkdf_info_printf(ctx, "%02d %s", cipher, context) <= 0) {
> + errno = ENOKEY;
> + return -1;
> + }
> + break;
> + default:
> errno = ENOKEY;
> return -1;
> }
Hmm. I _thought_ we had it all fixed...
Reviewed-by: Hannes Reinecke <hare@suse.de>
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/1] NVMe/TLS connection issues to SPDK
2025-07-21 2:17 [PATCH 0/1] NVMe/TLS connection issues to SPDK Chris Leech
2025-07-21 2:17 ` [PATCH 1/1] libnvme: TLS PSK derivation fixes Chris Leech
@ 2025-07-21 7:11 ` Hannes Reinecke
2025-07-21 15:44 ` Chris Leech
2025-08-12 22:05 ` Chris Leech
2025-08-12 22:11 ` [RFC PATCH 1/2] crypto: hkdf: add hkdf_expand_label() Chris Leech
3 siblings, 1 reply; 22+ messages in thread
From: Hannes Reinecke @ 2025-07-21 7:11 UTC (permalink / raw)
To: Chris Leech, linux-nvme; +Cc: Daniel Wagner, Prashanth Nayak, John Meneghini
On 7/21/25 04:17, Chris Leech wrote:
> I was attempting to debug connecting the Linux driver / libnvme /
> ktls-utils host stack to the SPDK nvmf_tgt over TLS, and ran into some
> issues.
>
> The TLS connection fails to complete a handshake because the TLS PSKs
> are different. The NVMe/TCP specified key derivation steps from the
> configured interchange format, to a retained PSK and finally the TLS
> PSK, is implemented incompatibly in libnvme and SPDK. After some
> investigation, I believe the SPDK implementation to be correct and am
> providing a libnvme patch to match it. With libnvme modified, I see the
> TLS handshake complete in tlshd.
>
> (Note that this was tested using the obsolete "version 0" PSK Identity
> and TLS PSK derivation from the TCP transport 1.0 specification, as SPDK
> has not been updated with the "version 1" changes)
>
> The NVMe/TCP host driver then quickly fails when SPDK sends a TLS "New
> Session Ticket" message before ICResp.
>
> While possibly pointless due to the transport specification prohibition
> on session resumption and 0-RTT data, I don't think this is necessarily
> wrong and the host driver should be able to safely ignore it and
> continue.
>
Sigh. The neverending discussion.
Originally I didn't implement key update for NVMe as neither gnutls nor
the kernel supported it.
Turns out that key update was implemented last year for ktls, and gnutls
has support functions (gnutls_session_key_update()) for this, too.
So we _could_ start looking into it if nvme wouldn't be using the
->read_sock() interface.
This interface just works on skbs, and as such we can't easily get hold
on the TLS alert data required for figuring out if a New Session Ticket
message had been sent.
I do have a patchset for converting that into a recvmsg() based
implementation, but that got shelved as we didn't have an efficient
iterator for data hashing. With the crc32 update from Eric Biggers
we now should have an iterator for crc32 data hashing, so I
guess I could revisit that.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-07-21 6:36 ` Hannes Reinecke
@ 2025-07-21 15:31 ` Chris Leech
2025-07-25 9:36 ` Hannes Reinecke
0 siblings, 1 reply; 22+ messages in thread
From: Chris Leech @ 2025-07-21 15:31 UTC (permalink / raw)
To: Hannes Reinecke
Cc: linux-nvme, Daniel Wagner, Prashanth Nayak, John Meneghini
On Mon, Jul 21, 2025 at 08:36:01AM +0200, Hannes Reinecke wrote:
> On 7/21/25 04:17, Chris Leech wrote:
> > There are issues with the Retained and TLS PSK derivations due to the
> > implementation not adhering to the RFC 8446 definition of the
> > HKDF-Expand-Label function.
> > ...
> Hmm. I _thought_ we had it all fixed...
I went into this expecting/hoping to find the problem on the spdk side,
and I'd be happy to wrong here (especially if backed up by another
interoperable implementor).
The lack of a full example in the nvme/tcp transport spec to verify
implemenatations against is kind of a bummer.
- Chris
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/1] NVMe/TLS connection issues to SPDK
2025-07-21 7:11 ` [PATCH 0/1] NVMe/TLS connection issues to SPDK Hannes Reinecke
@ 2025-07-21 15:44 ` Chris Leech
2025-07-22 6:27 ` Hannes Reinecke
0 siblings, 1 reply; 22+ messages in thread
From: Chris Leech @ 2025-07-21 15:44 UTC (permalink / raw)
To: Hannes Reinecke
Cc: linux-nvme, Daniel Wagner, Prashanth Nayak, John Meneghini
On Mon, Jul 21, 2025 at 09:11:22AM +0200, Hannes Reinecke wrote:
> On 7/21/25 04:17, Chris Leech wrote:
> > ...
> > The NVMe/TCP host driver then quickly fails when SPDK sends a TLS "New
> > Session Ticket" message before ICResp.
> >
> > While possibly pointless due to the transport specification prohibition
> > on session resumption and 0-RTT data, I don't think this is necessarily
> > wrong and the host driver should be able to safely ignore it and
> > continue.
> >
> Sigh. The neverending discussion.
> Originally I didn't implement key update for NVMe as neither gnutls nor
> the kernel supported it.
> Turns out that key update was implemented last year for ktls, and gnutls
> has support functions (gnutls_session_key_update()) for this, too.
> So we _could_ start looking into it if nvme wouldn't be using the
> ->read_sock() interface.
> This interface just works on skbs, and as such we can't easily get hold
> on the TLS alert data required for figuring out if a New Session Ticket
> message had been sent.
I think this is a bit different, as it's NewSessionTicket and not
KeyUpdate. NewSessionTicket seems to provide an age-limited PSK for
session resumption, allowing additional connections or re-connections to
bypass an expensive certificate handshake. We're only using retained
PSKs and prohibited from session resumption, so while spdk probably
shouldn't be sending NewSessionTicket it should also safe for the host
to just filter it out of the data stream and never use it.
- Chris
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/1] NVMe/TLS connection issues to SPDK
2025-07-21 15:44 ` Chris Leech
@ 2025-07-22 6:27 ` Hannes Reinecke
2025-07-24 14:35 ` Daniel Wagner
0 siblings, 1 reply; 22+ messages in thread
From: Hannes Reinecke @ 2025-07-22 6:27 UTC (permalink / raw)
To: Chris Leech; +Cc: linux-nvme, Daniel Wagner, Prashanth Nayak, John Meneghini
On 7/21/25 17:44, Chris Leech wrote:
> On Mon, Jul 21, 2025 at 09:11:22AM +0200, Hannes Reinecke wrote:
>> On 7/21/25 04:17, Chris Leech wrote:
>>> ...
>>> The NVMe/TCP host driver then quickly fails when SPDK sends a TLS "New
>>> Session Ticket" message before ICResp.
>>>
>>> While possibly pointless due to the transport specification prohibition
>>> on session resumption and 0-RTT data, I don't think this is necessarily
>>> wrong and the host driver should be able to safely ignore it and
>>> continue.
>>>
>> Sigh. The neverending discussion.
>> Originally I didn't implement key update for NVMe as neither gnutls nor
>> the kernel supported it.
>> Turns out that key update was implemented last year for ktls, and gnutls
>> has support functions (gnutls_session_key_update()) for this, too.
>> So we _could_ start looking into it if nvme wouldn't be using the
>> ->read_sock() interface.
>> This interface just works on skbs, and as such we can't easily get hold
>> on the TLS alert data required for figuring out if a New Session Ticket
>> message had been sent.
>
> I think this is a bit different, as it's NewSessionTicket and not
> KeyUpdate. NewSessionTicket seems to provide an age-limited PSK for
> session resumption, allowing additional connections or re-connections to
> bypass an expensive certificate handshake. We're only using retained
> PSKs and prohibited from session resumption, so while spdk probably
> shouldn't be sending NewSessionTicket it should also safe for the host
> to just filter it out of the data stream and never use it.
>
Ah, the NewSessionTicket thingie. Yes, that's completely pointless for
NVMe, as we _never_ re-use tickets for session re-establishment.
We only ever have one session, so the use-case of several short-lived
sessions doesn't apply here.
But with the current ->read_sock based receive workflow we
cannot filter out TLS NewSession messages from the NVMe layer
as we don't have access to the TCP stream itself.
So either we switch to a recvmsg() based workflow or we
need to modify the read_sock() implementation in net/tls
to filter out the NewSession message there.
Personally I'd rather switch over to recvmsg() as I never
liked the read_sock() interface anyway.
But that's my personal opinion; others may have different ideas.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/1] NVMe/TLS connection issues to SPDK
2025-07-22 6:27 ` Hannes Reinecke
@ 2025-07-24 14:35 ` Daniel Wagner
2025-07-24 15:07 ` Chris Leech
0 siblings, 1 reply; 22+ messages in thread
From: Daniel Wagner @ 2025-07-24 14:35 UTC (permalink / raw)
To: Hannes Reinecke; +Cc: Chris Leech, linux-nvme, Prashanth Nayak, John Meneghini
On Tue, Jul 22, 2025 at 08:27:21AM +0200, Hannes Reinecke wrote:
> Ah, the NewSessionTicket thingie. Yes, that's completely pointless for
> NVMe, as we _never_ re-use tickets for session re-establishment.
> We only ever have one session, so the use-case of several short-lived
> sessions doesn't apply here.
Not following the discussion, can you either nacking this patch or give a
reviewed tag? Thanks.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/1] NVMe/TLS connection issues to SPDK
2025-07-24 14:35 ` Daniel Wagner
@ 2025-07-24 15:07 ` Chris Leech
2025-07-24 15:37 ` Daniel Wagner
0 siblings, 1 reply; 22+ messages in thread
From: Chris Leech @ 2025-07-24 15:07 UTC (permalink / raw)
To: Daniel Wagner
Cc: Hannes Reinecke, linux-nvme, Prashanth Nayak, John Meneghini
On Thu, Jul 24, 2025 at 7:35 AM Daniel Wagner <dwagner@suse.de> wrote:
>
> On Tue, Jul 22, 2025 at 08:27:21AM +0200, Hannes Reinecke wrote:
> > Ah, the NewSessionTicket thingie. Yes, that's completely pointless for
> > NVMe, as we _never_ re-use tickets for session re-establishment.
> > We only ever have one session, so the use-case of several short-lived
> > sessions doesn't apply here.
>
> Not following the discussion, can you either nacking this patch or give a
> reviewed tag? Thanks.
Hi Daniel,
I believe Hannes has given a reviewed tag on the libvnme patch. The
discussion on the cover letter is about future work on the kernel
side.
If it would help I can capture that tag and then submit this as a github PR.
Actually, I think I might be missing an explicit NULL assignment on
the declaration of *str to keep _cleanup_free_ safe?
-Chris
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/1] NVMe/TLS connection issues to SPDK
2025-07-24 15:07 ` Chris Leech
@ 2025-07-24 15:37 ` Daniel Wagner
0 siblings, 0 replies; 22+ messages in thread
From: Daniel Wagner @ 2025-07-24 15:37 UTC (permalink / raw)
To: Chris Leech; +Cc: Hannes Reinecke, linux-nvme, Prashanth Nayak, John Meneghini
Hi Chris,
On Thu, Jul 24, 2025 at 08:07:03AM -0700, Chris Leech wrote:
> I believe Hannes has given a reviewed tag on the libvnme patch. The
> discussion on the cover letter is about future work on the kernel
> side.
Ah, thanks. Trimming emails would help.
> If it would help I can capture that tag and then submit this as a
> github PR.
The PR has the advantage that a CI build gets triggered. Submissions on
the mailing list are usually in good shape. Submissions via GitHub vary
quite a bit. I’m fine either way. :)
> Actually, I think I might be missing an explicit NULL assignment on
> the declaration of *str to keep _cleanup_free_ safe?
For _cleanup_free_ to work, the variable needs to be initialized (either
NULL or a valid pointer) before the block scope is left. Otherwise the
free sees a random stack value.
Thanks,
Daniel
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-07-21 15:31 ` Chris Leech
@ 2025-07-25 9:36 ` Hannes Reinecke
2025-07-25 18:08 ` Chris Leech
0 siblings, 1 reply; 22+ messages in thread
From: Hannes Reinecke @ 2025-07-25 9:36 UTC (permalink / raw)
To: Chris Leech; +Cc: linux-nvme, Daniel Wagner, Prashanth Nayak, John Meneghini
On 7/21/25 17:31, Chris Leech wrote:
> On Mon, Jul 21, 2025 at 08:36:01AM +0200, Hannes Reinecke wrote:
>> On 7/21/25 04:17, Chris Leech wrote:
>>> There are issues with the Retained and TLS PSK derivations due to the
>>> implementation not adhering to the RFC 8446 definition of the
>>> HKDF-Expand-Label function.
>>> ...
>> Hmm. I _thought_ we had it all fixed...
>
> I went into this expecting/hoping to find the problem on the spdk side,
> and I'd be happy to wrong here (especially if backed up by another
> interoperable implementor).
>
> The lack of a full example in the nvme/tcp transport spec to verify
> implemenatations against is kind of a bummer.
>
Okay, seems that you have been right after all.
I've re-read RFC 8466 and indeed you seem to be right about
the encoding of variable length vectors (cf RFC 8446 section 3.4).
So I guess we need to take this patch after all.
_However_: PSKs generated with after applying this patch will be
different than those prior to this patch.
Consequently there will be interop issues with existing implementations
(which will use the original encoding).
I guess we would need to wait for the target implementations to be fixed
or introduce a flag switching to the old / compat implementation to
avoid interop issues.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-07-25 9:36 ` Hannes Reinecke
@ 2025-07-25 18:08 ` Chris Leech
2025-07-28 7:12 ` Hannes Reinecke
0 siblings, 1 reply; 22+ messages in thread
From: Chris Leech @ 2025-07-25 18:08 UTC (permalink / raw)
To: Hannes Reinecke
Cc: linux-nvme, Daniel Wagner, Prashanth Nayak, John Meneghini
On Fri, Jul 25, 2025 at 11:36:20AM +0200, Hannes Reinecke wrote:
> On 7/21/25 17:31, Chris Leech wrote:
> > On Mon, Jul 21, 2025 at 08:36:01AM +0200, Hannes Reinecke wrote:
> > > On 7/21/25 04:17, Chris Leech wrote:
> > > > There are issues with the Retained and TLS PSK derivations due to the
> > > > implementation not adhering to the RFC 8446 definition of the
> > > > HKDF-Expand-Label function.
> > > > ...
> > > Hmm. I _thought_ we had it all fixed...
> >
> > I went into this expecting/hoping to find the problem on the spdk side,
> > and I'd be happy to wrong here (especially if backed up by another
> > interoperable implementor).
> >
> > The lack of a full example in the nvme/tcp transport spec to verify
> > implemenatations against is kind of a bummer.
> >
> Okay, seems that you have been right after all.
> I've re-read RFC 8466 and indeed you seem to be right about
> the encoding of variable length vectors (cf RFC 8446 section 3.4).
>
> So I guess we need to take this patch after all.
So we're back to agreeing on the correctness of the changes?
Honestly, I appreciate the scrutiny.
> _However_: PSKs generated with after applying this patch will be
> different than those prior to this patch.
> Consequently there will be interop issues with existing implementations
> (which will use the original encoding).
>
> I guess we would need to wait for the target implementations to be fixed
> or introduce a flag switching to the old / compat implementation to avoid
> interop issues.
Yes, it is a breaking change for compatibility with other out-of-spec
implementations. I'm hesitant to carry a flag for that, but it wouldn't
be too much trouble to implement.
Just don't touch your keyfile? The tls-key import/export commands are
operating on the final derived TLS PSKs right?
- Chris
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-07-25 18:08 ` Chris Leech
@ 2025-07-28 7:12 ` Hannes Reinecke
2025-08-08 16:18 ` John Meneghini
2025-08-12 4:33 ` Chris Leech
0 siblings, 2 replies; 22+ messages in thread
From: Hannes Reinecke @ 2025-07-28 7:12 UTC (permalink / raw)
To: Chris Leech; +Cc: linux-nvme, Daniel Wagner, Prashanth Nayak, John Meneghini
On 7/25/25 20:08, Chris Leech wrote:
> On Fri, Jul 25, 2025 at 11:36:20AM +0200, Hannes Reinecke wrote:
>> On 7/21/25 17:31, Chris Leech wrote:
>>> On Mon, Jul 21, 2025 at 08:36:01AM +0200, Hannes Reinecke wrote:
>>>> On 7/21/25 04:17, Chris Leech wrote:
>>>>> There are issues with the Retained and TLS PSK derivations due to the
>>>>> implementation not adhering to the RFC 8446 definition of the
>>>>> HKDF-Expand-Label function.
>>>>> ...
>>>> Hmm. I _thought_ we had it all fixed...
>>>
>>> I went into this expecting/hoping to find the problem on the spdk side,
>>> and I'd be happy to wrong here (especially if backed up by another
>>> interoperable implementor).
>>>
>>> The lack of a full example in the nvme/tcp transport spec to verify
>>> implemenatations against is kind of a bummer.
>>>
>> Okay, seems that you have been right after all.
>> I've re-read RFC 8466 and indeed you seem to be right about
>> the encoding of variable length vectors (cf RFC 8446 section 3.4).
>>
>> So I guess we need to take this patch after all.
>
> So we're back to agreeing on the correctness of the changes?
> Honestly, I appreciate the scrutiny.
>
Yeah, sorry. I indeed misread (or rather ignored) section 3 from the
RFC when coding the key derivation.
You are correct, and we need to prefix each string with the length.
But: this is an incompatible change. Once we integrate this patch
the keyring will end up with a _different_ derived PSK than it would
without that patch. So the _same_ PSK (in interchange format) will
result in different PSKs in the keyring, causing the handshake to
fail if the other side is not aware of that.
>> _However_: PSKs generated with after applying this patch will be
>> different than those prior to this patch.
>> Consequently there will be interop issues with existing implementations
>> (which will use the original encoding).
>>
>> I guess we would need to wait for the target implementations to be fixed
>> or introduce a flag switching to the old / compat implementation to avoid
>> interop issues.
>
> Yes, it is a breaking change for compatibility with other out-of-spec
> implementations. I'm hesitant to carry a flag for that, but it wouldn't
> be too much trouble to implement.
>
> Just don't touch your keyfile? The tls-key import/export commands are
> operating on the final derived TLS PSKs right?
>
That might be the way out, and in fact what we should be doing.
However, the fact still remains: after this patch has been applied
one _cannot_ generate new PSKs until the other side has been updated,
too.
For linux it's easy, we can just require 'version xyz' from libnvme
and the issue is solved. But for other implementations?
There are IHVs out there who already shipped their products with TLS
enablement, and they need to be fixed, too.
And their timeline is vastly longer than ours.
So to avoid us having to synchronize against all of the others I think
it might be easier to add a 'compat' flag of sorts to generate PSKs
with the 'original' derivation algorithm, and then increase the
libnvme version number once it's in.
Then we can point the IHVs to that number so that they reference
that version once their firmware is updated.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-07-28 7:12 ` Hannes Reinecke
@ 2025-08-08 16:18 ` John Meneghini
2025-08-12 4:33 ` Chris Leech
1 sibling, 0 replies; 22+ messages in thread
From: John Meneghini @ 2025-08-08 16:18 UTC (permalink / raw)
To: Hannes Reinecke, Chris Leech; +Cc: linux-nvme, Daniel Wagner, Prashanth Nayak
On 7/28/25 3:12 AM, Hannes Reinecke wrote:
> So to avoid us having to synchronize against all of the others I think
> it might be easier to add a 'compat' flag of sorts to generate PSKs
> with the 'original' derivation algorithm, and then increase the
> libnvme version number once it's in.
> Then we can point the IHVs to that number so that they reference
> that version once their firmware is updated.
This is a really bad precedent. Do you really want to create a new QUIK nightmare for NVMe/TLS?
If you add a compat flag they you will NEVER be able to get rid of it.
We should fix this correctly now and break backwards compatibility. This will force implementations to do it right.
Do it now or it will never happen.
/John
>
> Cheers,
>
> Hannes
> --
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-07-28 7:12 ` Hannes Reinecke
2025-08-08 16:18 ` John Meneghini
@ 2025-08-12 4:33 ` Chris Leech
2025-08-18 9:42 ` Hannes Reinecke
2025-08-20 8:10 ` Daniel Wagner
1 sibling, 2 replies; 22+ messages in thread
From: Chris Leech @ 2025-08-12 4:33 UTC (permalink / raw)
To: Hannes Reinecke
Cc: linux-nvme, Daniel Wagner, Prashanth Nayak, John Meneghini
On Mon, Jul 28, 2025 at 09:12:15AM +0200, Hannes Reinecke wrote:
> ...
> But: this is an incompatible change. Once we integrate this patch
> the keyring will end up with a _different_ derived PSK than it would
> without that patch. So the _same_ PSK (in interchange format) will
> result in different PSKs in the keyring, causing the handshake to
> fail if the other side is not aware of that.
>
> > > _However_: PSKs generated with after applying this patch will be
> > > different than those prior to this patch.
> > > Consequently there will be interop issues with existing implementations
> > > (which will use the original encoding).
> > >
> > > I guess we would need to wait for the target implementations to be fixed
> > > or introduce a flag switching to the old / compat implementation to avoid
> > > interop issues.
> >
> > Yes, it is a breaking change for compatibility with other out-of-spec
> > implementations. I'm hesitant to carry a flag for that, but it wouldn't
> > be too much trouble to implement.
> >
> > Just don't touch your keyfile? The tls-key import/export commands are
> > operating on the final derived TLS PSKs right?
> >
> That might be the way out, and in fact what we should be doing.
> However, the fact still remains: after this patch has been applied
> one _cannot_ generate new PSKs until the other side has been updated,
> too.
> For linux it's easy, we can just require 'version xyz' from libnvme
> and the issue is solved. But for other implementations?
> There are IHVs out there who already shipped their products with TLS
> enablement, and they need to be fixed, too.
> And their timeline is vastly longer than ours.
>
> So to avoid us having to synchronize against all of the others I think
> it might be easier to add a 'compat' flag of sorts to generate PSKs
> with the 'original' derivation algorithm, and then increase the
> libnvme version number once it's in.
> Then we can point the IHVs to that number so that they reference
> that version once their firmware is updated.
I've spent a bit of time on this, and while I don't have patches ready
to share just yet I do think it's worth commenting on the scope of the
change to keep the existing functionality behind a cli switch.
libnvme:
- keeping both sets of key derivation functions (this is not bad)
- need to expose that functionality, so new exported APIs
(I was thinking something like a flag field over a legacy-only function)
- #define NVME_TLS_PSK_LEGACY (1u << 0)
- char *nvme_generate_tls_key_identity_2(..., unsigned int flags)
- long nvme_insert_tls_key_versioned_2(..., unsigned int flags)
- support paths that load keys through nvme_fabrics_cfg [1]
(for nvme-cli fabrics commands that take --tls_key)
- add some sort of flag there, like nvme_fabrics_cfg.tls_legacy
- add to merge_config, nvmf_update_config, build_options,
supported_options, etc.
- the JSON schema/code would need to support this for persistent
configurations
nvme-cli:
- add a command line flag to keyring command functions
- convert to use of new libnvme APIs to pass flag
- gen_tls_key, check_tls_key
(I don't think nvme_tls_key does key conversions)
- add command line flag to generic parsing into nvme_fabrics_cfg
- handles connect, connect-all, discover, discover-all
- man pages, shell completions, etc.
It may be possible to keep this to just in libnvme and switch on an
environment variable, but that sounds like just as big of a support
nightmare as remembering to pass an option to every command.
If we don't have a resolution by then, maybe this is something we can
resolve at ALPSS.
[1] Side note; with nvme_fabrics_cfg _not_ being allocated through
libnvme, I don't see how the various additions there can be ABI safe. It
looks to me like this struct alone broke ABI in libnvme 1.4, 1.8, 1.9,
and 1.11.
- Chris
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/1] NVMe/TLS connection issues to SPDK
2025-07-21 2:17 [PATCH 0/1] NVMe/TLS connection issues to SPDK Chris Leech
2025-07-21 2:17 ` [PATCH 1/1] libnvme: TLS PSK derivation fixes Chris Leech
2025-07-21 7:11 ` [PATCH 0/1] NVMe/TLS connection issues to SPDK Hannes Reinecke
@ 2025-08-12 22:05 ` Chris Leech
2025-08-12 22:11 ` [RFC PATCH 1/2] crypto: hkdf: add hkdf_expand_label() Chris Leech
3 siblings, 0 replies; 22+ messages in thread
From: Chris Leech @ 2025-08-12 22:05 UTC (permalink / raw)
To: linux-nvme; +Cc: Hannes Reinecke, Daniel Wagner, John Meneghini
On Sun, Jul 20, 2025 at 07:17:17PM -0700, Chris Leech wrote:
> I was attempting to debug connecting the Linux driver / libnvme /
> ktls-utils host stack to the SPDK nvmf_tgt over TLS, and ran into some
> issues.
>
> The TLS connection fails to complete a handshake because the TLS PSKs
> are different. The NVMe/TCP specified key derivation steps from the
> configured interchange format, to a retained PSK and finally the TLS
> PSK, is implemented incompatibly in libnvme and SPDK. After some
> investigation, I believe the SPDK implementation to be correct and am
> providing a libnvme patch to match it. With libnvme modified, I see the
> TLS handshake complete in tlshd.
Ug, the kernel has key derivation code for secure channel concatenation
and it has the same issue.
RFC patchs to follow, only tested with a quick blktests nvme/063
currently.
- Chris
^ permalink raw reply [flat|nested] 22+ messages in thread
* [RFC PATCH 1/2] crypto: hkdf: add hkdf_expand_label()
2025-07-21 2:17 [PATCH 0/1] NVMe/TLS connection issues to SPDK Chris Leech
` (2 preceding siblings ...)
2025-08-12 22:05 ` Chris Leech
@ 2025-08-12 22:11 ` Chris Leech
2025-08-18 9:44 ` Hannes Reinecke
3 siblings, 1 reply; 22+ messages in thread
From: Chris Leech @ 2025-08-12 22:11 UTC (permalink / raw)
To: linux-nvme; +Cc: Hannes Reinecke, Daniel Wagner, John Meneghini
Provide an implementation of RFC 8446 (TLS 1.3) HKDF-Expand-Label
Signed-off-by: Chris Leech <cleech@redhat.com>
---
crypto/hkdf.c | 55 +++++++++++++++++++++++++++++++++++++++++++
include/crypto/hkdf.h | 4 ++++
2 files changed, 59 insertions(+)
diff --git a/crypto/hkdf.c b/crypto/hkdf.c
index 82d1b32ca6ce4..465bad6e6c93e 100644
--- a/crypto/hkdf.c
+++ b/crypto/hkdf.c
@@ -11,6 +11,7 @@
#include <crypto/sha2.h>
#include <crypto/hkdf.h>
#include <linux/module.h>
+#include <linux/unaligned.h>
/*
* HKDF consists of two steps:
@@ -129,6 +130,60 @@ int hkdf_expand(struct crypto_shash *hmac_tfm,
}
EXPORT_SYMBOL_GPL(hkdf_expand);
+/**
+ * hkdf_expand_label - HKDF-Expand-Label (RFC 8846 section 7.1)
+ * @hmac_tfm: hash context keyed with pseudorandom key
+ * @label: ASCII label without "tls13 " prefix
+ * @label_len: length of @label
+ * @context: context bytes
+ * @contextlen: length of @context
+ * @okm: output keying material
+ * @okmlen: length of @okm
+ *
+ * Build the TLS 1.3 HkdfLabel structure and invoke hkdf_expand().
+ *
+ * Returns 0 on success with output keying material stored in @okm,
+ * or a negative errno value otherwise.
+ */
+int hkdf_expand_label(struct crypto_shash *hmac_tfm,
+ const u8 *label, unsigned int labellen,
+ const u8 *context, unsigned int contextlen,
+ u8 *okm, unsigned int okmlen)
+{
+ int err;
+ u8 *info;
+ unsigned int infolen;
+ static const char tls13_prefix[] = "tls13 ";
+ unsigned int prefixlen = sizeof(tls13_prefix) - 1; /* exclude NUL */
+
+ if (WARN_ON(labellen > (255 - prefixlen)))
+ return -EINVAL;
+ if (WARN_ON(contextlen > 255))
+ return -EINVAL;
+
+ infolen = 2 + (1 + prefixlen + labellen) + (1 + contextlen);
+ info = kzalloc(infolen, GFP_KERNEL);
+ if (!info)
+ return -ENOMEM;
+
+ /* HkdfLabel.Length */
+ put_unaligned_be16(okmlen, info);
+
+ /* HkdfLabel.Label */
+ info[2] = prefixlen + labellen;
+ memcpy(info + 3, tls13_prefix, prefixlen);
+ memcpy(info + 3 + prefixlen, label, labellen);
+
+ /* HkdfLabel.Context */
+ info[3 + prefixlen + labellen] = contextlen;
+ memcpy(info + 4 + prefixlen + labellen, context, contextlen);
+
+ err = hkdf_expand(hmac_tfm, info, infolen, okm, okmlen);
+ kfree_sensitive(info);
+ return err;
+}
+EXPORT_SYMBOL_GPL(hkdf_expand_label);
+
struct hkdf_testvec {
const char *test;
const u8 *ikm;
diff --git a/include/crypto/hkdf.h b/include/crypto/hkdf.h
index 6a9678f508f5d..5e75d17a58abe 100644
--- a/include/crypto/hkdf.h
+++ b/include/crypto/hkdf.h
@@ -17,4 +17,8 @@ int hkdf_extract(struct crypto_shash *hmac_tfm, const u8 *ikm,
int hkdf_expand(struct crypto_shash *hmac_tfm,
const u8 *info, unsigned int infolen,
u8 *okm, unsigned int okmlen);
+int hkdf_expand_label(struct crypto_shash *hmac_tfm,
+ const u8 *label, unsigned int labellen,
+ const u8 *context, unsigned int contextlen,
+ u8 *okm, unsigned int okmlen);
#endif
--
2.49.0
From 17e645b802bd61b4f3182195c15935e2fa1155ad Mon Sep 17 00:00:00 2001
From: Chris Leech <cleech@redhat.com>
Date: Tue, 12 Aug 2025 12:41:08 -0700
Subject: [RFC PATCH 2/2] nvme-auth: use hkdf_expand_label()
When generating keying material during an authentication transaction
(secure channel concatenation), the HKDF-Expand-Label function is part
of the specified key derivation process.
The current open-coded implementation misses the length prefix
requirements on the HkdfLabel label and context variable-length vectors
(RFC 8446 Section 3.4).
Instead, use the hkdf_expand_label() function.
Signed-off-by: Chris Leech <cleech@redhat.com>
---
drivers/nvme/common/auth.c | 34 ++++++++++++++--------------------
1 file changed, 14 insertions(+), 20 deletions(-)
diff --git a/drivers/nvme/common/auth.c b/drivers/nvme/common/auth.c
index 91e273b89fea3..bfa6f0a82d3e9 100644
--- a/drivers/nvme/common/auth.c
+++ b/drivers/nvme/common/auth.c
@@ -715,10 +715,10 @@ int nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len,
{
struct crypto_shash *hmac_tfm;
const char *hmac_name;
- const char *psk_prefix = "tls13 nvme-tls-psk";
static const char default_salt[HKDF_MAX_HASHLEN];
- size_t info_len, prk_len;
- char *info;
+ size_t prk_len;
+ const char *label = "nvme-tls-psk";
+ const char *ctx;
unsigned char *prk, *tls_key;
int ret;
@@ -758,36 +758,30 @@ int nvme_auth_derive_tls_psk(int hmac_id, u8 *psk, size_t psk_len,
if (ret)
goto out_free_prk;
- /*
- * 2 additional bytes for the length field from HDKF-Expand-Label,
- * 2 additional bytes for the HMAC ID, and one byte for the space
- * separator.
- */
- info_len = strlen(psk_digest) + strlen(psk_prefix) + 5;
- info = kzalloc(info_len + 1, GFP_KERNEL);
- if (!info) {
+ /* Context is ASCII: "<hash-id-two-digits> <base64-digest>" */
+ ctx = kasprintf(GFP_KERNEL, "%02d %s", hmac_id, psk_digest);
+ if (!ctx) {
ret = -ENOMEM;
goto out_free_prk;
}
- put_unaligned_be16(psk_len, info);
- memcpy(info + 2, psk_prefix, strlen(psk_prefix));
- sprintf(info + 2 + strlen(psk_prefix), "%02d %s", hmac_id, psk_digest);
-
tls_key = kzalloc(psk_len, GFP_KERNEL);
if (!tls_key) {
ret = -ENOMEM;
- goto out_free_info;
+ goto out_free_ctx;
}
- ret = hkdf_expand(hmac_tfm, info, info_len, tls_key, psk_len);
+ ret = hkdf_expand_label(hmac_tfm,
+ label, strlen(label),
+ ctx, strlen(ctx),
+ tls_key, psk_len);
if (ret) {
kfree(tls_key);
- goto out_free_info;
+ goto out_free_ctx;
}
*ret_psk = tls_key;
-out_free_info:
- kfree(info);
+out_free_ctx:
+ kfree(ctx);
out_free_prk:
kfree(prk);
out_free_shash:
--
2.49.0
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-08-12 4:33 ` Chris Leech
@ 2025-08-18 9:42 ` Hannes Reinecke
2025-08-20 8:10 ` Daniel Wagner
1 sibling, 0 replies; 22+ messages in thread
From: Hannes Reinecke @ 2025-08-18 9:42 UTC (permalink / raw)
To: Chris Leech; +Cc: linux-nvme, Daniel Wagner, Prashanth Nayak, John Meneghini
On 8/12/25 06:33, Chris Leech wrote:
> On Mon, Jul 28, 2025 at 09:12:15AM +0200, Hannes Reinecke wrote:
>> ...
>> But: this is an incompatible change. Once we integrate this patch
>> the keyring will end up with a _different_ derived PSK than it would
>> without that patch. So the _same_ PSK (in interchange format) will
>> result in different PSKs in the keyring, causing the handshake to
>> fail if the other side is not aware of that.
>>
>>>> _However_: PSKs generated with after applying this patch will be
>>>> different than those prior to this patch.
>>>> Consequently there will be interop issues with existing implementations
>>>> (which will use the original encoding).
>>>>
>>>> I guess we would need to wait for the target implementations to be fixed
>>>> or introduce a flag switching to the old / compat implementation to avoid
>>>> interop issues.
>>>
>>> Yes, it is a breaking change for compatibility with other out-of-spec
>>> implementations. I'm hesitant to carry a flag for that, but it wouldn't
>>> be too much trouble to implement.
>>>
>>> Just don't touch your keyfile? The tls-key import/export commands are
>>> operating on the final derived TLS PSKs right?
>>>
>> That might be the way out, and in fact what we should be doing.
>> However, the fact still remains: after this patch has been applied
>> one _cannot_ generate new PSKs until the other side has been updated,
>> too.
>> For linux it's easy, we can just require 'version xyz' from libnvme
>> and the issue is solved. But for other implementations?
>> There are IHVs out there who already shipped their products with TLS
>> enablement, and they need to be fixed, too.
>> And their timeline is vastly longer than ours.
>>
>> So to avoid us having to synchronize against all of the others I think
>> it might be easier to add a 'compat' flag of sorts to generate PSKs
>> with the 'original' derivation algorithm, and then increase the
>> libnvme version number once it's in.
>> Then we can point the IHVs to that number so that they reference
>> that version once their firmware is updated.
>
> I've spent a bit of time on this, and while I don't have patches ready
> to share just yet I do think it's worth commenting on the scope of the
> change to keep the existing functionality behind a cli switch.
>
> libnvme:
> - keeping both sets of key derivation functions (this is not bad)
> - need to expose that functionality, so new exported APIs
> (I was thinking something like a flag field over a legacy-only function)
> - #define NVME_TLS_PSK_LEGACY (1u << 0)
> - char *nvme_generate_tls_key_identity_2(..., unsigned int flags)
> - long nvme_insert_tls_key_versioned_2(..., unsigned int flags)
>
> - support paths that load keys through nvme_fabrics_cfg [1]
> (for nvme-cli fabrics commands that take --tls_key)
> - add some sort of flag there, like nvme_fabrics_cfg.tls_legacy
> - add to merge_config, nvmf_update_config, build_options,
> supported_options, etc.
>
> - the JSON schema/code would need to support this for persistent
> configurations
>
> nvme-cli:
> - add a command line flag to keyring command functions
> - convert to use of new libnvme APIs to pass flag
> - gen_tls_key, check_tls_key
> (I don't think nvme_tls_key does key conversions)
>
> - add command line flag to generic parsing into nvme_fabrics_cfg
> - handles connect, connect-all, discover, discover-all
>
> - man pages, shell completions, etc.
>
> It may be possible to keep this to just in libnvme and switch on an
> environment variable, but that sounds like just as big of a support
> nightmare as remembering to pass an option to every command.
>
> If we don't have a resolution by then, maybe this is something we can
> resolve at ALPSS.
>
> [1] Side note; with nvme_fabrics_cfg _not_ being allocated through
> libnvme, I don't see how the various additions there can be ABI safe. It
> looks to me like this struct alone broke ABI in libnvme 1.4, 1.8, 1.9,
> and 1.11.
>
My thoughts are slightly different.
I do agree that we need to change the default from nvme-cli to use the
correct key derivation.
But for compat we should be using the 'tls-key' function, and add a
'compat' flag there. That way we don't need to modify 'gen-tls-key'
or 'check-tls-key', and have a single place to handle compat issues.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC PATCH 1/2] crypto: hkdf: add hkdf_expand_label()
2025-08-12 22:11 ` [RFC PATCH 1/2] crypto: hkdf: add hkdf_expand_label() Chris Leech
@ 2025-08-18 9:44 ` Hannes Reinecke
0 siblings, 0 replies; 22+ messages in thread
From: Hannes Reinecke @ 2025-08-18 9:44 UTC (permalink / raw)
To: Chris Leech, linux-nvme; +Cc: Daniel Wagner, John Meneghini
On 8/13/25 00:11, Chris Leech wrote:
> Provide an implementation of RFC 8446 (TLS 1.3) HKDF-Expand-Label
>
Looks good, but patches are garbled.
Please re-post as a separate patchset with a cover letter.
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-08-12 4:33 ` Chris Leech
2025-08-18 9:42 ` Hannes Reinecke
@ 2025-08-20 8:10 ` Daniel Wagner
2025-08-20 8:22 ` Hannes Reinecke
1 sibling, 1 reply; 22+ messages in thread
From: Daniel Wagner @ 2025-08-20 8:10 UTC (permalink / raw)
To: Chris Leech; +Cc: Hannes Reinecke, linux-nvme, Prashanth Nayak, John Meneghini
On Mon, Aug 11, 2025 at 09:33:06PM -0700, Chris Leech wrote:
> If we don't have a resolution by then, maybe this is something we can
> resolve at ALPSS.
>
> [1] Side note; with nvme_fabrics_cfg _not_ being allocated through
> libnvme, I don't see how the various additions there can be ABI safe. It
> looks to me like this struct alone broke ABI in libnvme 1.4, 1.8, 1.9,
> and 1.11.
I know and I was hopping it wouldn't cause troubles... nvme_fabrics_cfg
is a bit of a problem. I was planing to address this for v2.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-08-20 8:10 ` Daniel Wagner
@ 2025-08-20 8:22 ` Hannes Reinecke
2025-08-26 14:09 ` John Meneghini
0 siblings, 1 reply; 22+ messages in thread
From: Hannes Reinecke @ 2025-08-20 8:22 UTC (permalink / raw)
To: Daniel Wagner, Chris Leech; +Cc: linux-nvme, Prashanth Nayak, John Meneghini
On 8/20/25 10:10, Daniel Wagner wrote:
> On Mon, Aug 11, 2025 at 09:33:06PM -0700, Chris Leech wrote:
>> If we don't have a resolution by then, maybe this is something we can
>> resolve at ALPSS.
>>
>> [1] Side note; with nvme_fabrics_cfg _not_ being allocated through
>> libnvme, I don't see how the various additions there can be ABI safe. It
>> looks to me like this struct alone broke ABI in libnvme 1.4, 1.8, 1.9,
>> and 1.11.
>
> I know and I was hopping it wouldn't cause troubles... nvme_fabrics_cfg
> is a bit of a problem. I was planing to address this for v2.
In a similar vein: shouldn't we have a session at ALPSS around
libnvme v2?
Or, if not a session, maybe a beer topic?
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/1] libnvme: TLS PSK derivation fixes
2025-08-20 8:22 ` Hannes Reinecke
@ 2025-08-26 14:09 ` John Meneghini
0 siblings, 0 replies; 22+ messages in thread
From: John Meneghini @ 2025-08-26 14:09 UTC (permalink / raw)
To: Hannes Reinecke, Daniel Wagner, Chris Leech; +Cc: linux-nvme, Prashanth Nayak
On 8/20/25 4:22 AM, Hannes Reinecke wrote:
> On 8/20/25 10:10, Daniel Wagner wrote:
>> On Mon, Aug 11, 2025 at 09:33:06PM -0700, Chris Leech wrote:
>>> If we don't have a resolution by then, maybe this is something we can
>>> resolve at ALPSS.
>>>
>>> [1] Side note; with nvme_fabrics_cfg _not_ being allocated through
>>> libnvme, I don't see how the various additions there can be ABI safe. It
>>> looks to me like this struct alone broke ABI in libnvme 1.4, 1.8, 1.9,
>>> and 1.11.
>>
>> I know and I was hopping it wouldn't cause troubles... nvme_fabrics_cfg
>> is a bit of a problem. I was planing to address this for v2.
>
> In a similar vein: shouldn't we have a session at ALPSS around
> libnvme v2?
Yes, both Chris and I have proposed a talk to discuss NVMe/TLS at ALPSS.
> Or, if not a session, maybe a beer topic?
Sure, you can drink a beer when we talk about it. ;-)
/John
> Cheers,
>
> Hannes
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2025-08-26 14:37 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-07-21 2:17 [PATCH 0/1] NVMe/TLS connection issues to SPDK Chris Leech
2025-07-21 2:17 ` [PATCH 1/1] libnvme: TLS PSK derivation fixes Chris Leech
2025-07-21 6:36 ` Hannes Reinecke
2025-07-21 15:31 ` Chris Leech
2025-07-25 9:36 ` Hannes Reinecke
2025-07-25 18:08 ` Chris Leech
2025-07-28 7:12 ` Hannes Reinecke
2025-08-08 16:18 ` John Meneghini
2025-08-12 4:33 ` Chris Leech
2025-08-18 9:42 ` Hannes Reinecke
2025-08-20 8:10 ` Daniel Wagner
2025-08-20 8:22 ` Hannes Reinecke
2025-08-26 14:09 ` John Meneghini
2025-07-21 7:11 ` [PATCH 0/1] NVMe/TLS connection issues to SPDK Hannes Reinecke
2025-07-21 15:44 ` Chris Leech
2025-07-22 6:27 ` Hannes Reinecke
2025-07-24 14:35 ` Daniel Wagner
2025-07-24 15:07 ` Chris Leech
2025-07-24 15:37 ` Daniel Wagner
2025-08-12 22:05 ` Chris Leech
2025-08-12 22:11 ` [RFC PATCH 1/2] crypto: hkdf: add hkdf_expand_label() Chris Leech
2025-08-18 9:44 ` Hannes Reinecke
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).