Index | Thread | Search

From:
Theo Buehler <tb@theobuehler.org>
Subject:
Re: rpki-client: start reworking CMS signed object parsers
To:
Job Snijders <job@bsd.nl>
Cc:
tech@openbsd.org
Date:
Tue, 16 Jun 2026 21:24:50 +0200

Download raw body.

Thread
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.

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;