Skip to content

Allow overriding Secret data keys for the custom transport CA#9444

Open
xrl wants to merge 1 commit into
elastic:mainfrom
xrl:feature/transport-ca-secret-key-override
Open

Allow overriding Secret data keys for the custom transport CA#9444
xrl wants to merge 1 commit into
elastic:mainfrom
xrl:feature/transport-ca-secret-key-override

Conversation

@xrl

@xrl xrl commented May 16, 2026

Copy link
Copy Markdown

Problem

The custom transport CA Secret parser in pkg/controller/common/certificates/ca_secret.go hardcodes ca.crt/ca.key (with a legacy tls.crt/tls.key fallback) and explicitly rejects Secrets that contain both sets of keys as a configuration error.

cert-manager Certificate resources with isCA: true always populate their output Secret with all three of tls.crt, tls.key, and ca.crt (the latter being the issuing CA bundle). cert-manager does not allow renaming these data keys — secretTemplate only customizes Secret metadata, and additionalOutputFormats only appends extra fixed-name keys.

The combined effect is that ECK cannot consume a cert-manager-emitted Secret as the user-provided transport CA: the parser sees tls.* + ca.crt co-existing and returns both tls.* keys and ca.* keys exist in secret <ns>/<name>, this is likely a configuration error. The current workaround is to generate a CA outside the cluster with openssl and kubectl create secret generic it manually, which defeats the purpose of running cert-manager.

Change

Add an optional caSecretKeys field on TransportTLSOptions with caCertKey and caKeyKey string overrides. When either is set:

  • The parser reads only the user-named keys.
  • Both the legacy/modern auto-detection and the "both exist" conflict check are bypassed.

When caSecretKeys is unset (the default), behavior is identical to today.

spec:
  transport:
    tls:
      certificate:
        secretName: my-cert-manager-emitted-secret
      caSecretKeys:
        caCertKey: tls.crt
        caKeyKey: tls.key

Implementation notes

  • ParseCustomCASecret is preserved as a thin wrapper over the new ParseCustomCASecretWithKeys(s, certKeyOverride, keyKeyOverride). This keeps every other caller untouched and minimizes the diff against the existing auto-detect logic, which still runs verbatim when no overrides are supplied.
  • ReconcileOrRetrieveCA reads the overrides off the spec nil-safely and forwards them.
  • CRD YAML, Helm chart CRD copy, and zz_generated.deepcopy.go were regenerated with make generate-manifests.

Tests

Added TestParseCustomCASecretWithKeys in pkg/controller/common/certificates/ca_secret_test.go covering:

  1. Empty overrides → identical to ParseCustomCASecret.
  2. Both overrides set → reads tls.* even when ca.crt is also present (the cert-manager shape).
  3. Only cert override set → key falls back to ca.key.
  4. Only key override set → cert falls back to ca.crt.
  5. Override pointing at missing key → returns the existing "can't find X in Y/Z" error.
  6. Override skips the both-exist conflict check.

Existing TestParseCustomCASecret cases are unchanged and continue to pass.

Checklist

  • PR is against main.
  • gofmt clean, go vet clean, unit tests pass for the touched packages.
  • License headers preserved on all modified files.
  • Contributor License Agreement — will sign separately if not already on file.

The transport CA secret parser hardcodes `ca.crt`/`ca.key` (with a legacy
`tls.crt`/`tls.key` fallback) and rejects secrets that contain both sets of
keys as a configuration error. cert-manager always emits `tls.crt`, `tls.key`,
*and* `ca.crt` (the issuing CA bundle) in its output Secret, and does not
allow renaming these keys (`secretTemplate` only affects metadata,
`additionalOutputFormats` only adds extra keys). The "both exist" check thus
makes it impossible to point ECK directly at a cert-manager `Certificate`
that mints the transport CA.

Add an optional `caSecretKeys` field on `TransportTLSOptions` with `caCertKey`
and `caKeyKey` overrides. When either is set, the parser reads only the named
keys and bypasses the legacy/modern auto-detection and the "both exist"
conflict check. When unset, behavior is unchanged.

The new `ParseCustomCASecretWithKeys` is invoked from `ReconcileOrRetrieveCA`;
the original `ParseCustomCASecret` remains as a no-override wrapper so other
callers stay untouched.
@xrl xrl requested a review from a team as a code owner May 16, 2026 03:24
@prodsecmachine

prodsecmachine commented May 16, 2026

Copy link
Copy Markdown
Collaborator

Snyk checks have passed. No issues have been found so far.

Status Scan Engine Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues
Licenses 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@botelastic botelastic Bot added the triage label May 16, 2026
@xrl

xrl commented May 16, 2026

Copy link
Copy Markdown
Author

I signed the CLA 👌

@pkoutsovasilis

Copy link
Copy Markdown
Contributor

Hi @xrl 👋 thank you for the detailed write-up and for taking the time to contribute a PR for this - the problem is real and the cert-manager use case is a valid one.

After looking at the change, I think there's a simpler path that avoids the need for a new API field altogether. As @pebrc noted in #8617, the root cause is that the conflict check in ParseCustomCASecret was written before the cert-manager scenario was considered, and it's being overly strict.

A cert-manager Certificate with isCA: true emits tls.crt + tls.key + ca.crt (but no ca.key), so there's no real ambiguity - the tls.* keys are the right ones to use. I do believe that a simpler approach is to alter the conflict guard so it only fires when both complete sets are present:

// before
if (legacyKeyExists || legacyCrtExists) && (keyExists || crtExists) {

// after
if (legacyKeyExists && legacyCrtExists) && (keyExists && crtExists) {

With this one-line change the cert-manager secret just works without any user configuration, and we avoid adding a caSecretKeys field to the public API.

Would you be open to rework the PR along those lines? The test cases you wrote are a great starting point - they could be adapted into TestParseCustomCASecret to cover the alternative approach I propose above. 🙂

@pebrc

pebrc commented May 18, 2026

Copy link
Copy Markdown
Collaborator

tl;dr what @pkoutsovasilis wrote (I had a stale PR view when I wrote mine)

Thanks for your contribution but I think we would not want to expose these as top level fields in the CRD. For this specific problem, surfacing caSecretKeys feels like exposing an implementation detail to users because the incompatibility is with a well-known Secret shape: cert-manager writes tls.crt, tls.key, and ca.crt. The parser could accept that shape without adding CRD surface area.

A safer auto-detection rule might be:
• Use ca.crt + ca.key when both exist.
• Use tls.crt + tls.key when both exist and ca.key is absent, even if ca.crt is present.
• Keep rejecting truly ambiguous cases, especially when both private keys exist (ca.key and tls.key).

That preserves the original ambiguity guard while accepting cert-manager’s CA Certificate output. It also avoids a permanent public API field, generated CRD/docs churn, and future questions like validation of arbitrary Secret key names.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants