cockroachdb / cockroachdb/cockroach-operator
Incomplete external TLS Secret pairs are accepted for secure clusters
- Dominant language
- Go
- Stars
- 318
- Forks
- 104
- Avg merge
- 1d 6h
- Merged PRs (30d)
- 1
Description
## Environment
- Kubernetes: local kind cluster
- Operator image: `cockroachdb/cockroach-operator:v2.18.3`
- CockroachDB image: `cockroachdb/cockroach:v25.2.12`
- Test topology: 3-node secure `CrdbCluster`
## What Happened
The operator lets users provide external TLS Secrets for secure CockroachDB clusters, but it treats the `node TLS Secret` and the `client/root TLS Secret` as independent optional fields. This is not safe, because either the user provides both or the operator generates both.
If the user sets `tlsEnabled=true` and provides only `nodeTLSSecret`, the operator accepts the CR, skips generated certificate creation because `NodeTLSSecret` is non-empty, and still writes a secure StatefulSet that mounts both a node Secret and a client/root Secret. The node Secret exists, but the client/root Secret falls back to the generated name and does not exist:
```text
node secret = external-node-tls
client/root secret = -root
client/root exists = false
```
The pod then fails before `cockroach start` can run:
```text
MountVolume.SetUp failed for volume "certs" : secret "-root" not found
```
On the other hand, if the user sets `tlsEnabled=true` and provides only `clientTLSSecret`, the operator accepts the CR, generates node TLS material because `NodeTLSSecret` is empty, and then writes a secure StatefulSet that combines generated node certs with the external client/root Secret:
```text
node secret source = generated by operator
client/root secret source = external user-provided Secret
```
That is also an incomplete external TLS configuration. The operator should reject it before writing the secure workload, rather than silently mixing generated and external TLS.
## Expected Behavior
For `tlsEnabled=true`, external TLS Secrets should be all-or-nothing:
```text
nodeTLSSecret empty, clientTLSSecret empty -> OK, operator generates both
nodeTLSSecret set, clientTLSSecret set -> OK, user provides both
nodeTLSSecret set, clientTLSSecret empty -> reject
nodeTLSSecret empty, clientTLSSecret set -> reject
```
The best fix location is the admission webhook, because the CR itself is invalid before reconcile reaches certificate generation or StatefulSet construction. The check should run in both `ValidateCreate` and `ValidateUpdate`, because both create and update can produce the bad configuration.
## Where The Source Code Goes Wrong
The validating webhook already checks ingress, CockroachDB version, and volume mode, but it does not validate the TLS Secret pair. `ValidateCreate` starts at `apis/v1alpha1/webhook.go` and returns success without checking whether `NodeTLSSecret` and `ClientTLSSecret` are set together: [`apis/v1alpha1/webhook.go`](https://github.com/cockroachdb/cockroach-operator/blob/v2.18.3/apis/v1alpha1/webhook.go#L95-L119). `ValidateUpdate` has the same issue: [`apis/v1alpha1/webhook.go`](https://github.com/cockroachdb/cockroach-operator/blob/v2.18.3/apis/v1alpha1/webhook.go#L121-L154).
After admission accepts the incomplete CR, the certificate actor and StatefulSet builder make the two bad variants possible. First, generated certificate creation is skipped whenever `NodeTLSSecret` is set:
```go
if !cluster.Spec().TLSEnabled || cluster.Spec().NodeTLSSecret != "" {
log.V(DEBUGLEVEL).Info("Skipping TLS cert generation", "enabled", cluster.Spec().TLSEnabled, "secret", cluster.Spec().NodeTLSSecret)
return nil
}
```
Source: [`pkg/actor/generate_cert.go`](https://github.com/cockroachdb/cockroach-operator/blob/v2.18.3/pkg/actor/generate_cert.go#L68-L72).
As a result, with only external `nodeTLSSecret`, generated cert creation is skipped, so the fallback generated client/root Secret is never created.
Second, the secure StatefulSet always projects both a node Secret and a client/root Secret into the `certs` volume:
```go
Secret: &corev1.SecretProjection{
LocalObjectReference: corev1.LocalObjectReference{
Name: b.nodeTLSSecretName(),
},
...
}
...
Secret: &corev1.SecretProjection{
LocalObjectReference: corev1.LocalObjectReference{
Name: b.clientTLSSecretName(),
},
...
}
```
Source: [`pkg/resource/statefulset.go`](https://github.com/cockroachdb/cockroach-operator/blob/v2.18.3/pkg/resource/statefulset.go#L123-L150).
The builder chooses the two Secret names independently:
```go
func (b StatefulSetBuilder) nodeTLSSecretName() string {
if b.Spec().NodeTLSSecret == "" {
return b.Cluster.NodeTLSSecretName()
}
return b.Spec().NodeTLSSecret
}
func (b StatefulSetBuilder) clientTLSSecretName() string {
if b.Spec().ClientTLSSecret == "" {
return b.Cluster.ClientTLSSecretName()
}
return b.Spec().ClientTLSSecret
}
```
Source: [`pkg/resource/statefulset.go`](https://github.com/cockroachdb/cockroach-operator/blob/v2.18.3/pkg/resource/statefulset.go#L351-L365).
The node side and client/root side should be selected as one compatible bundle, not chosen separately. The mixed generated/external TLS material can create a secure StatefulSet whose certificates do not trust each other, so CockroachDB or operator-to-CockroachDB communication can fail later with TLS errors.
## Fix
The admission webhook should reject the invalid CR before the operator writes a broken secure StatefulSet. A minimal helper would be:
```go
func (r *CrdbCluster) ValidateTLSSecrets() error {
if !r.Spec.TLSEnabled {
return nil
}
nodeSet := r.Spec.NodeTLSSecret != ""
clientSet := r.Spec.ClientTLSSecret != ""
if nodeSet != clientSet {
return fmt.Errorf("nodeTLSSecret and clientTLSSecret must be set together when tlsEnabled is true")
}
return nil
}
```
Then `ValidateCreate` and `ValidateUpdate` should both append this error before returning success.
Contributor guide
No contributing guide indexed for this repository
Research direction
Start in apis/v1alpha1/webhook.go at ValidateCreate and ValidateUpdate, and review how the existing admission checks return validation errors. Verify the four tlsEnabled and Secret-field combinations described in the issue, including both create and update paths; done means incomplete pairs are rejected before reconciliation while both-empty and both-set configurations remain accepted.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go, kubernetes
- Domain
- devops, infrastructure, security
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 78/100