From: Claudio Jeker <cjeker@diehard.n-r-g.com>
Subject: Re: rpki-client: start reworking CMS signed object parsers
To: Theo Buehler <tb@theobuehler.org>
Cc: Job Snijders <job@bsd.nl>, tech@openbsd.org
Date: Wed, 17 Jun 2026 06:59:13 +0200

On Tue, Jun 16, 2026 at 09:24:50PM +0200, Theo Buehler wrote:
> On Tue, Jun 16, 2026 at 06:33:17PM +0000, Job Snijders wrote:
> > On Tue, Jun 16, 2026 at 06:01:51PM +0200, Theo Buehler wrote:
> > > The diff below starts unifying various CMS signed object parsers in
> > > preparation for a lot more refactoring of this long-accumulated
> > > copy-paste mess.
> > > 
> > > The signed object is passed as a void * object to various handlers, so
> > > these handlers all have the same signature.
> > > 
> > > In this step, *_parse() is essentially split into allocation plus:
> > > 
> > > 1. *_cert_info(), which checks some basic things on the EE cert (usually
> > >    inheritance, presence or absence of RFC 3779 extensions). For MFTs
> > >    it also extracts some info and hangs that off mft.
> > > 
> > > 2. the already existing *_parse_econtent()
> > > 
> > > 3. *_validate() that does some validation steps, sets ->signtime and 
> > >    ->valid. In most signed object handlers the validate step can't
> > >    currently fail. This is one of the many warts we've accumulated
> > >    and is marked with an /* XXX */.
> > > 
> > > This is all straightforward and should not change anything.
> > > 
> > > The reason I put cert_info after parse_econtent is that the latter
> > > usually has some helpers and I found it easier to reason about this if
> > > the future struct members are somewhat close to each other.
> 
> job didn't like passing in the signtime to the validate functions
> because that's only really needed for manifests and a bit of a weird
> thing to do. So avoid this for now and deal with this in a later step.
> 
> So this diff leaves the ->signtime and ->mftsize parts alone and drops
> the signtime argument from the validate functions. It is otherwise
> identical to the previous one.

Looks good to me. OK claudio@
 
