Hi Leonard,
On Mon, Sep 5, 2022 at 12:06 AM Leonard Crestez cdleonard@gmail.com 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/
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