(feat) support Mutual TLS (mTLS) certificate-based ACL authentication - #242
(feat) support Mutual TLS (mTLS) certificate-based ACL authentication#242sandeepkunusoth wants to merge 5 commits into
Conversation
sandeepkunusoth
changed the title
|
| Filename | Overview |
|---|---|
| api/v1alpha1/valkeycluster_types.go | Adds TLS auth enum types, directive mapping helpers, defaults, and CEL validation for certificate user mapping. |
| config/crd/bases/valkey.io_valkeyclusters.yaml | Regenerates the ValkeyCluster CRD schema with TLS auth defaults, enums, and validation. |
| config/crd/bases/valkey.io_valkeynodes.yaml | Regenerates the ValkeyNode CRD schema with the shared TLS auth fields and validation. |
| internal/controller/config.go | Renders new Valkey TLS auth directives, but omits the previous tls-auth-clients optional behavior when the new field is empty. |
| internal/controller/metrics_exporter.go | Passes the mounted TLS certificate and key to the exporter so it can connect when mTLS is required. |
| internal/controller/utils.go | Loads the TLS certificate and key into controller Valkey client TLS configs so internal connections can satisfy mTLS. |
| internal/controller/valkeynode_resources_test.go | Adds standalone ValkeyNode mTLS URI rendering coverage, but removes coverage for empty TLS config preserving tls-auth-clients optional. |
| test/e2e/valkeycluster_tls_test.go | Adds end-to-end coverage for TLS auth defaults, CN-based ACL mapping, mTLS rejection without certs, exporter, and replication paths. |
Sequence Diagram
sequenceDiagram
participant User as Operator user
participant API as Kubernetes API
participant Reconciler as ValkeyCluster/ValkeyNode reconcilers
participant Config as ConfigMap valkey.conf
participant Pod as Valkey pod
participant Client as TLS client/exporter/operator
User->>API: Apply TLS spec with authClients/authClientsUser
API->>API: Default enums and validate disabled+mapping rule
API->>Reconciler: Reconcile TLS-enabled resource
Reconciler->>Config: Render tls-auth-clients and tls-auth-clients-user
Reconciler->>Pod: Mount TLS Secret and roll/update pod
Client->>Pod: Connect with CA and optional client cert
Pod-->>Client: Enforce mTLS and map CN/URI to ACL user when configured
Reviews (8): Last reviewed commit: "fixed review comments" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
nice, the two-enum shape (authClients + authClientsUser) maps cleanly onto the valkey directives. one thing worth calling out in the design, since it bit us: turning on authClients: Required with authClientsUser: CN authenticates every client connection, including the operator's own and a couple of internal ones, so each of those needs a cert whose CN maps to an ACL user with the right grants. concretely:
so the feature is really "every internal connection becomes authenticated", and the CRD probably wants the operator/probe/replication certs provisioned and CN-mapped before Required can be safely set, otherwise enabling it bricks the cluster it's managing. two smaller notes:
happy to share how we wired the operator's own client cert if useful. |
ef6ea5f to
d0169ff
Compare
|
Sorry for delay in response.
The operator currently only supports Valkey 9.0+, so
i fixed this by reusing valkey tls server certs on clients side for operator user, redis exporter. i think we need to check more on if we want to move operator user to use mtls client cert as part of seperate Issue. Health checks already pass client certs. updated e2e tests to test scenario with TLSAuthClients |
bjosv
left a comment
bjosv
left a comment
There was a problem hiding this comment.
Just a quick look as we will have this under spec.networking.tls, but avoiding yes/no and using Required/Optional/Disabled as discussed in the issue has the additional benefit of minimizing the yaml-parser annoyance (bool vs string).
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
… to use valkey server certs. and updated CRD values to match valkey.conf directives Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
…ated docs Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
7234d2e to
7cf1429
Compare
📝 WalkthroughWalkthroughChangesmTLS authentication
Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant Kubernetes
participant ValkeyCluster
participant TLSClient
E2ETest->>Kubernetes: Create CA and TLS certificates
E2ETest->>ValkeyCluster: Apply required client authentication and CN mapping
TLSClient->>ValkeyCluster: Connect with client certificate
ValkeyCluster->>TLSClient: Map certificate CN to ACL user
TLSClient->>ValkeyCluster: Run ACL WHOAMI and replication write
TLSClient->>ValkeyCluster: Connect without client certificate
ValkeyCluster-->>TLSClient: Reject connection
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
internal/controller/config.go (1)
83-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the spelling in the comment.
"cetificate" must be "certificate". The conditional rendering itself is correct: when
AuthClientsis empty the operator emits no directive and Valkey applies its own default, which matches the API default.✏️ Proposed fix
if tls.AuthClientsUser != "" { - // Automatically authenticate TLS clients as ACL users based on their cetificate fields. + // Automatically authenticate TLS clients as ACL users based on their certificate fields. config["tls-auth-clients-user"] = string(tls.AuthClientsUser) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/config.go` around lines 83 - 90, Correct the spelling in the comment above the tls.AuthClientsUser conditional: change “cetificate” to “certificate”. Leave the conditional logic and configuration rendering unchanged.internal/controller/config_test.go (2)
122-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for an unset
AuthClients.
buildManagedConfigskipstls-auth-clientswhenAuthClientsis the empty string. No test covers that branch. A regression that always emits the directive would pass the current suite.🧪 Proposed test
+ It("omits tls-auth-clients directives when the fields are unset", func() { + cluster := getSampleCluster() + cluster.Spec.TLS = &valkeyiov1alpha1.TLSConfig{ + Certificate: valkeyiov1alpha1.CertificateRef{SecretName: "tls-secret"}, + } + conf := buildServerConfig(cluster) + Expect(conf).To(ContainSubstring("tls-port")) + Expect(conf).NotTo(ContainSubstring("tls-auth-clients")) + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/config_test.go` around lines 122 - 163, Add a test case within the TLS client-auth Describe block covering a TLSConfig with a certificate and AuthClients left unset, then call buildServerConfig and assert that tls-auth-clients is absent. Keep TLS configured so the assertion specifically exercises the empty AuthClients branch in buildManagedConfig.
89-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the two rejection assertions.
Line 93 checks
authClientsUser=CNwhile line 100 checksauthClientsUser=CN/URI. Both substrings match the same CEL message, so the CN case asserts less than the URI case. Use the same substring in both tests so a message change fails both consistently.♻️ Proposed change
- Expect(err.Error()).To(ContainSubstring("authClientsUser=CN")) + Expect(err.Error()).To(ContainSubstring("authClientsUser=CN/URI"))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/config_test.go` around lines 89 - 101, Update the rejection assertion in the “rejects authClients=no with authClientsUser=CN” test to use the same “authClientsUser=CN/URI” substring as the URI case, keeping both tests aligned with the shared CEL validation message.test/e2e/valkeycluster_tls_test.go (2)
478-481: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLoosen the
ACL WHOAMIoutput assertion.
utils.RunreturnsCombinedOutput, so any warning thatvalkey-cliwrites to stderr is included. A single warning line makesEqual("alice")fail even though the mapping worked. Match the user name instead.🧪 Proposed fix
- Expect(strings.TrimSpace(out)).To(Equal("alice"), + Expect(strings.TrimSpace(out)).To(ContainSubstring("alice"), "ACL WHOAMI should resolve to the user mapped from the certificate's CN")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_tls_test.go` around lines 478 - 481, Update the ACL WHOAMI assertion in the mTLS client log check to match the expected user name within the combined output rather than requiring an exact "alice" string. Keep the existing trimmed output and error validation, and preserve the certificate-CN mapping expectation.
486-532: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClarify which ACL identity the in-pod
valkey-cliuses.This test execs
valkey-cliinside the server container with/tls/tls.crt, which is the server certificate. The cluster runs withauthClientsUser: CN, so Valkey maps this connection to an ACL user named after the server certificate CN (valkey-cluster-mtls.default.svc.cluster.local). No ACL user with that name exists in the manifest at lines 392-396.If this test passes, it documents the fallback behavior for an unmapped CN, which is the same behavior the operator and the probes depend on. Add a short comment stating the expected identity, or assert
ACL WHOAMIhere as well so the fallback is explicit rather than incidental.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_tls_test.go` around lines 486 - 532, The mTLS client identity used by the in-pod valkey-cli is implicit and should be made explicit. In the test case around the primary-pod replication flow, document that the server certificate maps to the expected CN-based ACL identity and that this unmapped identity relies on the intended fallback behavior, or add an ACL WHOAMI assertion using the same TLS command options before validating replication.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/v1alpha1/valkeycluster_types.go`:
- Around line 305-307: Correct the typo in the comment for TLSAuthClientsUserURI
by changing “natches” to “matches”; do not modify the constant or its behavior.
- Around line 323-338: Update the TLS documentation in docs/tls.md to include an
upgrade/migration note for existing clusters that omit spec.tls.authClients: the
default changes from optional to yes, enabling mTLS, unless the CR explicitly
sets authClients: optional. Keep the existing description of the yes default and
authentication behavior intact.
In `@docs/tls.md`:
- Line 124: Update the subsection heading beginning “Never use nopass: true”
from level four to level three so it uses `###` and preserves the hierarchy
under the surrounding `## Security considerations` section.
- Around line 99-106: Update the cert-manager Certificate example for
valkey-client-alice by adding spec.usages with client auth alongside the
existing certificate configuration. Preserve the current metadata, secretName,
commonName, and issuerRef values.
- Around line 32-34: Expand the mTLS ACL documentation around the
authClients/authClientsUser example to cover every internal Valkey connection:
replication must use the _replication CN or URI, the exporter must use its
_exporter identity, and probes must have a matching certificate identity when
password AUTH is unavailable. State that certificate rotation must preserve the
mapped CN or URI for each identity.
In `@internal/controller/utils.go`:
- Around line 337-354: Define and use a dedicated operator client certificate
whose CN maps to an existing privileged ACL user instead of reusing the Valkey
server certificate in the TLS setup around tls.X509KeyPair; confirm and handle
the behavior when no matching ACL user exists. Apply the same client-identity
decision to internal/controller/utils.go lines 337-354 and
internal/controller/metrics_exporter.go lines 43-47, including how the
exporter's REDIS_USER/REDIS_PASSWORD interact with certificate-derived ACL
mapping.
In `@test/e2e/valkeycluster_tls_test.go`:
- Line 216: Restore explicit timeout and polling interval arguments on both
Eventually assertions in the Valkey TLS end-to-end test, including the readiness
check and metrics/exporter assertion near the referenced blocks. Use the longer
timeout and polling interval established by the surrounding or prior test
patterns rather than relying on Gomega defaults.
- Around line 363-376: Set an explicit timeout for the final
Eventually(verifyReady) readiness wait, using the existing 2-minute default or
an explicit 10-minute timeout consistent with the mTLS test’s intended bounds.
Keep the certificate and deployment readiness checks unchanged.
---
Nitpick comments:
In `@internal/controller/config_test.go`:
- Around line 122-163: Add a test case within the TLS client-auth Describe block
covering a TLSConfig with a certificate and AuthClients left unset, then call
buildServerConfig and assert that tls-auth-clients is absent. Keep TLS
configured so the assertion specifically exercises the empty AuthClients branch
in buildManagedConfig.
- Around line 89-101: Update the rejection assertion in the “rejects
authClients=no with authClientsUser=CN” test to use the same
“authClientsUser=CN/URI” substring as the URI case, keeping both tests aligned
with the shared CEL validation message.
In `@internal/controller/config.go`:
- Around line 83-90: Correct the spelling in the comment above the
tls.AuthClientsUser conditional: change “cetificate” to “certificate”. Leave the
conditional logic and configuration rendering unchanged.
In `@test/e2e/valkeycluster_tls_test.go`:
- Around line 478-481: Update the ACL WHOAMI assertion in the mTLS client log
check to match the expected user name within the combined output rather than
requiring an exact "alice" string. Keep the existing trimmed output and error
validation, and preserve the certificate-CN mapping expectation.
- Around line 486-532: The mTLS client identity used by the in-pod valkey-cli is
implicit and should be made explicit. In the test case around the primary-pod
replication flow, document that the server certificate maps to the expected
CN-based ACL identity and that this unmapped identity relies on the intended
fallback behavior, or add an ACL WHOAMI assertion using the same TLS command
options before validating replication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ebc8c63e-5da3-47d7-9a23-b6468b1a5c3a
📒 Files selected for processing (10)
api/v1alpha1/valkeycluster_types.goconfig/crd/bases/valkey.io_valkeyclusters.yamlconfig/crd/bases/valkey.io_valkeynodes.yamldocs/tls.mdinternal/controller/config.gointernal/controller/config_test.gointernal/controller/metrics_exporter.gointernal/controller/utils.gointernal/controller/valkeynode_resources_test.gotest/e2e/valkeycluster_tls_test.go
Signed-off-by: Sandeep Kunusoth <sandeepkunsoth000@gmail.com>
| if tls.AuthClients != "" { | ||
| if directive, ok := tls.AuthClients.AuthClientsDirective(); ok { | ||
| config["tls-auth-clients"] = directive | ||
| } | ||
| } |
There was a problem hiding this comment.
When TLSConfig.AuthClients is empty, this now omits tls-auth-clients. Valkey's default is yes, so existing stored TLS CRs created before this CRD default, or standalone ValkeyNode specs built without admission defaulting, will start requiring client certificates instead of keeping the operator's previous optional behavior.
| if tls.AuthClients != "" { | |
| if directive, ok := tls.AuthClients.AuthClientsDirective(); ok { | |
| config["tls-auth-clients"] = directive | |
| } | |
| } | |
| if tls.AuthClients == "" { | |
| config["tls-auth-clients"] = "optional" | |
| } else if directive, ok := tls.AuthClients.AuthClientsDirective(); ok { | |
| config["tls-auth-clients"] = directive | |
| } |
Artifacts
Repro: focused Go harness source
- Evidence file captured while the check ran.
Repro: failing Go harness output showing missing tls-auth-clients optional
- The full command output behind this check.
| By default, Valkey uses mutual TLS and requires clients to present a valid certificate verified against trusted root CAs configured via `tls-ca-cert-file` or `tls-ca-cert-dir`. You may use `tls-auth-clients no` to disable client authentication. | ||
|
|
||
| When `spec.tls.authClients` is omitted, the operator defaults it to `Optional` and renders `tls-auth-clients optional` so TLS clients can connect without presenting a client certificate. Set `authClients: Required` to enforce mTLS (`tls-auth-clients yes`), or `authClients: Disabled` to turn client certificate processing off (`tls-auth-clients no`). | ||
|
|
There was a problem hiding this comment.
should we set default value of "tls-auth-clients"] to "Required" inside operator or stick with current "Optional"? when spec.tlsAuthClients is empty in spec as by default valkey 9.* uses tls-auth-clients to yes
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
test/e2e/valkeycluster_tls_test.go (4)
502-523: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftUse a CN-matched ACL user for server-side TLS replication connections.
authClientsUser: CNrequires the client certificate CN to match an enabled ACL user. The replica connects using the server certificate with CNvalkey-cluster-mtls.default.svc.cluster.local, while onlyaliceexists as an ACL user. Add a matching internal ACL user or use a dedicated CN-based client certificate for replication traffic. Valkey does not map TLS clients to ACL users when the CN has no enabled ACL match.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_tls_test.go` around lines 502 - 523, The mTLS replication test must provision an enabled ACL user matching the server certificate CN valkey-cluster-mtls.default.svc.cluster.local before validating replication. Update the test setup around the replication case to add or configure that internal CN-based user, while preserving alice for regular client authentication.
476-483: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winProject only
server-tls/ca.crtinto client pods.The client commands read
ca.crt, but these test Secret mounts expose the fullserverCertSecret, includingtls.key. Apply a Secretitemsprojection forserver-tlsat lines 482 and 564. Keep the separate clientvolmounts forclient-tlsbecause those commands also need the client cert/key.Proposed Secret projection
"secret": { - "secretName": "%s" + "secretName": "%s", + "items": [{"key": "ca.crt", "path": "ca.crt"}] }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_tls_test.go` around lines 476 - 483, Update the Secret volume definitions in the client pod manifests generated by the relevant test setup blocks to project only server-tls/ca.crt from the server certificate Secret via Secret items. Apply this at both server-tls volume definitions, while preserving the separate client-tls volume mounts unchanged because client commands require the certificate and key.
384-392: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winWait for the leaf
Certificateresources to become Ready.
kubectl get secretonly proves the Secret exists; it does not prove cert-manager generated a fresh, valid certificate pair for these mTLS names. Add waits for the two leaf Certificates, such askubectl wait --for=condition=Ready certificate/<name> --timeout=<duration>, before creating theValkeyCluster.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_tls_test.go` around lines 384 - 392, Update the certificate readiness waits in the test setup before creating the ValkeyCluster to wait for both leaf Certificate resources, not just their Secrets. In the Eventually blocks around serverCertSecret and clientCertSecret, use kubectl wait for condition=Ready on each corresponding certificate name with an appropriate timeout, while preserving the existing retry behavior.
346-370: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDeclare
client authon the client certificate.Cert-manager uses only TLS Web Server Authentication EKU by default when
usagesis omitted.clientCertSecretis presented as a client certificate, so addclient authto prevent client certificate validation failures. If the server certificate is still shared, declare both TLS Web Server Authentication and TLS Web Client Authentication on that Certificate as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_tls_test.go` around lines 346 - 370, Update the client Certificate manifest for the certificate with commonName alice to declare TLS Web Client Authentication in its usages. If the server certificate is shared for client authentication, also declare both TLS Web Server Authentication and TLS Web Client Authentication on the valkey certificate, preserving its existing server usage.
♻️ Duplicate comments (1)
test/e2e/valkeycluster_tls_test.go (1)
225-232: 🩺 Stability & Availability | 🟠 MajorRestore explicit timeouts for external waits.
The first TLS suite still relies on default
Eventuallytiming after the five-minute timeout was removed. The new mTLS readiness, configuration, primary lookup, and replication waits repeat the same pattern. The replication callback can block for the fullWAIT 1 5000duration. Pass explicit timeout and polling values for these Kubernetes and Valkey operations.Also applies to: 271-288, 421-427, 452-462, 506-514, 529-539
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_tls_test.go` around lines 225 - 232, Restore explicit timeout and polling arguments on every Eventually wait in the TLS suite, including the pod readiness block around the visible kubectl command and the mTLS readiness, configuration, primary lookup, and replication waits identified by the comment. Choose values that accommodate external Kubernetes/Valkey operations, including the replication callback’s WAIT 1 5000 duration, rather than relying on Gomega defaults.
🧹 Nitpick comments (1)
test/e2e/valkeycluster_tls_test.go (1)
394-413: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winExercise the metrics exporter under strict mTLS.
This mTLS manifest enables
authClients: RequiredandauthClientsUser: CN, but it does not enable the exporter. The earlier exporter tests do not explicitly exercise this authentication mode. Add exporter coverage to this fixture, or add a separate test that verifies client credentials andredis_up=1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/e2e/valkeycluster_tls_test.go` around lines 394 - 413, Update the mTLS ValkeyCluster fixture in the test around clusterManifest to enable the metrics exporter under authClients: Required and authClientsUser: CN, then verify the exporter connects with the required client credentials and reports redis_up=1. Reuse the existing exporter test helpers and credential configuration rather than creating unrelated coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/mtls.md`:
- Around line 134-136: Update the strict mTLS guidance to state that resetpass
alone is insufficient: require restricting the default user or document that
prerequisite, and add coverage for a certificate with an unmatched CN/URI to
verify it cannot access default-user permissions.
- Around line 64-65: Update the rendered configuration example in the mTLS
documentation to match the operator output: remove quotes from the
tls-auth-clients value and show separate tls-auth-clients-user directives for CN
and URI instead of the combined CN/URI placeholder.
- Around line 38-40: Update the mTLS documentation around the
authClients/authClientsUser guidance to explicitly state that operator
connections, health checks, the exporter, and tls-replication links must present
certificates whose mapped CN or URI matches an enabled ACL user with the
required permissions. Also document that reused server-certificate rotation must
preserve this identity to avoid internal authentication failures or incorrect
ACL mapping.
In `@test/e2e/valkeycluster_tls_test.go`:
- Around line 570-573: Strengthen the assertion around the mtlsNoCertPodName
failure by inspecting the failed container’s termination reason and its logs,
rather than asserting only the Kubernetes Pod phase. Require the output to
contain the expected TLS client-certificate/authClients=Required error, while
preserving the existing failure expectation and timeout handling.
---
Outside diff comments:
In `@test/e2e/valkeycluster_tls_test.go`:
- Around line 502-523: The mTLS replication test must provision an enabled ACL
user matching the server certificate CN
valkey-cluster-mtls.default.svc.cluster.local before validating replication.
Update the test setup around the replication case to add or configure that
internal CN-based user, while preserving alice for regular client
authentication.
- Around line 476-483: Update the Secret volume definitions in the client pod
manifests generated by the relevant test setup blocks to project only
server-tls/ca.crt from the server certificate Secret via Secret items. Apply
this at both server-tls volume definitions, while preserving the separate
client-tls volume mounts unchanged because client commands require the
certificate and key.
- Around line 384-392: Update the certificate readiness waits in the test setup
before creating the ValkeyCluster to wait for both leaf Certificate resources,
not just their Secrets. In the Eventually blocks around serverCertSecret and
clientCertSecret, use kubectl wait for condition=Ready on each corresponding
certificate name with an appropriate timeout, while preserving the existing
retry behavior.
- Around line 346-370: Update the client Certificate manifest for the
certificate with commonName alice to declare TLS Web Client Authentication in
its usages. If the server certificate is shared for client authentication, also
declare both TLS Web Server Authentication and TLS Web Client Authentication on
the valkey certificate, preserving its existing server usage.
---
Duplicate comments:
In `@test/e2e/valkeycluster_tls_test.go`:
- Around line 225-232: Restore explicit timeout and polling arguments on every
Eventually wait in the TLS suite, including the pod readiness block around the
visible kubectl command and the mTLS readiness, configuration, primary lookup,
and replication waits identified by the comment. Choose values that accommodate
external Kubernetes/Valkey operations, including the replication callback’s WAIT
1 5000 duration, rather than relying on Gomega defaults.
---
Nitpick comments:
In `@test/e2e/valkeycluster_tls_test.go`:
- Around line 394-413: Update the mTLS ValkeyCluster fixture in the test around
clusterManifest to enable the metrics exporter under authClients: Required and
authClientsUser: CN, then verify the exporter connects with the required client
credentials and reports redis_up=1. Reuse the existing exporter test helpers and
credential configuration rather than creating unrelated coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f678663a-acd9-4135-8af7-34132b75c9a4
📒 Files selected for processing (8)
api/v1alpha1/valkeycluster_types.goconfig/crd/bases/valkey.io_valkeyclusters.yamlconfig/crd/bases/valkey.io_valkeynodes.yamldocs/mtls.mddocs/valkeycluster.mdinternal/controller/config.gointernal/controller/config_test.gotest/e2e/valkeycluster_tls_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- config/crd/bases/valkey.io_valkeynodes.yaml
- internal/controller/config_test.go
- internal/controller/config.go
- config/crd/bases/valkey.io_valkeyclusters.yaml
| With `authClients: Required` + `authClientsUser: CN`, any TLS client whose certificate has `CN=alice` is automatically authenticated as the ACL user `alice` -- no `AUTH` command required. Pass `resetpass: true` with this configuration so authentication relies exclusively on the client certificate. | ||
|
|
||
| With `authClients: Required`, Valkey requires a valid client certificate at the TLS handshake, but that does not disable password-based ACL authentication. Clients can still authenticate with `AUTH` as long as they present a client certificate signed by the configured CA. Today operator user, health check probes, redis exporter all present the server certificate to satisfy this. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'tls-auth-clients-user|tls-auth-clients|client auth|server certificate|health|exporter|replication' \
--glob '*.go' --glob '*.md' .Repository: valkey-io/valkey-operator
Length of output: 50384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/mtls.md relevant lines =="
sed -n '30,42p' docs/mtls.md
echo
echo "== TLS client/auth config constants and usage =="
rg -n -C 4 'TLSAuthClientsUser|authClientsUser|tls-auth-clients-user|tls-auth-clients|AuthClients|Certificate.*tls-auth|client cert|ClientTLS|tls-cert-file|tls-key-file' internal cmd api docs --glob '*.go' --glob '*.md'
echo
echo "== replica TLS config references =="
rg -n -C 4 'tls-replication|cluster-tls|replica.*tls|tls-client' internal docs --glob '*.go' --glob '*.md'
echo
echo "== certificate extension / key usage generation references =="
rg -n -C 4 'ExtendedKeyUsage|KeyUsage|ExtendedKeyUsageClientAuth|server certificate|client certificate|client auth|TLSAuth|authClients' internal controller internal valkey api docs --glob '*.go' --glob '*.md'Repository: valkey-io/valkey-operator
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== users.go outline =="
ast-grep outline internal/controller/users.go --view expanded || true
echo
echo "== users.go relevant sections =="
sed -n '1,260p' internal/controller/users.go
echo
echo "== cluster/client TLS client references =="
rg -n -C 4 'NewClient|Client(\(|[[:space:]]|:)|ClientTLS|Username|Password\b|User|Password|client-go|InClusterConfig|ServiceAccount' internal/valkey internal/controller/valkeycluster_controller.go internal/controller/valkeynode_controller.go internal/controller/metrics_exporter.go --glob '*.go'
echo
echo "== buildClusterClient definitions/usages =="
rg -n -C 6 'func .*build.*Client|Build.*Client|buildNodeClientOption|ClusterClient|NodeClient' internal --glob '*.go'Repository: valkey-io/valkey-operator
Length of output: 35853
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== read-only verifier: TLS auth mapping and internal client identities =="
python3 - <<'PY'
from pathlib import Path
import re
docs = Path("docs/mtls.md").read_text()
users = Path("internal/controller/users.go").read_text()
config = Path("internal/controller/config.go").read_text()
checks = {
"docs states replica links require internal client auth": bool(re.search(r"tls-replication yes|replica|TLS clients|certificate", config) and re.search(r"replica|Replicas|replication|Replica", users)),
"docs states server certificate is reused by operator/probes/exporter": bool(re.search(r"operator user, health check probes, redis exporter all present the server certificate", docs)),
"docs mentions client auth usage requirement": bool(re.search(r"\bclient auth\b|client certificate|cert-mapped|CN/URI", docs)),
"ACL system users include internal mTLS targets": bool(re.search(r"_operator\b", users) and re.search(r"_exporter\b", users) and re.search(r"_replication\b", users)),
"server renders tls-auth-clients-user from API": bool(re.search(r'tls-auth-clients-user"\s*= directive', config)),
}
for name, ok in checks.items():
print(f"{name}: {ok}")
print("docs internal client mention count:", len(re.findall(r".*operator user, health check probes, redis exporter all present the server certificate to satisfy this\.", docs)), "expected:", 1)
print("docs mentions client auth client-certificate requirements:", bool(re.search(r"client auth|client-certificate|certificate-to-user|CN/URI|rotation|server certificate", docs)))
PY
echo
echo "== focused source excerpts =="
sed -n '560,615p' internal/controller/valkeynode_controller.go
sed -n '70,95p' internal/controller/config.go
sed -n '40,105p' internal/controller/users.go
sed -n '35,45p' docs/mtls.mdRepository: valkey-io/valkey-operator
Length of output: 7752
🌐 Web query:
Valkey 9.0 tls-auth-clients-user ACL client certificate authentication tls-replication
💡 Result:
In Valkey 9.0, the tls-auth-clients-user configuration directive enables automatic authentication of TLS clients as Valkey ACL users based on fields extracted from their client certificates [1][2][3]. When this feature is enabled, the server inspects the client's TLS certificate during the handshake and attempts to map a specific field to an existing Valkey ACL user [2][4]. If a match is found, the connection is automatically authenticated as that user, eliminating the need for an explicit AUTH command [2][3]. If no matching user is found, the connection defaults to the unauthenticated default user [1][3]. Key details include: - Configuration Options: Supported values for tls-auth-clients-user include CN (Common Name from the certificate's Subject field), URI (URI from the certificate's Subject Alternative Name), and off (the default) to disable the feature [4][5]. - ACL Integration: It is recommended to configure the associated ACL users without passwords (e.g., using the nopass directive) when using this feature [3][6][4]. This ensures that authentication is enforced exclusively through mTLS certificates [3][4]. - Replication: Valkey treats replica servers and standard clients in the same way regarding TLS connections [7]. To use TLS for replication, the replica must be configured with tls-replication yes [7][8]. If tls-auth-clients-user is enabled on the primary, a replica presenting a certificate with a valid CN or URI that matches a Valkey user (configured with appropriate replication permissions) will be automatically authenticated as that user [3][4]. This feature simplifies mTLS-based infrastructure integration and enhances security by tightly coupling certificate identity with Valkey's Access Control List (ACL) system [2]. If authentication fails via this method, it may be logged in the ACL log with the reason tls-cert, and failures are tracked in the INFO stats under acl_access_denied_tls_cert [2].
Citations:
- 1: https://github.com/valkey-io/valkey/blob/9.0/valkey.conf
- 2: Add support for automatic client authentication via TLS certificate fields valkey#1920
- 3: https://valkey.io/topics/tls/
- 4: https://github.com/valkey-io/valkey/blob/fa239232/valkey.conf
- 5: https://github.com/valkey-io/valkey/blob/unstable/valkey.conf
- 6: https://valkey.io/topics/acl/
- 7: https://www.mankier.com/7/valkey-encryption
- 8: https://www.percona.com/blog/valkey-redis-setting-up-replication/
Document internal TLS client identity requirements.
With authClients: Required + authClientsUser, operator connections, health checks, the exporter, and tls-replication links are TLS clients. The identity mapped from their certificate must match an enabled ACL user with the required permissions, and rotating a reused server certificate must preserve that CN or URI. This prevents internal links from becoming unauthenticated or authenticating as the wrong ACL user.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/mtls.md` around lines 38 - 40, Update the mTLS documentation around the
authClients/authClientsUser guidance to explicitly state that operator
connections, health checks, the exporter, and tls-replication links must present
certificates whose mapped CN or URI matches an enabled ACL user with the
required permissions. Also document that reused server-certificate rotation must
preserve this identity to avoid internal authentication failures or incorrect
ACL mapping.
Source: MCP tools
| tls-auth-clients "yes" # rendered from authClients: Required | ||
| tls-auth-clients-user CN/URI # rendered from authClientsUser: CN or authClientsUser: URI |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | sed -n '1,200p' | rg 'docs/mtls.md|mtls|tls-auth|config.c|valkey|docs' || true
echo "== docs/mtls.md lines 50-75 =="
if [ -f docs/mtls.md ]; then
nl -ba docs/mtls.md | sed -n '50,75p'
fi
echo "== schema/controller references =="
rg -n "tls-auth-clients|authClients|authClientsUser|tls.auth.clients|authClientsUser" -S . --glob '!**/.git/**' --glob '!**/vendor/**' --glob '!**/node_modules/**' | sed -n '1,220p'Repository: valkey-io/valkey-operator
Length of output: 2172
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== docs/mtls.md lines 50-75 =="
awk 'NR>=50 && NR<=75 {printf "%6d\t%s\n", NR, $0}' docs/mtls.md
echo "== tls/auth config references =="
rg -n "tls-auth-clients|tls-auth-clients-user|authClients|authClientsUser|tls.auth.clients|AuthClients" -S internal api docs --glob '!**/.git/**' | sed -n '1,260p'
echo "== relevant source slices =="
python3 - <<'PY'
import os, pathlib
for p in pathlib.Path('.').rglob('*'):
if p.is_file() and p.suffix in {'.go','.yaml','.md'}:
try:
txt=p.read_text(errors='ignore')
except: continue
if any(s in txt for s in ['tls-auth-clients','authClientsUser','authClients:']):
print(f'\n--- {p} ---')
lines=txt.splitlines()
for i,l in enumerate(lines,1):
if any(s in l for s in ['tls-auth-clients','authClientsUser','authClients:', 'CN', 'URI']):
lo=max(1,i-4); hi=min(len(lines),i+5)
for n in range(lo,hi+1):
print(f'{n:5d}\t{lines[n-1]}')
PYRepository: valkey-io/valkey-operator
Length of output: 50383
Show the exact controller-rendered directive values.
The rendered config block shows tls-auth-clients "yes" and tls-auth-clients-user CN/URI, but the operator renders tls-auth-clients yes and separate CN or URI values. Use matching unquoted values and list the two tls-auth-clients-user alternatives separately.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/mtls.md` around lines 64 - 65, Update the rendered configuration example
in the mTLS documentation to match the operator output: remove quotes from the
tls-auth-clients value and show separate tls-auth-clients-user directives for CN
and URI instead of the combined CN/URI placeholder.
Source: MCP tools
| To enforce strict mTLS authentication: | ||
|
|
||
| Always set `resetpass: true` instead. This flushes all passwords and disables `nopass`, making password-based `AUTH` impossible. The user can then only be authenticated via the CN/URI from the client certificate. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 \
'user default|nopass|resetpass|tls-auth-clients-user|ACL WHOAMI' \
--glob '*.go' --glob '*.md' .Repository: valkey-io/valkey-operator
Length of output: 19977
🌐 Web query:
Valkey TLS mTLS tls-auth-clients-user unmatched certificate identity default user documentation
💡 Result:
In Valkey, the tls-auth-clients-user configuration directive allows the server to automatically authenticate clients as specific ACL users based on fields extracted from their presented TLS certificates [1][2][3]. When this feature is enabled, the server performs the following logic during the TLS handshake [2][3]: 1. It extracts the specified field (e.g., Common Name or Subject Alternative Name URI) from the client's TLS certificate [2][4][3]. 2. It attempts to find a matching Valkey ACL user whose username corresponds to that extracted value [2][3]. 3. If a match is found and the user is valid, the client is automatically authenticated as that user [2][3]. 4. If no matching user is found, or if the client certificate authentication fails to map to an enabled user, the client is not authenticated as the intended user and instead remains as the default user [1][2][3]. As a result, an unmatched certificate identity means the client will proceed as the default user [3]. This behavior is why it is recommended to configure users without passwords (e.g., ACL SETUSER on allcommands allkeys) when using this feature; this ensures that authentication is enforced strictly through the mTLS certificate identity rather than falling back to potentially permissive default user settings [1][3]. Supported configuration values for tls-auth-clients-user include: - CN: Uses the Common Name from the certificate's Subject field [2][3]. - URI: Uses the first URI entry from the certificate's Subject Alternative Name (SAN) extension [4][3]. - off: Disables the feature (default) [2][3]. Administrators can monitor these events using the INFO field acl_access_denied_tls_cert, which tracks the number of failed certificate-based authentication attempts [2].
Citations:
- 1: https://valkey.io/topics/tls/
- 2: Add support for automatic client authentication via TLS certificate fields valkey#1920
- 3: https://github.com/valkey-io/valkey/blob/fa239232/valkey.conf
- 4: Support TLS authentication using SAN URI valkey#3078
🌐 Web query:
Valkey client X509 authentication no matching user default user
💡 Result:
The message indicating "no matching user" in Valkey occurs when the server is configured to automatically authenticate clients using TLS certificate fields (via the tls-auth-clients-user configuration directive), but it cannot find a Valkey ACL user that matches the extracted value from the client's certificate [1][2][3]. When this happens, the following behavior occurs: 1. Automatic Authentication Failure: The TLS handshake completes successfully (assuming mTLS is configured), but the automatic association of the connection with a specific ACL user fails [1][2][3]. 2. Default User Fallback: The connection remains associated with the "default" user [1][2][3]. If the default user is not configured to be automatically authenticated (e.g., via nopass), the client will remain in an unauthenticated state and must perform a manual authentication step (e.g., using the AUTH command) to access restricted functionality [1][3][4]. 3. Logging: This event is recorded in the ACL log with the reason "tls-cert," which helps administrators identify certificate-based authentication failures [2][5]. To resolve this issue, ensure that a Valkey user exists with a name that exactly matches the field being extracted from the certificate (e.g., the Common Name or a URI from the SAN extension) [6][2][7]. You can verify your current configuration and create the necessary users using the ACL SETUSER command [1][8]. Additionally, if you intend for certificate authentication to be the only method for a specific user, you can use the resetpass command to remove existing passwords and ensure the user is authenticated exclusively via their certificate [4].
Citations:
- 1: https://valkey.io/topics/tls/
- 2: Add support for automatic client authentication via TLS certificate fields valkey#1920
- 3: https://github.com/valkey-io/valkey/blob/0f5db6a5/valkey.conf
- 4: https://valkey.io/topics/acl/
- 5: https://valkey.io/commands/acl-log/
- 6: Support TLS authentication using SAN URI valkey#3078
- 7: [NEW] Take username from client certificate valkey#1866
- 8: https://www.mankier.com/3/auth.3valkey
Do not claim that resetpass alone enforces strict mTLS.
resetpass removes passwords and nopass from the named ACL user. It does not reject a valid certificate whose CN or URI has no matching user; Valkey leaves that connection on the default user. If the default user remains privileged or otherwise accessible, an unknown certificate identity can still reach default-user permissions. Restrict the default user or document this prerequisite and test an unmatched CN/URI certificate.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/mtls.md` around lines 134 - 136, Update the strict mTLS guidance to
state that resetpass alone is insufficient: require restricting the default user
or document that prerequisite, and add coverage for a certificate with an
unmatched CN/URI to verify it cannot access default-user permissions.
Source: MCP tools
| _, err = utils.Run(exec.Command("kubectl", "wait", fmt.Sprintf("pod/%s", mtlsNoCertPodName), | ||
| "--for=jsonpath={.status.phase}=Failed", "--timeout=120s")) | ||
| Expect(err).NotTo(HaveOccurred(), | ||
| "client without a certificate should fail under authClients=Required") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the failure reason, not only the Pod phase.
A Failed phase does not prove that the missing certificate caused the failure. Image, DNS, CA, and command errors can also fail the pod. Inspect the container termination reason and logs, and require the expected TLS client-certificate error.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/e2e/valkeycluster_tls_test.go` around lines 570 - 573, Strengthen the
assertion around the mtlsNoCertPodName failure by inspecting the failed
container’s termination reason and its logs, rather than asserting only the
Kubernetes Pod phase. Require the output to contain the expected TLS
client-certificate/authClients=Required error, while preserving the existing
failure expectation and timeout handling.
|
@sandeepkunusoth Can you help me understand how this ties in with the wider TLS API design to be able to support additional TLS configurations like split-TLS configurations (see this comment too). |
4 participants
This PR closes (#243)
Summary
This PR extends the
spec.tlsconfiguration options forValkeyClusterresources to support client certificate enforcement (mTLS) and automatic user mapping based on the client certificate's Common Name (CN).Features
Implementation
TLSConfiginapi/v1alpha1/valkeycluster_types.gobacked by validated string enums:authClients: Optional / Required / Disabled (Defaults to Optional). Maps to Valkey directive tls-auth-clients.authClientsUser:CN/DNS/Disabled(Defaults toDisabled). Maps to Valkey directivetls-auth-clients-user.Testing
test/e2e/valkeycluster_tls_test.gorunning a scenario that validates successful mTLS connections and tests using client certificate.Checklist
Before submitting the PR make sure the following are checked:
pre-commit run --all-filesor hooks on commit)