[gmenher]

Summary

Add opt-in TLS and mutual TLS support for OpenShell's dedicated metrics listener while preserving plaintext metrics as the default for compatibility.

This is the first implementation PR for #3664. It intentionally focuses on the runtime listener, operator-provided Secrets, and the metrics trust boundary; chart-managed metrics PKI, monitoring resources, and metrics ingress policy configuration remain follow-up work.

Related Issue

Refs #3664

Changes

• Add dedicated metrics TLS configuration through gateway TOML, CLI flags, and environment variables.
• Serve /metrics over HTTPS when metrics TLS is configured, with optional client-certificate authentication.
• Keep the metrics client CA independent from the gateway client CA, and reject Helm configurations that reuse the effective gateway client CA Secret.
• Mount operator-provided server TLS and metrics client CA Secrets read-only through the Helm chart.
• Reuse the existing TLS reload machinery while identifying metrics-specific reload logs and OCSF events.
• When metrics TLS material is missing or malformed at startup, emit a metrics-specific lifecycle error, disable only the metrics listener, and never fall back to plaintext. Gateway and health listeners remain available.
• Rate-limit rejected TLS-handshake warnings: retain per-connection diagnostics at DEBUG, warn immediately for the first rejection, then report suppressed rejections at most once per minute.
• Document the runtime configuration, Helm values, trust boundary, secure scraper requirements, and startup/reload behavior.
• Add runtime, configuration, Helm rendering, trust-boundary, lifecycle-log, and rate-limiter coverage.

Validation

• cargo fmt --check
• cargo test -p openshell-core metrics_tls --lib
• cargo test -p openshell-server metrics_tls --lib
• cargo test -p openshell-server tls_metrics_listener_serves_https_and_rejects_plaintext --lib
• cargo test -p openshell-server metrics_mtls_trusts_only_the_metrics_client_ca --lib
• cargo test -p openshell-server test_reload_invalid_preserves_old --lib
• mise run helm:test (359 tests)
• mise run helm:lint
• mise run helm:docs:check
• mise run docs
• go build ./... and go test ./... in sdk/go
• Kubernetes E2E was run with a fresh ephemeral k3d cluster; the runner cleaned up the cluster after completion.

mise run ci is not marked successful: its concurrently launched Go tasks use incompatible local compiler-cache artifacts (go1.26.7 versus go1.26.2). The same Go build and test commands pass when run directly after clearing the local Go cache; this branch does not modify Go code.

The required Strix scan could not run in this environment because Docker could not pull usestrix/strix:latest (registry access denied).

Checklist

• Follows Conventional Commits.
• Commits are signed off (DCO).
• Architecture and deployment documentation updated.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[rhuss]

cc-review summary

Reviewed at f6b2b199e with a 6-dimension agent pass plus CodeRabbit. The feature is well-scoped and security-careful: client-cert auth is enforced with no fail-open, an incomplete TLS config fails closed with no plaintext fallback, the reload machinery is reused cleanly and tolerates bad reloads, and plaintext stays the default. The standout is the trust-boundary design: keeping the metrics client CA independent from the gateway client CA, and rejecting Helm configs that reuse it, correctly avoids widening the gateway client CA's authority to metrics scraping. All eight goals are delivered. No correctness or security defect.

Two non-blocking Production recommendations below, the first worth addressing before merge. Test-depth, Helm-maintainability, and naming/doc findings are omitted here to keep this focused; happy to share them.

Source: cc-review (6 agents + CodeRabbit)

[gmenher]

Rebased onto current main and addressed the two production-readiness follow-ups:

• invalid or unavailable metrics TLS material now disables only the metrics listener, without plaintext fallback or gateway/health disruption;
• rejected metrics TLS handshakes are rate-limited at WARN, with per-connection detail retained at DEBUG.

Focused Rust, Helm, documentation, and Kubernetes validation are recorded in the PR description. @rhuss, when you have a moment, could uou take another look? Thanks!!

[krishicks]

Written by Claude Code, reviewed by @krishicks

The overall shape looks good: a separate config type, partial input fails instead of falling back, and the listener never drops to plaintext. Inline comments below; the two Helm items are the most important.

[krishicks]

Written by Claude Code, reviewed by @krishicks

On Helm, this is only true for a Secret that exists but has bad contents. The metrics-tls-cert and metrics-tls-client-ca volumes in _gateway-workload.tpl are non-optional Secret volumes. If the named Secret is missing, or the CA Secret has no ca.crt key (it is selected via items), kubelet never starts the gateway container, so the control plane is unavailable too, not just metrics.

The mounts look correct as written: they render only when metrics.tls.enabled is set, and they match the chart's other TLS mounts, none of which use optional: true. A loud FailedMount is better than a quietly missing scrape target. Making them optional wouldn't help much anyway, because the gateway never starts a reload worker when the material is missing at startup, so a Secret created later still needs a restart.

Suggest fixing the docs instead: a missing Secret (or missing ca.crt key) blocks gateway pod startup, and only malformed contents in an existing Secret disable just the metrics listener. The same wording appears in README.md, docs/how-it-works/gateways/configuration.mdx, and skills/debug-openshell-cluster/SKILL.md.

[krishicks]

Written by Claude Code, reviewed by @krishicks

This guard compares Secret names only, so it misses other Secrets that contain the same CA. In the default pkiInitJob mode, certgen writes the gateway CA as ca.crt into the client Secret too (server.tls.clientTlsSecretName, default openshell-client-tls; see tls_secret in certgen.rs), and that is the Secret mounted into sandbox pods. With metrics.tls.clientCaSecretName: openshell-client-tls this check passes, and the metrics listener then trusts every sandbox client cert, which is the boundary this PR is meant to enforce.

Suggest also rejecting server.tls.clientTlsSecretName, and saying in the values/README text that the check is by name only and can't detect a different Secret holding the same CA.

[krishicks]

Written by Claude Code, reviewed by @krishicks

The flag is declared with require_equals = true and num_args = 0..=1, so --metrics-require-client-auth false (as this synopsis implies) is rejected. Only the bare flag or --metrics-require-client-auth=false parses. Suggest documenting it as **--metrics-require-client-auth**[=*BOOL*], or dropping require_equals.

[krishicks]

Written by Claude Code, reviewed by @krishicks

Nit: timeouts and handshake rejections share one limiter, so the periodic WARN carries a combined suppressed/total count but uses the message of whichever event happened to trigger it. During a burst of untrusted client certs, an operator can see "Metrics TLS handshakes timed out" with a count that is mostly certificate rejections. Separate limiters (or one combined, neutral message) would avoid that misdiagnosis.