Hi Leonard,

On Mon, Sep 5, 2022 at 12:06 AM Leonard Crestez [off-list ref] wrote:

This is similar to TCP-MD5 in functionality but it's sufficiently
different that packet formats and interfaces are incompatible.
Compared to TCP-MD5 more algorithms are supported and multiple keys
can be used on the same connection but there is still no negotiation
mechanism.

...

A completely unrelated series that implements the same features was posted
recently: https://lore.kernel.org/netdev/20220818170005.747015-1-dima@arista.com/ (local)

The biggest difference is that this series puts TCP-AO key on a global
instead of per-socket list and that it attempts to make kernel-mode
key selection decisions instead of very strictly requiring userspace
to make all decisions.

This is a departure from how md5 is implemented and the interface that
BGP developers are used to. The reason you switched your implementation
to a global database was to fix a minor race between key addition/deletion
and connections being accepted on a listening socket. This race can be
easily solved with a getsockopt() in user space. Thus it doesn’t justify
the complexity that a global key database brings to the implementation.
I have a few issues with that design that I would like to point out.

- Currently, a setsockopt on a given socket that adds a key will add it to the
global database. That opens up the door for buggy/malicious apps to install
bogus keys and mess up the connections of other apps. Also, it seems unusual
for a setsockopt to affect all sockets in a namespace. This requires all user
space apps to play nicely together.

- Having the keys be per-socket takes advantage of the existing socket lock,
simplifying synchronization and avoiding extra locks in the TCP stack.

- Caching of traffic keys becomes much easier with per-socket keys. Once
a connection is established it will typically have one or two keys on its list
with traffic keys cached. In your current implementation, a linked list of
potentially thousands of keys has to be linearly searched for each packet
and the traffic key has to be calculated before doing the actual hashing of
the packet. We believe a linear search with the extra hashing to calculate
the traffic keys will be detrimental to the performance of real world
deployments.

- Using a global database might have a benefit if the goal is to have
user space apps use tcp-ao transparently without any modifications.
This would require key matching on the local and remote ports.
But again, do we expect any apps other than BGP/LDP using tcp-ao?
If not, why the extra complexity in the kernel?

I believe my approach greatly simplifies userspace implementation.
The biggest difference in this iteration of the patch series is adding
per-key lifetime values based on RFC8177 in order to implement
kernel-mode key rollover.

We believe that key rotation should be done in user-space. One reason is that
different vendors might have slightly different behaviors during key rotation
and having the logic be in user-space is more flexible for fixing issues. It’s
not fun having to patch the kernel every time an interop issue is discovered.

Older versions still required userspace to tweak the NOSEND/NORECV flags
and always pick rnextkeyid explicitly, but now no active "key management"
should be required on established socket - Just set correct flags and
expiration dates and the kernel can perform key rollover itself. You can
see a (simple) test of that behavior here:

...

Best,

Salam