MIME-Version: 1.0
References: <23C40DD6-1C47-46FC-A746-8A1D8530AD3E@amazon.com>
In-Reply-To: <23C40DD6-1C47-46FC-A746-8A1D8530AD3E@amazon.com>
From: Ewan Young <kdbase.hack@gmail.com>
Date: Wed, 3 Jun 2026 14:32:12 +0800
Message-ID: 
 <CAON2xHNVaUqd57cm9-roMmFfXVAWz7+qWSNYm=1JyQpEi4zVWw@mail.gmail.com>
Subject: Re: [PATCH] Clarify that ssl_groups is for any key exchange groups
To: "Si, Evan" <evsi@amazon.com>
Cc: "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAON2xHNVaUqd57cm9-roMmFfXVAWz7%2BqWSNYm%3D1JyQpEi4zVWw%40mail.gmail.com>
Precedence: bulk

On Tue, Jun 2, 2026 at 4:05=E2=80=AFAM Si, Evan <evsi@amazon.com> 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_lis=
t. 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 t=
hese 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 n=
amed 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-6059367=
3A91B%40yesql.se
> [2] https://www.postgresql.org/docs/18/runtime-config-connection.html#GUC=
-SSL-GROUPS
>
>