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?
> >
> > --
> > :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)
> > + 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
