On Tue, Nov 29, 2022 at 11:16:25AM +0100, Theo Buehler wrote:
> 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)
>
> > 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.
I did not do this since the valid_x509() call is in a for loop and errstr
should probably be reset on every iteration.
> > 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);
Good catch and fixed.
valid_cert() can AFAIK no longer fail since the introduction of RFC 3779
support in libressl. At least I see no way for it to trigger unless there
is some major bug.
> > @@ -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
Done
> >
> > /* 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);
Yep. Fixed.
> > @@ -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().
I had a similar fix in my tree.
--
:wq Claudio
