On Tue, Jan 18, 2022 at 12:16:44PM +0100, Claudio Jeker wrote:
> How X509_verify_cert() is called in rpki-client is mostly the same in all
> places so move all this X509 boilerplate into valid_x509().
>
> This simplifies the x509 validation in the parser a fair but and will also
> make it easier for -f to validate certs.
>
> OK?
ok
I would suggest we merge the three if (crl != NULL) checks into one
(maybe in a follow-up).
The _roa and _gbr paths called the warnx() with the
X509_verify_cert_error_string() only conditionally. I guess we can
adjust that later if this turns out to be too noisy.
> --
> :wq Claudio
>
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.37
> diff -u -p -r1.37 parser.c
> --- parser.c 14 Jan 2022 15:00:23 -0000 1.37
> +++ parser.c 18 Jan 2022 11:01:45 -0000
> @@ -200,52 +200,74 @@ verify_cb(int ok, X509_STORE_CTX *store_
> }
>
> /*
> - * 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 unsign
> 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 unsign
> {
> 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 unsig
> {
> 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 unsig
> 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 unsign
> {
> 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);
> }
>
