Date:   Tue, 2 Nov 2021 21:18:41 -0600
From:   David Ahern <dsahern@...il.com>
To:     Leonard Crestez <cdleonard@...il.com>,
        David Ahern <dsahern@...nel.org>,
        Shuah Khan <shuah@...nel.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Eric Dumazet <edumazet@...gle.com>
Cc:     "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>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Mat Martineau <mathew.j.martineau@...ux.intel.com>,
        Christoph Paasch <cpaasch@...le.com>,
        Ivan Delalande <colona@...sta.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 v2] tcp: Initial support for RFC5925 auth option

On 11/1/21 10:34 AM, Leonard Crestez wrote:
> This is similar to TCP MD5 in functionality but it's sufficiently
> different that wire formats 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.
> 
> Expected use-case is protecting long-duration BGP/LDP connections
> between routers using pre-shared keys. The goal of this series is to
> allow routers using the linux TCP stack to interoperate with vendors
> such as Cisco and Juniper.
> 
> Both algorithms described in RFC5926 are implemented but the code is not
> very easily extensible beyond that. In particular there are several code
> paths making stack allocations based on RFC5926 maximum, those would
> have to be increased.
> 
> This version implements SNE and l3mdev awareness and adds more tests.
> Here are some known flaws and limitations:
> 
> * Interaction with TCP-MD5 not tested in all corners
> * Interaction with FASTOPEN not tested and unlikely to work because
> sequence number assumptions for syn/ack.
> * Not clear if crypto_shash_setkey might sleep. If some implementation
> do that then maybe they could be excluded through alloc flags.
> * Traffic key is not cached (reducing performance)
> * User is responsible for ensuring keys do not overlap.
> * There is no useful way to list keys, making userspace debug difficult.
> * There is no prefixlen support equivalent to md5. This is used in
> some complex FRR configs.
> 
> Test suite was added to tools/selftests/tcp_authopt. Tests are written
> in python using pytest and scapy and check the API in some detail and
> validate packet captures. Python code is already used in linux and in
> kselftests but virtualenvs not very much, this particular test suite
> uses `pip` to create a private virtualenv and hide dependencies.
> 
> This actually forms the bulk of the series by raw line-count. Since
> there is a lot of code it was mostly split on "functional area" so most
> files are only affected by a single code. A lot of those tests are
> relevant to TCP-MD5 so perhaps it might help to split into a separate
> series?
> 
> Some testing support is included in nettest and fcnal-test.sh, similar
> to the current level of tcp-md5 testing.
> 
> SNE was tested by creating connections in a loop until a large SEQ is
> randomly selected and then making it rollover. The "connect in a loop"
> step ran into timewait overflow and connection failure on port reuse.
> After spending some time on this issue and my conclusion is that AO
> makes it impossible to kill remainders of old connections in a manner
> similar to unsigned or md5sig, this is because signatures are dependent
> on ISNs.  This means that if a timewait socket is closed improperly then
> information required to RST the peer is lost.
> 
> The fact that AO completely breaks all connection-less RSTs is
> acknowledged in the RFC and the workaround of "respect timewait" seems
> acceptable.
> 
> Changes for frr (old): https://github.com/FRRouting/frr/pull/9442
> That PR was made early for ABI feedback, it has many issues.
> 

overall looks ok to me. I did not wade through the protocol details.

I did see the comment about no prefixlen support in the tests. A lot of
patches to absorb, perhaps I missed it. Does AuthOpt support for
prefixes? If not, you should consider adding that as a quick follow on
(within the same dev cycle). MD5 added prefix support for scalability;
seems like AO should be concerned about the same.

Powered by blists - more mailing lists