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.

> 
> -- 
> :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;
>  }
>