Re: Is it worth accepting multiple CRLs? - Mailing list pgsql-hackers

At Mon, 03 Aug 2020 16:20:40 +0900 (JST), Kyotaro Horiguchi <horikyota.ntt@gmail.com> wrote in 
> Thanks for the opinion. I'll continue working on this.

This is it, but.. 

Looking closer I realized that certificates are verified in each
backend so CRL cache doesn't work at all for the hashed directory
method. Therefore, all CRL files relevant to a certificate to be
verfied are loaded every time a backend starts.

The only advantage of this is avoiding irrelevant CRLs from being
loaded in exchange of loading relevant CRLs at every session
start. Session startup gets slower by many delta CRLs from the same
CA.

Seems far from promising.

regards.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


From 80a20df187bb2a8555996d7aecb951d345cf51c2 Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi <horikyoga.ntt@gmail.com>
Date: Tue, 21 Jul 2020 23:01:27 +0900
Subject: [PATCH v1] Allow directory name for GUC ssl_crl_file and connection
 option sslcrl

X509_STORE_load_locations accepts a directory, which leads to
on-demand loading method with which method only relevant CRLs are
loaded.
---
 doc/src/sgml/config.sgml                  | 10 ++++----
 doc/src/sgml/libpq.sgml                   | 10 +++++---
 doc/src/sgml/runtime.sgml                 | 30 ++++++++++++++++++++++
 src/backend/libpq/be-secure-openssl.c     | 12 ++++++++-
 src/interfaces/libpq/fe-secure-openssl.c  | 12 ++++++++-
 src/test/ssl/Makefile                     | 13 +++++++++-
 src/test/ssl/ssl/clientcrldir/9bb9e3c3.r0 | 11 ++++++++
 src/test/ssl/ssl/clientcrldir/a3d11bff.r0 | 11 ++++++++
 src/test/ssl/ssl/servercrldir/a3d11bff.r0 | 11 ++++++++
 src/test/ssl/ssl/servercrldir/a836cc2d.r0 | 11 ++++++++
 src/test/ssl/t/001_ssltests.pl            | 31 +++++++++++++++++++++--
 src/test/ssl/t/SSLServer.pm               |  5 +++-
 12 files changed, 152 insertions(+), 15 deletions(-)
 create mode 100644 src/test/ssl/ssl/clientcrldir/9bb9e3c3.r0
 create mode 100644 src/test/ssl/ssl/clientcrldir/a3d11bff.r0
 create mode 100644 src/test/ssl/ssl/servercrldir/a3d11bff.r0
 create mode 100644 src/test/ssl/ssl/servercrldir/a836cc2d.r0

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7a7177c550..a3e81619c9 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1182,11 +1182,11 @@ include_dir 'conf.d'
       <listitem>
        <para>
         Specifies the name of the file containing the SSL server certificate
-        revocation list (CRL).
-        Relative paths are relative to the data directory.
-        This parameter can only be set in the <filename>postgresql.conf</filename>
-        file or on the server command line.
-        The default is empty, meaning no CRL file is loaded.
+        revocation list (CRL) or the directory containing CRL files.  Relative
+        paths are relative to the data directory.  This parameter can only be
+        set in the <filename>postgresql.conf</filename> file or on the server
+        command line.  The default is empty, meaning no CRL file is
+        loaded. See <xref linkend="ssl-crl-files"/> for details.
        </para>
       </listitem>
      </varlistentry>
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index f7b765f76d..96bfbcfd82 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1705,10 +1705,12 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
       <listitem>
        <para>
         This parameter specifies the file name of the SSL certificate
-        revocation list (CRL).  Certificates listed in this file, if it
-        exists, will be rejected while attempting to authenticate the
-        server's certificate.  The default is
-        <filename>~/.postgresql/root.crl</filename>.
+        revocation list (CRL) or the directory name that contains CRL files.
+        Certificates listed in this file or directoty, if it exists, will be
+        rejected while attempting to authenticate the server's certificate.
+        The default is
+        <filename>~/.postgresql/root.crl</filename>. See
+        <xref linkend="ssl-crl-files"/> for details.
        </para>
       </listitem>
      </varlistentry>
diff --git a/doc/src/sgml/runtime.sgml b/doc/src/sgml/runtime.sgml
index c8698898f3..5d7fd48ad7 100644
--- a/doc/src/sgml/runtime.sgml
+++ b/doc/src/sgml/runtime.sgml
@@ -2486,6 +2486,36 @@ openssl x509 -req -in server.csr -text -days 365 \
    </para>
   </sect2>
 
