From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: rework pfkey (ipsec/tcpmd5) code
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Tue, 1 Oct 2024 13:21:06 +0200

On Tue, Oct 01, 2024 at 11:23:30AM +0200, Theo Buehler wrote:
> 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?

No, this code should probably use explicit_bzero(). I changed the memset()
in control.c. That's the only one right?
AFAIK -portable already includes explicit_bzero().

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

I think you're right but I prefer to do this in a 2nd step. Also need to
double check that the behaviour is right in all cases (especially on
session restart when a new key is installed). It got better since the
state is now update in one place.
 
> 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.

yes, leftover from a try to make akey also const.
 
> > 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().

Changed.
 
> >  			close(connfd);
> >  			return;
> >  		}
> 

Thanks

-- 
:wq Claudio