public inbox for [email protected]help / color / mirror / Atom feed
[PATCH] Clarify that ssl_groups is for any key exchange groups 6+ messages / 4 participants [nested] [flat]
* [PATCH] Clarify that ssl_groups is for any key exchange groups @ 2026-06-01 20:05 Si, Evan <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Si, Evan @ 2026-06-01 20:05 UTC (permalink / raw) To: [email protected] <[email protected]> Hi, The ssl_groups parameter introduced in Postgres 18 decided to use a short_desc: "Sets the group(s) to use for Diffie-Hellman key exchange" [1]. The documentation still references curves [2]. However, this parameter is just passed through to SSL_CTX_set1_groups_list. This means the parameter readily accepts values like a pure `MLKEM768`, assuming the crypto lib supports it, which is true since OpenSSL 3.5. Yet these Shor-safe groups are not DH key exchange. I think it makes sense to modify the documentation to a more generic one to reflect the capabilities of ssl_groups more accurately, e.g. "Sets the named groups to use for TLS key exchange." A more concrete patch suggestion is attached. Evan [1] https://www.postgresql.org/message-id/D44791DD-0CD9-48A7-9471-60593673A91B%40yesql.se [2] https://www.postgresql.org/docs/18/runtime-config-connection.html#GUC-SSL-GROUPS Attachments: [application/octet-stream] 0001-Clarify-that-ssl_groups-is-for-any-key-exchange-grou.patch (4.4K, 2-0001-Clarify-that-ssl_groups-is-for-any-key-exchange-grou.patch) download | inline diff: From b75f295de12246794e769509fde43e3f6d89052c Mon Sep 17 00:00:00 2001 From: Evan Si <[email protected]> Date: Mon, 1 Jun 2026 18:13:35 +0000 Subject: [PATCH] Clarify that ssl_groups is for any key exchange groups The current wording seems to suggest that the parameter is only meant for DH. This introduces minor wording tweaks across comments, short_desc, and the docs to reflect that it accepts any group. --- doc/src/sgml/config.sgml | 12 ++++++------ src/backend/libpq/be-secure-openssl.c | 12 ++++++------ src/backend/utils/misc/guc_parameters.dat | 2 +- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index cebae4b6d1b..8a94ecd6221 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1573,11 +1573,11 @@ include_dir 'conf.d' </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 + Specifies the named group to use for <acronym>TLS</acronym> key + exchange. It needs to be supported by all clients that + connect. Multiple groups can be specified by using a colon-separated + list. It does not need to match the key type used by the server + certificate. This parameter can only be set in the <filename>postgresql.conf</filename> file or on the server command line. The default is <literal>X25519:prime256v1</literal>. </para> @@ -1592,7 +1592,7 @@ include_dir 'conf.d' </note> <para> - <productname>OpenSSL</productname> names for the most common curves + <productname>OpenSSL</productname> names for the most common groups are: <literal>prime256v1</literal> (NIST P-256), <literal>secp384r1</literal> (NIST P-384), diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 7890e6c2de2..dcc61cb9b20 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -95,7 +95,7 @@ static int alpn_cb(SSL *ssl, unsigned int inlen, void *userdata); static bool initialize_dh(SSL_CTX *context, bool isServerStart); -static bool initialize_ecdh(SSL_CTX *context, bool isServerStart); +static bool initialize_groups(SSL_CTX *context, bool isServerStart); static const char *SSLerrmessageExt(unsigned long ecode, const char *replacement); static const char *SSLerrmessage(unsigned long ecode); static bool init_host_context(HostsLine *host, bool isServerStart); @@ -516,7 +516,7 @@ be_tls_init(bool isServerStart) /* set up ephemeral DH and ECDH keys */ if (!initialize_dh(context, isServerStart)) goto error; - if (!initialize_ecdh(context, isServerStart)) + if (!initialize_groups(context, isServerStart)) goto error; /* set up the allowed cipher list for TLSv1.2 and below */ @@ -2106,12 +2106,12 @@ initialize_dh(SSL_CTX *context, bool isServerStart) } /* - * Set ECDH parameters for generating ephemeral Elliptic Curve DH - * keys. This is much simpler than the DH parameters, as we just - * need to provide the name of the curve to OpenSSL. + * Set the group(s) to use for TLS key exchange. This is much simpler + * than the static DH parameters, as we just need to provide the + * colon-separated list of group names to OpenSSL. */ static bool -initialize_ecdh(SSL_CTX *context, bool isServerStart) +initialize_groups(SSL_CTX *context, bool isServerStart) { if (SSL_CTX_set1_groups_list(context, SSLECDHCurve) != 1) { diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.dat index afaa058b046..fd45e7d76ec 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -2790,7 +2790,7 @@ }, { name => 'ssl_groups', type => 'string', context => 'PGC_SIGHUP', group => 'CONN_AUTH_SSL', - short_desc => 'Sets the group(s) to use for Diffie-Hellman key exchange.', + short_desc => 'Sets the named groups to use for TLS key exchange.', long_desc => 'Multiple groups can be specified using a colon-separated list.', flags => 'GUC_SUPERUSER_ONLY', variable => 'SSLECDHCurve', -- 2.47.3 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Clarify that ssl_groups is for any key exchange groups @ 2026-06-03 06:32 Ewan Young <[email protected]> parent: Si, Evan <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Ewan Young @ 2026-06-03 06:32 UTC (permalink / raw) To: Si, Evan <[email protected]>; +Cc: [email protected] <[email protected]> On Tue, Jun 2, 2026 at 4:05 AM Si, Evan <[email protected]> wrote: > > Hi, > > The ssl_groups parameter introduced in Postgres 18 decided to use a short_desc: "Sets the group(s) to use for Diffie-Hellman key exchange" [1]. The documentation still references curves [2]. > > However, this parameter is just passed through to SSL_CTX_set1_groups_list. This means the parameter readily accepts values like a pure `MLKEM768`, assuming the crypto lib supports it, which is true since OpenSSL 3.5. Yet these Shor-safe groups are not DH key exchange. > > I think it makes sense to modify the documentation to a more generic one to reflect the capabilities of ssl_groups more accurately, e.g. "Sets the named groups to use for TLS key exchange." > > A more concrete patch suggestion is attached. > > Evan Hi, +1 for the idea. (I'm fairly new here, so please take my comments with a grain of salt.) I tried the patch on HEAD: it applies cleanly, and the new short_desc shows up correctly in postgres --describe-config. While reading it I noticed two small things: 1. The comment just above the renamed call in be_tls_init() still says "set up ephemeral DH and ECDH keys". Maybe it should be updated to match? 2. The SSLECDHCurve variable (and its "GUC variable for default ECDH curve" comment in be-secure.c) still uses the old naming. I wasn't sure if that was left out intentionally to keep the patch small -- if not, would it make sense to rename it too, for consistency with the initialize_groups() rename? Regards, Ewan > > [1] https://www.postgresql.org/message-id/D44791DD-0CD9-48A7-9471-60593673A91B%40yesql.se > [2] https://www.postgresql.org/docs/18/runtime-config-connection.html#GUC-SSL-GROUPS > > ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Clarify that ssl_groups is for any key exchange groups @ 2026-06-03 17:29 Si, Evan <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Si, Evan @ 2026-06-03 17:29 UTC (permalink / raw) To: Ewan Young <[email protected]>; +Cc: [email protected] <[email protected]> On 6/2/26, 11:32 PM, "Ewan Young" <[email protected] <mailto:[email protected]>> wrote: > > +1 for the idea. (I'm fairly new here, so please take my comments with > a grain of salt.) Thanks for the review! > 1. The comment just above the renamed call in be_tls_init() still > says "set up ephemeral DH and ECDH keys". Maybe it should be > updated to match? Right, that makes sense. I did a larger grep and updated comments where I found stale references to curves and (EC)DH. > 2. The SSLECDHCurve variable (and its "GUC variable for default ECDH > curve" comment in be-secure.c) still uses the old naming. I wasn't > sure if that was left out intentionally to keep the patch small -- > if not, would it make sense to rename it too, for consistency with > the initialize_groups() rename? This also seems reasonable. I didn't find usage of this extern outside of Postgres itself in the wild from a brief search. Attached a revision. Evan Attachments: [application/octet-stream] v2-0001-Clarify-that-ssl_groups-is-for-any-key-exchange-g.patch (6.7K, 2-v2-0001-Clarify-that-ssl_groups-is-for-any-key-exchange-g.patch) download | inline diff: From 090fca6246c166170d6b5ab819a4680712745124 Mon Sep 17 00:00:00 2001 From: Evan Si <[email protected]> Date: Mon, 1 Jun 2026 18:13:35 +0000 Subject: [PATCH v2] Clarify that ssl_groups is for any key exchange groups The current wording seems to suggest that the parameter is only meant for DH. This introduces minor wording tweaks across comments, short_desc, and the docs to reflect that it accepts any group. --- doc/src/sgml/config.sgml | 12 ++++++------ src/backend/libpq/be-secure-openssl.c | 16 ++++++++-------- src/backend/libpq/be-secure.c | 4 ++-- src/backend/utils/misc/guc_parameters.dat | 4 ++-- src/include/libpq/libpq.h | 2 +- src/test/ssl/t/SSL/Server.pm | 2 +- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index cebae4b6d1b..8a94ecd6221 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -1573,11 +1573,11 @@ include_dir 'conf.d' </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 + Specifies the named group to use for <acronym>TLS</acronym> key + exchange. It needs to be supported by all clients that + connect. Multiple groups can be specified by using a colon-separated + list. It does not need to match the key type used by the server + certificate. This parameter can only be set in the <filename>postgresql.conf</filename> file or on the server command line. The default is <literal>X25519:prime256v1</literal>. </para> @@ -1592,7 +1592,7 @@ include_dir 'conf.d' </note> <para> - <productname>OpenSSL</productname> names for the most common curves + <productname>OpenSSL</productname> names for the most common groups are: <literal>prime256v1</literal> (NIST P-256), <literal>secp384r1</literal> (NIST P-384), diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c index 7890e6c2de2..6172726bfbc 100644 --- a/src/backend/libpq/be-secure-openssl.c +++ b/src/backend/libpq/be-secure-openssl.c @@ -95,7 +95,7 @@ static int alpn_cb(SSL *ssl, unsigned int inlen, void *userdata); static bool initialize_dh(SSL_CTX *context, bool isServerStart); -static bool initialize_ecdh(SSL_CTX *context, bool isServerStart); +static bool initialize_groups(SSL_CTX *context, bool isServerStart); static const char *SSLerrmessageExt(unsigned long ecode, const char *replacement); static const char *SSLerrmessage(unsigned long ecode); static bool init_host_context(HostsLine *host, bool isServerStart); @@ -513,10 +513,10 @@ be_tls_init(bool isServerStart) SSL_CTX_set_options(context, SSL_OP_NO_CLIENT_RENEGOTIATION); #endif - /* set up ephemeral DH and ECDH keys */ + /* set up DH parameters and TLS named groups */ if (!initialize_dh(context, isServerStart)) goto error; - if (!initialize_ecdh(context, isServerStart)) + if (!initialize_groups(context, isServerStart)) goto error; /* set up the allowed cipher list for TLSv1.2 and below */ @@ -2106,14 +2106,14 @@ initialize_dh(SSL_CTX *context, bool isServerStart) } /* - * Set ECDH parameters for generating ephemeral Elliptic Curve DH - * keys. This is much simpler than the DH parameters, as we just - * need to provide the name of the curve to OpenSSL. + * Set the group(s) to use for TLS key exchange. This is much simpler + * than the static DH parameters, as we just need to provide the + * colon-separated list of group names to OpenSSL. */ static bool -initialize_ecdh(SSL_CTX *context, bool isServerStart) +initialize_groups(SSL_CTX *context, bool isServerStart) { - if (SSL_CTX_set1_groups_list(context, SSLECDHCurve) != 1) + if (SSL_CTX_set1_groups_list(context, SSLNamedGroups) != 1) { /* * OpenSSL 3.3.0 introduced proper error messages for group parsing diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c index 86ceea72e64..d69106e6545 100644 --- a/src/backend/libpq/be-secure.c +++ b/src/backend/libpq/be-secure.c @@ -52,8 +52,8 @@ bool ssl_loaded_verify_locations = false; char *SSLCipherSuites = NULL; char *SSLCipherList = NULL; -/* GUC variable for default ECDH curve. */ -char *SSLECDHCurve; +/* GUC variable for the named groups to use for TLS key exchange. */ +char *SSLNamedGroups; /* GUC variable: if false, prefer client ciphers */ bool SSLPreferServerCiphers; diff --git a/src/backend/utils/misc/guc_parameters.dat b/src/backend/utils/misc/guc_parameters.dat index afaa058b046..e5e01ce1cd6 100644 --- a/src/backend/utils/misc/guc_parameters.dat +++ b/src/backend/utils/misc/guc_parameters.dat @@ -2790,10 +2790,10 @@ }, { name => 'ssl_groups', type => 'string', context => 'PGC_SIGHUP', group => 'CONN_AUTH_SSL', - short_desc => 'Sets the group(s) to use for Diffie-Hellman key exchange.', + short_desc => 'Sets the named groups to use for TLS key exchange.', long_desc => 'Multiple groups can be specified using a colon-separated list.', flags => 'GUC_SUPERUSER_ONLY', - variable => 'SSLECDHCurve', + variable => 'SSLNamedGroups', boot_val => 'DEFAULT_SSL_GROUPS', }, diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h index d15073a0a93..906ac742b83 100644 --- a/src/include/libpq/libpq.h +++ b/src/include/libpq/libpq.h @@ -116,7 +116,7 @@ extern PGDLLIMPORT char *ssl_dh_params_file; extern PGDLLIMPORT bool ssl_sni; extern PGDLLIMPORT char *SSLCipherSuites; extern PGDLLIMPORT char *SSLCipherList; -extern PGDLLIMPORT char *SSLECDHCurve; +extern PGDLLIMPORT char *SSLNamedGroups; extern PGDLLIMPORT bool SSLPreferServerCiphers; #ifdef USE_SSL extern PGDLLIMPORT bool ssl_loaded_verify_locations; diff --git a/src/test/ssl/t/SSL/Server.pm b/src/test/ssl/t/SSL/Server.pm index 4400a432f42..624d36ff7ea 100644 --- a/src/test/ssl/t/SSL/Server.pm +++ b/src/test/ssl/t/SSL/Server.pm @@ -322,7 +322,7 @@ 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 and cipher suites for syntax testing + # use lists of TLS groups and cipher suites for syntax testing $node->append_conf('sslconfig.conf', 'ssl_groups=prime256v1:secp521r1'); $node->append_conf('sslconfig.conf', 'ssl_tls13_ciphers=TLS_AES_256_GCM_SHA384:TLS_AES_128_GCM_SHA256'); -- 2.47.3 ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Clarify that ssl_groups is for any key exchange groups @ 2026-06-04 03:00 Ewan Young <[email protected]> parent: Si, Evan <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Ewan Young @ 2026-06-04 03:00 UTC (permalink / raw) To: Si, Evan <[email protected]>; +Cc: [email protected] <[email protected]> On Thu, Jun 4, 2026 at 1:29 AM Si, Evan <[email protected]> wrote: > > On 6/2/26, 11:32 PM, "Ewan Young" <[email protected] <mailto:[email protected]>> wrote: > > > > +1 for the idea. (I'm fairly new here, so please take my comments with > > a grain of salt.) > > Thanks for the review! > > > 1. The comment just above the renamed call in be_tls_init() still > > says "set up ephemeral DH and ECDH keys". Maybe it should be > > updated to match? > > Right, that makes sense. I did a larger grep and updated comments where I found stale references to curves and (EC)DH. Thanks! I re-did the grep on v2 and found no remaining stale references. > > > 2. The SSLECDHCurve variable (and its "GUC variable for default ECDH > > curve" comment in be-secure.c) still uses the old naming. I wasn't > > sure if that was left out intentionally to keep the patch small -- > > if not, would it make sense to rename it too, for consistency with > > the initialize_groups() rename? > > This also seems reasonable. I didn't find usage of this extern outside of Postgres itself in the wild from a brief search. > > Attached a revision. > > Evan > I tested v2 on top of current master: - applies cleanly, builds without warnings (--with-openssl) - src/test/ssl TAP suite passes v2 looks good to me, and I have nothing further. Best regards, Ewan Young ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Clarify that ssl_groups is for any key exchange groups @ 2026-06-05 20:22 Daniel Gustafsson <[email protected]> parent: Ewan Young <[email protected]> 0 siblings, 1 reply; 6+ messages in thread From: Daniel Gustafsson @ 2026-06-05 20:22 UTC (permalink / raw) To: Ewan Young <[email protected]>; +Cc: Si, Evan <[email protected]>; [email protected] <[email protected]> > On 4 Jun 2026, at 05:00, Ewan Young <[email protected]> wrote: > v2 looks good to me, and I have nothing further. I have applied the docs portion of this patch to master and REL_18_STABLE, but given where we are in the release cycle I don't think the renames qualify as fixes during a freeze. Please feel free to register this patch in the upcoming commitfest to get the remaining portion into v20. You can add me as a reviewer on that entry to make sure it stays on my radar. -- Daniel Gustafsson ^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Clarify that ssl_groups is for any key exchange groups @ 2026-06-06 02:45 Evan <[email protected]> parent: Daniel Gustafsson <[email protected]> 0 siblings, 0 replies; 6+ messages in thread From: Evan @ 2026-06-06 02:45 UTC (permalink / raw) To: Daniel Gustafsson <[email protected]>; +Cc: Si, Evan <[email protected]>; [email protected] <[email protected]>; Ewan Young <[email protected]> On 6/5/2026 1:22 PM, Daniel Gustafsson wrote: >> On 4 Jun 2026, at 05:00, Ewan Young <[email protected]> wrote: > >> v2 looks good to me, and I have nothing further. > > I have applied the docs portion of this patch to master and REL_18_STABLE, but > given where we are in the release cycle I don't think the renames qualify as > fixes during a freeze. Please feel free to register this patch in the upcoming > commitfest to get the remaining portion into v20. You can add me as a reviewer > on that entry to make sure it stays on my radar. > Thanks! It looks like you found the commitfest entry for it right after the email. Otherwise for reference: https://commitfest.postgresql.org/patch/6843/ (Same person, different email.) ^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2026-06-06 02:45 UTC | newest] Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2026-06-01 20:05 [PATCH] Clarify that ssl_groups is for any key exchange groups Si, Evan <[email protected]> 2026-06-03 06:32 ` Ewan Young <[email protected]> 2026-06-03 17:29 Re: [PATCH] Clarify that ssl_groups is for any key exchange groups Si, Evan <[email protected]> 2026-06-04 03:00 ` Ewan Young <[email protected]> 2026-06-05 20:22 ` Daniel Gustafsson <[email protected]> 2026-06-06 02:45 ` Evan <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox