Skip to content

feat(moq-native): add --tls-system-roots to trust custom and system roots together#1711

Merged
kixelated merged 1 commit into
mainfrom
claude/brave-heyrovsky-da1c08
Jun 13, 2026
Merged

feat(moq-native): add --tls-system-roots to trust custom and system roots together#1711
kixelated merged 1 commit into
mainfrom
claude/brave-heyrovsky-da1c08

Conversation

@kixelated

Copy link
Copy Markdown
Collaborator

Summary

The client TLS root field was an override: in tls::Client::build(), system roots loaded only when --tls-root was empty. So passing a custom root replaced the system trust store entirely, and there was no way to trust a local self-signed CA and the public CAs at the same time. That breaks the common case of dialing both a local relay (private CA) and a remote one (public CA) from the same client.

This makes client root loading additive and adds a flag to control whether the system roots come along.

What changed

  • New field system_roots: Option<bool> on tls::Client (--tls-system-roots / MOQ_CLIENT_TLS_SYSTEM_ROOTS).

  • build() is now additive: effective value is system_roots.unwrap_or(root.is_empty()). System roots (when enabled) and every --tls-root go into the same store.

    invocation result
    (nothing) system roots — unchanged
    --tls-root local.pem custom only — unchanged
    --tls-root local.pem --tls-system-roots both (the motivating case)
    --tls-system-roots=false, no root, no fingerprint/disable-verify Error::NoRoots, fails fast
  • New Error::NoRoots variant. The empty-store guard is scoped to when default WebPKI verification is actually in play; --tls-fingerprint and --tls-disable-verify swap in their own verifier, so an empty store is fine there. The error message points at exactly those escape hatches.

  • Typed as Option<bool> per the config-flag convention so the TOML→CLI merge can't clobber a TOML-set value.

Compatibility

Additive and non-breaking — every existing invocation behaves identically. Client-side moq-native, so it targets main.

Public API changes

  • moq_native::tls::Client: new field system_roots: Option<bool> (struct is #[non_exhaustive], so additive).
  • moq_native::tls::Error: new variant NoRoots (enum is #[non_exhaustive], so additive).

Neither is breaking.

Cross-package sync

  • doc/bin/relay/config.md: documented tls.system_roots in the [client] section.

Test plan

  • tls::tests::build_rejects_no_roots — system roots disabled + no root + no alternate verifier → NoRoots.
  • tls::tests::build_allows_no_roots_when_verification_overridden — empty store is fine under disable_verify / fingerprint pinning.
  • config::tests::cli_does_not_clobber_toml_system_roots — TOML client.tls.system_roots = true survives the CLI re-parse.
  • cargo fmt / clippy clean via the pinned Nix toolchain.

(Written by Claude)

…oots together

The client TLS `root` field was an override: system roots loaded only when
no `--tls-root` was given. That made it impossible to trust a local
self-signed CA and the public CAs at once, so you couldn't dial both a
local relay and a remote one from the same client.

Make root loading additive. New `system_roots: Option<bool>` flag, defaulting
to enabled only when no custom root is set (preserving today's behavior).
Set it explicitly to trust the system roots alongside any custom root. The
combination of no custom root and system roots disabled is rejected up front
with `Error::NoRoots`, since WebPKI verification could never pass. Fingerprint
pinning and disable_verify swap in their own verifier, so an empty store stays
valid there.

Typed as `Option<bool>` per the config-flag convention so the TOML->CLI merge
can't clobber a TOML-set value; added a regression test in moq-relay.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 44091ea2-30b8-4f73-b894-5ff3f7dbfbc7

📥 Commits

Reviewing files that changed from the base of the PR and between 8dfd0b9 and ee8424e.

📒 Files selected for processing (3)
  • doc/bin/relay/config.md
  • rs/moq-native/src/tls.rs
  • rs/moq-relay/src/config.rs

Walkthrough

This PR adds configurable system_roots: Option<bool> support to control whether TLS client connections trust platform native root certificates. The change introduces a new Error::NoRoots variant, updates the Client struct with the new field and clap/serde wiring, refactors root store construction in Client::build to load native roots conditionally, and extends tests to cover the new validation paths. A regression test verifies TOML configuration is preserved across config re-parsing, and user documentation explains the new setting and its interaction with custom root configuration.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main feature: adding --tls-system-roots flag to enable trusting custom and system roots together, which addresses the core problem described in the PR.
Description check ✅ Passed The description comprehensively explains the problem, solution, API changes, compatibility considerations, and test coverage—all directly related to the changeset.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
✨ Simplify code
  • Create PR with simplified code
  • Commit simplified code in branch claude/brave-heyrovsky-da1c08

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kixelated
kixelated enabled auto-merge (squash) June 13, 2026 21:41
@kixelated
kixelated merged commit 4dce076 into main Jun 13, 2026
1 check passed
@kixelated
kixelated deleted the claude/brave-heyrovsky-da1c08 branch June 13, 2026 21:49
@moq-bot moq-bot Bot mentioned this pull request Jun 13, 2026
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.

1 participant