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

> On 14 Mar 2023, at 20:20, Jacob Champion <jchampion@timescale.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.  The patch
seems quite close to committable, I just have a few comments on it:

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


+    # brew cleanup removes the empty certs directory in OPENSSLDIR, causing
+    # OpenSSL to report unexpected errors ("unregistered scheme") during
+    # verification failures. Put it back for now as a workaround.
+    #
+    #   https://github.com/orgs/Homebrew/discussions/4030
+    #
+    # Note that $(brew --prefix openssl) will give us the opt/ prefix but not
+    # the etc/ prefix, so we hardcode the full path here. openssl@3 is pinned
+    # above to try to minimize the chances of this changing beneath us, but it's
+    # brittle...
+    mkdir -p "/opt/homebrew/etc/openssl@3/certs"
I can agree with the comment that this seems brittle. How about moving the installation of openssl to after the brew
cleanupstage to avoid the need for this? While that may leave more in the cache, it seems more palatable. Something
likethis essentially: 

    brew install <everything but openssl>
    brew cleanup -s
    # Comment about why OpenSSL is kept separate
    brew install openssl@3


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


+
+    /*
+     * 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.


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


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


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

--
Daniel Gustafsson