Message-ID: <b4fafaaf-2012-cf54-c614-84a6fd2467b7@nvidia.com>
Date:   Thu, 26 Jan 2023 08:34:57 +0000
From:   Boris Pismenny <borisp@...dia.com>
To:     Marcel Holtmann <marcel@...tmann.org>,
        Sabrina Dubroca <sd@...asysnail.net>
CC:     Dave Watson <davejwatson@...com>,
        "open list:NETWORKING [GENERAL]" <netdev@...r.kernel.org>
Subject: Re: Setting TLS_RX and TLS_TX crypto info more than once?

On 25/01/2023 11:24, Marcel Holtmann wrote:
> Hi Sabrina,
> 
>>> in commit 196c31b4b5447 you limited setsockopt for TLS_RX and TLS_TX
>>> crypto info to just one time.
>>
>> Looking at commit 196c31b4b5447, that check was already there, it only
>> got moved.
> 
> I still think that check is not needed. We should get rid of it for
> TLS 1.2 and TLS 1.3.
> 
> (and Ilya seems to have left Mellanox/nVidia anyway)
> 
>>> +       crypto_info = &ctx->crypto_send;
>>> +       /* Currently we don't support set crypto info more than one time */
>>> +       if (TLS_CRYPTO_INFO_READY(crypto_info))
>>> +               goto out;
>>>
>>> This is a bit unfortunate for TLS 1.3 where the majority of the TLS
>>> handshake is actually encrypted with handshake traffic secrets and
>>> only after a successful handshake, the application traffic secrets
>>> are applied.
>>>
>>> I am hitting this issue since I am just sending ClientHello and only
>>> reading ServerHello and then switching on TLS_RX right away to receive
>>> the rest of the handshake via TLS_GET_RECORD_TYPE. This works pretty
>>> nicely in my code.

That's quite cool!

>>>
>>> Since this limitation wasn’t there in the first place, can we get it
>>> removed again and allow setting the crypto info more than once? At
>>> least updating the key material (the cipher obviously has to match).
>>>
>>> I think this is also needed when having to do any re-keying since I
>>> have seen patches for that, but it seems they never got applied.
>>
>> The patches are still under discussion (I only posted them a week ago
>> so "never got applied" seems a bit harsh):
>> https://lore.kernel.org/all/cover.1673952268.git.sd@queasysnail.net/T/#u
> 
> My bad, I kinda remembered they are from end of 2020. Anyway, following
> that thread, I see you fixed my problem already.
> 
> The encrypted handshake portion is actually simple since it defines
> really clear boundaries for the handshake traffic secret. The deployed
> servers are a bit optimistic since they send you an unencrypted
> ServerHello followed right away by the rest of the handshake messages
> fully encrypted. I was surprised I can MSG_PEEK at the TLS record
> header and then just read n bytes of just the ServerHello leaving
> everything else in the receive buffer to be automatically decrypted
> once I set the keys. This allows for just having the TLS handshake
> implemented in userspace.
> 
> It is a little bit unfortunate that with the support for TLS 1.3, the
> old tls12_ structures for providing the crypto info have been used. I
> would have argued for providing the traffic_secret into the kernel and
> then the kernel could have easily derived key+iv by itself. And with
> that it could have done the KeyUpdate itself as well.

Well, we can always add a v2 if the use-case makes sense. There was no
TLS 1.3 when we upstreamed this, and I think that the above wouldn't
work for TLS1.2, right?
Having the kernel perform key derivation sounds nice, and it may help
the TLS_DEVICE rekey design I mentioned on another thread.

> 
> The other problem is actually that userspace needs to know when we are
> close to the limits of AES-GCM encryptions or when the sequence number
> is about to wrap. We need to feed back the status of the rec_seq back
> to userspace (and with that also from the HW offload).

I agree about the problem. But, the part about HW is imprecise, the
kernel has all the state needed (except maybe in TLS_HW_RECORD).

> 
> I would argue that it might have made sense not just specifying the
> starting req_seq, but also either an ending or some trigger when
> to tell userspace that it is a good time to re-key.

We didn't do the rekey design when we coded this, back then the logic
was that rekeys are rare and connections are typically restarted when
a rekey is requested.

I think that having an end req_seq makes sense only if we do the rekey
in the kernel. On the TLS_DEVICE side, having the end_seq helps because
the driver will know: (1) when to switch to the next key on transmit;
(2) when to use the old key for retransmit; and (3) when to delete the
old key as all old key data has been transmitted. This can also work
with any number of keys/rekeys.

> 
> Regards
> 
> Marcel
> 

Powered by blists - more mailing lists