Re: [RFC net-next 01/15] psp: add documentation

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 29 May 2024 20:47:02 -0400 Willem de Bruijn wrote:
> Jakub Kicinski wrote:
> > On Sun, 12 May 2024 21:24:23 -0400 Willem de Bruijn wrote:  
> > > 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.

Already run the sed, device key it is :)

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

Good catch. The TLS checks predate tcp_skb_can_collapse() (and MPTCP).
We've grown the check in tcp_shift_skb_data() and the logic
in tcp_grow_skb(), both missing the decrypted check.

I'll send some fixes, these are existing bugs :(

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

Hm. And we can avoid this by only enforcing encryption of data-less
segments once we've seen some encrypted data?

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