On Tue, Nov 29, 2022 at 10:55:02AM +0100, Claudio Jeker wrote:
> On Mon, Nov 28, 2022 at 07:40:40PM +0100, Theo Buehler wrote:
> > On Mon, Nov 28, 2022 at 07:18:24PM +0100, Claudio Jeker wrote:
> > > On Mon, Nov 28, 2022 at 06:02:56PM +0100, Theo Buehler wrote:
> > > > On Mon, Nov 28, 2022 at 05:14:48PM +0100, Claudio Jeker wrote:
> > > > > On Mon, Nov 28, 2022 at 04:50:24PM +0100, Theo Buehler wrote:
> > > > > > On Mon, Nov 28, 2022 at 04:02:11PM +0100, Claudio Jeker wrote:
> > > > > > > Since a long time any problem that caused rpki-client to not load
> > > > > > > a
> > > > > > > manifest resulted in the non helpful:
> > > > > > > rpki-client:
> > > > > > > rpki.afrinic.net/repository/member_repository/F36505B2/0569917622D711ED862FD6E0F1222468/0nALpPtwFyntPHjkS8xt-VQrqLw.mft:
> > > > > > > no valid mft available
> > > > > > >
> > > > > > > This hides in most cases the cause why the manifest verificatin
> > > > > > > failed.
> > > > > > > The following diff exposes the error from valid_x509() and with
> > > > > > > that some
> > > > > > > manifest errors change to e.g.:
> > > > > > > rpki-client:
> > > > > > > parent.rov.koenvanhove.nl/repo/KoenvanHove/1/C1F7424F77FBF669FB750C6DC7B649C5969DCD55.mft:
> > > > > > > CRL has expired
> > > > > > >
> > > > > > > or if the CRL is missing
> > > > > > >
> > > > > > > rpki-client:
> > > > > > > repo.pedjoeang.group/repo/localname/0/EF79F8E55B6A248EF9CF4CE70FF60C017BF1A3B0.mft:
> > > > > > > unable to get certificate CRL
> > > > > > >
> > > > > > > If the certificate is pointing to a manifest that does not exist
> > > > > > > the old
> > > > > > > "no valid mft available" is shown.
> > > > > > >
> > > > > > > I tried to keep original behaviour as much as possible but I think
> > > > > > > filemode can be further improved. And maybe we can remove verbose
> > > > > > > from
> > > > > > > other warnings as well.
> > > > > >
> > > > > > I like this a lot.
> > > > > >
> > > > > > I was wondering if valid_x509() should have a const char **errstr
> > > > > > instead of an int * as last argument. valid_x509() could do the
> > > > > > conversion from error code to error string itself. This way we don't
> > > > > > have to sprinkle X509_verify_cert_error_string() calls everywhere.
> > > > > >
> > > > > > Or we could introduce a warn_invalid_x509(const char *str, int err)
> > > > > > that
> > > > > > does the conversion from err using X509_verify_cert_error_string().
> > > > > >
> > > > > > One downside of X509_verify_cert_error_string() is that it isn't
> > > > > > thread
> > > > > > safe since it might return a pointer to a static buffer -- it should
> > > > > > not, but who can be sure...
> > > > >
> > > > > Tough call. It may also help other code paths to do the same. But in
> > > > > many
> > > > > cases a dynamic buffer would be needed.
> > > > >
> > > > > Not sure if it makes sense to introduce warn_invalid_x509(). What I
> > > > > don't
> > > > > like is the verbose check before the warning. I wonder if we still
> > > > > need
> > > > > that. My last run has 11 failed roas and 51 failed mfts. The mft
> > > > > errors
> > > > > already show up. Shouldn't the roa errors be shown as well?
> > > >
> > > > They should. Unless I'm completely confusing myself, this is a bug in
> > > > the diff and all the added if (verbose > 1) should be dropped.
> > > >
> > > > If the last argument (nowarn) of valid_x509() was 0 (everywhere except
> > > > in proc_parser_pre()), valid_x509() would print the error independently
> > > > of verbose. verbose > 1 would force printing the warning also for mfts,
> > > > but there it would be drowned in the other noise.
> > > >
> > > > - if (!valid_x509(file, ctx, x509, a, crl, 0)) {
> > > >
> > > > ...
> > > >
> > > > - if (!nowarn || verbose > 1)
> > > > - warnx("%s: %s", file,
> > > > X509_verify_cert_error_string(c));
> > > >
> > >
> > > Indeed. Better diff below.
> > > Still thinking about the idea with the const char **.
> >
> > One of the main reasons for suggesting it was the amount of awkward line
> > wrapping. There's now much less of this. We can easily switch if you
> > should change your mind.
> >
> > ok tb
>
> Now that X509_verify_cert_error_string() is always returning a constant
> string lets return the error string instead.
Fine with me. Couple of simple comments below, then it's
ok tb
(happy to review again if you prefer that)
>
> --
> :wq Claudio
>
> Index: extern.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/extern.h,v
> retrieving revision 1.161
> diff -u -p -r1.161 extern.h
> --- extern.h 26 Nov 2022 12:02:36 -0000 1.161
> +++ extern.h 29 Nov 2022 09:36:29 -0000
> @@ -629,7 +629,7 @@ int valid_filename(const char *, size_
> int valid_uri(const char *, size_t, const char *);
> int valid_origin(const char *, const char *);
> int valid_x509(char *, X509_STORE_CTX *, X509 *, struct auth *,
> - struct crl *, int);
> + struct crl *, const char **);
> int valid_rsc(const char *, struct cert *, struct rsc *);
> int valid_econtent_version(const char *, const ASN1_INTEGER *);
> int valid_aspa(const char *, struct cert *, struct aspa *);
> Index: filemode.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/filemode.c,v
> retrieving revision 1.17
> diff -u -p -r1.17 filemode.c
> --- filemode.c 26 Nov 2022 12:02:37 -0000 1.17
> +++ filemode.c 29 Nov 2022 09:42:42 -0000
> @@ -141,6 +141,7 @@ parse_load_certchain(char *uri)
> struct cert *cert;
> struct crl *crl;
> struct auth *a;
> + const char *errstr;
I'd prefer to initialize errstr to NULL here, even if valid_x509() does it, too.
> int i;
>
> for (i = 0; i < MAX_CERT_DEPTH; i++) {
> @@ -171,9 +172,11 @@ parse_load_certchain(char *uri)
> uri = filestack[i];
>
> crl = crl_get(&crlt, a);
> - if (!valid_x509(uri, ctx, cert->x509, a, crl, 0) ||
> - !valid_cert(uri, a, cert))
> + if (!valid_x509(uri, ctx, cert->x509, a, crl, &errstr) ||
> + !valid_cert(uri, a, cert)) {
> + warnx("%s: %s", uri, errstr);
If valid_x509() succeeds and valid_cert() fails, errstr is NULL, so we shouldn't
print it. valid_cert() should always emit a warning on failure (should we add a
default case to the switch in valid_cert() to be 100% sure?), so this should be
if (errstr != NULL)
warnx("%s: %s", uri, errstr);
> goto fail;
> + }
> cert->talid = a->cert->talid;
> a = auth_insert(&auths, cert, a);
> stack[i] = NULL;
> @@ -407,6 +410,7 @@ proc_parser_file(char *file, unsigned ch
> if (aia != NULL) {
> struct auth *a;
> struct crl *c;
> + const char *errstr;
> char *crl_uri;
> int status;
>
> @@ -418,7 +422,7 @@ proc_parser_file(char *file, unsigned ch
> a = auth_find(&auths, aki);
> c = crl_get(&crlt, a);
>
> - if ((status = valid_x509(file, ctx, x509, a, c, 0))) {
> + if ((status = valid_x509(file, ctx, x509, a, c, &errstr))) {
> switch (type) {
> case RTYPE_ROA:
> status = roa->valid;
> @@ -438,8 +442,11 @@ proc_parser_file(char *file, unsigned ch
> }
> if (status)
> printf("OK");
> - else
> + else {
> printf("Failed");
> + if (errstr != NULL)
> + printf(", %s", errstr);
> + }
> } else if (is_ta) {
> if ((tal = find_tal(cert)) != NULL) {
> cert = ta_parse(file, cert, tal->pkey, tal->pkeysz);
> Index: parser.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/parser.c,v
> retrieving revision 1.78
> diff -u -p -r1.78 parser.c
> --- parser.c 2 Nov 2022 12:43:02 -0000 1.78
> +++ parser.c 29 Nov 2022 09:45:33 -0000
> @@ -132,6 +132,7 @@ proc_parser_roa(char *file, const unsign
> struct auth *a;
> struct crl *crl;
> X509 *x509;
> + const char *errstr;
>
> if ((roa = roa_parse(&x509, file, der, len)) == NULL)
> return NULL;
> @@ -139,7 +140,8 @@ proc_parser_roa(char *file, const unsign
> a = valid_ski_aki(file, &auths, roa->ski, roa->aki);
> crl = crl_get(&crlt, a);
>
> - if (!valid_x509(file, ctx, x509, a, crl, 0)) {
> + if (!valid_x509(file, ctx, x509, a, crl, &errstr)) {
> + warnx("%s: %s", file, errstr);
> X509_free(x509);
> roa_free(roa);
> return NULL;
> @@ -232,6 +234,7 @@ parse_load_crl_from_mft(struct entity *e
> if (!valid_hash(f, flen, mft->crlhash, sizeof(mft->crlhash)))
> goto next;
> crl = crl_parse(fn, f, flen);
> +
> next:
> free(f);
> free(fn);
> @@ -255,19 +258,21 @@ next:
> */
> static struct mft *
> proc_parser_mft_pre(char *file, const unsigned char *der, size_t len,
> - struct entity *entp, enum location loc, struct crl **crl)
> + struct entity *entp, enum location loc, struct crl **crl,
> + const char **errstr)
> {
> struct mft *mft;
> X509 *x509;
> struct auth *a;
>
> *crl = NULL;
> + *errstr = NULL;
> if ((mft = mft_parse(&x509, file, der, len)) == NULL)
> return NULL;
> *crl = parse_load_crl_from_mft(entp, mft, loc);
>
> a = valid_ski_aki(file, &auths, mft->ski, mft->aki);
> - if (!valid_x509(file, ctx, x509, a, *crl, 1)) {
> + if (!valid_x509(file, ctx, x509, a, *crl, errstr)) {
> X509_free(x509);
> mft_free(mft);
> crl_free(*crl);
> @@ -285,13 +290,16 @@ proc_parser_mft_pre(char *file, const un
> * Return the mft on success or NULL on failure.
> */
> static struct mft *
> -proc_parser_mft_post(char *file, struct mft *mft, const char *path)
> +proc_parser_mft_post(char *file, struct mft *mft, const char *path,
> + const char *errstr)
> {
> /* check that now is not before from */
> time_t now = time(NULL);
>
> if (mft == NULL) {
> - warnx("%s: no valid mft available", file);
> + if (errstr == NULL)
> + errstr = "no valid mft available";
> + warnx("%s: %s", file, errstr);
> return NULL;
> }
>
> @@ -330,6 +338,7 @@ proc_parser_mft(struct entity *entp, str
> struct mft *mft1 = NULL, *mft2 = NULL;
> struct crl *crl, *crl1 = NULL, *crl2 = NULL;
> char *f, *file, *file1, *file2;
> + const char *err1, *err2;
> size_t flen;
>
> *mp = NULL;
> @@ -341,7 +350,7 @@ proc_parser_mft(struct entity *entp, str
> if (f == NULL && errno != ENOENT)
> warn("parse file %s", file1);
> mft1 = proc_parser_mft_pre(file1, f, flen, entp, DIR_VALID,
> - &crl1);
> + &crl1, &err1);
> free(f);
> }
> if (file2 != NULL) {
> @@ -349,22 +358,27 @@ proc_parser_mft(struct entity *entp, str
> if (f == NULL && errno != ENOENT)
> warn("parse file %s", file2);
> mft2 = proc_parser_mft_pre(file2, f, flen, entp, DIR_TEMP,
> - &crl2);
> + &crl2, &err2);
> free(f);
> }
>
> + /* overload error from temp file if it is set */
> + if (mft1 == NULL && mft2 == NULL)
> + if (err2 != NULL)
> + err1 = err2;
> +
> if (mft_compare(mft1, mft2) == 1) {
> mft_free(mft2);
> crl_free(crl2);
> free(file2);
> - *mp = proc_parser_mft_post(file1, mft1, entp->path);
> + *mp = proc_parser_mft_post(file1, mft1, entp->path, err1);
> crl = crl1;
> file = file1;
> } else {
> mft_free(mft1);
> crl_free(crl1);
> free(file1);
> - *mp = proc_parser_mft_post(file2, mft2, entp->path);
> + *mp = proc_parser_mft_post(file2, mft2, entp->path, err2);
> crl = crl2;
> file = file2;
> }
> @@ -393,6 +407,7 @@ proc_parser_cert(char *file, const unsig
> struct cert *cert;
> struct crl *crl;
> struct auth *a;
> + const char *errstr;
Again, I'd prefer to initialize to NULL
>
> /* Extract certificate data. */
>
> @@ -404,8 +419,9 @@ proc_parser_cert(char *file, const unsig
> a = valid_ski_aki(file, &auths, cert->ski, cert->aki);
> crl = crl_get(&crlt, a);
>
> - if (!valid_x509(file, ctx, cert->x509, a, crl, 0) ||
> + if (!valid_x509(file, ctx, cert->x509, a, crl, &errstr) ||
> !valid_cert(file, a, cert)) {
> + warnx("%s: %s", file, errstr);
and only warn if errstr is set
if (errstr != NULL)
warnx("%s: %s", file, errstr);
> cert_free(cert);
> return NULL;
> }
> @@ -465,10 +481,11 @@ proc_parser_root_cert(char *file, const
> static void
> proc_parser_gbr(char *file, const unsigned char *der, size_t len)
> {
> - struct gbr *gbr;
> - X509 *x509;
> - struct crl *crl;
> - struct auth *a;
> + struct gbr *gbr;
> + X509 *x509;
> + struct crl *crl;
> + struct auth *a;
> + const char *errstr;
>
> if ((gbr = gbr_parse(&x509, file, der, len)) == NULL)
> return;
> @@ -477,7 +494,8 @@ proc_parser_gbr(char *file, const unsign
> crl = crl_get(&crlt, a);
>
> /* return value can be ignored since nothing happens here */
> - valid_x509(file, ctx, x509, a, crl, 0);
> + if (!valid_x509(file, ctx, x509, a, crl, &errstr))
> + warnx("%s: %s", file, errstr);
>
> X509_free(x509);
> gbr_free(gbr);
> @@ -489,10 +507,11 @@ proc_parser_gbr(char *file, const unsign
> static struct aspa *
> proc_parser_aspa(char *file, const unsigned char *der, size_t len)
> {
> - struct aspa *aspa;
> - struct auth *a;
> - struct crl *crl;
> - X509 *x509;
> + struct aspa *aspa;
> + struct auth *a;
> + struct crl *crl;
> + X509 *x509;
> + const char *errstr;
>
> if ((aspa = aspa_parse(&x509, file, der, len)) == NULL)
> return NULL;
> @@ -500,7 +519,8 @@ proc_parser_aspa(char *file, const unsig
> a = valid_ski_aki(file, &auths, aspa->ski, aspa->aki);
> crl = crl_get(&crlt, a);
>
> - if (!valid_x509(file, ctx, x509, a, crl, 0)) {
> + if (!valid_x509(file, ctx, x509, a, crl, &errstr)) {
> + warnx("%s: %s", file, errstr);
> X509_free(x509);
> aspa_free(aspa);
> return NULL;
> @@ -526,11 +546,12 @@ proc_parser_aspa(char *file, const unsig
> static struct tak *
> proc_parser_tak(char *file, const unsigned char *der, size_t len)
> {
> - struct tak *tak;
> - X509 *x509;
> - struct crl *crl;
> - struct auth *a;
> - int rc = 0;
> + struct tak *tak;
> + X509 *x509;
> + struct crl *crl;
> + struct auth *a;
> + const char *errstr;
> + int rc = 0;
>
> if ((tak = tak_parse(&x509, file, der, len)) == NULL)
> return NULL;
> @@ -538,8 +559,10 @@ proc_parser_tak(char *file, const unsign
> a = valid_ski_aki(file, &auths, tak->ski, tak->aki);
> crl = crl_get(&crlt, a);
>
> - if (!valid_x509(file, ctx, x509, a, crl, 0))
> + if (!valid_x509(file, ctx, x509, a, crl, &errstr)) {
> + warnx("%s: %s", file, errstr);
> goto out;
> + }
>
> /* TAK EE must be signed by self-signed CA */
> if (a->parent != NULL)
> Index: validate.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/rpki-client/validate.c,v
> retrieving revision 1.47
> diff -u -p -r1.47 validate.c
> --- validate.c 26 Nov 2022 12:02:37 -0000 1.47
> +++ validate.c 29 Nov 2022 09:36:18 -0000
> @@ -369,18 +369,20 @@ build_crls(const struct crl *crl, STACK_
> /*
> * 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
> + * and sets err pointer to the error returned by X509_verify_cert().
* and sets *errstr to the error returned by X509_verify_cert_error_string().
> */
> int
> valid_x509(char *file, X509_STORE_CTX *store_ctx, X509 *x509, struct auth *a,
> - struct crl *crl, int nowarn)
> + struct crl *crl, const char **errstr)
> {
> X509_VERIFY_PARAM *params;
> ASN1_OBJECT *cp_oid;
> STACK_OF(X509) *chain;
> STACK_OF(X509_CRL) *crls = NULL;
> unsigned long flags;
> - int c;
> + int error;
>
> + *errstr = NULL;
> build_chain(a, &chain);
> build_crls(crl, &crls);
>
> @@ -405,9 +407,8 @@ valid_x509(char *file, X509_STORE_CTX *s
> X509_STORE_CTX_set0_crls(store_ctx, crls);
>
> if (X509_verify_cert(store_ctx) <= 0) {
> - c = X509_STORE_CTX_get_error(store_ctx);
> - if (!nowarn || verbose > 1)
> - warnx("%s: %s", file, X509_verify_cert_error_string(c));
> + error = X509_STORE_CTX_get_error(store_ctx);
> + *errstr = X509_verify_cert_error_string(error);
> X509_STORE_CTX_cleanup(store_ctx);
> sk_X509_free(chain);
> sk_X509_CRL_free(crls);
