Skip to content

fix(tls): load system trust anchors for verified clients#6

Open
alanhoff wants to merge 4 commits intoendel:mainfrom
alanhoff:fix/tls-client-trust-anchor-defaults
Open

fix(tls): load system trust anchors for verified clients#6
alanhoff wants to merge 4 commits intoendel:mainfrom
alanhoff:fix/tls-client-trust-anchor-defaults

Conversation

@alanhoff
Copy link
Copy Markdown

Summary

  • fix default verified-client TLS behavior by loading system trust anchors when no custom CA bundle is configured
  • keep ownership of internally-created CA bundles in the event-loop client lifecycle and release temporary client connections on init failures
  • document the default verification behavior and add unit coverage

Vulnerability

ClientConfig.skip_cert_verify already defaults to false, but before this patch Client.init left ca_bundle as null unless callers set ca_cert_path. That meant hostname verification could run without any trusted roots, so a self-signed or attacker-controlled certificate for the requested name could still be accepted.

Examples

  • a client connecting to api.example.com from a hostile Wi-Fi network with the default ClientConfig could accept an attacker certificate that presents the same hostname
  • an app using event_loop.Client(...).init(.{ .server_name = "localhost" }) against a local proxy or test harness would believe verification was enabled while trust-anchor validation was silently skipped unless ca_cert_path was set manually

Testing

  • zig build
  • zig build test
  • zig build fuzz

alanhoff and others added 2 commits March 17, 2026 16:37
Use the Zig system trust store by default when client certificate verification is enabled and no custom CA file is supplied. Keep ownership of the allocated CA bundle in the client lifecycle and cover the builder behavior with unit tests.

Co-authored-by: Codex <noreply@openai.com>
Clarify the TLS verification defaults in the RFC 5280 note and release the temporary client connection on the init error path before ownership is transferred.

Co-authored-by: Codex <noreply@openai.com>
Copilot AI review requested due to automatic review settings March 17, 2026 19:45
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes event_loop.Client’s default TLS verification behavior by ensuring a trust store is present when certificate verification is enabled, addressing a scenario where hostname verification could proceed without any trust anchors.

Changes:

  • Add buildClientTlsConfig to auto-load the system CA bundle when skip_cert_verify=false and no ca_cert_path is provided.
  • Track ownership/lifecycle of internally-created CA bundles in event_loop.Client and add init-time cleanup on failures.
  • Update documentation and add unit tests covering the default CA-bundle behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/test_all.zig Imports event_loop.zig so its new unit tests run under the main test aggregator.
src/event_loop.zig Implements system trust-anchor loading for verified clients, adds CA-bundle ownership tracking, and adds unit tests.
SPEC/RFC5280_CHAIN_VALIDATION.md Documents default event_loop.ClientConfig trust-anchor behavior and updates caveats.
README.md Updates ClientConfig.ca_cert_path docs to clarify system-root fallback behavior.
Comments suppressed due to low confidence (1)

src/event_loop.zig:1117

  • errdefer conn.deinit(); becomes unsafe once conn is copied into conn_ptr.* later (line 1120): if any subsequent init step fails (address parse, socket creation, xev init, etc.), both conn_ptr.deinit() (via its errdefer) and this conn.deinit() errdefer will run, causing a double-deinit/double-free of the same connection internals. Restructure to move the connection directly into the heap allocation (e.g., assign conn_ptr.* = try connection.connect(...) and only errdefer on conn_ptr), or otherwise ensure the conn errdefer is canceled once ownership transfers.
            errdefer conn.deinit();
            // Heap-allocate so pointers remain stable
            const conn_ptr = try alloc.create(connection.Connection);
            errdefer {
                conn_ptr.deinit();

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

alanhoff and others added 2 commits March 17, 2026 16:51
Move the connection directly into the heap allocation and guard cleanup with a single initialization flag so the client init error path cannot double-free connection internals.

Co-authored-by: Codex <noreply@openai.com>
Document that callers constructing tls13.TlsConfig directly must still supply ca_bundle for trust-anchor verification, while event_loop.ClientConfig now auto-populates the verified default path.

Co-authored-by: Codex <noreply@openai.com>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes the default “verified client” TLS behavior in the event-loop client by ensuring a trust anchor set is present when certificate verification is enabled, aligning runtime behavior with ClientConfig.skip_cert_verify = false and the documented security model.

Changes:

  • Automatically load the system root CA bundle when skip_cert_verify=false and no ca_cert_path is provided (unless a full custom tls_config override is supplied).
  • Correct ownership/lifetime handling for internally created CA bundles and ensure partially-initialized connections are cleaned up safely on init failure paths.
  • Add documentation and unit tests covering the default verification/trust-anchor behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
src/test_all.zig Ensures event_loop.zig tests are included in the overall test suite.
src/event_loop.zig Builds a safer default client TLS config by loading system roots, tracks CA bundle ownership, and hardens init cleanup.
SPEC/RFC5280_CHAIN_VALIDATION.md Documents the new default behavior and clarifies caveats for direct tls13.TlsConfig usage.
README.md Updates ClientConfig.ca_cert_path documentation to note the system root store default for verified clients.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +895 to +906
if (!config.skip_cert_verify) {
const bundle_ptr = try alloc.create(Certificate.Bundle);
errdefer alloc.destroy(bundle_ptr);

if (config.ca_cert_path) |ca_path| {
bundle_ptr.* = .{};
errdefer bundle_ptr.deinit(alloc);
try bundle_ptr.addCertsFromFilePath(alloc, std.fs.cwd(), ca_path);
} else {
bundle_ptr.* = try tls13.loadSystemCaBundle(alloc);
errdefer bundle_ptr.deinit(alloc);
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants