Persistent TLS keys take #2

[hreinecke]

Hey all,

this is my next attempt to implement persistent TLS key handling. The original idea of storing the key serial number in the JSON configuration file doesn't really work out as the key serial number is ephemeral, and might change whenever a key is updated/revoked or somesuch. So after a reboot the key won't be present, the we won't be able to load the keys at all.
With the pull request we now store the TLS keys in 'PSK interchange format' as mandated in the NVMe TCP transport specification, which then contains the actual key data. And, obviously, the parser has been updated to read the TLS keys
from the PSK interchange format, too.
The original pull request #783 would export all keys via the configuration file, thereby losing the distinction between keys selected from the user (ie via the '--tls_key' option) and keys auto-selected during TLS handshake (when only '--tls' was specified. To fix this this PR now also adds the functions nvme_scan_tls_keys() such that nvme-cli can add an 'export' and 'import' functionality for TLS keys.

[igaw]

Expect these couple nitpicks it looks good. I've to read up on our discussion about the ideas from @mwilck about the prefixing tls_key

[hreinecke]

I'd rather not go that way. Prefixing it will be just adding complexity for no little gain. Especially as we don't have any real exploiter for any of these prefixes. My idea here is to rather delegate it to the kernel keyring; I've got a patchset converting the authentication stuff over to use 'struct key' and store all keys in the kernel keyring. Then one can use either a keys directly (ie the currrent interface) or just reference the key serial from the keystore. That way we can decouple the provisioning step from the nvme-cli interface, and we wouldn't need the prefix idea.

[igaw]

Make sense. In the end the prefix idea is make nvme-cli compatible to other tools (cryptsetup?) which have to support legacy(?) setups. The kernel keyring seems to be the newest player in town and the right way forward from what I understand. But hey, I am no security guy, so I don't care too much about this.

And if we still need to have to support different means, I don't see a problem to add the prefix stuff later. We will face the same troubles as we face now.

[mwilck]

@hreinecke,

My idea here is to rather delegate it to the kernel keyring; I've got a patchset converting the authentication stuff over to use 'struct key' and store all keys in the kernel keyring. Then one can use either a keys directly (ie the currrent interface) or just reference the key serial from the keystore. That way we can decouple the provisioning step from the nvme-cli interface, and we wouldn't need the prefix idea.

So you want a separate tool that loads the key into the keyring. This is fine. Still, this separate tool needs to be configured how to obtain the key material it feeds into the keyring. It will need to look up keys specific to host NQN, subsystem, NQN and possibly other parameters (like chosen HMAC function), and presumably it will be able to handle various types of key sources roughly along the lines I listed earlier, whatever final syntax is chosen.

So the new tool will need to look up configuration items by host and subsystem NQN.
IMO it'd make a lot of sense to have this tool use config.json for this purpose.
config.json has exactly the structure which is needed to look up NVMe connection-specific parameters, including library functions to do this comfortably in C and in Python. If we design the new tool with a separate configuration file, that file would necessarily duplicate the structure and a considerable part of the content that config.json already has. Using config.json would also be a usability benefit for end users, as they would find all configuration items related to a given host/subsys association in a single place.

If we decide to let the key provisioning tool use config.json, it would make sense to use either prefixing, or some other syntax that make it feasible to derive the method by which the key material will be obtained.

About "just reference the key serial from the keystore", I am not sure how that's supposed to work. You said in #783 that the key serial is "ephemeral", thus it's not suitable for being stored in a static configuration file. Users can figure it out by examining the key ring, but that is cumbersome and doesn't scale. I believe that key lookup in the kernel should work by host NQN/subsys NQN association exclusively. That should be possible already, as the key description contains all information about the NQNs.

[hreinecke]

@hreinecke,

My idea here is to rather delegate it to the kernel keyring; I've got a patchset converting the authentication stuff over to use 'struct key' and store all keys in the kernel keyring. Then one can use either a keys directly (ie the currrent interface) or just reference the key serial from the keystore. That way we can decouple the provisioning step from the nvme-cli interface, and we wouldn't need the prefix idea.