> Index: aspa.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/aspa.c,v
> diff -u -p -r1.42 aspa.c
> --- aspa.c	8 Feb 2026 12:35:07 -0000	1.42
> +++ aspa.c	16 Jun 2026 19:15:05 -0000
> @@ -117,9 +117,10 @@ aspa_parse_providers(const char *fn, str
>   * Returns zero on failure, non-zero on success.
>   */
>  static int
> -aspa_parse_econtent(const char *fn, struct aspa *aspa, const unsigned char *d,
> +aspa_parse_econtent(const char *fn, void *obj, const unsigned char *d,
>      size_t dsz)
>  {
> +	struct aspa		*aspa = obj;
>  	const unsigned char	*oder;
>  	ASProviderAttestation	*aspa_asn1;
>  	int			 rc = 0;
> @@ -152,6 +153,32 @@ aspa_parse_econtent(const char *fn, stru
>  	return rc;
>  }
>  
> +static int
> +aspa_cert_info(const char *fn, void *obj, const struct cert *cert)
> +{
> +	if (cert->num_ips > 0) {
> +		warnx("%s: superfluous IP Resources extension present", fn);
> +		return 0;
> +	}
> +
> +	if (x509_any_inherits(cert->x509)) {
> +		warnx("%s: inherit elements not allowed in EE cert", fn);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int
> +aspa_validate(const char *fn, void *obj, struct cert *cert)
> +{
> +	struct aspa *aspa = obj;
> +
> +	aspa->valid = valid_aspa(fn, cert, aspa);
> +
> +	return 1; /* XXX */
> +}
> +
>  /*
>   * Parse a full ASPA file.
>   * Returns the payload or NULL if the file was malformed.
> @@ -178,20 +205,11 @@ aspa_parse(struct cert **out_cert, const
>  		err(1, NULL);
>  	aspa->signtime = signtime;
>  
> -	if (cert->num_ips > 0) {
> -		warnx("%s: superfluous IP Resources extension present", fn);
> -		goto out;
> -	}
> -
> -	if (x509_any_inherits(cert->x509)) {
> -		warnx("%s: inherit elements not allowed in EE cert", fn);
> +	if (!aspa_cert_info(fn, aspa, cert))
>  		goto out;
> -	}
> -
>  	if (!aspa_parse_econtent(fn, aspa, cms, cmsz))
>  		goto out;
> -
> -	aspa->valid = valid_aspa(fn, cert, aspa);
> +	(void)aspa_validate(fn, aspa, cert);
>  
>  	*out_cert = cert;
>  	cert = NULL;
> Index: mft.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/mft.c,v
> diff -u -p -r1.138 mft.c
> --- mft.c	16 May 2026 07:27:03 -0000	1.138
> +++ mft.c	16 Jun 2026 19:15:05 -0000
> @@ -290,9 +290,10 @@ mft_has_unique_names_and_hashes(const ch
>   * Returns 0 on failure and 1 on success.
>   */
>  static int
> -mft_parse_econtent(const char *fn, struct mft *mft, const unsigned char *d,
> +mft_parse_econtent(const char *fn, void *obj, const unsigned char *d,
>      size_t dsz)
>  {
> +	struct mft		*mft = obj;
>  	const unsigned char	*oder;
>  	Manifest		*mft_asn1;
>  	FileAndHash		*fh;
> @@ -374,6 +375,55 @@ mft_parse_econtent(const char *fn, struc
>  	return rc;
>  }
>  
> +static int
> +mft_cert_info(const char *fn, void *obj, const struct cert *cert)
> +{
> +	struct mft *mft = obj;
> +	char *crlfile;
> +
> +	if (!x509_inherits(cert->x509)) {
> +		warnx("%s: RFC 3779 extensions not set to inherit", fn);
> +		return 0;
> +	}
> +
> +	if ((mft->aki = strdup(cert->aki)) == NULL)
> +		err(1, NULL);
> +	if ((mft->sia = strdup(cert->signedobj)) == NULL)
> +		err(1, NULL);
> +
> +	crlfile = strrchr(cert->crl, '/');
> +	if (crlfile == NULL) {
> +		warnx("%s: RFC 6487 section 4.8.6: "
> +		    "invalid CRL distribution point", fn);
> +		return 0;
> +	}
> +	crlfile++;
> +	if (!valid_mft_filename(crlfile, strlen(crlfile)) ||
> +	    rtype_from_file_extension(crlfile) != RTYPE_CRL) {
> +		warnx("%s: RFC 6487 section 4.8.6: CRL: "
> +		    "bad CRL distribution point extension", fn);
> +		return 0;
> +	}
> +	if ((mft->crl = strdup(crlfile)) == NULL)
> +		err(1, NULL);
> +
> +	return 1;
> +}
> +
> +static int
> +mft_validate(const char *fn, void *obj, struct cert *cert)
> +{
> +	struct mft *mft = obj;
> +
> +	if (mft->signtime > mft->nextupdate) {
> +		warnx("%s: dating issue: CMS signing-time after MFT nextUpdate",
> +		    fn);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  /*
>   * Parse the objects that have been published in the manifest.
>   * Return mft if it conforms to RFC 9286, otherwise NULL.
> @@ -387,7 +437,6 @@ mft_parse(struct cert **out_cert, const 
>  	int		 rc = 0;
>  	size_t		 cmsz;
>  	unsigned char	*cms;
> -	char		*crlfile;
>  	time_t		 signtime = 0;
>  
>  	assert(*out_cert == NULL);
> @@ -402,40 +451,12 @@ mft_parse(struct cert **out_cert, const 
>  	mft->signtime = signtime;
>  	mft->mftsize = len;
>  
> -	if ((mft->aki = strdup(cert->aki)) == NULL)
> -		err(1, NULL);
> -	if ((mft->sia = strdup(cert->signedobj)) == NULL)
> -		err(1, NULL);
> -
> -	if (!x509_inherits(cert->x509)) {
> -		warnx("%s: RFC 3779 extension not set to inherit", fn);
> -		goto out;
> -	}
> -
> -	crlfile = strrchr(cert->crl, '/');
> -	if (crlfile == NULL) {
> -		warnx("%s: RFC 6487 section 4.8.6: "
> -		    "invalid CRL distribution point", fn);
> +	if (!mft_cert_info(fn, mft, cert))
>  		goto out;
> -	}
> -	crlfile++;
> -	if (!valid_mft_filename(crlfile, strlen(crlfile)) ||
> -	    rtype_from_file_extension(crlfile) != RTYPE_CRL) {
> -		warnx("%s: RFC 6487 section 4.8.6: CRL: "
> -		    "bad CRL distribution point extension", fn);
> -		goto out;
> -	}
> -	if ((mft->crl = strdup(crlfile)) == NULL)
> -		err(1, NULL);
> -
> -	if (mft_parse_econtent(fn, mft, cms, cmsz) == 0)
> +	if (!mft_parse_econtent(fn, mft, cms, cmsz))
>  		goto out;
> -
> -	if (mft->signtime > mft->nextupdate) {
> -		warnx("%s: dating issue: CMS signing-time after MFT nextUpdate",
> -		    fn);
> +	if (!mft_validate(fn, mft, cert))
>  		goto out;
> -	}
>  
>  	*out_cert = cert;
>  	cert = NULL;
> Index: roa.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/roa.c,v
> diff -u -p -r1.87 roa.c
> --- roa.c	9 Sep 2025 08:23:24 -0000	1.87
> +++ roa.c	16 Jun 2026 19:15:05 -0000
> @@ -64,9 +64,10 @@ ASN1_SEQUENCE(ROAIPAddress) = {
>   * Returns zero on failure, non-zero on success.
>   */
>  static int
> -roa_parse_econtent(const char *fn, struct roa *roa, const unsigned char *d,
> +roa_parse_econtent(const char *fn, void *obj, const unsigned char *d,
>      size_t dsz)
>  {
> +	struct roa			*roa = obj;
>  	const unsigned char		*oder;
>  	RouteOriginAttestation		*roa_asn1;
>  	const ROAIPAddressFamily	*addrfam;
> @@ -204,6 +205,37 @@ roa_parse_econtent(const char *fn, struc
>  	return rc;
>  }
>  
> +static int
> +roa_cert_info(const char *fn, void *obj, const struct cert *cert)
> +{
> +	if (x509_any_inherits(cert->x509)) {
> +		warnx("%s: inherit elements not allowed in EE cert", fn);
> +		return 0;
> +	}
> +
> +	if (cert->num_ases > 0) {
> +		warnx("%s: superfluous AS Resources extension present", fn);
> +		return 0;
> +	}
> +
> +	if (cert->num_ips == 0) {
> +		warnx("%s: no IP address present", fn);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int
> +roa_validate(const char *fn, void *obj, struct cert *cert)
> +{
> +	struct roa *roa = obj;
> +
> +	roa->valid = valid_roa(fn, cert, roa);
> +
> +	return 1; /* XXX */
> +}
> +
>  /*
>   * Parse a full RFC 9582 file.
>   * Returns the ROA or NULL if the document was malformed.
> @@ -230,29 +262,11 @@ roa_parse(struct cert **out_cert, const 
>  		err(1, NULL);
>  	roa->signtime = signtime;
>  
> -	if (!roa_parse_econtent(fn, roa, cms, cmsz))
> -		goto out;
> -
> -	if (x509_any_inherits(cert->x509)) {
> -		warnx("%s: inherit elements not allowed in EE cert", fn);
> -		goto out;
> -	}
> -
> -	if (cert->num_ases > 0) {
> -		warnx("%s: superfluous AS Resources extension present", fn);
> +	if (!roa_cert_info(fn, roa, cert))
>  		goto out;
> -	}
> -
> -	if (cert->num_ips == 0) {
> -		warnx("%s: no IP address present", fn);
> +	if (!roa_parse_econtent(fn, roa, cms, cmsz))
>  		goto out;
> -	}
> -
> -	/*
> -	 * If the ROA isn't valid, we accept it anyway and depend upon
> -	 * the code around roa_read() to check the "valid" field itself.
> -	 */
> -	roa->valid = valid_roa(fn, cert, roa);
> +	(void)roa_validate(fn, roa, cert);
>  
>  	*out_cert = cert;
>  	cert = NULL;
> Index: rsc.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/rsc.c,v
> diff -u -p -r1.44 rsc.c
> --- rsc.c	16 May 2026 07:27:03 -0000	1.44
> +++ rsc.c	16 Jun 2026 19:15:05 -0000
> @@ -288,9 +288,10 @@ rsc_parse_checklist(const char *fn, stru
>   * Returns zero on failure, non-zero on success.
>   */
>  static int
> -rsc_parse_econtent(const char *fn, struct rsc *rsc, const unsigned char *d,
> +rsc_parse_econtent(const char *fn, void *obj, const unsigned char *d,
>      size_t dsz)
>  {
> +	struct rsc		*rsc = obj;
>  	const unsigned char	*oder;
>  	RpkiSignedChecklist	*rsc_asn1;
>  	ResourceBlock		*resources;
> @@ -339,6 +340,27 @@ rsc_parse_econtent(const char *fn, struc
>  	return rc;
>  }
>  
> +static int
> +rsc_cert_info(const char *fn, void *obj, const struct cert *cert)
> +{
> +	if (x509_any_inherits(cert->x509)) {
> +		warnx("%s: inherit elements not allowed in EE cert", fn);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int
> +rsc_validate(const char *fn, void *obj, struct cert *cert)
> +{
> +	struct rsc *rsc = obj;
> +
> +	rsc->valid = valid_rsc(fn, cert, rsc);
> +
> +	return 1; /* XXX */
> +}
> +
>  /*
>   * Parse a full RFC 9323 file.
>   * Returns the RSC or NULL if the object was malformed.
> @@ -365,15 +387,11 @@ rsc_parse(struct cert **out_cert, const 
>  		err(1, NULL);
>  	rsc->signtime = signtime;
>  
> -	if (x509_any_inherits(cert->x509)) {
> -		warnx("%s: inherit elements not allowed in EE cert", fn);
> +	if (!rsc_cert_info(fn, rsc, cert))
>  		goto out;
> -	}
> -
>  	if (!rsc_parse_econtent(fn, rsc, cms, cmsz))
>  		goto out;
> -
> -	rsc->valid = valid_rsc(fn, cert, rsc);
> +	(void)rsc_validate(fn, rsc, cert);
>  
>  	*out_cert = cert;
>  	cert = NULL;
> Index: spl.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/spl.c,v
> diff -u -p -r1.16 spl.c
> --- spl.c	24 Aug 2025 12:34:39 -0000	1.16
> +++ spl.c	16 Jun 2026 19:15:05 -0000
> @@ -98,9 +98,10 @@ prefix_cmp(enum afi afi, const struct ip
>   * Returns zero on failure, non-zero on success.
>   */
>  static int
> -spl_parse_econtent(const char *fn, struct spl *spl, const unsigned char *d,
> +spl_parse_econtent(const char *fn, void *obj, const unsigned char *d,
>      size_t dsz)
>  {
> +	struct spl			*spl = obj;
>  	const unsigned char		*oder;
>  	SignedPrefixList		*spl_asn1;
>  	const AddressFamilyPrefixes	*afp;
> @@ -209,6 +210,37 @@ spl_parse_econtent(const char *fn, struc
>  	return rc;
>  }
>  
> +static int
> +spl_cert_info(const char *fn, void *obj, const struct cert *cert)
> +{
> +	if (x509_any_inherits(cert->x509)) {
> +		warnx("%s: inherit elements not allowed in EE cert", fn);
> +		return 0;
> +	}
> +
> +	if (cert->num_ases == 0) {
> +		warnx("%s: AS Resources extension missing", fn);
> +		return 0;
> +	}
> +
> +	if (cert->num_ips > 0) {
> +		warnx("%s: superfluous IP Resources extension present", fn);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int
> +spl_validate(const char *fn, void *obj, struct cert *cert)
> +{
> +	struct spl *spl = obj;
> +
> +	spl->valid = valid_spl(fn, cert, spl);
> +
> +	return 1; /* XXX */
> +}
> +
>  /*
>   * Parse a full Signed Prefix List file.
>   * Returns the SPL, or NULL if the object was malformed.
> @@ -235,29 +267,11 @@ spl_parse(struct cert **out_cert, const 
>  		err(1, NULL);
>  	spl->signtime = signtime;
>  
> -	if (!spl_parse_econtent(fn, spl, cms, cmsz))
> -		goto out;
> -
> -	if (x509_any_inherits(cert->x509)) {
> -		warnx("%s: inherit elements not allowed in EE cert", fn);
> +	if (!spl_cert_info(fn, spl, cert))
>  		goto out;
> -	}
> -
> -	if (cert->num_ases == 0) {
> -		warnx("%s: AS Resources extension missing", fn);
> -		goto out;
> -	}
> -
> -	if (cert->num_ips > 0) {
> -		warnx("%s: superfluous IP Resources extension present", fn);
> +	if (!spl_parse_econtent(fn, spl, cms, cmsz))
>  		goto out;
> -	}
> -
> -	/*
> -	 * If the SPL isn't valid, we accept it anyway and depend upon
> -	 * the code around spl_read() to check the "valid" field itself.
> -	 */
> -	spl->valid = valid_spl(fn, cert, spl);
> +	(void)spl_validate(fn, spl, cert);
>  
>  	*out_cert = cert;
>  	cert = NULL;
> Index: tak.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/tak.c,v
> diff -u -p -r1.29 tak.c
> --- tak.c	2 Dec 2025 12:47:48 -0000	1.29
> +++ tak.c	16 Jun 2026 19:15:05 -0000
> @@ -140,9 +140,10 @@ parse_takey(const char *fn, const TAKey 
>   * Returns zero on failure, non-zero on success.
>   */
>  static int
> -tak_parse_econtent(const char *fn, struct tak *tak, const unsigned char *d,
> +tak_parse_econtent(const char *fn, void *obj, const unsigned char *d,
>      size_t dsz)
>  {
> +	struct tak		*tak = obj;
>  	const unsigned char	*oder;
>  	TAK			*tak_asn1;
>  	int			 rc = 0;
> @@ -183,6 +184,30 @@ tak_parse_econtent(const char *fn, struc
>  	return rc;
>  }
>  
> +static int
> +tak_cert_info(const char *fn, void *obj, const struct cert *cert)
> +{
> +	if (!x509_inherits(cert->x509)) {
> +		warnx("%s: RFC 3779 extension not set to inherit", fn);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
> +static int
> +tak_validate(const char *fn, void *obj, struct cert *cert)
> +{
> +	struct tak *tak = obj;
> +
> +	if (strcmp(cert->aki, tak->current->ski) != 0) {
> +		warnx("%s: current TAKey's SKI does not match EE AKI", fn);
> +		return 0;
> +	}
> +
> +	return 1;
> +}
> +
>  /*
>   * Parse a full RFC 9691 Trust Anchor Key file.
>   * Returns the TAK or NULL if the object was malformed.
> @@ -209,18 +234,12 @@ tak_parse(struct cert **out_cert, const 
>  		err(1, NULL);
>  	tak->signtime = signtime;
>  
> -	if (!x509_inherits(cert->x509)) {
> -		warnx("%s: RFC 3779 extension not set to inherit", fn);
> +	if (!tak_cert_info(fn, tak, cert))
>  		goto out;
> -	}
> -
>  	if (!tak_parse_econtent(fn, tak, cms, cmsz))
>  		goto out;
> -
> -	if (strcmp(cert->aki, tak->current->ski) != 0) {
> -		warnx("%s: current TAKey's SKI does not match EE AKI", fn);
> +	if (!tak_validate(fn, tak, cert))
>  		goto out;
> -	}
>  
>  	*out_cert = cert;
>  	cert = NULL;
> 

-- 
:wq Claudio