feat(ledgerctl): add --tls-server-name to verify by name while dialing by IP by gfyrag · Pull Request #1595 · formancehq/ledger

gfyrag · 2026-07-16T09:42:24Z

Problem

On a TLS-enabled cluster, ledgerctl is unusable when run from inside a pod (e.g. kubectl exec). TLS verification matches the --server host against the server certificate's SANs, and the client never sets ServerName — so it verifies against whatever host is in --server. The default localhost:8888 fails because the operator-issued cert covers only the in-cluster DNS names (<pod>.<cluster>-headless.<ns>.svc.cluster.local, <cluster>.<ns>.svc.cluster.local) and deliberately never localhost/127.0.0.1.

Fix

Add a persistent --tls-server-name flag that overrides the verification hostname independently of the dial address — the curl --resolve / openssl -servername equivalent. This lets a client dial by IP/loopback while still validating an FQDN-only certificate.

ledgerctl --server 127.0.0.1:8888 \
  --tls-server-name "ledger-<cluster>.<ns>.svc.cluster.local" \
  --tls-ca-cert "$TLS_CA_CERT_FILE" ledgers list

The inter-node transport already does exactly this (connection_pool.go sets cfg.ServerName), so this brings ledgerctl in line with the existing pattern.

Details

Persistent flag --tls-server-name, env LEDGERCTL_TLS_SERVER_NAME, profile field tlsServerName (full CLI > env > profile > default precedence).
Sets tls.Config.ServerName in GetClientTransportCredentials.
Rejected together with --insecure (conflicting intent), matching the existing --tls-ca-cert guard — also catches a stray LEDGERCTL_INSECURE leaking in.
Wired into profile create, auth login (not dropped on re-login), and profile show.
Docs: docs/ops/cli.md global-flags table, profile create table, and a new subsection covering the in-pod use case (dial-FQDN vs dial-by-IP options).

Not included

No operator change: the operator already dials the in-cluster FQDN (podSelfServerAddr, backupServerAddr), so it has no mismatch and doesn't need the flag.

Tests

insecure + tls-server-name rejected; TLS-with-server-name succeeds.
Profile resolution of tlsServerName through the shared resolver.
go build ./..., go vet, and all ./cmd/ledgerctl/... tests green.

…g by IP TLS verification matches the --server host against the certificate SANs. Inside a TLS cluster pod, the operator-issued cert covers only the in-cluster DNS names (never localhost/127.0.0.1), so the default localhost:8888 dial fails with a name mismatch and ledgerctl is unusable. Add a persistent --tls-server-name flag (env LEDGERCTL_TLS_SERVER_NAME, profile tlsServerName) that overrides the verification hostname independently of the dial address, letting a client dial by IP/loopback while still validating the cert against a covered SAN. Rejected together with --insecure, matching --tls-ca-cert.

coderabbitai · 2026-07-16T09:42:34Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

🗂️ Base branches to auto review (1)

main

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 92b3a8ea-57d8-4647-ae87-437ef85a33ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch feat/ledgerctl-tls-server-name

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.}

NumaryBot · 2026-07-16T09:43:37Z

💬 Comments — automated review

The core TLS ServerName feature — flag registration, credential wiring, restore-path fix, profile-list column, and the ledgerctlOwnedFlagNames / env-binding issue — has all been addressed in prior commits and follow-up fixes (f71f411, 602fe6e). The mutual-exclusion guard concerns raised in the PR discussion thread (Threads 3 and 4) were explicitly flagged by the project bot and are actively tracked; no new material evidence beyond what is already in the thread is provided by the current reviewers. One remaining single-reviewer finding is concrete enough to keep: when LEDGERCTL_TLS_SERVER_NAME is set in the environment (rather than passed as a flag), the profile-create path does not read it for the owned flag, so the profile is silently saved without the TLS server name. This is a narrow but real usability gap; its severity is minor because it only affects the env-var-driven creation workflow, and the user can work around it with the explicit flag. No blockers or majors are confirmed beyond what has already been addressed or acknowledged.

NumaryBot

NumaryBot posted 1 new inline finding.

Summary: #1595 (comment)

…lied Address independent review of PR #1595: - restore subcommands built gRPC credentials by hand, reading only --server/--insecure and silently ignoring --tls-ca-cert and the new --tls-server-name. Route getRestoreClient through the shared GetClientTransportCredentials so restore honors the same persistent flags (and the insecure/CA/server-name mutual-exclusion guards) as every other command — the restore server is a plausible in-pod dial target where --tls-server-name is exactly what's needed. - Extract buildClientTLSConfig so the tls.Config is directly assertable; the TLSWithServerName test now checks ServerName is set rather than only that creds are non-nil (creds.Info().ServerName is deprecated). - profile list now surfaces the TLS SERVER NAME column, matching how --tls-ca-cert is displayed.

gfyrag · 2026-07-16T09:47:47Z

Addressed findings from an independent review pass (commit f71f411):

