feat(helm): optional mTLS Kafka support for watchers (KafkaUser + client-cert mounts), default off

[bborbe]

Summary

Mirrors the bborbe/agent chart 0.4.0 mTLS change for the maintainer watchers. Adds optional mTLS Kafka (default off — plaintext, quant unaffected). When watchers[].kafkaUser.enabled: true, the chart emits a Strimzi KafkaUser CR (type: tls) in strimziNamespace and mounts the client cert/key + cluster CA at the fixed /client-cert/file, /client-key/file, /server-cert/file paths github.com/bborbe/kafka reads for tls:// brokers. New per-watcher values kafkaUser.{enabled,cluster,strimziNamespace,userName,clientSecret,caCertSecret}; secrets referenced by name only. Chart 0.1.0 → 0.2.0. Unblocks the Octopus per-stage-Strimzi (mTLS) watcher deploy.

Test plan

• helm lint clean; make precommit green
• enabled: true → 1 KafkaUser CR (type: tls, strimzi ns, derived name <ns>-<watcher>) + 3 cert mounts + correct client/CA secret refs
• enabled: false (default): zero KafkaUser, zero cert mounts
• Default render byte-identical to master (only helm.sh/chart label) → plaintext/quant unaffected

[ben-s-pull-request-reviewer]

{
  "verdict": "request-changes",
  "summary": "The helm mTLS Kafka feature is well-designed and the default-off approach preserves backwards compatibility. However, there is a critical security issue: the secret volumes use `defaultMode: 420` (octal 0644) which makes mounted private keys world-readable. Additionally, the new KafkaUser CR and conditional cert rendering lack test coverage.",
  "comments": [
    {
      "file": "helm/templates/_helpers.tpl",
      "line": 35,
      "severity": "critical",
      "message": "security: `defaultMode: 420` (octal 0644) on the client-key secret volume makes the private key world-readable (rw-r--r--). For a TLS private key this is a severity finding. Change to `256` (octal 0400, rw-------). The other two volumes (client-cert, server-cert) are not sensitive but should be consistent."
    },
    {
      "file": "helm/templates/_helpers.tpl",
      "line": 42,
      "severity": "critical",
      "message": "security: same `defaultMode: 420` issue as line 35 — private key world-readable. Must be `256`."
    },
    {
      "file": "helm/templates/_helpers.tpl",
      "line": 49,
      "severity": "critical",
      "message": "security: same `defaultMode: 420` issue — consistent with lines 35 and 42, fix all three together."
    },
    {
      "file": "helm/templates/watcher-kafkauser.yaml",
      "line": 14,
      "severity": "major",
      "message": "correctness: KafkaUser default name `printf \"%s-%s\" (include \"maintainer.namespace\" $) $watcher.name` produces \"{namespace}-{watchername}\". If namespace is itself hyphenated (e.g. \"my-namespace\"), the resulting name has double hyphens. While RFC-1123 allows this, it can cause visual confusion and tooling issues. Consider using a more distinctive separator or validating the concatenation produces a valid DNS label (lowercase alphanumeric + single hyphens, not starting/ending with hyphen)."
    },
    {
      "file": "helm/templates/watcher-kafkauser.yaml",
      "line": 1,
      "severity": "major",
      "message": "tests: new KafkaUser CR template has no helm unittest coverage. Consider adding `helm/tests/watcher-kafkauser_test.yaml` that verifies: (1) KafkaUser is NOT rendered when `kafkaUser.enabled: false`, (2) KafkaUser IS rendered with correct name/namespace/labels when `kafkaUser.enabled: true`, (3) default name derivation when `userName` is empty."
    },
    {
      "file": "helm/templates/watchers.yaml",
      "line": 107,
      "severity": "major",
      "message": "tests: the conditional cert volumeMounts and volumes have no test coverage. A helm unittest should verify they are NOT present when `kafkaUser.enabled: false` and ARE present with correct structure when enabled."
    }
  ],
  "concerns_addressed": [
    "security: defaultMode 420 (octal 0644) makes mounted cert/key files world-readable; consider 256 (octal 0400) for private key — RAISED as critical finding, must fix",
    "security: kafkaUser secrets referenced by name only — ACKNOWLEDGED. Chart correctly documents this; external syncer responsibility is documented in comments. No fix needed in chart.",
    "correctness: KafkaUser name RFC-1123 compliance — RAISED as major finding. Default name derivation is technically valid but double-hyphen edge case exists.",
    "correctness: changelog vs Chart.yaml version consistency — VERIFIED: both show 0.2.0. CHANGELOG entry correctly says \"Chart 0.1.0 → 0.2.0\" and `Chart.yaml` shows `version: 0.2.0`. No inconsistency.",
    "tests: no helm unittest coverage for new KafkaUser CR — RAISED as major finding, should add tests"
  ]
}

