[PATCH 8/9] nvmet-tcp: support secure channel concatenation

[Hannes Reinecke]

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