Skip to content

Load the system CA bundle for verified clients#5

Closed
alanhoff wants to merge 2 commits intoendel:mainfrom
alanhoff:fix/client-default-ca-bundle
Closed

Load the system CA bundle for verified clients#5
alanhoff wants to merge 2 commits intoendel:mainfrom
alanhoff:fix/client-default-ca-bundle

Conversation

@alanhoff
Copy link
Copy Markdown

@alanhoff alanhoff commented Mar 17, 2026

Summary

  • load the system CA bundle automatically when ClientConfig keeps certificate verification enabled and no custom CA path is provided
  • keep ownership of client-created CA bundles in the event-loop client lifecycle so the verified path does not leak allocations
  • document the verified-client default and cover the builder behavior with unit tests

Testing

  • zig build
  • zig build test
  • zig build fuzz

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>
Copilot AI review requested due to automatic review settings March 17, 2026 19:37
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 updates the event-loop client TLS configuration path so that verified clients automatically use the system CA root store when no custom CA bundle is provided, while ensuring any internally-created CA bundle is owned and freed with the client lifecycle.

Changes:

  • Add a buildClientTlsConfig helper that loads the system CA bundle when skip_cert_verify=false and no ca_cert_path is provided.
  • Track and free internally-owned CA bundles in event_loop.Client to avoid allocation leaks.
  • Document the new verified-client behavior and add unit tests, wiring event_loop.zig into the test aggregator.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
src/event_loop.zig Introduces TLS config builder with system CA auto-load, adds CA bundle ownership tracking, and adds unit tests.
src/test_all.zig Imports event_loop.zig so its tests run in the aggregated test suite.
SPEC/RFC5280_CHAIN_VALIDATION.md Updates spec notes to reflect new event_loop.ClientConfig behavior for system roots.
README.md Documents that verified clients use the system root store when ca_cert_path is unset.
Comments suppressed due to low confidence (2)

SPEC/RFC5280_CHAIN_VALIDATION.md:46

  • skip_cert_verify is documented here as defaulting to true, but event_loop.ClientConfig.skip_cert_verify currently defaults to false (see src/event_loop.zig:860) and the README table reflects false. Please clarify which config this caveat refers to (e.g., tls13.TlsConfig vs event_loop.ClientConfig) and update the default accordingly to avoid misleading users about verified-client behavior.
### Caveats

- `skip_cert_verify` defaults to `true` for backward compatibility
- V1 certificates (no extensions) are accepted as CAs when no basicConstraints is present — this matches common practice but is less strict than RFC 5280's recommendation

src/event_loop.zig:1118

  • In Client.init, connection.connect(...) is called before alloc.create(connection.Connection). If alloc.create fails, the already-initialized conn value (which allocates internally) is dropped without deinit(), leaking allocations on this error path. Consider allocating conn_ptr first (with an init flag) or otherwise ensuring conn.deinit() is called when later allocations fail.
            const conn = try connection.connect(
                alloc,
                config.server_name,
                conn_config,
                built_tls_config.tls_config,
                null,
            );
            // Heap-allocate so pointers remain stable
            const conn_ptr = try alloc.create(connection.Connection);
            errdefer {
                conn_ptr.deinit();
                alloc.destroy(conn_ptr);
            }

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

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>
@alanhoff
Copy link
Copy Markdown
Author

Addressed the follow-up review points on the latest head:

  • clarified the skip_cert_verify default caveat so it distinguishes tls13.TlsConfig from event_loop.ClientConfig
  • added an errdefer conn.deinit() guard in Client.init so the temporary connection is released if a later allocation fails

Local validation rerun on the updated branch:

  • zig build
  • zig build test
  • zig build fuzz

@alanhoff
Copy link
Copy Markdown
Author

Superseded by #6, which uses the semantic branch/title convention and includes the security impact details in the PR description.

@alanhoff alanhoff closed this Mar 17, 2026
@alanhoff alanhoff deleted the fix/client-default-ca-bundle branch March 17, 2026 19:45
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