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

[Hannes Reinecke]

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