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

[Chris Leech <cleech@redhat.com>]

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