Re: [PATCH 08/31] net/tcp: Introduce TCP_AO setsockopt()s

[Dmitry Safonov]

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.

>> +struct tcp_ao { /* setsockopt(TCP_AO) */
>> +    struct __kernel_sockaddr_storage tcpa_addr;
>> +    char    tcpa_alg_name[64];
>> +    __u16    tcpa_flags;
>
> This field accept TCP_AO_CMDF_CURR and TCP_AO_CMDF_NEXT which means that
> you are combining key addition with key selection. Not clear it
> shouldn't just always be a separate sockopt?

I don't see any downside. A user can add a key and start using it immediately
with one syscall instead of two. It's not necessary, one can do it in
2 setsockopt()s if they want.

[..]
> 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).

[..]
> 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.

On contrary, I see that as a really big feature. RFC5926 was published in 2010,
when sha1 was yet hard to break. These days sha1 is considered insecure.
I.e., the first link from Google:

> Starting with version 56, released this month, Google Chrome will mark all
> SHA-1-signed HTTPS certificates as unsafe. Other major browser vendors
> plan to do the same.
> "Hopefully these new efforts of Google of making a real-world attack possible
> will lead to vendors and infrastructure managers quickly removing SHA-1 from
> their products and configurations as, despite it being a deprecated algorithm,
> some vendors still sell products that do not support more modern hashing
> algorithms or charge an extra cost to do so," [..]

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.

I prefer UAPI that takes crypto algo name as a string, rather than new
defined magic number from one of kernel headers.
IOW,
: strcpy(ao.tcpa_alg_name, "cmac(aes128)");
: setsockopt(sk, IPPROTO_TCP, opt, &ao, sizeof(ao));
is better than
: ao.tcp_alg = TCP_AO_CMAC_MAGIC_DEFINE;
: setsockopt(sk, IPPROTO_TCP, opt, &ao, sizeof(ao));

Neither I see a point in more patches adding new
#define TCP_AO_NEW_ALGO

BTW, I had some patches to add testing in fcnal-test.sh and covered
the following algorithms, that worked just fine (test changes not
included in v1):
hmac(sha1) cmac(aes128) hmac(rmd128) hmac(rmd160) hmac(sha512)
hmac(sha384) hmac(sha256) hmac(md5) hmac(sha224) hmac(sha3-512)

No point in artificially disabling them or introducing new magic #defines.

Thanks,
          Dmitry