From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: rework pfkey (ipsec/tcpmd5) code
To: tech@openbsd.org
Date: Tue, 1 Oct 2024 11:23:30 +0200

On Fri, Sep 27, 2024 at 05:14:04PM +0200, Claudio Jeker wrote:
> This diff reworks the pfkey code to handle ipsec and tcpmd5 to be
> independent of struct peer. Instead use struct auth_config and struct
> auth_state. Make sure the auth_config (which includes the secrets, remains
> in the parent (apart from -portable where Linux needs the md5 keys in the
> SE to work)). Only the auth_config method is shared with the SE since it
> is needed to turn on the TCP_MD5SIG socketopts.
> By spliting the auth_config out of peer_config the sharing can be better
> controlled. To be extra sure adjust the control_imsg_relay() function to
> double clear the buffer so it is not sent to any bgpctl tool.

I assume that you use memset() instead of explicit_bzero() because of
portable?

pfkey_remove() has an if (as->established) check at the start but all the
callers in pfkey.c check that beforehand. So these callers could be simplified.

Minor comments below.

ok tb

> Index: bgpd/pfkey.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/pfkey.c,v
> diff -u -p -r1.68 pfkey.c
> --- bgpd/pfkey.c	7 Nov 2022 22:39:13 -0000	1.68
> +++ bgpd/pfkey.c	27 Sep 2024 14:54:23 -0000

[...]

> +static int
>  pfkey_send(int sd, uint8_t satype, uint8_t mtype, uint8_t dir,
> -    struct bgpd_addr *src, struct bgpd_addr *dst, uint32_t spi,
> +    const struct bgpd_addr *src, const struct bgpd_addr *dst, uint32_t spi,
>      uint8_t aalg, int alen, char *akey, uint8_t ealg, int elen, char *ekey,
>      uint16_t sport, uint16_t dport)
>  {
> @@ -385,7 +373,7 @@ pfkey_send(int sd, uint8_t satype, uint8
>  			iov[iov_cnt].iov_base = &sa_akey;
>  			iov[iov_cnt].iov_len = sizeof(sa_akey);
>  			iov_cnt++;
> -			iov[iov_cnt].iov_base = akey;
> +			iov[iov_cnt].iov_base = ((void *)akey);

no need to add casts here.

>  			iov[iov_cnt].iov_len = ((alen + 7) / 8) * 8;
>  			smsg.sadb_msg_len += sa_akey.sadb_key_len;
>  			iov_cnt++;
> @@ -395,7 +383,7 @@ pfkey_send(int sd, uint8_t satype, uint8
>  			iov[iov_cnt].iov_base = &sa_ekey;
>  			iov[iov_cnt].iov_len = sizeof(sa_ekey);
>  			iov_cnt++;
> -			iov[iov_cnt].iov_base = ekey;
> +			iov[iov_cnt].iov_base = ((void *)ekey);

same here

[...]

> Index: bgpd/session.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/bgpd/session.c,v
> diff -u -p -r1.482 session.c
> --- bgpd/session.c	9 Sep 2024 12:59:49 -0000	1.482
> +++ bgpd/session.c	27 Sep 2024 14:54:23 -0000
> @@ -1032,14 +1032,15 @@ session_accept(int listenfd)
>  		}
>  
>  open:
> -		if (p->conf.auth.method != AUTH_NONE && sysdep.no_pfkey) {
> +		if (p->auth_conf.method != AUTH_NONE && sysdep.no_pfkey) {
>  			log_peer_warnx(&p->conf,
>  			    "ipsec or md5sig configured but not available");
>  			close(connfd);
>  			return;
>  		}
>  
> -		if (tcp_md5_check(connfd, p) == -1) {
> +		if (tcp_md5_check(connfd, &p->auth_conf) == -1) {
> +			log_peer_warnx(&p->conf, "check md5sig");

I think this should be log_peer_warn() since you now explicitly set errno
in tcp_md5_check().

>  			close(connfd);
>  			return;
>  		}

[...]