+  <sect2 id="ssl-crl-files">
+    <title>Certification Revokation List files</title>
+
+    <para> The server setting <xref linkend="guc-ssl-crl-file"/> and the
+    connection option <xref linkend="libpq-connect-sslcrl"/> specify either a
+    file containing one or more CRL, or a directory containing a separate file
+    for every CRL. In the first method, file method, the all CRLs in the file
+    is loaded at server start time or by reloading config file
+    (<command>pg_ctl reload</command>). In the second method, hashed directory
+    method, CRL files are loaded on-demand, that is, only the relevant CRL
+    files are loaded at connection time.
+    </para>
+    <para>
+    The CRL file used for the file method can contain multiple CRLs, like
+    certificates, by just concatenated if it is in PEM format.  In the hashed
+    directory method, every file in the directory has the name
+    with <parameter>hash</parameter>.r<parameter>N</parameter> format,
+    where <parameter>hash</parameter> is the hash value of the issuer of the
+    CRL and <parameter>N</parameter> is a sequence number that starts at
+    zero. The hash value is calculated using openssl command. In both cases
+    the CRLs from all CAs involved in a certificate chain are needed to verify
+    a certificate, even if some or all of them are empty.
+<programlisting>
+$ openssl crl -hash -noout -in foo.crl
+98668507
+$ cp foo.crl $PGDATA/crldir/98668507.r0
+</programlisting>
+    </para>
+  </sect2>
+
  </sect1>
 
  <sect1 id="gssapi-enc">
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 8b21ff4065..d85dc710a4 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -289,8 +289,18 @@ be_tls_init(bool isServerStart)
 
         if (cvstore)
         {
+            char *filename = NULL;
+            char *dirname = NULL;
+            struct stat statbuf;
+
+            /* identify whether it is a file or a directoty */
+            if (stat(ssl_crl_file, &statbuf) == 0 && S_ISDIR(statbuf.st_mode))
+                dirname = ssl_crl_file;
+            else
+                filename = ssl_crl_file;
+
             /* Set the flags to check against the complete CRL chain */
-            if (X509_STORE_load_locations(cvstore, ssl_crl_file, NULL) == 1)
+            if (X509_STORE_load_locations(cvstore, filename, dirname) == 1)
             {
                 X509_STORE_set_flags(cvstore,
                                      X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);
diff --git a/src/interfaces/libpq/fe-secure-openssl.c b/src/interfaces/libpq/fe-secure-openssl.c
index d609a38bbe..1ae60226b1 100644
--- a/src/interfaces/libpq/fe-secure-openssl.c
+++ b/src/interfaces/libpq/fe-secure-openssl.c
@@ -936,6 +936,10 @@ initialize_SSL(PGconn *conn)
 
         if ((cvstore = SSL_CTX_get_cert_store(SSL_context)) != NULL)
         {
+            char *filename = NULL;
+            char *dirname = NULL;
+            struct stat statbuf;
+
             if (conn->sslcrl && strlen(conn->sslcrl) > 0)
                 strlcpy(fnbuf, conn->sslcrl, sizeof(fnbuf));
             else if (have_homedir)
@@ -943,9 +947,15 @@ initialize_SSL(PGconn *conn)
             else
                 fnbuf[0] = '\0';
 
+            /* Identify whether it is a file or a directory */
+            if (stat(fnbuf, &statbuf) == 0 && S_ISDIR(statbuf.st_mode))
+                dirname = fnbuf;
+            else
+                filename = fnbuf;
+
             /* Set the flags to check against the complete CRL chain */
             if (fnbuf[0] != '\0' &&
-                X509_STORE_load_locations(cvstore, fnbuf, NULL) == 1)
+                X509_STORE_load_locations(cvstore, filename, dirname) == 1)
             {
                 X509_STORE_set_flags(cvstore,
                                      X509_V_FLAG_CRL_CHECK | X509_V_FLAG_CRL_CHECK_ALL);
diff --git a/src/test/ssl/Makefile b/src/test/ssl/Makefile
index 777ee39413..b04e532295 100644
--- a/src/test/ssl/Makefile
+++ b/src/test/ssl/Makefile
@@ -30,12 +30,14 @@ SSLFILES := $(CERTIFICATES:%=ssl/%.key) $(CERTIFICATES:%=ssl/%.crt) \
     ssl/client+client_ca.crt ssl/client-der.key \
     ssl/client-encrypted-pem.key ssl/client-encrypted-der.key
 
+SSLDIRS := ssl/clientcrldir ssl/servercrldir
+
 # This target re-generates all the key and certificate files. Usually we just
 # use the ones that are committed to the tree without rebuilding them.
 #
 # This target will fail unless preceded by sslfiles-clean.
 #
-sslfiles: $(SSLFILES)
+sslfiles: $(SSLFILES) $(SSLDIRS)
 
 # OpenSSL requires a directory to put all generated certificates in. We don't
 # use this for anything, but we need a location.
@@ -146,10 +148,19 @@ ssl/root+server.crl: ssl/root.crl ssl/server.crl
     cat $^ > $@
 ssl/root+client.crl: ssl/root.crl ssl/client.crl
     cat $^ > $@
+ssl/servercrldir: ssl/server.crl
+    mkdir ssl/servercrldir
+    cp ssl/server.crl ssl/servercrldir/`openssl crl -hash -noout -in ssl/server.crl`.r0
+    cp ssl/root.crl ssl/servercrldir/`openssl crl -hash -noout -in ssl/root.crl`.r0
+ssl/clientcrldir: ssl/client.crl
+    mkdir ssl/clientcrldir
+    cp ssl/client.crl ssl/clientcrldir/`openssl crl -hash -noout -in ssl/client.crl`.r0
+    cp ssl/root.crl ssl/clientcrldir/`openssl crl -hash -noout -in ssl/root.crl`.r0
 
 .PHONY: sslfiles-clean
 sslfiles-clean:
     rm -f $(SSLFILES) ssl/client_ca.srl ssl/server_ca.srl ssl/client_ca-certindex* ssl/server_ca-certindex*
ssl/root_ca-certindex*ssl/root_ca.srl ssl/temp_ca.crt ssl/temp_ca_signed.crt
 
+    rm -rf $(SSLDIRS)
 
 clean distclean maintainer-clean:
     rm -rf tmp_check
diff --git a/src/test/ssl/ssl/clientcrldir/9bb9e3c3.r0 b/src/test/ssl/ssl/clientcrldir/9bb9e3c3.r0
new file mode 100644
index 0000000000..a667680e04
--- /dev/null
+++ b/src/test/ssl/ssl/clientcrldir/9bb9e3c3.r0
@@ -0,0 +1,11 @@
+-----BEGIN X509 CRL-----
+MIIBnjCBhzANBgkqhkiG9w0BAQsFADBCMUAwPgYDVQQDDDdUZXN0IENBIGZvciBQ
+b3N0Z3JlU1FMIFNTTCByZWdyZXNzaW9uIHRlc3QgY2xpZW50IGNlcnRzFw0xODEx
+MjcxMzQwNTVaFw00NjA0MTQxMzQwNTVaMBQwEgIBAhcNMTgxMTI3MTM0MDU1WjAN
+BgkqhkiG9w0BAQsFAAOCAQEAXjLxA9Qc6gAudwUHBxMIq5EHBcuNEX5e3GNlkyNf
+8I0DtHTPfJPvmAG+i6lYz//hHmmjxK0dR2ucg79XgXI/6OpDqlxS/TG1Xv52wA1p
+xz6GaJ2hC8Lk4/vbJo/Rrzme2QsI7xqBWya0JWVrehttqhFxPzWA5wID8X7G4Kb4
+pjVnzqYzn8A9FBiV9t10oZg60aVLqt3kbyy+U3pefvjhj8NmQc7uyuVjWvYZA0vG
+nnDUo4EKJzHNIYLk+EfpzKWO2XAWBLOT9SyyNCeMuQ5p/2pdAt9jtWHenms2ajo9
+2iUsHS91e3TooP9yNYuNcN8/wXY6H2Xm+dCLcEnkcr7EEw==
+-----END X509 CRL-----
diff --git a/src/test/ssl/ssl/clientcrldir/a3d11bff.r0 b/src/test/ssl/ssl/clientcrldir/a3d11bff.r0
new file mode 100644
index 0000000000..e879cf25a7
--- /dev/null
+++ b/src/test/ssl/ssl/clientcrldir/a3d11bff.r0
@@ -0,0 +1,11 @@
+-----BEGIN X509 CRL-----
+MIIBhTBvMA0GCSqGSIb3DQEBCwUAMEAxPjA8BgNVBAMMNVRlc3Qgcm9vdCBDQSBm
+b3IgUG9zdGdyZVNRTCBTU0wgcmVncmVzc2lvbiB0ZXN0IHN1aXRlFw0xODExMjcx
+MzQwNTVaFw00NjA0MTQxMzQwNTVaMA0GCSqGSIb3DQEBCwUAA4IBAQB8OSDym4/a
+qbZOrZvOOhmKrd7AJSTgAadtdK0CX3v58Ym3EmZK7gQFdBuFCXnvbue/x6avZHgz
+4pYFlJmL0IiD4QuTzsoo+LzifrmTzteO9oEJNLd2bjfEnpE5Wdaw6Yuy2Xb5edy5
+lQhNZdc8w3FiXhPOEUAi7EbdfDwn4G/fvEjpzyVb2wCujDUUePUGGayjKIM4PUu4
+pixM6gt9FFL27l47lQ3g0PbvB3TnU3oqcB3Y17FjbxjFc6AsGXholNetoEE2/49E
+PEYzOH7/PtxlZUtoCqZM+741LuI6Q7z4/P2X/IY33lMy6Iiyc41C94l/P7fCkMLG
+AlO+O0a4SpYS
+-----END X509 CRL-----
diff --git a/src/test/ssl/ssl/servercrldir/a3d11bff.r0 b/src/test/ssl/ssl/servercrldir/a3d11bff.r0
new file mode 100644
index 0000000000..e879cf25a7
--- /dev/null
+++ b/src/test/ssl/ssl/servercrldir/a3d11bff.r0
@@ -0,0 +1,11 @@
+-----BEGIN X509 CRL-----
+MIIBhTBvMA0GCSqGSIb3DQEBCwUAMEAxPjA8BgNVBAMMNVRlc3Qgcm9vdCBDQSBm
+b3IgUG9zdGdyZVNRTCBTU0wgcmVncmVzc2lvbiB0ZXN0IHN1aXRlFw0xODExMjcx
+MzQwNTVaFw00NjA0MTQxMzQwNTVaMA0GCSqGSIb3DQEBCwUAA4IBAQB8OSDym4/a
+qbZOrZvOOhmKrd7AJSTgAadtdK0CX3v58Ym3EmZK7gQFdBuFCXnvbue/x6avZHgz
+4pYFlJmL0IiD4QuTzsoo+LzifrmTzteO9oEJNLd2bjfEnpE5Wdaw6Yuy2Xb5edy5
+lQhNZdc8w3FiXhPOEUAi7EbdfDwn4G/fvEjpzyVb2wCujDUUePUGGayjKIM4PUu4
+pixM6gt9FFL27l47lQ3g0PbvB3TnU3oqcB3Y17FjbxjFc6AsGXholNetoEE2/49E
+PEYzOH7/PtxlZUtoCqZM+741LuI6Q7z4/P2X/IY33lMy6Iiyc41C94l/P7fCkMLG
+AlO+O0a4SpYS
+-----END X509 CRL-----
diff --git a/src/test/ssl/ssl/servercrldir/a836cc2d.r0 b/src/test/ssl/ssl/servercrldir/a836cc2d.r0
new file mode 100644
index 0000000000..717951c26a
--- /dev/null
+++ b/src/test/ssl/ssl/servercrldir/a836cc2d.r0
@@ -0,0 +1,11 @@
+-----BEGIN X509 CRL-----
+MIIBnjCBhzANBgkqhkiG9w0BAQsFADBCMUAwPgYDVQQDDDdUZXN0IENBIGZvciBQ
+b3N0Z3JlU1FMIFNTTCByZWdyZXNzaW9uIHRlc3Qgc2VydmVyIGNlcnRzFw0xODEx
+MjcxMzQwNTVaFw00NjA0MTQxMzQwNTVaMBQwEgIBBhcNMTgxMTI3MTM0MDU1WjAN
+BgkqhkiG9w0BAQsFAAOCAQEAbVuJXemxM6HLlIHGWlQvVmsmG4ZTQWiDnZjfmrND
+xB4XsvZNPXnFkjdBENDROrbDRwm60SJDW73AbDbfq1IXAzSpuEyuRz61IyYKo0wq
+nmObJtVdIu3bVlWIlDXaP5Emk3d7ouCj5f8Kyeb8gm4pL3N6e0eI63hCaS39hhE6
+RLGh9HU9ht1kKfgcTwmB5b2HTPb4M6z1AmSIaMVqZTjIspsUgNF2+GBm3fOnOaiZ
+SEXWtgjMRXiIHbtU0va3LhSH5OSW0mh+L9oGUQDYnyuudnWGpulhqIp4qVkJRDDu
+41HpD83dV2uRtBLvc25AFHj7kXBflbO3gvGZVPYf1zVghQ==
+-----END X509 CRL-----
diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index fd2727b568..8b0787ce7f 100644
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -13,7 +13,7 @@ use SSLServer;
 
 if ($ENV{with_openssl} eq 'yes')
 {
-    plan tests => 93;
+    plan tests => 100;
 }
 else
 {
@@ -214,6 +214,12 @@ test_connect_fails(
     "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/client.crl",
     qr/SSL error/,
     "CRL belonging to a different CA");
+# The same for CRL directory, fails
+test_connect_fails(
+    $common_connstr,
+    "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/clientcrldir",
+    qr/SSL error/,
+    "directory CRL belonging to a different CA");
 
 # With the correct CRL, succeeds (this cert is not revoked)
 test_connect_ok(
@@ -221,6 +227,12 @@ test_connect_ok(
     "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/root+server.crl",
     "CRL with a non-revoked cert");
 
+# With the correct server CRL directory, succeeds (this cert is not revoked)
+test_connect_ok(
+    $common_connstr,
+    "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/servercrldir",
+    "HOGE directory CRL with a non-revoked cert");
+
 # Check that connecting with verify-full fails, when the hostname doesn't
 # match the hostname in the server's certificate.
 $common_connstr =
@@ -346,7 +358,12 @@ test_connect_fails(
     $common_connstr,
     "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/root+server.crl",
     qr/SSL error/,
-    "does not connect with client-side CRL");
+    "does not connect with client-side CRL file");
+test_connect_fails(
+    $common_connstr,
+    "sslrootcert=ssl/root+server_ca.crt sslmode=verify-ca sslcrl=ssl/servercrldir",
+    qr/SSL error/,
+    "does not connect with client-side CRL directory");
 
 # pg_stat_ssl
 command_like(
@@ -545,6 +562,16 @@ test_connect_ok(
 test_connect_fails($common_connstr, "sslmode=require sslcert=ssl/client.crt",
     qr/SSL error/, "intermediate client certificate is missing");
 
+# test server-side CRL directory
+switch_server_cert($node, 'server-revoked', undef, 1);
+
+# revoked client cert
+test_connect_fails(
+    $common_connstr,
+    "user=ssltestuser sslcert=ssl/client-revoked.crt sslkey=ssl/client-revoked_tmp.key",
+    qr/SSL error/,
+    "certificate authorization fails with revoked client cert with server-side CRL directory");
+
 # clean up
 foreach my $key (@keys)
 {
diff --git a/src/test/ssl/t/SSLServer.pm b/src/test/ssl/t/SSLServer.pm
index 1e392b8fbf..5bc3bc91d5 100644
--- a/src/test/ssl/t/SSLServer.pm
+++ b/src/test/ssl/t/SSLServer.pm
@@ -151,6 +151,8 @@ sub configure_test_server_for_ssl
     copy_files("ssl/root+client_ca.crt", $pgdata);
     copy_files("ssl/root_ca.crt",        $pgdata);
     copy_files("ssl/root+client.crl",    $pgdata);
+    mkdir("$pgdata/clientcrldir");
+    copy_files("ssl/clientcrldir/*", "$pgdata/clientcrldir/");
 
     # Stop and restart server to load new listen_addresses.
     $node->restart;
@@ -168,6 +170,7 @@ sub switch_server_cert
     my $node     = $_[0];
     my $certfile = $_[1];
     my $cafile   = $_[2] || "root+client_ca";
+    my $crlfile  = ($_[3] ? "clientcrldir" : "root+client.crl");
     my $pgdata   = $node->data_dir;
 
     open my $sslconf, '>', "$pgdata/sslconfig.conf";
@@ -175,7 +178,7 @@ sub switch_server_cert
     print $sslconf "ssl_ca_file='$cafile.crt'\n";
     print $sslconf "ssl_cert_file='$certfile.crt'\n";
     print $sslconf "ssl_key_file='$certfile.key'\n";
-    print $sslconf "ssl_crl_file='root+client.crl'\n";
+    print $sslconf "ssl_crl_file='$crlfile'\n";
     close $sslconf;
 
     $node->restart;
-- 
2.18.4