Message-ID: <6657cc86ddf97_37107c29438@willemb.c.googlers.com.notmuch>
Date: Wed, 29 May 2024 20:47:02 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Jakub Kicinski <kuba@...nel.org>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: netdev@...r.kernel.org, 
 pabeni@...hat.com, 
 borisp@...dia.com, 
 gal@...dia.com, 
 cratiu@...dia.com, 
 rrameshbabu@...dia.com, 
 steffen.klassert@...unet.com, 
 tariqt@...dia.com
Subject: Re: [RFC net-next 01/15] psp: add documentation

Jakub Kicinski wrote:
> On Sun, 12 May 2024 21:24:23 -0400 Willem de Bruijn wrote:
> > Jakub Kicinski wrote:
> > > +PSP Security Protocol (PSP) was defined at Google and published in:
> > > +
> > > +https://raw.githubusercontent.com/google/psp/main/doc/PSP_Arch_Spec.pdf
> > > +
> > > +This section briefly covers protocol aspects crucial for understanding
> > > +the kernel API. Refer to the protocol specification for further details.
> > > +
> > > +Note that the kernel implementation and documentation uses the term
> > > +"secret state" in place of "master key", it is both less confusing
> > > +to an average developer and is less likely to run afoul any naming
> > > +guidelines.  
> > 
> > There is some value in using the same terminology in the code as in
> > the spec.
> > 
> > And the session keys are derived from a key. That is more precise than
> > state. Specifically, counter-mode KDF from an AES key.
> > 
> > Perhaps device key, instead of master key? 
> 
> Weak preference towards secret state, but device key works, too.

Totally your choice. I just wanted to make sure this was considered.
 
> > > +Derived Rx keys
> > > +---------------
> > > +
> > > +PSP borrows some terms and mechanisms from IPsec. PSP was designed
> > > +with HW offloads in mind. The key feature of PSP is that Rx keys for every
> > > +connection do not have to be stored by the receiver but can be derived
> > > +from secret state and information present in packet headers.  
> > 
> > A second less obvious, but neat, feature is that it supports an
> > encryption offset, such that (say) the L4 ports are integrity
> > protected, but not encrypted, to allow for in-network telemetry.
> 
> I know, but the opening paragraph has:
> 
>    This section briefly covers protocol aspects crucial for
>    understanding the kernel API. Refer to the protocol specification for further details.
> 
> :) .. and I didn't implement the offset, yet. (It's trivial to add and
> ETOOMANYPATCHES.)

Ack, sounds good.

> 
> > > +This makes it possible to implement receivers which require a constant
> > > +amount of memory regardless of the number of connections (``O(1)`` scaling).
> > > +
> > > +Tx keys have to be stored like with any other protocol,  
> > 
> > Keys can optionally be passed in descriptor.
> 
> Added: Preferably, the Tx keys should be provided with the packet (e.g.
> as part of the descriptors).
> 
> > > +The expectation is that higher layer protocols will take care of
> > > +protocol and key negotiation. For example one may use TLS key exchange,
> > > +announce the PSP capability, and switch to PSP if both endpoints
> > > +are PSP-capable.  
> > 
> > > +Securing a connection
> > > +---------------------
> > > +
> > > +PSP encryption is currently only supported for TCP connections.
> > > +Rx and Tx keys are allocated separately. First the ``rx-assoc``
> > > +Netlink command needs to be issued, specifying a target TCP socket.
> > > +Kernel will allocate a new PSP Rx key from the NIC and associate it
> > > +with given socket. At this stage socket will accept both PSP-secured
> > > +and plain text TCP packets.
> > > +
> > > +Tx keys are installed using the ``tx-assoc`` Netlink command.
> > > +Once the Tx keys are installed all data read from the socket will
> > > +be PSP-secured. In other words act of installing Tx keys has the secondary
> > > +effect on the Rx direction, requring all received packets to be encrypted.  
> > 
> > Consider clarifying the entire state diagram from when one pair
> > initiates upgrade.
> 
> Not sure about state diagram, there are only 3 states. Or do you mean
> extend TCP state diagrams? I think a table may be better:
> 
> Event         | Normal TCP      | Rx PSP key present | Tx PSP key present |
> ---------------------------------------------------------------------------
> Rx plain text | accept          | accept             | drop               |
> 
> Rx PSP (good) | drop            | accept             | accept             |
> 
> Rx PSP (bad)  | drop            | drop               | drop               |
> 
> Tx            | plain text      | plain text         | encrypted *        |
> 
> * data enqueued before Tx key in installed will not be encrypted
>   (either initial send nor retranmissions)
> 
> 
> What should I add?

I've mostly been concerned about the below edge cases.

If both peers are in TCP_ESTABLISHED for the during of the upgrade,
and data is aligned on message boundary, things are straightforward.

The retransmit logic is clear, as this is controlled by skb->decrypted
on the individual skbs on the retransmit queue.

That also solves another edge case: skb geometry changes on retransmit
(due to different MSS or segs, using tcp_fragment, tso_fragment,
tcp_retrans_try_collapse, ..) maintain skb->decrypted. It's not
possible that skb is accidentally created that combines plaintext and
ciphertext content.

Although.. does this require adding that skb->decrypted check to
tcp_skb_can_collapse?
 
> > And some edge cases:
> > 
> > - retransmits
> > - TCP fin handshake, if only one peer succeeds
> 
> So FIN when one end is "locked down" and the other isn't?

If one peer can enter the state where it drops all plaintext, while
the other decides to close the connection before completing the
upgrade, and thus sends a plaintext FIN.

If (big if) that can happen, then the connection cannot be cleanly
closed.
 
> > - TCP control socket response to encrypted pkt
> 
> Control sock ignores PSP.

Another example where a peer stays open and stays retrying if it has
upgraded and drops all plaintext.

Powered by blists - more mailing lists