Re: [PATCH] Add `verify-system` sslmode to use system CA pool for server cert

> On 31 Mar 2023, at 19:59, Jacob Champion <jchampion(at)timescale(dot)com> wrote:

>> + # Let tests differentiate between vanilla OpenSSL and LibreSSL.
>> + AC_CHECK_DECLS([LIBRESSL_VERSION_NUMBER], [], [], [#include <openssl/opensslv.h>])
>> We have a check for SSL_CTX_set_cert_cb which is specifically done since it's
>> not present in Libressl. Rather than spending more cycles in autoconf/meson,
>> couldn't we use HAVE_SSL_CTX_SET_CERT_CB for this test? (Longer term, maybe we
>> should make the checks properly distinguish between OpenSSL and LibreSSL as
>> they are diverging, but thats not for this patch to tackle.)
>
> I can make that change; note that it'll also skip some of the new tests
> with OpenSSL 1.0.1, where there's no SSL_CTX_set_cert_cb. If that's
> acceptable, it should be an easy switch.

I'm not sure I follow, AFAICT it's present all the way till 3.1 at least? What
am I missing?

>> I can agree with the comment that this seems brittle. How about moving the installation of openssl to after the brew cleanup stage to avoid the need for this? While that may leave more in the cache, it seems more palatable. Something like this essentially:
>>
>> brew install <everything but openssl>
>> brew cleanup -s
>> # Comment about why OpenSSL is kept separate
>> brew install openssl(at)3
>
> That looks much better to me, but it didn't work when I tried it. One or
> more of the packages above it (and/or the previous cache?) has already
> installed OpenSSL as one of its dependencies, so the last `brew install`
> becomes a no-op. I tried an `install --force` as well, but that didn't
> seem to do anything differently. :/

Ugh, that's very unfortunate, I guess we're stuck with this then. If we can't
make brew cleanup not remove it then any hack applied to make it stick around
will be equally brittle so we might as well mkdir it back.

>> + if (SSL_CTX_set_default_verify_paths(SSL_context) != 1)
>> OpenSSL documents this as "A missing default location is still treated as a
>> success.", is that something we need to document or in any way deal with?
>> (Skimming the OpenSSL code I'm not sure it's actually correct in v3+, but I
>> might very well have missed something.)
>
> I think it's still true in v3+, because that sounds exactly like the
> brew issue we're working around in Cirrus. I'm not sure if there's much
> for us to do in that case, short of reimplementing the OpenSSL defaults
> logic and checking it ourselves. (And even that would look different
> between OpenSSL and LibreSSL...)

Right, that's clearly not something we want to do.

> Is there something we could document that's more helpful than "make sure
> your installation isn't broken"?

I wonder if there is an openssl command line example for verifying defaults
that we can document and refer to?

--
Daniel Gustafsson