Skip to content

25.3.8-fips: add TLSv1.3 support to disableProtocols configuration#1569

Open
mkmkme wants to merge 19 commits intoreleases/25.3.8-fipsfrom
mkmkme/fips/client-disable-tlsv1_3
Open

25.3.8-fips: add TLSv1.3 support to disableProtocols configuration#1569
mkmkme wants to merge 19 commits intoreleases/25.3.8-fipsfrom
mkmkme/fips/client-disable-tlsv1_3

Conversation

@mkmkme
Copy link
Collaborator

@mkmkme mkmkme commented Mar 23, 2026 •
edited
Loading

The disableProtocols mechanism only handled sslv2 through tlsv1_2. Specifying "tlsv1_3" in the config was silently ignored, so TLS 1.3 connections could not be disabled. This matters for FIPS testing where we need to verify that disabling all protocols actually prevents all connections.

Add PROTO_TLSV1_3 to the Protocols enum, handle SSL_OP_NO_TLSv1_3 in Context::disableProtocols(), and parse "tlsv1_3" in all four config readers (SSLManager, TLSHandler, PostgreSQLHandler, KeeperServer).

Changelog category (leave one):

  • Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

add TLSv1.3 support to disableProtocols configuration

Documentation entry for user-facing changes

...

CI/CD Options

Exclude tests:

  • Fast test
  • Integration Tests
  • Stateless tests
  • Stateful tests
  • Performance tests
  • All with ASAN
  • All with TSAN
  • All with MSAN
  • All with UBSAN
  • All with Coverage
  • All with Aarch64
  • All Regression
  • Disable CI Cache

Regression jobs to run:

  • Fast suites (mostly <1h)
  • Aggregate Functions (2h)
  • Alter (1.5h)
  • Benchmark (30m)
  • ClickHouse Keeper (1h)
  • Iceberg (2h)
  • LDAP (1h)
  • Parquet (1.5h)
  • RBAC (1.5h)
  • SSL Server (1h)
  • S3 (2h)
  • S3 Export (2h)
  • Swarms (30m)
  • Tiered Storage (2h)

DimensionWieldr and others added 18 commits March 10, 2026 17:08
@DimensionWieldr
ssl-shim non-split pass, split-handshake fails
725cfd7 
Made-with: Cursor
Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
Integrated CH SSL tests!
f4233e2 
Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
Deduplicate glibc_compat.c and add gtest include path for ssl-shim/ha…
6396426 
…ndshaker

Move the identical glibc_compat.c files from ssl-shim/ and ssl-handshaker/
into a shared programs/ssl-common/ directory. Also add the gtest include
path required by test_util.cc to both CMakeLists.

Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
Remove unnecessary gtest include path from ssl-shim/handshaker
4e4f1cf 
The FIPS 2.0.0 shim sources do not include any gtest headers,
so this include path is not needed.

Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
ACVP compat
5f2cf93 
Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
Fix build-guard mismatch and scope --allow-multiple-definition
2a24ee7 
Gate ssl-shim/ssl-handshaker/acvp-server declarations in main.cpp with
per-target ENABLE_CLICKHOUSE_* defines (via config_tools.h) that match
the exact CMake conditions under which targets are created, preventing
unresolved symbols when FIPS_CLICKHOUSE is set without AWSLC_SRC_DIR
or on non-Linux platforms.

Move the --allow-multiple-definition linker flag from the global
clickhouse target into each of the three library targets as an INTERFACE
property, so the flag only enters the link when those specific libraries
are actually consumed.

