Date:   Fri, 9 Sep 2022 14:41:58 -0700
From:   Salam Noureddine <noureddine@...sta.com>
To:     Leonard Crestez <cdleonard@...il.com>
Cc:     David Ahern <dsahern@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Philip Paeps <philip@...uble.is>,
        Shuah Khan <shuah@...nel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Kuniyuki Iwashima <kuniyu@...zon.co.jp>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Jakub Kicinski <kuba@...nel.org>,
        Yuchung Cheng <ycheng@...gle.com>,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        Christoph Paasch <cpaasch@...le.com>,
        Ivan Delalande <colona@...sta.com>,
        Caowangbao <caowangbao@...wei.com>,
        Priyaranjan Jha <priyarjha@...gle.com>, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 00/26] tcp: Initial support for RFC5925 auth option

Hi Leonard,

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

Powered by blists - more mailing lists