Date:   Mon, 23 Jan 2023 10:13:58 +0000
From:   Boris Pismenny <borisp@...dia.com>
To:     Gal Pressman <gal@...dia.com>, Jakub Kicinski <kuba@...nel.org>,
        Sabrina Dubroca <sd@...asysnail.net>,
        Tariq Toukan <tariqt@...dia.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Frantisek Krenzelok <fkrenzel@...hat.com>
Subject: Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

On 19/01/2023 10:27, Gal Pressman wrote:
> On 19/01/2023 4:55, Jakub Kicinski wrote:
>> On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
>>> 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
>>>> On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:  
>>>>> This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
>>>>> [1]). A sender transmits a KeyUpdate message and then changes its TX
>>>>> key. The receiver should react by updating its RX key before
>>>>> processing the next message.
>>>>>
>>>>> This patchset implements key updates by:
>>>>>  1. pausing decryption when a KeyUpdate message is received, to avoid
>>>>>     attempting to use the old key to decrypt a record encrypted with
>>>>>     the new key
>>>>>  2. returning -EKEYEXPIRED to syscalls that cannot receive the
>>>>>     KeyUpdate message, until the rekey has been performed by userspace  
>>>>
>>>> Why? We return to user space after hitting a cmsg, don't we?
>>>> If the user space wants to keep reading with the old key - 🤷️  
>>>
>>> But they won't be able to read anything. Either we don't pause
>>> decryption, and the socket is just broken when we look at the next
>>> record, or we pause, and there's nothing to read until the rekey is
>>> done. I think that -EKEYEXPIRED is better than breaking the socket
>>> just because a read snuck in between getting the cmsg and setting the
>>> new key.
>>
>> IDK, we don't interpret any other content types/cmsgs, and for well
>> behaved user space there should be no problem (right?).
>> I'm weakly against, if nobody agrees with me you can keep as is.
>>
>>>>>  3. passing the KeyUpdate message to userspace as a control message
>>>>>  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
>>>>>     setsockopts
>>>>>
>>>>> This API has been tested with gnutls to make sure that it allows
>>>>> userspace libraries to implement key updates [2]. Thanks to Frantisek
>>>>> Krenzelok <fkrenzel@...hat.com> for providing the implementation in
>>>>> gnutls and testing the kernel patches.  
>>>>
>>>> Please explain why - the kernel TLS is not faster than user space, 
>>>> the point of it is primarily to enable offload. And you don't add
>>>> offload support here.  
>>>
>>> Well, TLS1.3 support was added 4 years ago, and yet the offload still
>>> doesn't support 1.3 at all.
>>
>> I'm pretty sure some devices support it. None of the vendors could 
>> be bothered to plumb in the kernel support, yet, tho.
> 
> Our device supports TLS 1.3, it's in our plans to add driver/kernel support.
> 
>> I don't know of anyone supporting rekeying.
> 
> Boris, Tariq, do you know?

Rekeying is not trivial to get right with offload. There are at least
two problems to solve:
1. On transmit, we need to handle both the new and the old key for new
and old (retransmitted) data, respectively. Our device will be able to
hold both keys in parallel and to choose the right one at the cost of an
if statement in the data-path. Alternatively, we can just fallback to
software for the old key and focus on the new key.
2. On Rx, packets with the new key may arrive before the key is
installed unless we design a mechanism for preemptively setting the next
key in HW. As a result, we may get a resync on every rekey.

Have you considered an API to preemptively set the next key in the
kernel such that there is never a need to stop the datapath? I think
that the change in SSL libraries is minor and it can really help KTLS.

Powered by blists - more mailing lists