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

[Leonard Crestez]

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@xxxxxxxxx/

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