Download raw body.
rpki-client: improve purpose handling
We should not just distinguish between CA and BGPsec router certs. It's
always been confusing and I think the below is clearer, slightly
stricter and more correct.
This introduces two more cert purposes to cover TA certs and EE certs.
With a little bit more effort in x509_get_purpose() we can distinguish
between all four kinds of certificates we expect to see in the RPKI.
Most of the diff is straightforward. I factored the X509_check_purpose()
magic into x509_cache_extensions() since that's clearer in the callers.
I also sprinkled a few calls to this at some places where I found myself
checking over and over again. The expensive part of extension caching
is done only once since libcrypto sets a flag on the cert.
Check in cert_parse_ee_cert() that the purpose is EE, in ta_parse()
that it is a TA and reject EE certs in cert_parse_pre() (well,
technically BGPsec Router certs are also EE certs... They're special
and few and far between). The caller should tell us if we should expect
a TA or not. It would be great if it could indicate CA or BGPsec router,
so we could just pass in the expected purpose, but I don't think it can.
In x509_get_purpose() there are a few changes. The extension caching is
for my future self. X509_check_ca() is a strange API. In the RPKI it
should only return 0 or 1, but is_ca == 4 is a conceivable misissuance.
The rest is summarized in the doc comment.
I left two XXX for follow-ups.
Index: cert.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cert.c,v
diff -u -p -r1.141 cert.c
--- cert.c 7 Jun 2024 08:36:54 -0000 1.141
+++ cert.c 8 Jun 2024 02:44:55 -0000
@@ -744,6 +744,15 @@ cert_parse_ee_cert(const char *fn, int t
if (!cert_check_subject_and_issuer(fn, x))
goto out;
+ if (!x509_cache_extensions(x, fn))
+ goto out;
+
+ if ((cert->purpose = x509_get_purpose(x, fn)) != CERT_PURPOSE_EE) {
+ warnx("%s: expected EE cert, got %s", fn,
+ purpose2str(cert->purpose));
+ goto out;
+ }
+
if (X509_get_key_usage(x) != KU_DIGITAL_SIGNATURE) {
warnx("%s: RFC 6487 section 4.8.4: KU must be digitalSignature",
fn);
@@ -827,11 +836,8 @@ cert_parse_pre(const char *fn, const uns
goto out;
}
- /* Cache X509v3 extensions, see X509_check_ca(3). */
- if (X509_check_purpose(x, -1, -1) <= 0) {
- warnx("%s: could not cache X509v3 extensions", fn);
+ if (!x509_cache_extensions(x, fn))
goto out;
- }
if (X509_get_version(x) != 2) {
warnx("%s: RFC 6487 4.1: X.509 version must be v3", fn);
@@ -957,11 +963,12 @@ cert_parse_pre(const char *fn, const uns
goto out;
if (!x509_get_notafter(x, fn, &cert->notafter))
goto out;
- cert->purpose = x509_get_purpose(x, fn);
/* Validation on required fields. */
-
+ cert->purpose = x509_get_purpose(x, fn);
switch (cert->purpose) {
+ case CERT_PURPOSE_TA:
+ /* XXX - caller should indicate if it expects TA or CA cert */
case CERT_PURPOSE_CA:
if ((pkey = X509_get0_pubkey(x)) == NULL) {
warnx("%s: X509_get0_pubkey failed", fn);
@@ -1015,6 +1022,9 @@ cert_parse_pre(const char *fn, const uns
goto out;
}
break;
+ case CERT_PURPOSE_EE:
+ warn("%s: unexpected EE cert", fn);
+ goto out;
default:
warnx("%s: x509_get_purpose failed in %s", fn, __func__);
goto out;
@@ -1117,12 +1127,9 @@ ta_parse(const char *fn, struct cert *p,
"trust anchor may not specify CRL resource", fn);
goto badcert;
}
- /*
- * XXX - this check for BGPsec router certs doesn't make all that much
- * sense. Consider introducing a TA purpose for self-issued CA certs.
- */
- if (p->purpose == CERT_PURPOSE_BGPSEC_ROUTER) {
- warnx("%s: BGPsec cert cannot be a trust anchor", fn);
+ if (p->purpose != CERT_PURPOSE_TA) {
+ warnx("%s: expected trust anchor purpose, got %s", fn,
+ purpose2str(p->purpose));
goto badcert;
}
/*
Index: cms.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/cms.c,v
diff -u -p -r1.45 cms.c
--- cms.c 24 May 2024 12:57:20 -0000 1.45
+++ cms.c 7 Jun 2024 19:38:55 -0000
@@ -324,11 +324,8 @@ cms_parse_validate_internal(X509 **xp, c
goto out;
}
- /* Cache X509v3 extensions, see X509_check_ca(3). */
- if (X509_check_purpose(*xp, -1, -1) <= 0) {
- warnx("%s: could not cache X509v3 extensions", fn);
+ if (!x509_cache_extensions(*xp, fn))
goto out;
- }
if (!x509_get_notafter(*xp, fn, ¬after))
goto out;
Index: extern.h
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
diff -u -p -r1.221 extern.h
--- extern.h 4 Jun 2024 04:17:18 -0000 1.221
+++ extern.h 8 Jun 2024 03:16:34 -0000
@@ -107,8 +107,10 @@ struct cert_ip {
enum cert_purpose {
CERT_PURPOSE_INVALID,
+ CERT_PURPOSE_TA,
CERT_PURPOSE_CA,
- CERT_PURPOSE_BGPSEC_ROUTER
+ CERT_PURPOSE_EE,
+ CERT_PURPOSE_BGPSEC_ROUTER,
};
/*
@@ -901,6 +903,7 @@ struct ibuf *io_buf_recvfd(int, struct i
/* X509 helpers. */
void x509_init_oid(void);
+int x509_cache_extensions(X509 *, const char *);
int x509_get_aia(X509 *, const char *, char **);
int x509_get_aki(X509 *, const char *, char **);
int x509_get_sia(X509 *, const char *, char **);
@@ -922,6 +925,7 @@ time_t x509_find_expires(time_t, struc
/* printers */
char *nid2str(int);
+const char *purpose2str(enum cert_purpose);
char *time2str(time_t);
void x509_print(const X509 *);
void tal_print(const struct tal *);
Index: filemode.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
diff -u -p -r1.43 filemode.c
--- filemode.c 6 Jun 2024 07:19:10 -0000 1.43
+++ filemode.c 7 Jun 2024 21:04:24 -0000
@@ -157,7 +157,8 @@ parse_load_cert(char *uri)
if (cert == NULL)
goto done;
if (cert->purpose != CERT_PURPOSE_CA) {
- warnx("AIA reference to bgpsec cert %s", uri);
+ warnx("AIA reference to %s in %s",
+ purpose2str(cert->purpose), uri);
goto done;
}
/* try to load the CRL of this cert */
Index: main.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/main.c,v
diff -u -p -r1.259 main.c
--- main.c 7 Jun 2024 08:22:53 -0000 1.259
+++ main.c 7 Jun 2024 20:46:09 -0000
@@ -618,6 +618,7 @@ entity_process(struct ibuf *b, struct st
}
cert = cert_read(b);
switch (cert->purpose) {
+ case CERT_PURPOSE_TA:
case CERT_PURPOSE_CA:
queue_add_from_cert(cert);
break;
@@ -626,7 +627,7 @@ entity_process(struct ibuf *b, struct st
repo_stat_inc(rp, talid, type, STYPE_BGPSEC);
break;
default:
- errx(1, "unexpected cert purpose received");
+ errx(1, "unexpected %s", purpose2str(cert->purpose));
break;
}
cert_free(cert);
Index: print.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/print.c,v
diff -u -p -r1.54 print.c
--- print.c 6 Jun 2024 05:57:36 -0000 1.54
+++ print.c 7 Jun 2024 19:40:13 -0000
@@ -65,6 +65,25 @@ nid2str(int nid)
return buf;
}
+const char *
+purpose2str(enum cert_purpose purpose)
+{
+ switch (purpose) {
+ case CERT_PURPOSE_INVALID:
+ return "invalid cert";
+ case CERT_PURPOSE_TA:
+ return "TA cert";
+ case CERT_PURPOSE_CA:
+ return "CA cert";
+ case CERT_PURPOSE_EE:
+ return "EE cert";
+ case CERT_PURPOSE_BGPSEC_ROUTER:
+ return "BGPsec Router cert";
+ default:
+ return "unknown cert";
+ }
+}
+
char *
time2str(time_t t)
{
Index: x509.c
===================================================================
RCS file: /cvs/src/usr.sbin/rpki-client/x509.c,v
diff -u -p -r1.94 x509.c
--- x509.c 7 Jun 2024 08:36:54 -0000 1.94
+++ x509.c 8 Jun 2024 03:22:40 -0000
@@ -133,6 +133,26 @@ x509_init_oid(void)
}
/*
+ * A number of critical OpenSSL API functions can't properly indicate failure
+ * and are unreliable if the extensions aren't already cached. An old trick is
+ * to cache the extensions using an error-checked call to X509_check_purpose()
+ * with a purpose of -1. This way functions such as X509_check_ca(), X509_cmp(),
+ * X509_get_key_usage(), X509_get_extended_key_usage() won't lie.
+ *
+ * Should be called right after deserialization and is essentially free to call
+ * multiple times.
+ */
+int
+x509_cache_extensions(X509 *x509, const char *fn)
+{
+ if (X509_check_purpose(x509, -1, 0) <= 0) {
+ warnx("%s: could not cache X509v3 extensions", fn);
+ return 0;
+ }
+ return 1;
+}
+
+/*
* Parse X509v3 authority key identifier (AKI), RFC 6487 sec. 4.8.3.
* Returns the AKI or NULL if it could not be parsed.
* The AKI is formatted as a hex string.
@@ -246,18 +266,34 @@ x509_get_ski(X509 *x, const char *fn, ch
}
/*
- * Check the certificate's purpose: CA or BGPsec Router.
- * Return a member of enum cert_purpose.
+ * Check the cert's purpose: the cA bit in basic constraints distinguishes
+ * between TA/CA and EE/BGPsec router. TAs are self-signed, CAs not self-issued,
+ * EEs have no extended key usage, BGPsec router have id-kp-bgpsec-router OID.
*/
enum cert_purpose
x509_get_purpose(X509 *x, const char *fn)
{
BASIC_CONSTRAINTS *bc = NULL;
EXTENDED_KEY_USAGE *eku = NULL;
- int crit;
+ int crit, ext_flags, is_ca;
enum cert_purpose purpose = CERT_PURPOSE_INVALID;
- if (X509_check_ca(x) == 1) {
+ if (!x509_cache_extensions(x, fn))
+ goto out;
+
+ ext_flags = X509_get_extension_flags(x);
+
+ /* This weird API can return 0, 1, 2, 4, 5 but can't error... */
+ if ((is_ca = X509_check_ca(x)) > 1) {
+ if (is_ca == 4)
+ warnx("%s: RFC 6487: sections 4.8.1 and 4.8.4: "
+ "no basic constraints, but keyCertSign set", fn);
+ else
+ warnx("%s: unexpected legacy certificate", fn);
+ goto out;
+ }
+
+ if (is_ca) {
bc = X509_get_ext_d2i(x, NID_basic_constraints, &crit, NULL);
if (bc == NULL) {
if (crit != -1)
@@ -278,28 +314,50 @@ x509_get_purpose(X509 *x, const char *fn
"Constraint must be absent", fn);
goto out;
}
- purpose = CERT_PURPOSE_CA;
- /* XXX - we may want to check EXFLAG_SI and add a TA purpose. */
+ /*
+ * EXFLAG_SI means that issuer and subject are identical.
+ * EXFLAG_SS is SI plus the AKI is absent or matches the SKI.
+ * Thus, exactly the trust anchors should have EXFLAG_SS set
+ * and we should never see EXFLAG_SI without EXFLAG_SS.
+ */
+ if ((ext_flags & EXFLAG_SS) != 0)
+ purpose = CERT_PURPOSE_TA;
+ else if ((ext_flags & EXFLAG_SI) == 0)
+ purpose = CERT_PURPOSE_CA;
+ else
+ warnx("%s: RFC 6487, sections 4.8.3: "
+ "self-issued cert with AKI-SKI mismatch", fn);
goto out;
}
- if (X509_get_extension_flags(x) & EXFLAG_BCONS) {
+ if ((ext_flags & EXFLAG_BCONS) != 0) {
warnx("%s: Basic Constraints ext in non-CA cert", fn);
goto out;
}
+ /*
+ * EKU is only defined for BGPsec Router certs and must be absent from
+ * EE certs.
+ */
eku = X509_get_ext_d2i(x, NID_ext_key_usage, &crit, NULL);
if (eku == NULL) {
if (crit != -1)
warnx("%s: error parsing EKU", fn);
else
- warnx("%s: EKU: extension missing", fn);
+ purpose = CERT_PURPOSE_EE; /* EKU absent */
goto out;
}
if (crit != 0) {
warnx("%s: EKU: extension must not be marked critical", fn);
goto out;
}
+
+ /*
+ * XXX - this isn't quite correct: other EKU OIDs are allowed per
+ * RFC 8209, section 3.1.3.2, e.g., anyEKU could potentially help
+ * avoid tripping up validators that don't know about the BGPsec
+ * router purpose. Drop check or downgrade from error to warning?
+ */
if (sk_ASN1_OBJECT_num(eku) != 1) {
warnx("%s: EKU: expected 1 purpose, have %d", fn,
sk_ASN1_OBJECT_num(eku));
rpki-client: improve purpose handling