From: Theo Buehler <tb@theobuehler.org>
Subject: Re: rpki-client: better extension order in cert_parse_pre
To: tech@openbsd.org
Date: Thu, 19 Jun 2025 08:34:10 +0200

On Thu, Jun 19, 2025 at 07:52:33AM +0200, Claudio Jeker wrote:
> On Thu, Jun 19, 2025 at 07:27:35AM +0200, Theo Buehler wrote:
> > On Thu, Jun 19, 2025 at 07:14:55AM +0200, Claudio Jeker wrote:
> > > On Thu, Jun 19, 2025 at 12:55:54AM +0200, Theo Buehler wrote:
> > > > The random order in which cert_parse_pre's switch handles extensions
> > > > has confused me too many times. I'd like this to match the order in
> > > > RFC 6487, section 4.8. This isn't perfect either - ski comes before aki
> > > > and aia comes before sia. Still, it's better.
> > > > 
> > > > To make this easy to review, I'll do it in a couple of steps. Here's
> > > > the first one, covering sections 4.8.1-4.8.7.
> > > > 
> > > >        4.8.1.  Basic Constraints  . . . . . . . . . . . . . . . . . .  8
> > > >        4.8.2.  Subject Key Identifier . . . . . . . . . . . . . . . .  9
> > > >        4.8.3.  Authority Key Identifier . . . . . . . . . . . . . . .  9
> > > >        4.8.4.  Key Usage  . . . . . . . . . . . . . . . . . . . . . .  9
> > > >        4.8.5.  Extended Key Usage . . . . . . . . . . . . . . . . . .  9
> > > >        4.8.6.  CRL Distribution Points  . . . . . . . . . . . . . . . 10
> > > >        4.8.7.  Authority Information Access . . . . . . . . . . . . . 10
> > > 
> > > Go for it. OK claudio@
> > 
> > And here's the second step, moving the two sbgp extensions to their
> > proper spot:
> > 
> >        4.8.7.  Authority Information Access . . . . . . . . . . . . . 10
> >        4.8.8.  Subject Information Access . . . . . . . . . . . . . . 11
> >        4.8.9.  Certificate Policies . . . . . . . . . . . . . . . . . 12
> >        4.8.10. IP Resources . . . . . . . . . . . . . . . . . . . . . 12
> >        4.8.11. AS Resources . . . . . . . . . . . . . . . . . . . . . 12
> 
> Sure. Do you want to add a comment how this is sorted?

Can do. I have added this:

+		/* The switch is ordered following RFC 6487, section 4.8. */
 		switch (nid = OBJ_obj2nid(obj)) {
 		case NID_basic_constraints:
 			if (bc++ > 0)

I am working on refactoring, merging and streamlining the logic of
cert_parse_ee_cert() (which is way too lax) and cert_parse_pre() (which
is about as strict as it can be but messy since it grew organically).

Last baby step for this: make counters match.

We could drop the initialization for the nid to appease some tooling
but I'm sure it will upset some other tools, so I left it.

Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
diff -u -p -r1.161 cert.c
--- cert.c	19 Jun 2025 06:20:23 -0000	1.161
+++ cert.c	19 Jun 2025 06:22:46 -0000
@@ -815,10 +815,10 @@ cert_parse_pre(const char *fn, const uns
 	const ASN1_BIT_STRING	*issuer_uid = NULL, *subject_uid = NULL;
 	ASN1_OBJECT		*obj;
 	EVP_PKEY		*pkey;
-	int			 nid, ip, as, sia, cp, crldp, aia, aki, ski,
-				 eku, bc, ku;
+	int			 nid, bc, ski, aki, ku, eku, crldp, aia, sia,
+				 cp, ip, as;
 
-	nid = ip = as = sia = cp = crldp = aia = aki = ski = eku = bc = ku = 0;
+	nid = bc = ski = aki = ku = eku = crldp = aia = sia = cp = ip = as = 0;
 
 	/* just fail for empty buffers, the warning was printed elsewhere */
 	if (der == NULL)