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
> It seems OpenSSL made X509_verify_cert_error_string() thread safe by only
> returning static strings. So maybe we can ignore the issue and just assume
> all is well.
>
> --
> :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 28 Nov 2022 10:42:11 -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 *, int *);
> 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 28 Nov 2022 18:04:57 -0000
> @@ -141,7 +141,7 @@ parse_load_certchain(char *uri)
> struct cert *cert;
> struct crl *crl;
> struct auth *a;
> - int i;
> + int i, e;
>
> for (i = 0; i < MAX_CERT_DEPTH; i++) {
> filestack[i] = uri;
> @@ -171,9 +171,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, &e) ||
> + !valid_cert(uri, a, cert)) {
> + warnx("%s: %s", uri, X509_verify_cert_error_string(e));
> goto fail;
> + }
> cert->talid = a->cert->talid;
> a = auth_insert(&auths, cert, a);
> stack[i] = NULL;
> @@ -275,7 +277,7 @@ proc_parser_file(char *file, unsigned ch
> char filehash[SHA256_DIGEST_LENGTH];
> char *hash;
> enum rtype type;
> - int is_ta = 0;
> + int is_ta = 0, e;
>
> if (num++ > 0) {
> if ((outformats & FORMAT_JSON) == 0)
> @@ -418,7 +420,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, &e))) {
> switch (type) {
> case RTYPE_ROA:
> status = roa->valid;
> @@ -438,8 +440,12 @@ proc_parser_file(char *file, unsigned ch
> }
> if (status)
> printf("OK");
> - else
> + else {
> printf("Failed");
> + if (e != 0)
> + printf(", %s",
> + X509_verify_cert_error_string(e));
> + }
> } 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 28 Nov 2022 17:14:31 -0000
> @@ -132,6 +132,7 @@ proc_parser_roa(char *file, const unsign
> struct auth *a;
> struct crl *crl;
> X509 *x509;
> + int e;
>
> 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, &e)) {
> + warnx("%s: %s", file, X509_verify_cert_error_string(e));
> 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,20 @@ 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, int *error)
> {
> struct mft *mft;
> X509 *x509;
> struct auth *a;
>
> *crl = NULL;
> + *error = 0;
> 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, error)) {
> X509_free(x509);
> mft_free(mft);
> crl_free(*crl);
> @@ -285,13 +289,17 @@ 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, int
> error)
> {
> /* check that now is not before from */
> time_t now = time(NULL);
>
> if (mft == NULL) {
> - warnx("%s: no valid mft available", file);
> + if (error != 0)
> + warnx("%s: %s", file,
> + X509_verify_cert_error_string(error));
> + else
> + warnx("%s: no valid mft available", file);
> return NULL;
> }
>
> @@ -331,6 +339,7 @@ proc_parser_mft(struct entity *entp, str
> struct crl *crl, *crl1 = NULL, *crl2 = NULL;
> char *f, *file, *file1, *file2;
> size_t flen;
> + int err1, err2;
>
> *mp = NULL;
> file1 = parse_filepath(entp->repoid, entp->path, entp->file, DIR_VALID);
> @@ -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 != 0)
> + 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;
> + int e;
>
> /* 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, &e) ||
> !valid_cert(file, a, cert)) {
> + warnx("%s: %s", file, X509_verify_cert_error_string(e));
> cert_free(cert);
> return NULL;
> }
> @@ -469,6 +485,7 @@ proc_parser_gbr(char *file, const unsign
> X509 *x509;
> struct crl *crl;
> struct auth *a;
> + int e;
>
> 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, &e))
> + warnx("%s: %s", file, X509_verify_cert_error_string(e));
>
> X509_free(x509);
> gbr_free(gbr);
> @@ -493,6 +511,7 @@ proc_parser_aspa(char *file, const unsig
> struct auth *a;
> struct crl *crl;
> X509 *x509;
> + int e;
>
> 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, &e)) {
> + warnx("%s: %s", file, X509_verify_cert_error_string(e));
> X509_free(x509);
> aspa_free(aspa);
> return NULL;
> @@ -530,7 +550,7 @@ proc_parser_tak(char *file, const unsign
> X509 *x509;
> struct crl *crl;
> struct auth *a;
> - int rc = 0;
> + int rc = 0, e;
>
> if ((tak = tak_parse(&x509, file, der, len)) == NULL)
> return NULL;
> @@ -538,8 +558,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, &e)) {
> + warnx("%s: %s", file, X509_verify_cert_error_string(e));
> 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 28 Nov 2022 14:48:26 -0000
> @@ -369,17 +369,17 @@ 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().
> */
> int
> valid_x509(char *file, X509_STORE_CTX *store_ctx, X509 *x509, struct auth *a,
> - struct crl *crl, int nowarn)
> + struct crl *crl, int *err)
> {
> X509_VERIFY_PARAM *params;
> ASN1_OBJECT *cp_oid;
> STACK_OF(X509) *chain;
> STACK_OF(X509_CRL) *crls = NULL;
> unsigned long flags;
> - int c;
>
> build_chain(a, &chain);
> build_crls(crl, &crls);
> @@ -405,9 +405,7 @@ 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));
> + *err = X509_STORE_CTX_get_error(store_ctx);
> X509_STORE_CTX_cleanup(store_ctx);
> sk_X509_free(chain);
> sk_X509_CRL_free(crls);
