Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Apoorv Kothari <apoorvko@amazon.com>]

> 2023-01-18, 18:55:22 -0800, 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.
> 
> I was concerned (I don't know if it's realistic) about a userspace
> application with two threads:
> 
> 
>   Thread A            Thread B
>   --------            --------
> 
>   read cmsg
> 
>                       read some data (still on the old key)
> 
>   sets the new key
> 
> 
> I guess one could claim that's a userspace bug.
> 
> František's implementation in gnutls doesn't seem to rely on this.
> 
> Apoorv, since you were also looking into key updates, do you have an
> opinion on pausing decryption/reads until userspace has provides the
> new key?
> 

There are a few reason I can think of why we would want the pausing behavior.

0) If possible, we should enforce correctness and prevent the userspace from
doing something bad.

1) Pausing better aligns with the RFC since all subsequent messages
should be encrypted with the new key.

From the RFC https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3
   After sending a KeyUpdate message, the sender SHALL send all
   its traffic using the next generation of keys, computed as described
   in Section 7.2.  Upon receiving a KeyUpdate, the receiver MUST update
   its receiving keys.

2) Its possible to receive multiple KeyUpdate messages. If we allow the
userspace to read more records, they would read multiple KeyUpdate messages.
However since we currently track `bool key_update_pending;` using a bool,
we would only require a single rekey and end up in a bad state. Its possible
to fix this by not using bool but then the logic get more complicate and not
worth it IMO.

> > > > >  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@redhat.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.
> > I don't know of anyone supporting rekeying.
> >
> > > IIRC support for KeyUpdates is mandatory in TLS1.3, so currently the
> > > kernel can't claim to support 1.3, independent of offloading.
> > 
> > The problem is that we will not be able to rekey offloaded connections.
> > For Tx it's a non-trivial problem given the current architecture.
> > The offload is supposed to be transparent, we can't fail the rekey just
> > because the TLS gotten offloaded.
> 
> What's their plan when the peer sends a KeyUpdate request then? Let
> the connection break?
> 
> -- 
> Sabrina