Skip to content

trust the OS certificate store for TLS connections#41

Merged
alxnddr merged 1 commit into
mainfrom
trust-system-ca
Jul 9, 2026
Merged

trust the OS certificate store for TLS connections#41
alxnddr merged 1 commit into
mainfrom
trust-system-ca

Conversation

@alxnddr

@alxnddr alxnddr commented Jul 8, 2026

Copy link
Copy Markdown
Member

Closes #16

Summary by CodeRabbit

  • Bug Fixes
    • Improved TLS/certificate handling for CLI and network connections by trusting the system CA store when available.
    • Updated connection error messages to better explain certificate trust issues and how to resolve them with a CA bundle.

@coderabbitai

coderabbitai Bot commented Jul 8, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f5ed852a-2aaa-4081-9584-72d2bad52c7d

📥 Commits

Reviewing files that changed from the base of the PR and between f31632d and 541e4e0.

📒 Files selected for processing (3)
  • src/cli.ts
  • src/core/http/network-error.ts
  • src/core/http/system-ca.ts

📝 Walkthrough

Walkthrough

Adds a new trustSystemCa() function that merges OS-trusted CA certificates into Node's default TLS trust store, invokes it at CLI startup before command processing, and extends the TLS connection error message with guidance about NODE_EXTRA_CA_CERTS.

Changes

System CA Trust

Layer / File(s) Summary
System CA trust helper
src/core/http/system-ca.ts
New trustSystemCa() function checks for tls.getCACertificates/tls.setDefaultCACertificates support, and if available and system certs exist, merges them with defaults and sets them as the new default CA list.
CLI startup wiring
src/cli.ts
Imports trustSystemCa and calls it at the start of run(), before rawArgs parsing, help/usage handling, and unknown-command checks.
TLS error message guidance
src/core/http/network-error.ts
Extends the TLS-error hint in networkMessage with an additional clause about untrusted certificates and setting NODE_EXTRA_CA_CERTS.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI as cli.ts run()
  participant SystemCa as trustSystemCa()
  participant TLS as Node TLS store

  User->>CLI: invoke mb command
  CLI->>SystemCa: trustSystemCa()
  SystemCa->>TLS: getCACertificates()
  SystemCa->>TLS: setDefaultCACertificates(merged list)
  CLI->>CLI: continue command parsing/routing
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: trusting the OS certificate store for TLS connections.
Linked Issues check ✅ Passed The PR adds system CA trust support and surfaces the TLS guidance needed to address issue #16.
Out of Scope Changes check ✅ Passed The changes stay focused on TLS certificate trust behavior and related messaging, with no clear unrelated additions.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch trust-system-ca

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.

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown

Published @metabase/cli@0.2.1-alpha.trust-system-ca.541e4e0 with dist-tag alpha-trust-system-ca.

Install: npm install -g @metabase/cli@alpha-trust-system-ca

@alxnddr alxnddr requested a review from ranquild July 9, 2026 00:06
@alxnddr alxnddr merged commit 45f3efe into main Jul 9, 2026
17 checks passed
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.

Cannot connect to instances with system store trusted certificates

2 participants