cockroachdb / cockroachdb/cockroach-operator

Incomplete external TLS Secret pairs are accepted for secure clusters

Open Beginner friendly
#1,160 0 comments 0 reactions 0 assignees View on GitHub
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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.