From: Theo Buehler Subject: Re: rpki-client: start reworking CMS signed object parsers To: Job Snijders Cc: tech@openbsd.org Date: Tue, 16 Jun 2026 21:24:50 +0200 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;