[PATCH 1/1] libnvme: TLS PSK derivation fixes

[Hannes Reinecke]

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 at 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