[Medium] restore ignored TLS flags — getRestoreClient built credentials by hand, reading only --server/--insecure and silently dropping --tls-ca-cert and the new --tls-server-name. Now routed through the shared GetClientTransportCredentials, so restore honors the same persistent flags and mutual-exclusion guards as every other command (this also fixes a pre-existing --tls-ca-cert gap). The restore server is a plausible in-pod dial target, so --tls-server-name is exactly relevant there.
[Medium] feature line untested — extracted buildClientTLSConfig and the test now asserts ServerName is actually applied (the previous NotNil check couldn't catch a deleted assignment; creds.Info().ServerName is deprecated so the config is asserted directly).
[Low] profile list — now surfaces a TLS SERVER NAME column, matching how --tls-ca-cert is shown.

go build ./..., go vet, gofmt, and all ./cmd/ledgerctl/... tests green.

NumaryBot

NumaryBot posted 1 new inline finding.

Summary: #1595 (comment)

NumaryBot review finding: --tls-server-name was missing from ledgerctlOwnedFlagNames, so bindSubcommandEnv bound the bare TLS_SERVER_NAME env var for subcommands that redeclare the flag locally (profile create). A stray TLS_SERVER_NAME would then silently populate and persist tlsServerName, bypassing the LEDGERCTL_TLS_SERVER_NAME contract and pinning TLS verification to the wrong name. Add it to the owned set (alongside tls-ca-cert) and cover it in TestSubcommandLocalFlagsIgnoreBareEnv.

NumaryBot

NumaryBot posted 2 new inline findings.

Summary: #1595 (comment)

NumaryBot review findings: profile create and the auth login bootstrap path persisted a profile without validating flag compatibility, since the insecure/TLS mutual-exclusion guard only ran inside GetClientTransportCredentials at connection time. A user passing --insecure together with --tls-server-name (or --tls-ca-cert, a pre-existing gap) would store a profile every later command then immediately rejects. Extract the rule into a shared cmdutil.ValidateTLSFlags (single source of truth, covering both --tls-ca-cert and --tls-server-name for consistency) and call it from GetClientTransportCredentials, profile create, and auth login before persisting — failing fast at write time. Add a direct table test for the helper.

NumaryBot

NumaryBot posted 1 new inline finding.

Summary: #1595 (comment)

NumaryBot · 2026-07-16T10:15:58Z

 	cmd.Flags().String("server", "", "gRPC server address (required)")
 	cmd.Flags().Bool("insecure", false, "Use insecure connection (no TLS)")
 	cmd.Flags().String("tls-ca-cert", "", "Path to CA certificate file (PEM)")
+	cmd.Flags().String("tls-server-name", "", "Hostname to verify against the server certificate SANs, overriding the --server host")


🟡 [minor] LEDGERCTL_TLS_SERVER_NAME not applied when creating a profile via environment variable

When profile create is invoked without an explicit --tls-server-name flag but with LEDGERCTL_TLS_SERVER_NAME set, the owned-flag exclusion in bindSubcommandEnv means the env var is not read for the local flag, and ResolveConnectionFlags is not called for profile subcommands. The profile is therefore saved without tlsServerName, so subsequent commands that rely on the persisted value fail unless the user repeats --tls-server-name explicitly every time.

Suggestion: Explicitly read LEDGERCTL_TLS_SERVER_NAME (and similarly LEDGERCTL_TLS_CA_CERT) in the profile-create path before persisting the profile, either by calling a shared resolver or by directly checking the prefixed env var and populating the flag default.

Working as intended — declining this one. ResolveConnectionFlags deliberately skips profile-management commands (see resolve.go: "they define local flags with the same names and must not be contaminated by the active profile or environment variables"), so profile create never inherits any LEDGERCTL_* value — not just LEDGERCTL_TLS_SERVER_NAME but also LEDGERCTL_SERVER, LEDGERCTL_INSECURE, LEDGERCTL_TLS_CA_CERT, etc. --tls-server-name is fully consistent with every other connection flag here. Auto-baking an ambient env var into a newly-created profile would break that contract and risk silent cross-contamination between profiles (an env set for cluster A leaking into a profile meant for cluster B). Users who want env-driven values use them at command time (where ResolveConnectionFlags does apply them), not at profile-create time. No change.

codecov · 2026-07-16T10:24:55Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.73%. Comparing base (29742f6) to head (50a0a9c).
⚠️ Report is 40 commits behind head on release/v3.0.

Additional details and impacted files

@@               Coverage Diff                @@
##           release/v3.0    #1595      +/-   ##
================================================
+ Coverage         74.24%   74.73%   +0.48%     
================================================
  Files               404      430      +26     
  Lines             42557    45722    +3165     
================================================
+ Hits              31598    34171    +2573     
- Misses             8116     8502     +386     
- Partials           2843     3049     +206

Flag	Coverage Δ
e2e	`74.73% <ø> (+0.48%)`	⬆️
scenario	`74.73% <ø> (+0.48%)`	⬆️
unit	`74.73% <ø> (+0.48%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

gfyrag requested review from Azorlogh and paul-nicolas as code owners July 16, 2026 09:42

NumaryBot reviewed Jul 16, 2026

View reviewed changes

Comment thread cmd/ledgerctl/profile/create.go

NumaryBot reviewed Jul 16, 2026

View reviewed changes

Comment thread cmd/ledgerctl/profile/create.go

NumaryBot reviewed Jul 16, 2026

View reviewed changes

Comment thread cmd/ledgerctl/profile/create.go

Comment thread cmd/ledgerctl/auth/login.go

NumaryBot reviewed Jul 16, 2026

View reviewed changes

paul-nicolas approved these changes Jul 16, 2026

View reviewed changes

gfyrag merged commit 90dadde into release/v3.0 Jul 16, 2026
14 checks passed

gfyrag deleted the feat/ledgerctl-tls-server-name branch July 16, 2026 15:08

Uh oh!

Conversation

gfyrag commented Jul 16, 2026

Problem

Fix

Details

Not included

Tests

Uh oh!

coderabbitai Bot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

NumaryBot commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💬 Comments — automated review

Uh oh!

NumaryBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gfyrag commented Jul 16, 2026

Uh oh!

NumaryBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NumaryBot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

NumaryBot left a comment

Choose a reason for hiding this comment

Uh oh!

NumaryBot Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

gfyrag Jul 16, 2026

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jul 16, 2026

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

coderabbitai Bot commented Jul 16, 2026 •

edited

Loading

NumaryBot commented Jul 16, 2026 •

edited

Loading