From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rework rpki-client certificate discovery
To: Theo Buehler <tb@theobuehler.org>
Cc: Job Snijders <job@openbsd.org>, tech@openbsd.org
Date: Mon, 20 May 2024 15:44:06 +0200

On Mon, May 20, 2024 at 03:40:21PM +0200, Theo Buehler wrote:
> On Mon, May 20, 2024 at 02:49:30PM +0200, Claudio Jeker wrote:
> > On Thu, May 16, 2024 at 06:41:09PM +0000, Job Snijders wrote:
> > > Dear Claudio, tb,
> > > 
> > > Thank you for your work on this! Two nits:
> > > 
> > > On Thu, May 16, 2024 at 12:51:17PM +0200, Claudio Jeker wrote:
> > > > +	if (issuer == NULL) {
> > > > +		cert->certid = cert->talid;
> > > > +	} else {
> > > > +		cert->certid = ++certid;
> > > > +		if (certid > CERTID_MAX)
> > > > +			err(1, "%s: too many certificates", fn);
> > > 
> > > Is err() here the best approach? Could an alternative be to stop
> > > processing additional CA certs, cobble on, write output files, and exit
> > > with a non-zero error exit code to alert the operator? So for example
> > > return NULL instead of adding the CA cert to the auth tree.
> > > 
> > > > +		na->depth = issuer->depth + 1;
> > > > +	}
> > > > +
> > > > +	if (na->depth >= MAX_CERT_DEPTH) {
> > > > +		warnx("%s: stack depth exhausted", fn);
> > > 
> > > maybe: warnx("%s: maximum certificate chain depth exhausted", fn); ?
> > > 
> > > Other than that things look good. I've loaded this diff on
> > > console.rpki-client.org and some other systems. Let's have this run over
> > > the weekend before committing.
> > > 
> > 
> > See below on how I would solve the two bits above.
> > Now if you hit the limit the code will spam you with many "unable to get
> > local issuer certificate" errors plus "RFC 6487: unknown cert with SKI
> > HEXDUMPOFSKI". So there is a fair bit of spam at least there is no
> > additional spam from auth_insert().
> 
> With that change rpki-client will still exit with 0, right? I don't
> think we want to load this config containing predominantly garbage
> from the rogue CA into bgpd.
> 
> I wonder if we're not better off landing the diff without this
> particular nit fixed and do this as a separate diff afterward.

Happy to do so. It is indeed a bit more complex when we hit the limit.
Doing an errx(1) there solves many problems :)
 
> > 
> > -- 
> > :wq Claudio
> > 
> >  struct auth *
> > -auth_insert(struct auth_tree *auths, struct cert *cert, struct auth *issuer)
> > +auth_insert(const char *fn, struct auth_tree *auths, struct cert *cert,
> > +    struct auth *issuer)
> >  {
> >  	struct auth *na;
> >  
> > -	na = malloc(sizeof(*na));
> > +	na = calloc(1, sizeof(*na));
> >  	if (na == NULL)
> >  		err(1, NULL);
> >  
> > +	if (issuer == NULL) {
> > +		cert->certid = cert->talid;
> > +	} else {
> > +		cert->certid = ++certid;
> > +		if (certid > CERTID_MAX) {
> > +			if (certid == CERTID_MAX + 1)
> > +				warnx("%s: too many certificates in store", fn);
> > +			free(na);
> > +			return NULL;
> > +		}
> > +		na->depth = issuer->depth + 1;
> > +	}
> > +
> > +	if (na->depth >= MAX_CERT_DEPTH) {
> > +		warnx("%s: maximum certificate chain depth exhausted", fn);
> > +		free(na);
> > +		return NULL;
> > +	}
> > +
> >  	na->issuer = issuer;
> >  	na->cert = cert;
> >  	na->any_inherits = x509_any_inherits(cert->x509);
> >  
> >  	if (RB_INSERT(auth_tree, auths, na) != NULL)
> > -		err(1, "auth tree corrupted");
> > +		errx(1, "auth tree corrupted");
> >  
> >  	return na;
> >  }
> > 
> 

-- 
:wq Claudio