schema: review-report-v2
target: "Let OpenSSL auto-select DH parameters by default (CommitFest 7151)"
base: 061065e28f086e3e2d7191a7785b9f218b7976d8
head: 061065e28f086e3e2d7191a7785b9f218b7976d8
verdict: ready-for-committer
blocker_counts:
  verified: 0
  unverified: 0
scope_identity: diff-1c2869e3213b098446abf57bfafbf7556a6b2fd4e474c534293ce4da57ddec08
model: glm-5.2:cloud

CommitFest 7151 Review: Let OpenSSL auto-select DH parameters by default

Patch: "Let OpenSSL auto-select DH parameters by default" by Peter Eisentraut Message-ID: 953bceb7-340d-4717-9682-0ebe65c4b57e@eisentraut.org Thread-ID: 653832 (2026-08-11) Verdict: ready-for-committer Blockers: 0 verified, 0 unverified

Patch Summary

The patch removes the hardcoded 2048-bit DH parameter blob (FILE_DH2048, RFC 3526 MODP group) from src/include/libpq/libpq-be.h, removes the load_dh_buffer() helper from src/backend/libpq/be-secure-openssl.c, and rewrites initialize_dh() so that when ssl_dh_params_file is empty it calls SSL_CTX_set_dh_auto(context, 1) instead of falling back to the compiled-in blob. The "Ephemeral DH" section of README.SSL is removed, and the ssl_dh_params_file docs are updated to mark the parameter obsolescent and cross-reference ssl_groups for TLS 1.3.

Notable behavior change: When ssl_dh_params_file is set but loading fails during reload, the code no longer falls back to built-in params — it returns false (keeping the old SSL context). The author identifies this as a bugfix.

Changed files:

• doc/src/sgml/config.sgml — ssl_dh_params_file docs updated
• src/backend/libpq/README.SSL — "Ephemeral DH" section removed
• src/backend/libpq/be-secure-openssl.c — initialize_dh() rewritten, load_dh_buffer() removed
• src/include/libpq/libpq-be.h — FILE_DH2048 macro removed

Verification Summary

SEC-1 (BLOCKER) — REFUTED: The security reviewer claimed SSL_CTX_set_dh_auto() is not available in LibreSSL, breaking the build on a documented-supported platform. This claim is false: the official LibreSSL ssl.h header (verified from the libressl/openbsd GitHub repository) defines both SSL_CTRL_SET_DH_AUTO (118) and SSL_CTX_set_dh_auto(ctx, onoff) as a preprocessor macro wrapping SSL_CTX_ctrl, identical to OpenSSL's definition. The macro is always available when <openssl/ssl.h> is included in both OpenSSL >= 1.1.0 and LibreSSL >= 3.4. No #ifdef guard or configure probe is needed, consistent with how PostgreSQL uses other macro-based OpenSSL APIs.

Repair Validation Summary

Three verified findings have concrete repair patches that were validated by apply-check, compile, and static inspection:

1. PATCH-1 (stale GUC long_desc): Update guc_parameters.dat line 2801 to replace "compiled-in default parameters" with "SSL library selects the DH parameters." Build-validated.
2. PATCH-2 (docs wording): Replace "well-known DH parameters" with "appropriate DH parameters" in config.sgml line 1690. Applies cleanly.
3. SEC-4 (stale comment): Replace misleading comment in load_dh_file() at line 1523. Build-validated.

Four findings (SEC-2, SEC-3, PATCH-3, PATCH-5) are cannot-verify with typed safety waivers — they are design choices, net improvements, or pre-existing conditions that require no code fix.

Changed-Code Coverage

Coverage requirement: Required (1 instrumentable C file changed) Measurement lane: host-disposable (fresh gcov counter epoch) Test suite: make installcheck PG_TEST_EXTRA=ssl in src/test/ssl — all 4 TAP tests passed (450 tests)

Coverage Metrics

Coverage rate: 1/11 changed executable lines (9.09%)

Covered changed line

• Line 2118: if (SSL_CTX_set_dh_auto(context, 1) != 1) — covered (count=1117). This is the patch's core new behavior: the SSL_CTX_set_dh_auto default path.

Uncovered regions

Region A (lines 2097-2112): ssl_dh_params_file-set path — coverable-now

• 8 lines uncovered: load_dh_file call, if (!dh) check, SSL_CTX_set_tmp_dh call, error path, DH_free calls
• Pre-existing test gap: no TAP test has ever set ssl_dh_params_file
• The patch restructures this path (removes load_dh_buffer fallback) but does not introduce new behavior
• Concrete test example provided: TAP test that generates DH params with openssl dhparam, sets ssl_dh_params_file, verifies SSL connections

Region B (lines 2120-2124): SSL_CTX_set_dh_auto failure path — infeasible-with-reason

• 2 lines uncovered: defensive error check for SSL_CTX_set_dh_auto failure
• SSL_CTX_set_dh_auto is a flag-setting call (SSL_CTX_ctrl with SSL_CTRL_SET_DH_AUTO) that cannot fail through external configuration
• Waived: not-instrumentable — not triggerable without OpenSSL internal corruption

Uncovered branches

9 uncovered branches on changed lines:

• Lines 2099, 2102: coverable-now (reachable by setting ssl_dh_params_file)
• Line 2102 error branch, lines 2104: requires-special-harness-or-injection-point (need OpenSSL error injection)
• Line 2118 error branch, lines 2120: infeasible-with-reason (SSL_CTX_set_dh_auto cannot fail)

Touched functions

Non-instrumentable changes

• doc/src/sgml/config.sgml — documentation only
• src/backend/libpq/README.SSL — documentation only
• src/include/libpq/libpq-be.h — header file, FILE_DH2048 macro removal (no executable code)

COV-* finding determination

No COV-* finding warranted. The uncovered lines are either a pre-existing test gap (the ssl_dh_params_file-set path was equally untested before the patch) or infeasible defensive error checks. The patch's core new behavior (SSL_CTX_set_dh_auto at line 2118) is covered.

Security Review

The security review identified one BLOCKER (SEC-1, refuted) and three MINOR/NIT findings:

• SEC-1 (BLOCKER, refuted): Claim that SSL_CTX_set_dh_auto breaks LibreSSL build — false; LibreSSL provides the macro identically to OpenSSL.
• SEC-2 (MINOR, cannot-verify): No DH strength floor — design choice consistent with OpenSSL recommendations; DH group strength scales with certificate key size.
• SEC-3 (MINOR, cannot-verify): Reload behavior change — net security improvement; consistent with all other SSL init failure handling; prevents silent downgrade.
• SEC-4 (NIT, verified): Stale comment in load_dh_file — repair patch provided.

No memory-safety defects found in the new DH ownership/free paths. DH_free is correctly called on both success and failure paths in the ssl_dh_params_file-set branch. The SSL_CTX_set_dh_auto path allocates no DH* object.

Test Gap Analysis

No existing tests exercise ssl_dh_params_file or DH parameter loading in any form. The 450 SSL TAP tests (001_ssltests.pl, 002_scram.pl, 003_sslinfo.pl, 004_sni.pl) do not set ssl_dh_params_file. This is a pre-existing gap, not introduced by the patch. The new SSL_CTX_set_dh_auto default path is exercised by every SSL test connection (count=1117).

Thread Reconciliation

Status: no-thread-found

The original patch email (953bceb7-340d-4717-9682-0ebe65c4b57e@eisentraut.org, Thread-ID 653832, 2026-08-11) was found in the imported mail archive. No replies or review messages were found in the thread. All findings are dispositioned as new.

Provenance

Deterministic Verdict

Verdict: ready-for-committer

• SEC-1 (the only BLOCKER) is refuted — SSL_CTX_set_dh_auto is available in both OpenSSL and LibreSSL
• No surviving BLOCKER findings
• No correctness/security MAJOR findings
• No live thread objections (no-thread-found)
• Findings are all MINOR/NIT: 3 verified with concrete repair patches, 4 cannot-verify with typed waivers, 1 refuted
• The three actionable repairs (PATCH-1, PATCH-2, SEC-4) are minor docs/comment fixes that can be folded into the patch or applied as follow-up

Recommended actions before commit:

1. Update guc_parameters.dat long_desc for ssl_dh_params_file (PATCH-1)
2. Change "well-known" to "appropriate" in config.sgml (PATCH-2, optional)
3. Update stale comment in load_dh_file() (SEC-4)