Re: rpki-client x509 verification in common function
On Tue, Jan 18, 2022 at 02:41:38PM +0100, Claudio Jeker wrote: > 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. I looked a bit more into this. So I added this reduced verbosity in Rev 1.17 of main.c. This was before the Elk Lakes hackathon that fixed most of the validation code. For example in Rev 1.39 of main a few months later the lookup of CRLs changed to use the AKI for lookups and away from using some sort of normalized name. I think this and some of the later diffs made this workaround obsolete. -- :wq Claudio
Re: rpki-client x509 verification in common function
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.c14 Jan 2022 15:00:23 - 1.37
> > +++ parser.c18 Jan 2022 11:01:45 -
> > @@ -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(, file, der, len)) == NULL)
> > - return NULL;
> > + STACK_OF(X509_CRL) *crls = NULL;
> > + int c;
> >
> > - a = valid_ski_aki(file, , roa->ski, roa->aki);
> > build_chain(a, );
> > - crl = get_crl(a);
> > - build_crls(crl, );
> > + if (crl != NULL)
> > + build_crls(crl, );
> >
> > 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(, file, der, len)) == NULL)
> > + return NULL;
> > +
> > + a = valid_ski_aki(file, , 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, , roa))
> > roa->valid = 1;
> >
> > - sk_X509_free(chain);
>
Re: rpki-client x509 verification in common function
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 - 1.37
> +++ parser.c 18 Jan 2022 11:01:45 -
> @@ -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(, file, der, len)) == NULL)
> - return NULL;
> + STACK_OF(X509_CRL) *crls = NULL;
> + int c;
>
> - a = valid_ski_aki(file, , roa->ski, roa->aki);
> build_chain(a, );
> - crl = get_crl(a);
> - build_crls(crl, );
> + if (crl != NULL)
> + build_crls(crl, );
>
> 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(, file, der, len)) == NULL)
> + return NULL;
> +
> + a = valid_ski_aki(file, , 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, , 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(, file, der, len)) == NULL)
> return NULL;
>
> a = valid_ski_aki(file, , mft->ski, mft->aki);
> - build_chain(a, );
>
>
rpki-client x509 verification in common function
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?
--
: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.c14 Jan 2022 15:00:23 - 1.37
+++ parser.c18 Jan 2022 11:01:45 -
@@ -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(, file, der, len)) == NULL)
- return NULL;
+ STACK_OF(X509_CRL) *crls = NULL;
+ int c;
- a = valid_ski_aki(file, , roa->ski, roa->aki);
build_chain(a, );
- crl = get_crl(a);
- build_crls(crl, );
+ if (crl != NULL)
+ build_crls(crl, );
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(, file, der, len)) == NULL)
+ return NULL;
+
+ a = valid_ski_aki(file, , 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, , 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(, file, der, len)) == NULL)
return NULL;
a = valid_ski_aki(file, , mft->ski, mft->aki);
- build_chain(a, );
- 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,