Signed-off-by: Julian Huang <jhuang@altinity.com>
Made-with: Cursor
@DimensionWieldr
Adopt full upstream musl posix_spawn implementation
0a7b757 
Replace the partial posix_spawn with the complete upstream musl
implementation (https://git.musl-libc.org/cgit/musl/tree/src/process/posix_spawn.c),
adapted for the glibc sysroot headers used by ClickHouse.

Key safety improvements from upstream:
- Pipe fd clobbering protection: if a file action targets the
  error-reporting pipe fd, dup it to an unoccupied fd first
- Close-on-exec set after file actions (pipe may have been moved)
- Block all signals before pipe2/clone; unblock after exec
- EPIPE-aware error reporting back to parent
- Support for POSIX_SPAWN_SETSID, SETPGROUP, RESETIDS, SETSIGDEF
- Larger stack (1024 + PATH_MAX)

Adaptations from upstream musl:
- Uses glibc sysroot field names (__ss/__sd vs __mask/__def)
- Keeps __posix_spawnx exec-function parameter (glibc attr has no __fn)
- Omits LOCK(__abort_lock) (musl-internal, not available)
- Omits __get_handler_set (musl-internal; signals are blocked for the
  child's brief pre-exec window so parent handlers cannot fire)
- Uses clone() instead of musl-internal __clone()

Signed-off-by: Julian Huang <jhuang@altinity.com>
Made-with: Cursor
@DimensionWieldr
Revert posix_spawn to original CH implementation; Create posix_spawn_…
0d84455 
…2, used only for ssl/acvp tests

Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
Isolate posix_spawn_2 so it's only called by AWS-LC .cc files, and no…
ee3a8be 
…t the rest of CH

Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
CMake downloads aws-lc source if AWSLC_SRC_DIR not specified
fe08c18 
Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
Actually, probably better to extend docker extraction to avoid downlo…
675c80a 
…ading source twice

Signed-off-by: Julian Huang <jhuang@altinity.com>
@DimensionWieldr
README!
82500cb 
Signed-off-by: Julian Huang <jhuang@altinity.com>
@Enmk @claude
Less invasive SSL test harness
1ab51f9 
Compile ssl-shim, ssl-handshaker, and acvp-server test harness code
inside the Docker build rather than with manual /usr/bin/c++ invocations
in CMake. A new build_test_harness.sh script does partial linking
(ld -r) with libstdc++ and then uses objcopy --prefix-symbols to
namespace all internal symbols, avoiding collisions with ClickHouse's
libc++. This eliminates --allow-multiple-definition, hardcoded x86
library paths, and the AWSLC_SRC_DIR configure-time requirement.
The three per-program CMakeLists.txt are replaced by IMPORTED library
targets in contrib/openssl-cmake/CMakeLists.txt. posix_spawn_2.c
gains aarch64 raw-syscall wrappers so both Dockerfiles work natively.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@Enmk
Fix aarch64 build: replace SYS_dup2 with SYS_dup3 in posix_spawn_2.c
9c61bc5 
aarch64 Linux does not define SYS_dup2 — the syscall was omitted from
the aarch64 kernel ABI since dup3 supersedes it. Use SYS_dup3 with
flags=0 which is semantically identical. Both call sites are already
guarded by fd != op->fd, so dup3's EINVAL-on-equal-fds is not a concern.
@Enmk
Merge pull request #1560 from Altinity/features/25.3/fips-ch-binary-e…
87c961e 
…xtended-testing

SSL/ACVP Test Integration for FIPS - second try
@Enmk
Revert "SSL/ACVP Test Integration for FIPS - second try"
527b583
@Enmk
Merge pull request #1565 from Altinity/revert-1560-features/25.3/fips…
7c8d816 
…-ch-binary-extended-testing

Revert "SSL/ACVP Test Integration for FIPS - second try"
@mkmkme
add TLSv1.3 support to disableProtocols configuration
41d23f5 
The disableProtocols mechanism only handled sslv2 through tlsv1_2.
Specifying "tlsv1_3" in the config was silently ignored, so TLS 1.3
connections could not be disabled. This matters for FIPS testing where
we need to verify that disabling all protocols actually prevents all
connections.

Add PROTO_TLSV1_3 to the Protocols enum, handle SSL_OP_NO_TLSv1_3 in
Context::disableProtocols(), and parse "tlsv1_3" in all four config
readers (SSLManager, TLSHandler, PostgreSQLHandler, KeeperServer).
@mkmkme mkmkme added fips Work related to Altinity FIPS releases fips-25.3 labels Mar 23, 2026
@mkmkme mkmkme changed the title 25.3.8-fipsadd TLSv1.3 support to disableProtocols configuration 25.3.8-fips: add TLSv1.3 support to disableProtocols configuration Mar 23, 2026
@mkmkme
Copy link
Collaborator Author

mkmkme commented Mar 23, 2026

AI audit note: This review comment was generated by AI (gpt-5.3-codex).

Audit update for PR #1569 (TLSv1.3 support for disableProtocols):

Confirmed defects:

No confirmed defects in reviewed scope.

Coverage summary:

Scope reviewed: Latest PR diff: `Context::Protocols` + `PROTO_TLSV1_3`, `Context::disableProtocols` (`SSL_OP_NO_TLSv1_3`), `SSLManager::initDefaultContext` token parsing, `TLSHandler` / `PostgreSQLHandler` / `KeeperServer` `disableProtocols` parsing, `SSLManager.h` docs (example XML + property bullet); static call graph from config init to `SSL_CTX_set_options`.
Categories failed: None.
Categories passed: Bitmask uniqueness; `#if defined(SSL_OP_NO_TLSv1_3)` guard; parser parity across four sites; header doc consistency (example vs "Valid protocol names"); brace/`#if` structure in `Context.cpp` on PR head.
Assumptions/limits: Static review of fetched `refs/pull/1569/head`; no live TLS handshake or integration test run; user-facing docs under `docs/` outside this diff were not required for parity with the prior finding.

@DimensionWieldr
Copy link
Collaborator

DimensionWieldr commented Mar 23, 2026 •
edited
Loading

Previously failing regression tests involving disabling protocols now pass. Looks like disableProtocols no longer ignores tlsv1_3.

Also ran AI Audit from my end. AI said "No confirmed defects in reviewed scope." :)

Looks good to me!

@Enmk Enmk force-pushed the releases/25.3.8-fips branch from 7c8d816 to d1e8b14 Compare March 23, 2026 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

fips Work related to Altinity FIPS releases fips-25.3

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mkmkme @DimensionWieldr @Enmk