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

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

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