Date:   Sat, 3 Sep 2022 12:35:56 +0300
From:   Leonard Crestez <cdleonard@...il.com>
To:     Dmitry Safonov <dima@...sta.com>
Cc:     Andy Lutomirski <luto@...capital.net>,
        Ard Biesheuvel <ardb@...nel.org>,
        Bob Gilligan <gilligan@...sta.com>,
        David Ahern <dsahern@...nel.org>,
        Dmitry Safonov <0x7f454c46@...il.com>,
        Eric Biggers <ebiggers@...nel.org>,
        Francesco Ruggeri <fruggeri@...sta.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Hideaki YOSHIFUJI <yoshfuji@...ux-ipv6.org>,
        Ivan Delalande <colona@...sta.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Salam Noureddine <noureddine@...sta.com>,
        Shuah Khan <shuah@...nel.org>, netdev@...r.kernel.org,
        linux-crypto@...r.kernel.org, Eric Dumazet <edumazet@...gle.com>,
        linux-kernel@...r.kernel.org,
        "David S. Miller" <davem@...emloft.net>
Subject: Re: [PATCH 08/31] net/tcp: Introduce TCP_AO setsockopt()s

On 8/31/22 21:48, Dmitry Safonov wrote:
> On 8/23/22 15:45, Leonard Crestez wrote:
>> On 8/18/22 19:59, Dmitry Safonov wrote:
> [..]
>>> +#define TCP_AO            38    /* (Add/Set MKT) */
>>> +#define TCP_AO_DEL        39    /* (Delete MKT) */
>>> +#define TCP_AO_MOD        40    /* (Modify MKT) */
>>
>> The TCP_AO_MOD sockopt doesn't actually modify and MKT, it only controls
>> per-socket properties. It is equivalent to my TCP_AUTHOPT sockopt while
>> TCP_AO is equivalent to TCP_AUTHOPT_KEY. My equivalent of TCP_AO_DEL
>> sockopt is a flag inside tcp_authopt_key.
> 
> Fair point, the comment could be "Modify AO", rather than "Modify MKT".
> On the other side, this can later support more per-key changes than in
> the initial proposal: i.e., zero per-key counters. Password and rcv/snd
> ids can't change to follow RFC text, but non-essentials may.
> So, the comment to the command here is not really incorrect.

I think it makes sense to at least separate per-key and per-socket 
options. This way a sockopt for per-socket info doesn't contain fields 
used to identify keys which is much clearer.

>> I also have two fields called "recv_keyid" and "recv_rnextkeyid" which
>> inform userspace about what the remote is sending, I'm not seeing an
>> equivalent on your side.
> 
> Sounds like a good candidate for getsockopt() for logs/debugging.
> 
>> The specification around send_keyid in the RFC is conflicting:
>> * User must be able to control it
> 
> I don't see where you read it, care to point it out?
> I see choosing the current_key by marking the preferred key during
> an establishment of a connection, but I don't see any "MUST control
> current_key". We allow changing current_key, but that's actually
> not something required by RFC, the only thing required is to respect
> rnext_key that's asked by peer.
> 
>> * Implementation must respect rnextkeyid in incoming packet
>>
>> I solved this apparent conflict by adding a
>> "TCP_AUTHOPT_FLAG_LOCK_KEYID" flag so that user can choose if it wants
>> to control the sending key or let it be controlled from the other side.
> 
> That's exactly violating the above "Implementation must respect
> rnextkeyid in incoming packet". See RFC5925 (7.5.2.e).

This is based on paragraphs towards the end of Section 7.1:

 >> TCP SEND, or a sequence of commands resulting in a SEND, MUST be
augmented so that the preferred outgoing MKT (current_key) and/or the
preferred incoming MKT (rnext_key) of a connection can be indicated.

This is for TCP SEND, not just open/connect. I'm reading this as a
requirement that userspace *MUST* be able to control the current key. 
Yes, it does seem contradict 7.5.2.e which is why I implemented this as 
a "key lock flag".

 >> TCP RECEIVE, or the sequence of commands resulting in a RECEIVE,
MUST be augmented so that the KeyID and RNextKeyID of a recently
received segment is available to the user out of band (e.g., as an
additional parameter to RECEIVE or via a STATUS call).

It seems to me that it *MUST* be possible for userspace to read the 
incoming rnextkeyid and handle it by itself. It could choose to follow 
7.5.2.e or it could do something entirely different. When it can't 
respect rnextkeyid because the key is not yet valid then userspace has 
more information to make an alternative current_key decision.

> 
> [..]
>> Only two algorithms are defined in RFC5926 and you have to treat one of
>> them as a special case. I remain convinced that generic support for
>> arbitrary algorithms is undesirable; it's better for the algorithm to be
>> specified as an enum.
> 
> So, why limit a new TCP sign feature to already insecure algorithms?
> One can already use any crypto algorithms for example, in tunnels.
> And I don't see any benefit in defining new magic macros, only downside.

Adding support for arbitrary algorithms increases complexity for no 
real-world gain. There are also lots of corner cases that must be 
treated correctly like odd traffic_keylen and maclen, having an enum
means that userspace can't attempt to trick us. The ABI is also smaller.

There's also a special case in one of the two concrete KDFs defined by 
RFC5925. What if there are more, will the ABI be expanded to support all 
the cases?

Disagreements over whether a particular form of extensibility is 
"useful" are unlikely to result in any sort of useful conclusion. I'm 
lazy so I only care about interop with existing implementations from 
Juniper and Cisco.

--
Regards,
Leonard

Powered by blists - more mailing lists