On Wed, Dec 29, 2021 at 01:12:25PM +0100, Claudio Jeker wrote:
> On Wed, Dec 29, 2021 at 01:06:30AM +0100, Theo Buehler wrote:
> > On Tue, Dec 28, 2021 at 05:08:46PM +0100, Claudio Jeker wrote:
> > > On Mon, Dec 27, 2021 at 12:23:32PM +0100, Theo Buehler wrote:
> > > > On Sat, Dec 25, 2021 at 05:48:53PM +0100, Claudio Jeker wrote:
> > > > [...]
> > > > > I would love to get rid of X509_V_FLAG_IGNORE_CRITICAL and use a
> > > > > callback
> > > > > to ensure the right extensions are critical but I never managed to
> > > > > understand how the X509_verify_cert() callback actually works.
> > > > > Documentation seems to be non-existent.
> > > >
> > > > My understanding is that X509_V_FLAG_IGNORE_CRITICAL is a big hammer
> > > > that was necessary as long as libcrypto didn't know about the RFC 3779
> > > > extensions. Without it X509_verify_cert() would error on encountering
> > > > them. This changed shortly after Gouveia when we enabled the RFC 3779
> > > > code in libcrypto. I think setting the flag is no longer appropriate in
> > > > -current.
> > > >
> > > > Below is a diff that removes X509_V_FLAG_IGNORE_CRITICAL and adds a
> > > > debug verify callback that intercepts unhandled critical extensions. It
> > > > lets through the RFC 3779 extensions and errors on everything else.
> > > >
> > > > If you run with this on -current, the callback doesn't print anything.
> > > > On -stable you'll see that it logs at least one of the two extensions
> > > > per cert while X509_verify_cert() is walking through the chains.
> > > >
> > > > If we want to drop X509_V_FLAG_IGNORE_CRITICAL, something like this
> > > > will be needed as long as rpki-client wants to support LibreSSL < 3.5.
> > > > The idea is that we handle RFC 3779 extensions ourselves, so we can
> > > > safely ignore them in the verifier. All other critical extensions that
> > > > neither rpki-client nor libcrypto know about should be verification
> > > > failures.
> > > >
> > > > You can play with this on -current by disabling RFC 3779 again: remove
> > > > or comment the #ifndef LIBRESSL_CRYPTO_INTERNAL around
> > > > OPENSSL_NO_RFC3779
> > > > in lib/libcrypto/opensslfeatures.h then make clean, make includes, make
> > > > and make install.
> > >
> > > I like this. A few questions inline.
> >
> > cool
> >
> > > > + fprintf(stderr, "%s cb: ok %d, %s, depth %d\n",
> > > > + descr, ok, X509_verify_cert_error_string(error), depth);
> > >
> > > Is there a reason you decided to use fprintf() over warnx() here?
> >
> > Only that I didn't really intend this for commit and was therefore a bit
> > lazy. I should have made that clearer.
> >
> > > And especially for the error cases below. AFAIK rpki-client uses warnx()
> > > in almost all such cases.
> > > Also we normally print the path of the file causing the error but I guess
> > > this is tricky to pass in here. I guess one could use
> > > X509_STORE_CTX_set_app_data() for that.
> >
> > Good idea, yes, that works.
> >
> > > > +static int
> > > > +roa_verify_cb(int ok, X509_STORE_CTX *store_ctx)
> > > > +{
> > > > + return generic_verify(ok, store_ctx, "roa");
> > > > +}
> > >
> > > Is there no other way to avoid this extra wrapping of functions?
> >
> > It was the first thing that came to mind so I could tell where the calls
> > originated.
> >
> > > See above I guess using X509_STORE_CTX_set_app_data() and
> > > X509_STORE_CTX_get_app_data() these wrappers become unnecessary.
> >
> > I think the suffix of the file gives enough of a clue, so that's
> > definitely no longer needed.
>
> Absolutly. Idealy we can pass the path to rpki-client -f in the future to
> get a verbose output of the object in question.
>
> > This should be closer to commit now. I removed the switch over the error
> > since I guess we would need to handle unhandled critical CRL extensions
> > differently. I switched the fprintf to warnx, although some more of
> > them should perhaps be fatal since it's most likely an allocation
> > failure.
>
> Not too worried about those errors, I doubt the will be seen often.
> Lets decide if someone actually runs into those.
>
> > Since this is super noisy (multiple lines per processed file), I reduced
> > to printing one line in the ordinary path and only on verbosity > 1. We
> > might even want to crank that higher or ditch this last warnx altogether.
>
> I think the verbose > 1 warnx could indeed be removed. With modern libs it
> will be never hit and for old libs the code takes care of this.
> Would simplify the code a bit more.
>
> > I also suppressed printing the cert error and the ok variable since I
> > don't think this is actually interesting, whereas the depth should give
> > a valuable clue where to look next, so I kept it.
> >
> > ok?
>
> I really need to try this on a system with old libressl. Will hopefully
> manage to do that today. Apart from that I'm OK with this going in.
> We can cleanup the last bits in tree.
I finally tested this on a Linux box with libressl 3.2.5 and it does work.
So the diff is OK claudio@
--
:wq Claudio
