From: Theo Buehler <tb@theobuehler.org>
Subject: Re: bgpd: add tcp md5sum and ipsec support for rtr sessions
To: tech@openbsd.org
Date: Wed, 9 Oct 2024 11:51:07 +0200

On Wed, Oct 09, 2024 at 11:32:49AM +0200, Claudio Jeker wrote:
> On Wed, Oct 09, 2024 at 11:22:19AM +0200, Theo Buehler wrote:
> > On Wed, Oct 09, 2024 at 10:34:20AM +0200, Claudio Jeker wrote:
> > > This adds the parse.y and printconf.c bits to configure tcp md5sum and
> > > ipsec for rtr sessions.
> > > 
> > > I tested that this does not break tcp md5 for BGP sessions but I have no
> > > rtr cache that supports tcp md5 at hand so that part is untested. Also
> > > ipsec is untested.
> > > 
> > > As usual with touching parse.y it can't be easy. I wanted to use one
> > > single set of yacc rules for the auth_config.  Since manual IPsec
> > > requires two flows (in and out) one needs to merge the authconf and
> > > so there is now merge_auth_conf() with a overly complex if statement.
> > > I hope I got that one right.
> > 
> > It reads fine.
> > 
> > ok tb
> > 
> > Just one thing:
> > 
> > > +static int
> > > +merge_auth_conf(struct auth_config *to, struct auth_config *from)
> > > +{
> > > +	if (to->method != 0) {
> > > +		/* extra magic for manual ipsec rules */
> > > +		if (to->method == from->method &&
> > > +		    (to->method == AUTH_IPSEC_MANUAL_ESP ||
> > > +		    to->method == AUTH_IPSEC_MANUAL_AH)) {
> > > +			if (to->spi_in == 0 && from->spi_in != 0) {
> > > +				to->spi_in = from->spi_in;
> > > +				to->auth_alg_in = from->auth_alg_in;
> > > +				to->enc_alg_in = from->enc_alg_in;
> > > +				memcpy(to->enc_key_in, from->enc_key_in,
> > > +				    sizeof(to->enc_key_in));
> > > +				to->enc_keylen_in = from->enc_keylen_in;
> > > +				to->auth_keylen_in = from->auth_keylen_in;
> > > +				return 1;
> > > +			} else if (to->spi_out == 0 && from->spi_out != 0) {
> > > +				to->spi_out = from->spi_out;
> > > +				to->auth_alg_out = from->auth_alg_out;
> > > +				to->enc_alg_out = from->enc_alg_out;
> > > +				memcpy(to->enc_key_out, from->enc_key_out,
> > > +				    sizeof(to->enc_key_out));
> > > +				to->enc_keylen_out = from->enc_keylen_out;
> > > +				to->auth_keylen_out = from->auth_keylen_out;
> > > +				return 1;
> > > +			}
> > 
> > Should we really "fall through" and raise the redefined auth method
> > error from here? Feels like something else is badly wrong.
> 
> This is as far as I can tell similar to what the current code does:
> 
>                         if (curpeer->auth_conf.method &&
>                             (((curpeer->auth_conf.spi_in && $3 == 1) ||
>                             (curpeer->auth_conf.spi_out && $3 == 0)) ||
>                             ($2 == 1 && curpeer->auth_conf.method !=
>                             AUTH_IPSEC_MANUAL_ESP) ||
>                             ($2 == 0 && curpeer->auth_conf.method !=
>                             AUTH_IPSEC_MANUAL_AH))) {
>                                 yyerror("auth method cannot be redefined");
> 
> Now that if statement is even worse so I may have missed something.

I don't think you did.

> The error is a bit non-specific but *shrug* until now nobody complained

fair. It wasn't meant as an objection :)

>  
> > > +		}
> > > +		yyerror("auth method cannot be redefined");
> > > +		return 0;
> > > +	}
> > > +	*to = *from;
> > > +	return 1;
> > > +}
> > > +
> > 
> 
> -- 
> :wq Claudio