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

On 8/23/22 15:45, Leonard Crestez wrote:

On 8/18/22 19:59, Dmitry Safonov wrote:

[..]

quoted

+#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.

quoted

+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