Re: [PATCH 00/31] net/tcp: Add TCP-AO support

[Leonard Crestez <cdleonard@gmail.com>]

On 8/18/22 19:59, Dmitry Safonov wrote:
> This patchset implements the TCP-AO option as described in RFC5925. There
> is a request from industry to move away from TCP-MD5SIG and it seems the time
> is right to have a TCP-AO upstreamed. This TCP option is meant to replace
> the TCP MD5 option and address its shortcomings. Specifically, it provides
> more secure hashing, key rotation and support for long-lived connections
> (see the summary of TCP-AO advantages over TCP-MD5 in (1.3) of RFC5925).
> The patch series starts with six patches that are not specific to TCP-AO
> but implement a general crypto facility that we thought is useful
> to eliminate code duplication between TCP-MD5SIG and TCP-AO as well as other
> crypto users. These six patches are being submitted separately in
> a different patchset [1]. Including them here will show better the gain
> in code sharing. Next are 18 patches that implement the actual TCP-AO option,
> followed by patches implementing selftests.
> 
> The patch set was written as a collaboration of three authors (in alphabetical
> order): Dmitry Safonov, Francesco Ruggeri and Salam Noureddine. Additional
> credits should be given to Prasad Koya, who was involved in early prototyping
> a few years back. There is also a separate submission done by Leonard Crestez
> whom we thank for his efforts getting an implementation of RFC5925 submitted
> for review upstream [2]. This is an independent implementation that makes
> different design decisions.

Is this based on something that Arista has had running for a while now 
or is a recent new development?

> For example, we chose a similar design to the TCP-MD5SIG implementation and
> used setsockopt()s to program per-socket keys, avoiding the extra complexity
> of managing a centralized key database in the kernel. A centralized database
> in the kernel has dubious benefits since it doesn’t eliminate per-socket
> setsockopts needed to specify which sockets need TCP-AO and what are the
> currently preferred keys. It also complicates traffic key caching and
> preventing deletion of in-use keys.

My implementation started with per-socket lists but switched to a global 
list because this way is much easier to manage from userspace. In 
practice userspace apps will want to ensure that all sockets use the 
same set of keys anyway.

> In this implementation, a centralized database of keys can be thought of
> as living in user space and user applications would have to program those
> keys on matching sockets. On the server side, the user application programs
> keys (MKTS in TCP-AO nomenclature) on the listening socket for all peers that
> are expected to connect. Prefix matching on the peer address is supported.
> When a peer issues a successful connect, all the MKTs matching the IP address
> of the peer are copied to the newly created socket. On the active side,
> when a connect() is issued all MKTs that do not match the peer are deleted
> from the socket since they will never match the peer. This implementation
> uses three setsockopt()s for adding, deleting and modifying keys on a socket.
> All three setsockopt()s have extensive sanity checks that prevent
> inconsistencies in the keys on a given socket. A getsockopt() is provided
> to get key information from any given socket.

My series doesn't try to prevent inconsistencies inside the key lists 
because it's not clear that the kernel should prevent userspace from 
shooting itself in the foot. Worst case is connection failure on 
misconfiguration which seems fine.

The RFC doesn't specify in detail how key management is to be performed, 
for example if two valid keys are available it doesn't mention which one 
should be used. Some guidance is found in RFC8177 but again not very much.

I implemented an ABI that can be used by userspace for RFC8177-style key 
management and asked for feedback but received very little. If you had 
come with a clear ABI proposal I would have tried to implement it.

Here's a link to our older discussion:

https://lore.kernel.org/netdev/e7f0449a-2bad-99ad-4737-016a0e6b8b84@gmail.com/

Seeing an entirely distinct unrelated implementation is very unexpected. 
What made you do this?

--
Regards,
Leonard