On 7/25/24 19:21, Eric Biggers wrote:
On Thu, Jul 25, 2024 at 01:50:19PM +0200, Hannes Reinecke wrote:
On 7/23/24 03:48, Eric Biggers wrote:
On Mon, Jul 22, 2024 at 04:21:21PM +0200, Hannes Reinecke wrote:
+ ret = nvme_auth_generate_digest(sq->ctrl->shash_id, psk, psk_len,
+ sq->ctrl->subsysnqn,
+ sq->ctrl->hostnqn, &digest);
+ if (ret) {
+ pr_warn("%s: ctrl %d qid %d failed to generate digest, error %d\n",
+ __func__, sq->ctrl->cntlid, sq->qid, ret);
+ goto out_free_psk;
+ }
+ ret = nvme_auth_derive_tls_psk(sq->ctrl->shash_id, psk, psk_len,
+ digest, &tls_psk);
+ if (ret) {
+ pr_warn("%s: ctrl %d qid %d failed to derive TLS PSK, error %d\n",
+ __func__, sq->ctrl->cntlid, sq->qid, ret);
+ goto out_free_digest;
+ }
This reuses 'psk' as both an HMAC key and as input keying material for HKDF.
It's *probably* still secure, but this violates cryptographic best practices in
that it reuses a key for multiple purposes. Is this a defect in the spec?
This is using a digest calculated from the actual PSK key material, true.
You are right that with that we probably impact cryptographic
reliability, but that that is what the spec mandates.
How set in stone is this specification? Is it finalized and has it been
implemented by anyone else? Your code didn't correctly implement the spec
anyway, so at least you must not have done any interoperability testing.
Well ... thing is, this _is_ the first implementation. Anyone else does
interop testing against us.
The spec is pretty much set in stone here; sure we can update the spec,
but that takes time. I can ask the primary author if he's willing to
engage in a conversation about the cryptographic implications if you are
up to it.
Actual reason for this modification was the need to identify the TLS PSKs
for each connection, _and_ support key refresh.
We identify TLS PSKs by the combination of '<hash> <hostnqn> <subsysnqn>',
where '<hostnqn>' is the identification of the
initiator (source), and '<subsynqn>' the identification of the
target. But as we regenerate the PSK for each reset we are having
a hard time identifying the newly generated PSK by the original
'<hash> <hostnqn> <subsysnqn>' tuple only.
We cannot delete the original TLS PSK directly as it might be used
by other connections, so there will be a time where both PSKs
are valid and have to be stored in the keyring.
And keeping a global 'revision' counter is horrible, the alternative
of having a per-connection instance counter is similarly unpleasant.
Not ideal, true, so if you have better ideas I'd like to hear them.
But thanks for your consideration. I'll be bringing them up.
You are already using HKDF, so you could just use that, similar to how fscrypt
uses HKDF to derive both its key identifiers and its actual subkeys. HKDF can
be used to derive both public and non-public values; there's no need to use a
separate algorithm such as plain HMAC just because you need a public value.
I will check. But at this time I fear we have to stick with this
implementation because that's what the spec mandates.
But thanks for the review, very much appreciated.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +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
