[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240530125120.24dd7f98@kernel.org>
Date: Thu, 30 May 2024 12:51:20 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: 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
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.
Powered by blists - more mailing lists