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));
>
> > >
> > > --
> > > :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 11:08:31 -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,13 @@ 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)) {
> > > + if (verbose > 1)
drop if (verbose > 1)
> > > + 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 +279,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 +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, &e))) {
> > > switch (type) {
> > > case RTYPE_ROA:
> > > status = roa->valid;
> > > @@ -438,8 +442,12 @@ proc_parser_file(char *file, unsigned ch
> > > }
> > > if (status)
> > > printf("OK");
> > > - else
> > > + else {
> > > + if (verbose > 1)
> > > + warnx("%s: %s", file,
> > > + X509_verify_cert_error_string(e));
> > > printf("Failed");
> > > + }
> > > } 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 14:39:37 -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,9 @@ 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)) {
> > > + if (verbose > 1)
> > > + warnx("%s: %s", file, X509_verify_cert_error_string(e));
> > > X509_free(x509);
> > > roa_free(roa);
> > > return NULL;
> > > @@ -232,6 +235,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 +259,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 *err)
> > > {
> > > struct mft *mft;
> > > X509 *x509;
> > > struct auth *a;
> > >
> > > *crl = NULL;
> > > + *err = 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, err)) {
> > > X509_free(x509);
> > > mft_free(mft);
> > > crl_free(*crl);
> > > @@ -285,13 +290,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 +340,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 +351,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 +359,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 +408,7 @@ proc_parser_cert(char *file, const unsig
> > > struct cert *cert;
> > > struct crl *crl;
> > > struct auth *a;
> > > + int e;
> > >
> > > /* Extract certificate data. */
> > >
> > > @@ -404,8 +420,10 @@ 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)) {
> > > + if (verbose > 1)
> > > + warnx("%s: %s", file, X509_verify_cert_error_string(e));
> > > cert_free(cert);
> > > return NULL;
> > > }
> > > @@ -469,6 +487,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 +496,9 @@ 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))
> > > + if (verbose > 1)
> > > + warnx("%s: %s", file, X509_verify_cert_error_string(e));
> > >
> > > X509_free(x509);
> > > gbr_free(gbr);
> > > @@ -493,6 +514,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 +522,9 @@ 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)) {
> > > + if (verbose > 1)
> > > + warnx("%s: %s", file, X509_verify_cert_error_string(e));
> > > X509_free(x509);
> > > aspa_free(aspa);
> > > return NULL;
> > > @@ -530,7 +554,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 +562,11 @@ 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)) {
> > > + if (verbose > 1)
> > > + 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);
> > >
> >
>
> --
> :wq Claudio
