Support configuring multiple ECDH curves

The ssl_ecdh_curve GUC only accepts a single value, but the TLS
handshake can list multiple curves in the groups extension (the
extension has been renamed to contain more than elliptic curves).
This changes the GUC to accept a colon-separated list of curves.
This commit also renames the GUC to ssl_groups to match the new
nomenclature for the TLS extension.

Original patch by Erica Zhang with additional hacking by me.

Author: Erica Zhang <[email protected]>
Author: Daniel Gustafsson <[email protected]>
Reviewed-by: Jacob Champion <[email protected]>
Reviewed-by: Andres Freund <[email protected]>
Reviewed-by: Peter Eisentraut <[email protected]>
Reviewed-by: Jelte Fennema-Nio <[email protected]>
Discussion: https://postgr.es/m/[email protected]

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 934ef5e46910ecdcde632f9426e913d6e7e033ea..f8d862a6ce405d58f1d2c2b568881504706e68fa 100644 (file)
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -1452,20 +1452,20 @@ include_dir 'conf.d'
       </listitem>
      </varlistentry>
 
-     <varlistentry id="guc-ssl-ecdh-curve" xreflabel="ssl_ecdh_curve">
-      <term><varname>ssl_ecdh_curve</varname> (<type>string</type>)
+     <varlistentry id="guc-ssl-groups" xreflabel="ssl_groups">
+      <term><varname>ssl_groups</varname> (<type>string</type>)
       <indexterm>
-       <primary><varname>ssl_ecdh_curve</varname> configuration parameter</primary>
+       <primary><varname>ssl_groups</varname> configuration parameter</primary>
       </indexterm>
       </term>
       <listitem>
        <para>
         Specifies the name of the curve to use in <acronym>ECDH</acronym> key
         exchange.  It needs to be supported by all clients that connect.
+        Multiple curves can be specified by using a colon-separated list.
         It does not need to be the same curve used by the server's Elliptic
-        Curve key.
-        This parameter can only be set in the <filename>postgresql.conf</filename>
-        file or on the server command line.
+        Curve key.  This parameter can only be set in the
+        <filename>postgresql.conf</filename> file or on the server command line.
         The default is <literal>prime256v1</literal>.
        </para>
 
@@ -1475,9 +1475,16 @@ include_dir 'conf.d'
         <literal>prime256v1</literal> (NIST P-256),
         <literal>secp384r1</literal> (NIST P-384),
         <literal>secp521r1</literal> (NIST P-521).
-        The full list of available curves can be shown with the command
-        <command>openssl ecparam -list_curves</command>.  Not all of them
-        are usable in <acronym>TLS</acronym> though.
+        An incomplete list of available groups can be shown with the command
+        <command>openssl ecparam -list_curves</command>.  Not all of them are
+        usable with <acronym>TLS</acronym> though, and many supported group
+        names and aliases are omitted.
+       </para>
+
+       <para>
+        In <productname>PostgreSQL</productname> versions before 18.0 this
+        setting was named <literal>ssl_ecdh_curve</literal> and only accepted
+        a single value.
        </para>
       </listitem>
      </varlistentry>

diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 9d503104be3a878e632d53b75137a253c4d06960..c8cd81d8537be0fef1e36803d911376c6857afe4 100644 (file)
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -76,6 +76,7 @@ static int    alpn_cb(SSL *ssl,
                    void *userdata);
 static bool initialize_dh(SSL_CTX *context, bool isServerStart);
 static bool initialize_ecdh(SSL_CTX *context, bool isServerStart);
+static const char *SSLerrmessageExt(unsigned long ecode, const char *replacement);
 static const char *SSLerrmessage(unsigned long ecode);
 
 static char *X509_NAME_to_cstring(X509_NAME *name);
@@ -1409,35 +1410,50 @@ static bool
 initialize_ecdh(SSL_CTX *context, bool isServerStart)
 {
 #ifndef OPENSSL_NO_ECDH
-   EC_KEY     *ecdh;
-   int         nid;
-
-   nid = OBJ_sn2nid(SSLECDHCurve);
-   if (!nid)
-   {
-       ereport(isServerStart ? FATAL : LOG,
-               (errcode(ERRCODE_CONFIG_FILE_ERROR),
-                errmsg("ECDH: unrecognized curve name: %s", SSLECDHCurve)));
-       return false;
-   }
-
-   ecdh = EC_KEY_new_by_curve_name(nid);
-   if (!ecdh)
+   if (SSL_CTX_set1_groups_list(context, SSLECDHCurve) != 1)
    {
+       /*
+        * OpenSSL 3.3.0 introduced proper error messages for group parsing
+        * errors, earlier versions returns "no SSL error reported" which is
+        * far from helpful. For older versions, we replace with a better
+        * error message. Injecting the error into the OpenSSL error queue
+        * need APIs from OpenSSL 3.0.
+        */
        ereport(isServerStart ? FATAL : LOG,
-               (errcode(ERRCODE_CONFIG_FILE_ERROR),
-                errmsg("ECDH: could not create key")));
+               errcode(ERRCODE_CONFIG_FILE_ERROR),
+               errmsg("failed to set group names specified in ssl_groups: %s",
+                      SSLerrmessageExt(ERR_get_error(),
+                                       _("No valid groups found"))),
+               errhint("Ensure that each group name is spelled correctly and supported by the installed version of OpenSSL"));
        return false;
    }
-
-   SSL_CTX_set_options(context, SSL_OP_SINGLE_ECDH_USE);
-   SSL_CTX_set_tmp_ecdh(context, ecdh);
-   EC_KEY_free(ecdh);
 #endif
 
    return true;
 }
 
+/*
+ * Obtain reason string for passed SSL errcode with replacement
+ *
+ * The error message supplied in replacement will be used in case the error
+ * code from OpenSSL is 0, else the error message from SSLerrmessage() will
+ * be returned.
+ *
+ * Not all versions of OpenSSL place an error on the queue even for failing
+ * operations, which will yield "no SSL error reported" by SSLerrmessage. This
+ * function can be used to ensure that a proper error message is displayed for
+ * versions reporting no error, while using the OpenSSL error via SSLerrmessage
+ * for versions where there is one.
+ */
+static const char *
+SSLerrmessageExt(unsigned long ecode, const char *replacement)
+{
+   if (ecode == 0)
+       return replacement;
+   else
+       return SSLerrmessage(ecode);
+}
+
 /*
  * Obtain reason string for passed SSL errcode
  *

diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 507a5d329a38545cb8eea194c56ce7c4426f04d5..90e91225b4e59a173cb388e5b0af6283b6f355e1 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -190,6 +190,7 @@ static const unit_conversion time_unit_conversion_table[] =
 static const char *const map_old_guc_names[] = {
    "sort_mem", "work_mem",
    "vacuum_mem", "maintenance_work_mem",
+   "ssl_ecdh_curve", "ssl_groups",
    NULL
 };
 

diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 2c4cc8cd41b5d2b2f581ac94a62b87d09c74f783..859e6658e779edaf6befaa7d641d7403fd209a15 100644 (file)
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -4656,9 +4656,9 @@ struct config_string ConfigureNamesString[] =
    },
 
    {
-       {"ssl_ecdh_curve", PGC_SIGHUP, CONN_AUTH_SSL,
-           gettext_noop("Sets the curve to use for ECDH."),
-           NULL,
+       {"ssl_groups", PGC_SIGHUP, CONN_AUTH_SSL,
+           gettext_noop("Sets the group(s) to use for Diffie-Hellman key exchange."),
+           gettext_noop("Multiple groups can be specified using colon-separated list."),
            GUC_SUPERUSER_ONLY
        },
        &SSLECDHCurve,

diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index 667e0dc40a24f72ee7b4c02a13b7d7798c5888a6..204eab5f1a722bddc683b64befdbf25f3c814059 100644 (file)
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -112,7 +112,7 @@
 #ssl_key_file = 'server.key'
 #ssl_ciphers = 'HIGH:MEDIUM:+3DES:!aNULL'  # allowed SSL ciphers
 #ssl_prefer_server_ciphers = on
-#ssl_ecdh_curve = 'prime256v1'
+#ssl_groups = 'prime256v1'
 #ssl_min_protocol_version = 'TLSv1.2'
 #ssl_max_protocol_version = ''
 #ssl_dh_params_file = ''

diff --git a/src/test/ssl/t/001_ssltests.pl b/src/test/ssl/t/001_ssltests.pl
index 8eaf9deae798246cf4069ab273fc9d693f603eef..131460a1fea834eb62f098c73ea9b07aff3ced39 100644 (file)
--- a/src/test/ssl/t/001_ssltests.pl
+++ b/src/test/ssl/t/001_ssltests.pl
@@ -116,6 +116,18 @@ ssl_max_protocol_version=''});
 $result = $node->restart(fail_ok => 1);
 is($result, 1, 'restart succeeds with correct SSL protocol bounds');
 
+# Test parsing colon-separated groups. Resetting to a default value to clear
+# the error is fine since the call to switch_server_cert in the client side
+# tests will overwrite ssl_groups with a known set of groups.
+$node->append_conf('sslconfig.conf', qq{ssl_groups='bad:value'});
+my $log_size = -s $node->logfile;
+$result = $node->restart(fail_ok => 1);
+is($result, 0, 'restart fails with incorrect groups');
+ok($node->log_contains(qr/no SSL error reported/) == 0,
+   'error message translated');
+$node->append_conf('ssl_config.conf', qq{ssl_groups='prime256v1'});
+$result = $node->restart(fail_ok => 1);
+
 ### Run client-side tests.
 ###
 ### Test that libpq accepts/rejects the connection correctly, depending

diff --git a/src/test/ssl/t/SSL/Server.pm b/src/test/ssl/t/SSL/Server.pm
index de06f6f242f22a6b847d5b9853030b682e518163..c1b25a4ebf6f4f3067e6bf4ecd92f45142c41087 100644 (file)
--- a/src/test/ssl/t/SSL/Server.pm
+++ b/src/test/ssl/t/SSL/Server.pm
@@ -300,6 +300,9 @@ sub switch_server_cert
    ok(unlink($node->data_dir . '/sslconfig.conf'));
    $node->append_conf('sslconfig.conf', "ssl=on");
    $node->append_conf('sslconfig.conf', $backend->set_server_cert(\%params));
+   # use lists of ECDH curves for syntax testing
+   $node->append_conf('sslconfig.conf', 'ssl_groups=prime256v1:secp521r1');
+
    $node->append_conf('sslconfig.conf',
        "ssl_passphrase_command='" . $params{passphrase_cmd} . "'")
      if defined $params{passphrase_cmd};