Re: [PATCH] Clarify that ssl_groups is for any key exchange groups

On Thu, Jun 4, 2026 at 1:29 AM Si, Evan <evsi@amazon.com> wrote:

On 6/2/26, 11:32 PM, "Ewan Young" <kdbase.hack@gmail.com <mailto:kdbase.hack@gmail.com>> 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

On 4 Jun 2026, at 05:00, Ewan Young <kdbase.hack@gmail.com> 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

On 6/5/2026 1:22 PM, Daniel Gustafsson wrote:

On 4 Jun 2026, at 05:00, Ewan Young <kdbase.hack@gmail.com> 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.)

Hi Evan,

Thank you for the patch. I was reviewing this as part of the commitfest for
v20.

I agree with the premise, and it's easy to demonstrate. With OpenSSL 3.6.2,
"openssl list -tls-groups" reports MLKEM512, MLKEM768 and MLKEM1024, none
of which are Diffie-Hellman so the old short_desc is simply inaccurate for
a growing set of valid values.

v2 patch no longer applies to master. The config.sgml change is already
upstream (55136e378c6 on master, backpatched as a5112c9b62d on
REL_18_STABLE branch), and the be-secure-openssl.c prototype change has
drifted because init_host_context() gained a hasWarned argument in
9e526a14d7c. "git apply --3way" absorbs both and leaves exactly the
intended +14/-14 across five files, so a straight rebase with the docs
change dropped is all that's needed.

On the remaining rename:
- SSLNamedGroups is consistent with existing style; we already have
SSLCipherList for ssl_ciphers and SSLCipherSuites for ssl_tls13_ciphers, so
mirroring the GUC name exactly isn't the convention.
- Keeping "ssl_ecdh_curve" in map_old_guc_names() is correct. A grep of the
patched tree leaves only that entry and the "versions before 18.0" note in
config.sgml, which are both deliberate.

I built the rebased patch with --with-openssl (OpenSSL 3.6.2) with no
warnings, and "postgres --describe-config" shows the new description.

One minor point in the already-committed docs, "Specifies the named group
to use for TLS key exchange" is singular while a following sentence says
multiple groups can be specified.

Overall the patch looks in a good shape.

Regards,
Surya Poondla

On 8/27/26 12:23 PM, surya poondla wrote:

Thank you for the patch. I was reviewing this as part of the commitfest for
v20.

Thanks for the review!> v2 patch no longer applies to master.
Apologies, forgot to rebase the patch after it was partially committed.
Nonetheless the cfbot seems to have capably rebased the patch already.

One minor point in the already-committed docs, "Specifies the named group
to use for TLS key exchange" is singular while a following sentence says
multiple groups can be specified.

Makes sense. Attached tweaks to "group(s)" across the board.

Evan

Hi Evan,

Thank you for the v3 patch, it applies cleanly to the current master.

The generated GUC table picks up SSLNamedGroups, and no references to
SSLECDHCurve remain anywhere in src/ or doc/.

The patch looks good.

Regards,
Surya Poondla