On Tue, Jan 18, 2022 at 02:09:08PM +0100, Theo Buehler wrote:
> 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).
Sure, I tried to keep this as mechanical as possible since this is nasty
code that does not permit errors.
> 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.
Yes, I forgot to mention that. I think this are some left-overs from the
time where CRLs were optional. At least I see no reason why
X509_V_ERR_UNABLE_TO_GET_CRL errors are only printed at verbose level 1 or
higher.
> > --
> > :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);
> > }
> >
>
--
:wq Claudio
