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

Leonard Crestez
2022-08-23T16:38:33Z

On 8/18/22 19:59, Dmitry Safonov wrote:
> Add 3 setsockopt()s:
> 1. to add a new Master Key Tuple (MKT) on a socket
> 2. to delete present MKT from a socket
> 3. to change flags of an MKT
>
> Userspace has to introduce keys on every socket it wants to use TCP-AO
> option on, similarly to TCP_MD5SIG/TCP_MD5SIG_EXT.
> RFC5925 prohibits definition of MKTs that would match the same peer,
> so do sanity checks on the data provided by userspace. Be as
> conservative as possible, including refusal of defining MKT on
> an established connection with no AO, removing the key in-use and etc.
>
> (1) and (2) are to be used by userspace key manager to add/remove keys.
> (3) main purpose is to set rnext_key, which (as prescribed by RFC5925)
> is the key id that will be requested in TCP-AO header from the peer to
> sign their segments with.
>
> At this moment the life of ao_info ends in tcp_v4_destroy_sock().

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

> +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?

> + __u8 tcpa_prefix;
> + __u8 tcpa_sndid;
> + __u8 tcpa_rcvid;
> + __u8 tcpa_maclen;
> + __u8 tcpa_keyflags;
> + __u8 tcpa_keylen;
> + __u8 tcpa_key[TCP_AO_MAXKEYLEN];
> +} __attribute__((aligned(8)));
> +
> +struct tcp_ao_del { /* setsockopt(TCP_AO_DEL) */
> + struct __kernel_sockaddr_storage tcpa_addr;
> + __u16 tcpa_flags;
> + __u8 tcpa_prefix;
> + __u8 tcpa_sndid;
> + __u8 tcpa_rcvid;
> + __u8 tcpa_current;
> + __u8 tcpa_rnext;
> +} __attribute__((aligned(8)));
> +
> +struct tcp_ao_mod { /* setsockopt(TCP_AO_MOD) */
> + __u16 tcpa_flags;
> + __u8 tcpa_current;
> + __u8 tcpa_rnext;
> +} __attribute__((aligned(8)));

This is quite similar to my "struct tcp_authopt" in the fact that it is
intented to support controlling the "current keys".

* tcpa_current is equivalent to send_keyid
* tcpa_rnext is equivalent to send_rnextkeyid

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.

The specification around send_keyid in the RFC is conflicting:
* User must be able to control it
* 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.

The "send_rnextkeyid" is also optional in my patch, if
TCP_AUTHOPT_FLAG_LOCK_RNEXTKEYID is not passed then the recv_id of the
sending key is sent.

Here's a link to my implementation of key selection controls:

https://lore.kernel.org/netdev/2956d99e7fbf9ff2a8cc720c67baaef35bc32343.1660852705.git.cdleonard@gmail.com/

> +static int tcp_ao_parse_crypto(struct tcp_ao *cmd, struct tcp_ao_key *key)
> +{
> + unsigned int syn_tcp_option_space;
> + struct crypto_pool_ahash hp;
> + bool is_kdf_aes_128_cmac = false;
> + struct crypto_ahash *tfm;
> + int err, pool_id;
> +
> + /* Force null-termination of tcpa_alg_name */
> + cmd->tcpa_alg_name[ARRAY_SIZE(cmd->tcpa_alg_name) - 1] = '\0';
> +
> + /* RFC5926, 3.1.1.2. KDF_AES_128_CMAC */
> + if (!strcmp("cmac(aes128)", cmd->tcpa_alg_name)) {
> + strcpy(cmd->tcpa_alg_name, "cmac(aes)");
> + is_kdf_aes_128_cmac = (cmd->tcpa_keylen != 16);
> + }

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.

--
Regards,
Leonard