Unify the various X509_verify_cert() calls and the boiler plate code around

it into its own function valid_x509(). Simplifies the code substantially.
This may report a few more errors for .roa and .gbr files but IMO that
special case was a left-over from long time ago.
OK tb@

1 files changed, 56 insertions, 103 deletions

diff --git a/usr.sbin/rpki-client/parser.c b/usr.sbin/rpki-client/parser.c
index bf3e25d27a1..88fda210e7a 100644
--- a/usr.sbin/rpki-client/parser.c
+++ b/usr.sbin/rpki-client/parser.c
@@ -1,4 +1,4 @@
-/*	$OpenBSD: parser.c,v 1.38 2022/01/18 13:06:43 claudio Exp $ */
+/*	$OpenBSD: parser.c,v 1.39 2022/01/18 13:46:07 claudio Exp $ */
 /*
  * Copyright (c) 2019 Claudio Jeker <claudio@openbsd.org>
  * Copyright (c) 2019 Kristaps Dzonsons <kristaps@bsd.lv>
@@ -200,52 +200,74 @@ verify_cb(int ok, X509_STORE_CTX *store_ctx)
 }
 
 /*
- * Parse and validate a ROA.
- * This is standard stuff.
- * Returns the roa on success, NULL on failure.
+ * Validate the X509 certificate.  If crl is NULL don't check CRL.
+ * Returns 1 for valid certificates, returns 0 if there is a verify error
  */
-static struct roa *
-proc_parser_roa(char *file, const unsigned char *der, size_t len)
+static int
+valid_x509(char *file, X509 *x509, struct auth *a, struct crl *crl)
 {
-	struct roa		*roa;
-	X509			*x509;
-	int			 c;
-	struct auth		*a;
 	STACK_OF(X509)		*chain;
-	STACK_OF(X509_CRL)	*crls;
-	struct crl		*crl;
-
-	if ((roa = roa_parse(&x509, file, der, len)) == NULL)
-		return NULL;
+	STACK_OF(X509_CRL)	*crls = NULL;
+	int			 c;
 
-	a = valid_ski_aki(file, &auths, roa->ski, roa->aki);
 	build_chain(a, &chain);
-	crl = get_crl(a);
-	build_crls(crl, &crls);
+	if (crl != NULL)
+		build_crls(crl, &crls);
 
 	assert(x509 != NULL);
 	if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL))
 		cryptoerrx("X509_STORE_CTX_init");
+
 	X509_STORE_CTX_set_verify_cb(ctx, verify_cb);
 	if (!X509_STORE_CTX_set_app_data(ctx, file))
 		cryptoerrx("X509_STORE_CTX_set_app_data");
-	X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK);
+	if (crl != NULL)
+		X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK);
 	X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH);
 	X509_STORE_CTX_set0_trusted_stack(ctx, chain);
-	X509_STORE_CTX_set0_crls(ctx, crls);
+	if (crl != NULL)
+		X509_STORE_CTX_set0_crls(ctx, crls);
 
 	if (X509_verify_cert(ctx) <= 0) {
 		c = X509_STORE_CTX_get_error(ctx);
+		warnx("%s: %s", file, X509_verify_cert_error_string(c));
 		X509_STORE_CTX_cleanup(ctx);
-		if (verbose > 0 || c != X509_V_ERR_UNABLE_TO_GET_CRL)
-			warnx("%s: %s", file, X509_verify_cert_error_string(c));
-		X509_free(x509);
-		roa_free(roa);
 		sk_X509_free(chain);
 		sk_X509_CRL_free(crls);
-		return NULL;
+		return 0;
 	}
+
 	X509_STORE_CTX_cleanup(ctx);
+	sk_X509_free(chain);
+	sk_X509_CRL_free(crls);
+	return 1;
+}
+
+/*
+ * Parse and validate a ROA.
+ * This is standard stuff.
+ * Returns the roa on success, NULL on failure.
+ */
+static struct roa *
+proc_parser_roa(char *file, const unsigned char *der, size_t len)
+{
+	struct roa		*roa;
+	struct crl		*crl;
+	struct auth		*a;
+	X509			*x509;
+
+	if ((roa = roa_parse(&x509, file, der, len)) == NULL)
+		return NULL;
+
+	a = valid_ski_aki(file, &auths, roa->ski, roa->aki);
+	crl = get_crl(a);
+
+	if (!valid_x509(file, x509, a, crl)) {
+		X509_free(x509);
+		roa_free(roa);
+		return NULL;
+	}
+	X509_free(x509);
 
 	/*
 	 * Check CRL to figure out the soonest transitive expiry moment
@@ -270,10 +292,6 @@ proc_parser_roa(char *file, const unsigned char *der, size_t len)
 	if (valid_roa(file, &auths, roa))
 		roa->valid = 1;
 
-	sk_X509_free(chain);
-	sk_X509_CRL_free(crls);
-	X509_free(x509);
-
 	return roa;
 }
 
@@ -336,38 +354,18 @@ proc_parser_mft(char *file, const unsigned char *der, size_t len,
 {
 	struct mft		*mft;
 	X509			*x509;
-	int			 c;
 	struct auth		*a;
-	STACK_OF(X509)		*chain;
 
 	if ((mft = mft_parse(&x509, file, der, len)) == NULL)
 		return NULL;
 
 	a = valid_ski_aki(file, &auths, mft->ski, mft->aki);
-	build_chain(a, &chain);
-
-	if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL))
-		cryptoerrx("X509_STORE_CTX_init");
-
-	/* CRL checks disabled here because CRL is referenced from mft */
-	X509_STORE_CTX_set_verify_cb(ctx, verify_cb);
-	if (!X509_STORE_CTX_set_app_data(ctx, file))
-		cryptoerrx("X509_STORE_CTX_set_app_data");
-	X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH);
-	X509_STORE_CTX_set0_trusted_stack(ctx, chain);
 