So you want a separate tool that loads the key into the keyring. This is fine. Still, this separate tool needs to be configured how to obtain the key material it feeds into the keyring. It will need to look up keys specific to host NQN, subsystem, NQN and possibly other parameters (like chosen HMAC function), and presumably it will be able to handle various types of key sources roughly along the lines I listed earlier, whatever final syntax is chosen.

Yes. But we don't need to decide that now, and can discuss it once that tool is implemented.

So the new tool will need to look up configuration items by host and subsystem NQN. IMO it'd make a lot of sense to have this tool use config.json for this purpose. config.json has exactly the structure which is needed to look up NVMe connection-specific parameters, including library functions to do this comfortably in C and in Python. If we design the new tool with a separate configuration file, that file would necessarily duplicate the structure and a considerable part of the content that config.json already has. Using config.json would also be a usability benefit for end users, as they would find all configuration items related to a given host/subsys association in a single place.

But we will lose the ability to do a key rotation. IE the problem is that the command-line
nvme connect -t tcp -a -s --tls
(ie select keys automatically from the keystore) will be converted into
nvme connect -t tcp -a -s --tls_key <key_id>
when using the config.json file.
With the former we will be do an automatic key rotation. With the latter we can't.
Or, rather, will have to update the json file whenever a new key is selected.

If we decide to let the key provisioning tool use config.json, it would make sense to use either prefixing, or some other syntax that make it feasible to derive the method by which the key material will be obtained.

... or just copy out all keys from the keystore upon shutdown, and read them in when starting up.
Which is what I tried to do here.

About "just reference the key serial from the keystore", I am not sure how that's supposed to work. You said in #783 that the key serial is "ephemeral", thus it's not suitable for being stored in a static configuration file. Users can figure it out by examining the key ring, but that is cumbersome and doesn't scale. I believe that key lookup in the kernel should work by host NQN/subsys NQN association exclusively. That should be possible already, as the key description contains all information about the NQNs.

Which wouldn't it scale to look up from the keyring? Sure, lookup will take longer the more keys are stored, but they will be revoked upon key rotation so we shouldn't have any stale keys in the keystore.

[mwilck]

Ok. So we'll have a separate tool, and this tool will probably have a separate configuration file.

I don't want to delay the inclusion of this PR with my comments. I just want to make sure we will be able to support multiple configurable ways to obtain key material in the future, without having to change the design.

[mwilck]

The code looks good to me. Just a few nits.

[mwilck]

Which wouldn't it scale to look up from the keyring?

Sorry for bad choice of words. Perhaps I am still missing something from the big picture. I was wondering how the user would figure out the serial number to feed into the connect command. I figured the user could examine the keyring e.g. with keyctl to derive the serial number. Doing this manually would be slow, that's what I meant with "doesn't scale".

[hreinecke]

Which wouldn't it scale to look up from the keyring?

Sorry for bad choice of words. Perhaps I am still missing something from the big picture. I was wondering how the user would figure out the serial number to feed into the connect command. I figured the user could examine the keyring e.g. with keyctl to derive the serial number. Doing this manually would be slow, that's what I meant with "doesn't scale".

Well, he doesn't have to (which is kinda the point of my argumentation all along.) When you just specify '--tls' the kernel will lookup the first matching key from the keystore, irrespective of the key serial number (that is what I meant with 'ephemeral'). For lookup we use the key identity, constructed from the host NQN and subsystem NQN plus the hash algorithm (do check nvme_tls_psk_default() in the kernel for the actual algorithm). This is the same identity used for the TLS handshake, so it must match for the handshake to succeed.

[mwilck]

Well, he doesn't have to (which is kinda the point of my argumentation all along.) When you just specify '--tls' the kernel will lookup the first matching key from the keystore, irrespective of the key serial number.

Thanks. I got that part. I just misunderstood the sentence about "referencing the key serial from the keystore".

[igaw]

Just rebased to the current head to resolve the conflicts.

[igaw]

Thanks!