From: Theo Buehler <tb@theobuehler.org>
Subject: Re: rework rpki-client certificate discovery
To: tech@openbsd.org
Date: Wed, 15 May 2024 20:04:22 +0200

On Wed, May 15, 2024 at 05:04:57PM +0200, Claudio Jeker wrote:
> In rpki-client we use the SKI / AKI to find the signing certificate and
> build the validation stack based on this data. The problem is that the SKI
> is not strictly unique and for example the NRO TAL abuses this.
> 
> Instead of indexing certs by SKI we now use a unique per certificate id
> which is then passed back and forth between main process and the parser.
> This way we never lose track on the cert that signed the current object
> and can look that one up via its id.  This allows for multiple validation
> paths to co-exist.

I never liked the auth lookup by SKI and I am really happy to see it go.
I've long suspected that we throw away important information and the new
approach shows that this is indeed the case. I am much happier with this.

The comparison of the aki with the issuer's ski ensures that we don't
throw away information - this is redundant with check_issuer()'s call to
X509_check_akid() in the verifier anyway.

One other thing worth mentioning is that by keeping track of the cert
depth in auth_insert() we reinstate the maximum depth check that was
lost when doing the partial chain optimization.

> Now on top of this the filepath_add() api had to be extended since we now
> allow a file to be visited more then once as long as it is reached by a
> different TAL. Inside the same TAL an object can not be revisited.
> 
> Last but not least the filemode had to be adjusted, now the code keeps
> track of the various AIA uri and uses an own rb tree to look them up.
> Once there is a hit the stack can be unrolled like before and all new
> certs are added to both the auth and the uripath trees.

I think this is also an improvement.