-	if (X509_verify_cert(ctx) <= 0) {
-		c = X509_STORE_CTX_get_error(ctx);
-		X509_STORE_CTX_cleanup(ctx);
-		warnx("%s: %s", file, X509_verify_cert_error_string(c));
+	if (!valid_x509(file, x509, a, NULL)) {
 		mft_free(mft);
 		X509_free(x509);
-		sk_X509_free(chain);
 		return NULL;
 	}
-
-	X509_STORE_CTX_cleanup(ctx);
-	sk_X509_free(chain);
 	X509_free(x509);
 
 	mft->repoid = repoid;
@@ -396,10 +394,8 @@ proc_parser_cert(char *file, const unsigned char *der, size_t len)
 {
 	struct cert		*cert;
 	X509			*x509;
-	int			 c;
-	struct auth		*a = NULL;
-	STACK_OF(X509)		*chain;
-	STACK_OF(X509_CRL)	*crls;
+	struct auth		*a;
+	struct crl		*crl;
 
 	/* Extract certificate data and X509. */
 
@@ -408,35 +404,13 @@ proc_parser_cert(char *file, const unsigned char *der, size_t len)
 		return NULL;
 
 	a = valid_ski_aki(file, &auths, cert->ski, cert->aki);
-	build_chain(a, &chain);
-	build_crls(get_crl(a), &crls);
-
-	assert(x509 != NULL);
-	if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL))
-		cryptoerrx("X509_STORE_CTX_init");
-
-	X509_STORE_CTX_set_verify_cb(ctx, verify_cb);
-	if (!X509_STORE_CTX_set_app_data(ctx, file))
-		cryptoerrx("X509_STORE_CTX_set_app_data");
-	X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK);
-	X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH);
-	X509_STORE_CTX_set0_trusted_stack(ctx, chain);
-	X509_STORE_CTX_set0_crls(ctx, crls);
+	crl = get_crl(a);
 
-	if (X509_verify_cert(ctx) <= 0) {
-		c = X509_STORE_CTX_get_error(ctx);
-		warnx("%s: %s", file, X509_verify_cert_error_string(c));
-		X509_STORE_CTX_cleanup(ctx);
+	if (!valid_x509(file, x509, a, crl)) {
 		cert_free(cert);
-		sk_X509_free(chain);
-		sk_X509_CRL_free(crls);
 		X509_free(x509);
 		return NULL;
 	}
-
-	X509_STORE_CTX_cleanup(ctx);
-	sk_X509_free(chain);
-	sk_X509_CRL_free(crls);
 	X509_free(x509);
 
 	cert->talid = a->cert->talid;
@@ -597,39 +571,18 @@ proc_parser_gbr(char *file, const unsigned char *der, size_t len)
 {
 	struct gbr		*gbr;
 	X509			*x509;
-	int			 c;
 	struct auth		*a;
-	STACK_OF(X509)		*chain;
-	STACK_OF(X509_CRL)	*crls;
+	struct crl		*crl;
 
 	if ((gbr = gbr_parse(&x509, file, der, len)) == NULL)
 		return;
 
 	a = valid_ski_aki(file, &auths, gbr->ski, gbr->aki);
+	crl = get_crl(a);
 
-	build_chain(a, &chain);
-	build_crls(get_crl(a), &crls);
-
-	assert(x509 != NULL);
-	if (!X509_STORE_CTX_init(ctx, NULL, x509, NULL))
-		cryptoerrx("X509_STORE_CTX_init");
-	X509_STORE_CTX_set_verify_cb(ctx, verify_cb);
-	if (!X509_STORE_CTX_set_app_data(ctx, file))
-		cryptoerrx("X509_STORE_CTX_set_app_data");
-	X509_STORE_CTX_set_flags(ctx, X509_V_FLAG_CRL_CHECK);
-	X509_STORE_CTX_set_depth(ctx, MAX_CERT_DEPTH);
-	X509_STORE_CTX_set0_trusted_stack(ctx, chain);
-	X509_STORE_CTX_set0_crls(ctx, crls);
-
-	if (X509_verify_cert(ctx) <= 0) {
-		c = X509_STORE_CTX_get_error(ctx);
-		if (verbose > 0 || c != X509_V_ERR_UNABLE_TO_GET_CRL)
-			warnx("%s: %s", file, X509_verify_cert_error_string(c));
-	}
+	/* return value can be ignored since nothing happens here */
+	valid_x509(file, x509, a, crl);
 
-	X509_STORE_CTX_cleanup(ctx);
-	sk_X509_free(chain);
-	sk_X509_CRL_free(crls);
 	X509_free(x509);
 	gbr_free(gbr);
 }