From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: bgpd: add tcp md5sum and ipsec support for rtr sessions
To: Theo Buehler <tb@theobuehler.org>
Cc: tech@openbsd.org
Date: Wed, 9 Oct 2024 11:32:49 +0200

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.
The error is a bit non-specific but *shrug* until now nobody complained
 
> > +		}
> > +		yyerror("auth method cannot be redefined");
> > +		return 0;
> > +	}
> > +	*to = *from;
> > +	return 1;
> > +}
> > +
> 

-- 
:wq Claudio