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

On 3/31/23 02:14, Daniel Gustafsson wrote:
>> On 14 Mar 2023, at 20:20, Jacob Champion <jchampion(at)timescale(dot)com> wrote:
>
>> Rebased over yesterday's Meson changes in v8.
>
> I had a look at this and agree that it's something we should do.

Great, thanks for the review!

> + # 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 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. :/

> + libpq_append_conn_error(conn, "weak sslmode \"%s\" may not be used with sslrootcert=system",
> + conn->sslmode);
> I think we should help the user by indicating which sslmode we allow in this
> message.

Added in v9.

> +
> + /*
> + * sslmode is not specified. Let it be filled in with the compiled
> + * default for now, but if sslrootcert=system, we'll override the
> + * default later before returning.
> + */
> + sslmode_default = option;
> As a not to self and other reviewers, "git am" misplaced this when applying the
> patch such that the result was syntactically correct but semantically wrong,
> causing very weird test errors.

Lovely... I've formatted v9 with a longer patch context.

> + sslmode_default->val = strdup("verify-full");
> This needs to be checked for OOM error.

Whoops, should be fixed now.

> - if (fnbuf[0] != '\0' &&
> - stat(fnbuf, &buf) == 0)
> + if (strcmp(fnbuf, "system") == 0)
> I'm not a fan of magic values, but sadly I don't have a better idea for this.
> We should however document that the keyword takes precedence over a file with
> the same name (even though the collision is unlikely).

Added a note to the docs.

> + 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...)

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

--Jacob