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

Jakub Kicinski
2024-05-30T19:51:21Z

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.