[bborbe]

Addressed the CRITICAL: cert volume defaultMode 420 (0644) → 288 (0440) — drops world-read on the private key. Note the suggested 256 (0400) would break these pods: secret files are owned root:fsGroup, watchers run runAsUser: 65534 + fsGroup: 65534, so 0400 (owner-only) denies the non-root process; 0440 keeps group-read via fsGroup. (The bborbe/agent chart gets the same fix for parity.)

On the two MAJORs (non-blocking):

• helm unittest coverage — the chart has no helm-unittest harness (neither this nor the agent chart); verified instead via helm template render checks (enabled → 1 KafkaUser type: tls + 3 cert mounts; disabled → zero; default render byte-identical to master). A test harness is a separate follow-up, not this PR.
• double-hyphen in derived name — non-issue: <namespace>-<watcher> yields single hyphens (agent-github-pr-watcher); a double only arises from already-invalid RFC-1123 inputs.

superseded by new automated review

[ben-s-pull-request-reviewer]

Now I have all the changed files. Let me analyze them against the PR's stated concerns.

Analysis of Changed Files:

1. helm/templates/watcher-kafkauser.yaml — KafkaUser CR

• apiVersion: kafka.strimzi.io/v1beta2 ✓
• kind: KafkaUser ✓
• authentication.type: tls ✓
• Name derivation matches watchers.yaml line 115: printf "%s-%s" (include "maintainer.namespace" $) $watcher.name
• strimzi.io/cluster label correctly references $ku.cluster
• Secrets are NOT embedded — Strimzi issues them, external syncer places them

2. helm/templates/watchers.yaml — Client cert/key/CA mounts

• Line 107-109: conditional if (($watcher.kafkaUser | default dict).enabled) — correctly gates the volumeMounts ✓
• Lines 113-117: conditional volumes section uses same pattern ✓
• secretName references only (no inline values) ✓
• defaultMode: 288 (decimal) = 0440 octal — owner+group read, no world read ✓

3. helm/templates/_helpers.tpl — New helpers

• maintainer.kafkaCertVolumeMounts: hardcoded fixed paths /client-cert/file, /client-key/file, /server-cert/file — matches what github.com/bborbe/kafka reads for tls:// brokers ✓
• maintainer.kafkaCertVolumes: uses .clientSecret and .caCertSecret from the dict arg; items correctly project user.crt→file, user.key→file, ca.crt→file ✓
• nil-safe via default dict pattern in callers ✓

4. helm/values.yaml — New kafkaUser block

• enabled: false default ✓
• All secret names referenced by string only (no inline base64) ✓
• userName defaults to derived namespace-watcherName when empty ✓
• caCertSecret default my-cluster-cluster-ca-cert is a reference name, not inline ✓

5. helm/Chart.yaml — version 0.1.0 → 0.2.0 ✓

6. CHANGELOG.md — Accurate description of the feature ✓

No issues found. The implementation correctly:

• References secrets by name only (Strimzi issues, external syncer places)
• Conditionally adds cert volumes only when kafkaUser.enabled=true
• Uses nil-safe default dict patterns throughout
• Defaults to enabled: false (backwards-compatible, renders byte-identical for plaintext clusters)
• Uses correct defaultMode: 288 (0440 octal) for fsGroup pod access

{
  "verdict": "approve",
  "summary": "The mTLS Kafka feature is correctly implemented: secrets are referenced by name only (Strimzi issues, external syncer places), cert volumes are conditionally added only when kafkaUser.enabled=true, nil-safe default patterns are used throughout, and the default renders byte-identical to before for plaintext clusters.",
  "comments": [],
  "concerns_addressed": [
    "security: Strimzi KafkaUser spec correct (type: tls) in watcher-kafkauser.yaml — no inline secrets",
    "security: Client cert/key and CA cert mounts in watchers.yaml reference secrets by name only — no inline values",
    "correctness: Certificate mounts are conditionally added only when kafkaUser.enabled=true (watchers.yaml:107-109, 113-117)",
    "correctness: New helper templates in _helpers.tpl use nil-safe default dict pattern, handle empty values gracefully",
    "correctness: values.yaml defaults are correct — enabled: false is intentional, secrets are name references"
  ]
}