Skip to content

fix: fall back to system trust store when CA cert is absent from TLS …#1698

Open
asaxena7-atlas wants to merge 1 commit intoOT-CONTAINER-KIT:mainfrom
asaxena7-atlas:use-default-trust-store-if-no-ca
Open

fix: fall back to system trust store when CA cert is absent from TLS …#1698
asaxena7-atlas wants to merge 1 commit intoOT-CONTAINER-KIT:mainfrom
asaxena7-atlas:use-default-trust-store-if-no-ca

Conversation

@asaxena7-atlas
Copy link
Copy Markdown

@asaxena7-atlas asaxena7-atlas commented Mar 2, 2026

Description

When using TLS with publicly trusted certificates (e.g. Let's Encrypt, Google Trust Services), users no longer need to supply a ca.crt in their TLS secret. The operator’s container image already trusts these CAs via its system trust store. Previously, the operator would fail if ca.crt was missing from the secret, even when it wasn’t needed.

This change makes the CA certificate optional:

  • If CaKeyFile is not explicitly set in the CR and ca.crt is absent from the secret, the operator falls back to the system trust store (RootCAs: nil in Go’s tls.Config).
  • If CaKeyFile is explicitly set but the corresponding key is missing from the secret, the operator fails fast with a clear error (treating it as misconfiguration).
  • redis-cli commands, health probes, and exporter env vars conditionally omit --cacert / REDIS_EXPORTER_TLS_CA_CERT_FILE when no explicit CA is configured, preventing references to non-existent files.

Fixes #1697.


Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • Tests have been added/modified and all tests pass.
  • Functionality/bugs have been confirmed to be unchanged or fixed.
  • I have performed a self-review of my own code.

Additional context

Files changed:

  • internal/k8sutils/secrets.go

    • getRedisTLSConfig now returns a tls.Config with RootCAs = nil (system trust) when the CA is absent and not explicitly configured.
  • internal/controller/common/redis/heal.go

    • Applies the same CA-optional logic in the duplicated getRedisTLSConfig.
  • internal/k8sutils/redis.go

    • getRedisTLSArgs omits --cacert when CaKeyFile is empty.
  • internal/k8sutils/statefulset.go

    • GenerateTLSEnvironmentVariables and getExporterEnvironmentVariables conditionally emit CA-related env vars.
    • GenerateAuthAndTLSArgs and health probes now use shell parameter expansion (${REDIS_TLS_CA_KEY:+--cacert ...}) to conditionally include --cacert.
  • Tests

    • secrets_test.go, redis_test.go, and statefulset_test.go have been updated to cover absent-CA scenarios.

This change is fully backward compatible: existing setups that include ca.crt in their secret continue to work identically.

return &tls.Config{
Certificates: []tls.Certificate{cert},
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: true, //nolint:gosec // No CA cert available; skip server verification

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
Comment thread internal/k8sutils/secrets.go Outdated
return &tls.Config{
Certificates: []tls.Certificate{cert},
MinVersion: tls.VersionTLS12,
InsecureSkipVerify: true, //nolint:gosec // No CA cert available; skip server verification

Check failure

Code scanning / CodeQL

Disabled TLS certificate check

InsecureSkipVerify should not be used in production code.
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 5, 2026

Codecov Report

❌ Patch coverage is 45.83333% with 52 lines in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (main@7ae2c18). Learn more about missing BASE report.

Files with missing lines Patch % Lines
internal/controller/common/redis/heal.go 0.00% 34 Missing ⚠️
internal/k8sutils/secrets.go 70.00% 4 Missing and 2 partials ⚠️
internal/agent/bootstrap/redis/config.go 0.00% 4 Missing ⚠️
internal/agent/bootstrap/sentinel/config.go 0.00% 4 Missing ⚠️
internal/k8sutils/redis.go 66.66% 2 Missing ⚠️
internal/k8sutils/tls_keys.go 81.81% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1698   +/-   ##
=======================================
  Coverage        ?   29.60%           
=======================================
  Files           ?       84           
  Lines           ?     6709           
  Branches        ?        0           
=======================================
  Hits            ?     1986           
  Misses          ?     4523           
  Partials        ?      200           

☔ View full report in Codecov by Sentry.
📢 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.

@artkoshelev
Copy link
Copy Markdown

Hey folks, can some take a look at this PR? @shubham-cmyk @drivebyer @iamabhishek-dubey

@asaxena7-atlas asaxena7-atlas force-pushed the use-default-trust-store-if-no-ca branch 3 times, most recently from 2354560 to 30e7593 Compare April 21, 2026 04:50
…secret

Make CA certificate optional: when CaCertFile is not explicitly set and ca.crt is
missing from the secret, use the system certificate pool instead of failing.
Explicitly configured but missing CA keys still fail as misconfiguration.

Signed-off-by: asaxena7-atlas <208444049+asaxena7-atlas@users.noreply.github.com>
@asaxena7-atlas asaxena7-atlas force-pushed the use-default-trust-store-if-no-ca branch from 30e7593 to e3502ec Compare April 21, 2026 04:53
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.

TLS: operator should fall back to system trust store when CA cert is absent from secret

3 participants