> More cleanup needs to follow (I'm looking at you valid_aki_ski())
> 
> This was developped together with tb@ at a femto-hackathon.

A nit and a bug in the beloved valid_ski_aki() below, with those fixed

ok tb

But I'd be happy to see some more testing.

> -- 
> :wq Claudio
> 
> Index: cert.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
> diff -u -p -r1.130 cert.c
> --- cert.c	21 Apr 2024 19:27:44 -0000	1.130
> +++ cert.c	15 May 2024 13:43:14 -0000
> @@ -34,6 +34,8 @@ extern ASN1_OBJECT	*carepo_oid;	/* 1.3.6
>  extern ASN1_OBJECT	*manifest_oid;	/* 1.3.6.1.5.5.7.48.10 (rpkiManifest) */
>  extern ASN1_OBJECT	*notify_oid;	/* 1.3.6.1.5.5.7.48.13 (rpkiNotify) */
>  
> +static int certid = TALSZ_MAX;
> +
>  /*
>   * Append an IP address structure to our list of results.
>   * This will also constrain us to having at most one inheritance
> @@ -1103,6 +1105,7 @@ cert_buffer(struct ibuf *b, const struct
>  	io_simple_buffer(b, &p->notafter, sizeof(p->notafter));
>  	io_simple_buffer(b, &p->purpose, sizeof(p->purpose));
>  	io_simple_buffer(b, &p->talid, sizeof(p->talid));
> +	io_simple_buffer(b, &p->certid, sizeof(p->certid));
>  	io_simple_buffer(b, &p->repoid, sizeof(p->repoid));
>  	io_simple_buffer(b, &p->ipsz, sizeof(p->ipsz));
>  	io_simple_buffer(b, &p->asz, sizeof(p->asz));
> @@ -1136,6 +1139,7 @@ cert_read(struct ibuf *b)
>  	io_read_buf(b, &p->notafter, sizeof(p->notafter));
>  	io_read_buf(b, &p->purpose, sizeof(p->purpose));
>  	io_read_buf(b, &p->talid, sizeof(p->talid));
> +	io_read_buf(b, &p->certid, sizeof(p->certid));
>  	io_read_buf(b, &p->repoid, sizeof(p->repoid));
>  	io_read_buf(b, &p->ipsz, sizeof(p->ipsz));
>  	io_read_buf(b, &p->asz, sizeof(p->asz));
> @@ -1167,7 +1171,11 @@ cert_read(struct ibuf *b)
>  static inline int
>  authcmp(struct auth *a, struct auth *b)
>  {
> -	return strcmp(a->cert->ski, b->cert->ski);
> +	if (a->cert->certid > b->cert->certid)
> +		return 1;
> +	if (a->cert->certid < b->cert->certid)
> +		return -1;
> +	return 0;
>  }
>  
>  RB_GENERATE_STATIC(auth_tree, auth, entry, authcmp);
> @@ -1185,33 +1193,48 @@ auth_tree_free(struct auth_tree *auths)
>  }
>  
>  struct auth *
> -auth_find(struct auth_tree *auths, const char *aki)
> +auth_find(struct auth_tree *auths, int id)
>  {
>  	struct auth a;
>  	struct cert c;
>  
> -	/* we look up the cert where the ski == aki */
> -	c.ski = (char *)aki;
> +	c.certid = id;
>  	a.cert = &c;
>  
>  	return RB_FIND(auth_tree, auths, &a);
>  }
>  
>  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)
> +			err(1, "%s: too many certificates", fn);
> +		na->depth = issuer->depth + 1;
> +	}
> +
> +	if (na->depth >= MAX_CERT_DEPTH) {
> +		warnx("%s: stack 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;
>  }
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> diff -u -p -r1.217 extern.h
> --- extern.h	21 Apr 2024 19:27:44 -0000	1.217
> +++ extern.h	15 May 2024 14:41:35 -0000
> @@ -24,6 +24,9 @@
>  #include <openssl/x509.h>
>  #include <openssl/x509v3.h>
>  
> +#define CTASSERT(x)	extern char  _ctassert[(x) ? 1 : -1 ] \
> +			    __attribute__((__unused__))
> +
>  enum cert_as_type {
>  	CERT_AS_ID, /* single identifier */
>  	CERT_AS_INHERIT, /* inherit from issuer */
> @@ -120,6 +123,7 @@ struct cert {
>  	struct cert_as	*as; /* list of AS numbers and ranges */
>  	size_t		 asz; /* length of "asz" */
>  	int		 talid; /* cert is covered by which TAL */
> +	int		 certid;
>  	unsigned int	 repoid; /* repository of this cert file */
>  	char		*repo; /* CA repository (rsync:// uri) */
>  	char		*mft; /* manifest (rsync:// uri) */
> @@ -212,6 +216,7 @@ struct mft {
>  	size_t		 filesz; /* number of filenames */
>  	unsigned int	 repoid;
>  	int		 talid;
> +	int		 certid;
>  };
>  
>  /*
> @@ -495,14 +500,16 @@ struct auth {
>  	struct cert	*cert; /* owner information */
>  	struct auth	*issuer; /* pointer to issuer or NULL for TA cert */
>  	int		 any_inherits;
> +	int		 depth;
>  };
>  /*
>   * Tree of auth sorted by ski
>   */
>  RB_HEAD(auth_tree, auth);
>  
> -struct auth	*auth_find(struct auth_tree *, const char *);
> -struct auth	*auth_insert(struct auth_tree *, struct cert *, struct auth *);
> +struct auth	*auth_find(struct auth_tree *, int);
> +struct auth	*auth_insert(const char *, struct auth_tree *, struct cert *,
> +		    struct auth *);
>  
>  enum http_result {
>  	HTTP_FAILED,	/* anything else */
> @@ -560,6 +567,7 @@ struct entity {
>  	size_t		 datasz;	/* length of optional data blob */
>  	unsigned int	 repoid;	/* repository identifier */
>  	int		 talid;		/* tal identifier */
> +	int		 certid;
>  	enum rtype	 type;		/* type of entity (not RTYPE_EOF) */
>  	enum location	 location;	/* which directory the file lives in */
>  };
> @@ -731,10 +739,8 @@ void		 crl_tree_free(struct crl_tree *);
>  
>  /* Validation of our objects. */
>  
> -struct auth	*valid_ski_aki(const char *, struct auth_tree *,
> +struct auth	*valid_ski_aki(const char *, struct auth_tree *, int,
>  		    const char *, const char *, const char *);
> -int		 valid_ta(const char *, struct auth_tree *,
> -		    const struct cert *);
>  int		 valid_cert(const char *, struct auth *, const struct cert *);
>  int		 valid_roa(const char *, struct cert *, struct roa *);
>  int		 valid_filehash(int, const char *, size_t);
> @@ -826,7 +832,7 @@ void		 proc_http(char *, int) __attribut
>  void		 proc_rrdp(int) __attribute__((noreturn));
>  
>  /* Repository handling */
> -int		 filepath_add(struct filepath_tree *, char *, time_t);
> +int		 filepath_add(struct filepath_tree *, char *, int, time_t);
>  void		 rrdp_clear(unsigned int);
>  void		 rrdp_session_save(unsigned int, struct rrdp_session *);
>  void		 rrdp_session_free(struct rrdp_session *);
> @@ -981,6 +987,7 @@ int	mkpathat(int, const char *);
>  
>  /* Maximum number of TAL files we'll load. */
>  #define	TALSZ_MAX		8
> +#define	CERTID_MAX		1000000
>  
>  /*
>   * Maximum number of elements in the sbgp-ipAddrBlock (IP) and
> Index: filemode.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> diff -u -p -r1.41 filemode.c
> --- filemode.c	21 Apr 2024 19:27:44 -0000	1.41
> +++ filemode.c	15 May 2024 14:42:41 -0000
> @@ -47,6 +47,50 @@ static struct crl_tree	 crlt = RB_INITIA
>  
>  struct tal		*talobj[TALSZ_MAX];
>  
> +struct uripath {
> +	RB_ENTRY(uripath)	entry;
> +	const char		*uri;
> +	struct cert		*cert;
> +};
> +
> +static RB_HEAD(uripath_tree, uripath) uritree;
> +
> +static inline int
> +uripathcmp(const struct uripath *a, const struct uripath *b)
> +{
> +	return strcmp(a->uri, b->uri);
> +}
> +
> +RB_PROTOTYPE(uripath_tree, uripath, entry, uripathcmp);
> +
> +static void
> +uripath_add(const char *uri, struct cert *cert)
> +{
> +	struct uripath *up;
> +
> +	if ((up = calloc(1, sizeof(*up))) == NULL)
> +		err(1, NULL);
> +	if ((up->uri = strdup(uri)) == NULL)
> +		err(1, NULL);
> +	up->cert = cert;
> +	if (RB_INSERT(uripath_tree, &uritree, up) != NULL)
> +		errx(1, "corrupt AIA lookup tree");
> +}
> +
> +static struct cert *
> +uripath_lookup(const char *uri)
> +{
> +	struct uripath needle = { .uri = uri };
> +	struct uripath *up;
> +
> +	up = RB_FIND(uripath_tree, &uritree, &needle);
> +	if (up == NULL)
> +		return NULL;
> +	return up->cert;
> +}
> +
> +RB_GENERATE(uripath_tree, uripath, entry, uripathcmp);
> +
>  /*
>   * Use the X509 CRL Distribution Points to locate the CRL needed for
>   * verification.
> @@ -132,7 +176,7 @@ parse_load_cert(char *uri)
>   * tree. Once the TA is located in the chain the chain is validated in
>   * reverse order.
>   */
> -static void
> +static struct auth *
>  parse_load_certchain(char *uri)
>  {
>  	struct cert *stack[MAX_CERT_DEPTH] = { 0 };
> @@ -144,18 +188,16 @@ parse_load_certchain(char *uri)
>  	int i;
>  
>  	for (i = 0; i < MAX_CERT_DEPTH; i++) {
> +		if ((cert = uripath_lookup(uri)) != NULL) {
> +			a = auth_find(&auths, cert->certid);
> +			break;
> +		}
>  		filestack[i] = uri;
>  		stack[i] = cert = parse_load_cert(uri);
>  		if (cert == NULL || cert->purpose != CERT_PURPOSE_CA) {
> -			warnx("failed to build authority chain");
> -			goto fail;
> -		}
> -		if (auth_find(&auths, cert->ski) != NULL) {
> -			assert(i == 0);
> +			warnx("failed to build authority chain: %s", uri);
>  			goto fail;
>  		}
> -		if ((a = auth_find(&auths, cert->aki)) != NULL)
> -			break;	/* found chain to TA */
>  		uri = cert->aia;
>  	}
>  
> @@ -166,9 +208,9 @@ parse_load_certchain(char *uri)
>  	}
>  
>  	/* TA found play back the stack and add all certs */
> -	for (; i >= 0; i--) {
> -		cert = stack[i];
> -		uri = filestack[i];
> +	for (; i > 0; i--) {
> +		cert = stack[i - 1];
> +		uri = filestack[i - 1];
>  
>  		crl = crl_get(&crlt, a);
>  		if (!valid_x509(uri, ctx, cert->x509, a, crl, &errstr) ||
> @@ -178,14 +220,16 @@ parse_load_certchain(char *uri)
>  			goto fail;
>  		}
>  		cert->talid = a->cert->talid;
> -		a = auth_insert(&auths, cert, a);
> +		a = auth_insert(uri, &auths, cert, a);
> +		uripath_add(uri, cert);
>  		stack[i] = NULL;
>  	}
>  
> -	return;
> +	return a;
>  fail:
>  	for (i = 0; i < MAX_CERT_DEPTH; i++)
>  		cert_free(stack[i]);
> +	return NULL;
>  }
>  
>  static void
> @@ -195,7 +239,7 @@ parse_load_ta(struct tal *tal)
>  	struct cert *cert;
>  	unsigned char *f = NULL;
>  	char *file;
> -	size_t flen;
> +	size_t flen, i;
>  
>  	/* does not matter which URI, all end with same filename */
>  	filename = strrchr(tal->uri[0], '/');
> @@ -217,11 +261,14 @@ parse_load_ta(struct tal *tal)
>  		goto out;
>  
>  	cert->talid = tal->id;
> +	auth_insert(file, &auths, cert, NULL);
> +	for (i = 0; i < tal->urisz; i++) {
> +		if (strncasecmp(tal->uri[i], RSYNC_PROTO, RSYNC_PROTO_LEN) != 0)
> +			continue;
> +		/* Add all rsync uri since any of them could be use as AIA. */

my typo: s/use/used

> +		uripath_add(tal->uri[i], cert);
> +	}
>  
> -	if (!valid_ta(file, &auths, cert))
> -		cert_free(cert);
> -	else
> -		auth_insert(&auths, cert, NULL);
>  out:
>  	free(file);
>  	free(f);
> @@ -297,7 +344,7 @@ proc_parser_file(char *file, unsigned ch
>  	struct spl *spl = NULL;
>  	struct tak *tak = NULL;
>  	struct tal *tal = NULL;
> -	char *aia = NULL, *aki = NULL;
> +	char *aia = NULL;
>  	char *crl_uri = NULL;
>  	time_t *expires = NULL, *notafter = NULL;
>  	struct auth *a;
> @@ -349,7 +396,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (aspa == NULL)
>  			break;
>  		aia = aspa->aia;
> -		aki = aspa->aki;
>  		expires = &aspa->expires;
>  		notafter = &aspa->notafter;
>  		break;
> @@ -363,7 +409,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (cert == NULL)
>  			break;
>  		aia = cert->aia;
> -		aki = cert->aki;
>  		x509 = cert->x509;
>  		if (X509_up_ref(x509) == 0)
>  			errx(1, "%s: X509_up_ref failed", __func__);
> @@ -381,7 +426,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (mft == NULL)
>  			break;
>  		aia = mft->aia;
> -		aki = mft->aki;
>  		expires = &mft->expires;
>  		notafter = &mft->nextupdate;
>  		break;
> @@ -390,7 +434,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (gbr == NULL)
>  			break;
>  		aia = gbr->aia;
> -		aki = gbr->aki;
>  		expires = &gbr->expires;
>  		notafter = &gbr->notafter;
>  		break;
> @@ -399,7 +442,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (geofeed == NULL)
>  			break;
>  		aia = geofeed->aia;
> -		aki = geofeed->aki;
>  		expires = &geofeed->expires;
>  		notafter = &geofeed->notafter;
>  		break;
> @@ -408,7 +450,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (roa == NULL)
>  			break;
>  		aia = roa->aia;
> -		aki = roa->aki;
>  		expires = &roa->expires;
>  		notafter = &roa->notafter;
>  		break;
> @@ -417,7 +458,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (rsc == NULL)
>  			break;
>  		aia = rsc->aia;
> -		aki = rsc->aki;
>  		expires = &rsc->expires;
>  		notafter = &rsc->notafter;
>  		break;
> @@ -426,7 +466,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (spl == NULL)
>  			break;
>  		aia = spl->aia;
> -		aki = spl->aki;
>  		expires = &spl->expires;
>  		notafter = &spl->notafter;
>  		break;
> @@ -435,7 +474,6 @@ proc_parser_file(char *file, unsigned ch
>  		if (tak == NULL)
>  			break;
>  		aia = tak->aia;
> -		aki = tak->aki;
>  		expires = &tak->expires;
>  		notafter = &tak->notafter;
>  		break;
> @@ -453,9 +491,7 @@ proc_parser_file(char *file, unsigned ch
>  	if (aia != NULL) {
>  		x509_get_crl(x509, file, &crl_uri);
>  		parse_load_crl(crl_uri);
> -		if (auth_find(&auths, aki) == NULL)
> -			parse_load_certchain(aia);
> -		a = auth_find(&auths, aki);
> +		a = parse_load_certchain(aia);
>  		c = crl_get(&crlt, a);
>  
>  		if ((status = valid_x509(file, ctx, x509, a, c, &errstr))) {
> Index: main.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
> diff -u -p -r1.257 main.c
> --- main.c	8 Apr 2024 14:02:13 -0000	1.257
> +++ main.c	14 May 2024 14:58:48 -0000
> @@ -156,6 +156,7 @@ entity_read_req(struct ibuf *b, struct e
>  	io_read_buf(b, &ent->location, sizeof(ent->location));
>  	io_read_buf(b, &ent->repoid, sizeof(ent->repoid));
>  	io_read_buf(b, &ent->talid, sizeof(ent->talid));
> +	io_read_buf(b, &ent->certid, sizeof(ent->certid));
>  	io_read_str(b, &ent->path);
>  	io_read_str(b, &ent->file);
>  	io_read_str(b, &ent->mftaki);
> @@ -176,6 +177,7 @@ entity_write_req(const struct entity *en
>  	io_simple_buffer(b, &ent->location, sizeof(ent->location));
>  	io_simple_buffer(b, &ent->repoid, sizeof(ent->repoid));
>  	io_simple_buffer(b, &ent->talid, sizeof(ent->talid));
> +	io_simple_buffer(b, &ent->certid, sizeof(ent->certid));
>  	io_str_buffer(b, ent->path);
>  	io_str_buffer(b, ent->file);
>  	io_str_buffer(b, ent->mftaki);
> @@ -191,7 +193,7 @@ entity_write_repo(const struct repo *rp)
>  	enum location loc = DIR_UNKNOWN;
>  	unsigned int repoid;
>  	char *path, *altpath;
> -	int talid = 0;
> +	int talid = 0, certid = 0;
>  
>  	repoid = repo_id(rp);
>  	path = repo_basedir(rp, 0);
> @@ -201,6 +203,7 @@ entity_write_repo(const struct repo *rp)
>  	io_simple_buffer(b, &loc, sizeof(loc));
>  	io_simple_buffer(b, &repoid, sizeof(repoid));
>  	io_simple_buffer(b, &talid, sizeof(talid));
> +	io_simple_buffer(b, &certid, sizeof(certid));
>  	io_str_buffer(b, path);
>  	io_str_buffer(b, altpath);
>  	io_buf_buffer(b, NULL, 0); /* ent->mftaki */
> @@ -233,7 +236,7 @@ entityq_flush(struct entityq *q, struct 
>   */
>  static void
>  entityq_add(char *path, char *file, enum rtype type, enum location loc,
> -    struct repo *rp, unsigned char *data, size_t datasz, int talid,
> +    struct repo *rp, unsigned char *data, size_t datasz, int talid, int certid,
>      char *mftaki)
>  {
>  	struct entity	*p;
> @@ -244,6 +247,7 @@ entityq_add(char *path, char *file, enum
>  	p->type = type;
>  	p->location = loc;
>  	p->talid = talid;
> +	p->certid = certid;
>  	p->mftaki = mftaki;
>  	p->path = path;
>  	if (rp != NULL)
> @@ -419,7 +423,7 @@ queue_add_from_mft(const struct mft *mft
>  		if ((mftaki = strdup(mft->aki)) == NULL)
>  			err(1, NULL);
>  		entityq_add(npath, nfile, f->type, f->location, rp, NULL, 0,
> -		    mft->talid, mftaki);
> +		    mft->talid, mft->certid, mftaki);
>  	}
>  }
>  
> @@ -442,7 +446,7 @@ queue_add_file(const char *file, enum rt
>  	if ((nfile = strdup(file)) == NULL)
>  		err(1, NULL);
>  	/* Not in a repository, so directly add to queue. */
> -	entityq_add(NULL, nfile, type, DIR_UNKNOWN, NULL, buf, len, talid,
> +	entityq_add(NULL, nfile, type, DIR_UNKNOWN, NULL, buf, len, talid, 0,
>  	    NULL);
>  }
>  
> @@ -478,7 +482,7 @@ queue_add_from_tal(struct tal *tal)
>  	data = tal->pkey;
>  	tal->pkey = NULL;
>  	entityq_add(NULL, nfile, RTYPE_CER, DIR_VALID, repo, data,
> -	    tal->pkeysz, tal->id, NULL);
> +	    tal->pkeysz, tal->id, tal->id, NULL);
>  }
>  
>  /*
> @@ -547,7 +551,7 @@ queue_add_from_cert(const struct cert *c
>  	}
>  
>  	entityq_add(npath, nfile, RTYPE_MFT, DIR_UNKNOWN, repo, NULL, 0,
> -	    cert->talid, NULL);
> +	    cert->talid, cert->certid, NULL);
>  }
>  
>  /*
> @@ -591,7 +595,7 @@ entity_process(struct ibuf *b, struct st
>  	if (filemode)
>  		goto done;
>  
> -	if (filepath_add(&fpt, file, mtime) == 0) {
> +	if (filepath_add(&fpt, file, talid, mtime) == 0) {
>  		warnx("%s: File already visited", file);
>  		goto done;
>  	}
> Index: mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> diff -u -p -r1.113 mft.c
> --- mft.c	20 Apr 2024 15:45:41 -0000	1.113
> +++ mft.c	14 May 2024 13:47:40 -0000
> @@ -537,6 +537,7 @@ mft_buffer(struct ibuf *b, const struct 
>  
>  	io_simple_buffer(b, &p->repoid, sizeof(p->repoid));
>  	io_simple_buffer(b, &p->talid, sizeof(p->talid));
> +	io_simple_buffer(b, &p->certid, sizeof(p->certid));
>  	io_str_buffer(b, p->path);
>  
>  	io_str_buffer(b, p->aia);
> @@ -569,6 +570,7 @@ mft_read(struct ibuf *b)
>  
>  	io_read_buf(b, &p->repoid, sizeof(p->repoid));
>  	io_read_buf(b, &p->talid, sizeof(p->talid));
> +	io_read_buf(b, &p->certid, sizeof(p->certid));
>  	io_read_str(b, &p->path);
>  
>  	io_read_str(b, &p->aia);
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> diff -u -p -r1.135 parser.c
> --- parser.c	21 Apr 2024 19:27:44 -0000	1.135
> +++ parser.c	15 May 2024 10:23:45 -0000
> @@ -137,7 +137,8 @@ proc_parser_roa(char *file, const unsign
>  	if ((roa = roa_parse(&x509, file, entp->talid, der, len)) == NULL)
>  		return NULL;
>  
> -	a = valid_ski_aki(file, &auths, roa->ski, roa->aki, entp->mftaki);
> +	a = valid_ski_aki(file, &auths, entp->certid, roa->ski, roa->aki,
> +	    entp->mftaki);
>  	crl = crl_get(&crlt, a);
>  
>  	if (!valid_x509(file, ctx, x509, a, crl, &errstr)) {
> @@ -172,7 +173,8 @@ proc_parser_spl(char *file, const unsign
>  	if ((spl = spl_parse(&x509, file, entp->talid, der, len)) == NULL)
>  		return NULL;
>  
> -	a = valid_ski_aki(file, &auths, spl->ski, spl->aki, entp->mftaki);
> +	a = valid_ski_aki(file, &auths, entp->certid, spl->ski, spl->aki,
> +	    entp->mftaki);
>  	crl = crl_get(&crlt, a);
>  
>  	if (!valid_x509(file, ctx, x509, a, crl, &errstr)) {
> @@ -336,7 +338,7 @@ proc_parser_mft_pre(struct entity *entp,
>  	if (*crl == NULL)
>  		*crl = parse_load_crl_from_mft(entp, mft, DIR_VALID, crlfile);
>  
> -	a = valid_ski_aki(file, &auths, mft->ski, mft->aki, NULL);
> +	a = valid_ski_aki(file, &auths, entp->certid, mft->ski, mft->aki, NULL);
>  	if (!valid_x509(file, ctx, x509, a, *crl, errstr))
>  		goto err;
>  	X509_free(x509);
> @@ -344,6 +346,7 @@ proc_parser_mft_pre(struct entity *entp,
>  
>  	mft->repoid = entp->repoid;
>  	mft->talid = a->cert->talid;
> +	mft->certid = entp->certid;
>  
>  	now = get_current_time();
>  	/* check that now is not before from */
> @@ -493,7 +496,7 @@ proc_parser_mft(struct entity *entp, str
>   */
>  static struct cert *
>  proc_parser_cert(char *file, const unsigned char *der, size_t len,
> -    const char *mftaki)
> +    const struct entity *entp)
>  {
>  	struct cert	*cert;
>  	struct crl	*crl;
> @@ -507,7 +510,8 @@ proc_parser_cert(char *file, const unsig
>  	if (cert == NULL)
>  		return NULL;
>  
> -	a = valid_ski_aki(file, &auths, cert->ski, cert->aki, mftaki);
> +	a = valid_ski_aki(file, &auths, entp->certid, cert->ski, cert->aki,
> +	    entp->mftaki);
>  	crl = crl_get(&crlt, a);
>  
>  	if (!valid_x509(file, ctx, cert->x509, a, crl, &errstr) ||
> @@ -531,7 +535,7 @@ proc_parser_cert(char *file, const unsig
>  	 * Add validated CA certs to the RPKI auth tree.
>  	 */
>  	if (cert->purpose == CERT_PURPOSE_CA)
> -		auth_insert(&auths, cert, a);
> +		auth_insert(file, &auths, cert, a);
>  
>  	return cert;
>  }
> @@ -557,19 +561,12 @@ proc_parser_root_cert(char *file, const 
>  	cert = ta_parse(file, cert, pkey, pkeysz);
>  	if (cert == NULL)
>  		return NULL;
> -
> -	if (!valid_ta(file, &auths, cert)) {
> -		warnx("%s: certificate not a valid ta", file);
> -		cert_free(cert);
> -		return NULL;
> -	}
> -
>  	cert->talid = talid;
>  
>  	/*
>  	 * Add valid roots to the RPKI auth tree.
>  	 */
> -	auth_insert(&auths, cert, NULL);
> +	auth_insert(file, &auths, cert, NULL);
>  
>  	return cert;
>  }
> @@ -590,7 +587,8 @@ proc_parser_gbr(char *file, const unsign
>  	if ((gbr = gbr_parse(&x509, file, entp->talid, der, len)) == NULL)
>  		return NULL;
>  
> -	a = valid_ski_aki(file, &auths, gbr->ski, gbr->aki, entp->mftaki);
> +	a = valid_ski_aki(file, &auths, entp->certid, gbr->ski, gbr->aki,
> +	    entp->mftaki);
>  	crl = crl_get(&crlt, a);
>  
>  	/* return value can be ignored since nothing happens here */
> @@ -623,7 +621,8 @@ proc_parser_aspa(char *file, const unsig
>  	if ((aspa = aspa_parse(&x509, file, entp->talid, der, len)) == NULL)
>  		return NULL;
>  
> -	a = valid_ski_aki(file, &auths, aspa->ski, aspa->aki, entp->mftaki);
> +	a = valid_ski_aki(file, &auths, entp->certid, aspa->ski, aspa->aki,
> +	    entp->mftaki);
>  	crl = crl_get(&crlt, a);
>  
>  	if (!valid_x509(file, ctx, x509, a, crl, &errstr)) {
> @@ -658,7 +657,8 @@ proc_parser_tak(char *file, const unsign
>  	if ((tak = tak_parse(&x509, file, entp->talid, der, len)) == NULL)
>  		return NULL;
>  
> -	a = valid_ski_aki(file, &auths, tak->ski, tak->aki, entp->mftaki);
> +	a = valid_ski_aki(file, &auths, entp->certid, tak->ski, tak->aki,
> +	    entp->mftaki);
>  	crl = crl_get(&crlt, a);
>  
>  	if (!valid_x509(file, ctx, x509, a, crl, &errstr)) {
> @@ -764,8 +764,7 @@ parse_entity(struct entityq *q, struct m
>  				    f, flen, entp->data, entp->datasz,
>  				    entp->talid);
>  			else
> -				cert = proc_parser_cert(file, f, flen,
> -				    entp->mftaki);
> +				cert = proc_parser_cert(file, f, flen, entp);
>  			if (cert != NULL)
>  				mtime = cert->notbefore;
>  			io_simple_buffer(b, &mtime, sizeof(mtime));
> Index: repo.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/repo.c,v
> diff -u -p -r1.57 repo.c
> --- repo.c	21 Apr 2024 19:27:44 -0000	1.57
> +++ repo.c	15 May 2024 14:41:59 -0000
> @@ -116,13 +116,14 @@ static void		 remove_contents(char *);
>   * Database of all file path accessed during a run.
>   */
>  struct filepath {
> -	RB_ENTRY(filepath)	entry;
> +	RB_ENTRY(filepath)	 entry;
>  	char			*file;
>  	time_t			 mtime;
> +	unsigned int		 talmask;
>  };
>  
>  static inline int
> -filepathcmp(struct filepath *a, struct filepath *b)
> +filepathcmp(const struct filepath *a, const struct filepath *b)
>  {
>  	return strcmp(a->file, b->file);
>  }
> @@ -133,22 +134,28 @@ RB_PROTOTYPE(filepath_tree, filepath, en
>   * Functions to lookup which files have been accessed during computation.
>   */
>  int
> -filepath_add(struct filepath_tree *tree, char *file, time_t mtime)
> +filepath_add(struct filepath_tree *tree, char *file, int id, time_t mtime)
>  {
> -	struct filepath *fp;
> +	struct filepath *fp, *rfp;
> +
> +	CTASSERT(TALSZ_MAX < 8 * sizeof(fp->talmask));
> +	assert(id >= 0 && id < 8 * (int)sizeof(fp->talmask));
>  
> -	if ((fp = malloc(sizeof(*fp))) == NULL)
> +	if ((fp = calloc(1, sizeof(*fp))) == NULL)
>  		err(1, NULL);
> -	fp->mtime = mtime;
>  	if ((fp->file = strdup(file)) == NULL)
>  		err(1, NULL);
> +	fp->mtime = mtime;
>  
> -	if (RB_INSERT(filepath_tree, tree, fp) != NULL) {
> +	if ((rfp = RB_INSERT(filepath_tree, tree, fp)) != NULL) {
>  		/* already in the tree */
>  		free(fp->file);
>  		free(fp);
> -		return 0;
> +		if (rfp->talmask & (1 << id))
> +			return 0;
> +		fp = rfp;
>  	}
> +	fp->talmask |= (1 << id);
>  
>  	return 1;
>  }
> @@ -906,7 +913,7 @@ rrdp_handle_file(unsigned int id, enum p
>  
>  	/* write new content or mark uri as deleted. */
>  	if (pt == PUB_DEL) {
> -		filepath_add(&rr->deleted, uri, 0);
> +		filepath_add(&rr->deleted, uri, 0, 0);
>  	} else {
>  		fp = filepath_find(&rr->deleted, uri);
>  		if (fp != NULL) {
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> diff -u -p -r1.73 validate.c
> --- validate.c	19 Mar 2024 05:04:13 -0000	1.73
> +++ validate.c	15 May 2024 12:36:15 -0000
> @@ -85,11 +85,12 @@ valid_ip(struct auth *a, enum afi afi,
>   * Return the issuer by its AKI, or NULL on failure.
>   */
>  struct auth *
> -valid_ski_aki(const char *fn, struct auth_tree *auths,
> +valid_ski_aki(const char *fn, struct auth_tree *auths, int id,
>      const char *ski, const char *aki, const char *mftaki)
>  {
>  	struct auth *a;
>  
> +	/* XXX - ski is now unused. Inline these functions in parser.c? */
>  	if (mftaki != NULL) {
>  		if (strcmp(aki, mftaki) != 0) {
>  			warnx("%s: AKI doesn't match Manifest AKI", fn);
> @@ -97,32 +98,16 @@ valid_ski_aki(const char *fn, struct aut
>  		}
>  	}
>  
> -	if (auth_find(auths, ski) != NULL) {
> -		warnx("%s: RFC 6487: duplicate SKI", fn);
> -		return NULL;
> -	}
> -
> -	a = auth_find(auths, aki);
> +	a = auth_find(auths, id);
>  	if (a == NULL)
> -		warnx("%s: RFC 6487: unknown AKI", fn);
> -
> -	return a;
> -}
> +		warnx("%s: RFC 6487: unknown cert", fn);

I think we want to do

	if (a == NULL) {
		warnx("%s: RFC 6487: unknown cert", fn);
		return NULL;
	}

because...

>  
> -/*
> - * Validate a trust anchor by making sure that the SKI is unique.
> - * Returns 1 if valid, 0 otherwise.
> - */
> -int
> -valid_ta(const char *fn, struct auth_tree *auths, const struct cert *cert)
> -{
> -	/* SKI must not be a dupe. */
> -	if (auth_find(auths, cert->ski) != NULL) {
> -		warnx("%s: RFC 6487: duplicate SKI", fn);
> -		return 0;
> +	if (strcmp(aki, a->cert->ski) != 0) {

... at this point we don't know that a != NULL. I think it's nicer than:

	else if (strcmp(aki, a->cert->ski) != 0) {


> +		warnx("%s: AKI doesn't match with cert SKI", fn);
> +		return NULL;
>  	}
>  
> -	return 1;
> +	return a;
>  }
>  
>  /*
>