From b14dd1cee43838ba5ad83074ab33f64721db0acc Mon Sep 17 00:00:00 2001 From: Upayan Date: Tue, 6 Oct 2026 10:26:08 +0530 Subject: [PATCH] helm: drop fromToml dependency in security-configmap.yaml (fixes #11611) (#11614) * helm: drop fromToml dependency in security-configmap.yaml (fixes #11611) fromToml was added to Helm in v3.17.0 (helm/helm#12026, merged 2024-09-12, one day after v3.16.0 was cut). This chart declares no minimum Helm version (no Chart.yaml kubeVersion, nothing in the README), and the call in security-configmap.yaml:21 is an unconditional *parse*-time failure on Helm < v3.17.0 - Go's text/template parses a file's entire body before evaluating any {{if}}, so this breaks the chart (any topology, any values) even when securityConfigEnabled is false and the ConfigMap would render nothing. Replaces the fromToml-based dig lookup with a small regex-based helper (seaweedfs.existingTomlKey) that reads the same four "key = ..." JWT signing-key values out of a previously-rendered security.toml, preserving the existing fallback-to-random behavior exactly. Verified: - helm lint (v3.16.3 and v4.3.0): clean - helm template with chart defaults: byte-identical output to the unpatched chart rendered via Helm v4 (which has fromToml) - the disabled/default path is untouched - helm template with security enabled, no prior ConfigMap: identical structure to the unpatched chart (helm v4), modulo the expected random key - Real helm install + helm upgrade round trip (live lookup, since "helm template" never evaluates lookup, even under the original fromToml code): the JWT signing key is identical across both releases - confirms key persistence across upgrades is preserved, not just "renders without erroring" - helm template with chart defaults, Helm v3.16.3: previously failed with a parse error naming fromToml as undefined; now renders successfully Fixes #11611. * helm: harden existingTomlKey against commented key lines and CRLF Addresses two review findings from greptile-apps on PR #11614: - The key-line regex matched the first "key = ..." anywhere in the section block, including a commented-out "# key = ..." line, which would shadow a real active key on a hand-edited or otherwise non-chart-generated ConfigMap. Anchored to line start with the Go regexp multiline flag ((?m)^key...), which a line starting with "#" cannot match. - The section-header match required an exact "]\n", so a ConfigMap with CRLF line endings would fail to match the block at all and regenerate the key instead of reusing it. Changed to "]\r?\n". Also adds a CI test ("Verify JWT signing key persistence across upgrades") exercising all of this end to end with a real helm install -> edit the live ConfigMap -> helm upgrade cycle, matching the existing "Verify SFTP host key secret lifecycle" test's shape: both edge cases are reproduced against a real ConfigMap and asserted on the post-upgrade rendered security.toml. Verified locally (same commands as the new CI step) against a real cluster before pushing. * helm: preserve JWT keys across supported TOML layouts * ci: use setup-python interpreter for JWT upgrade checks * helm: preserve keys under quoted TOML section headers * helm: ignore unrelated quoted TOML section headers --- .github/scripts/helm_jwt_keys.py | 240 ++++++++++++++++++ .github/workflows/helm_ci.yml | 33 ++- k8s/charts/seaweedfs/README.md | 13 + .../seaweedfs/templates/shared/_helpers.tpl | 40 +++ .../templates/shared/security-configmap.yaml | 10 +- 5 files changed, 329 insertions(+), 7 deletions(-) create mode 100755 .github/scripts/helm_jwt_keys.py diff --git a/.github/scripts/helm_jwt_keys.py b/.github/scripts/helm_jwt_keys.py new file mode 100755 index 000000000..8b7998420 --- /dev/null +++ b/.github/scripts/helm_jwt_keys.py @@ -0,0 +1,240 @@ +#!/usr/bin/env python3 +"""Check JWT extraction and, with --context, real Helm upgrade persistence.""" + +import argparse +import base64 +import json +from pathlib import Path +import shutil +import subprocess +import sys +import tempfile +import uuid + +try: + import tomllib +except ModuleNotFoundError: # CI also exercises Python 3.10. + import tomli as tomllib + + +ROOT = Path(__file__).resolve().parents[2] +CHART = ROOT / "k8s/charts/seaweedfs" +SECTIONS = ("jwt.signing", "jwt.signing.read", "jwt.filer_signing", "jwt.filer_signing.read") +KEYS = {section: f"active-{index}" for index, section in enumerate(SECTIONS)} +ESCAPED_HEADER = '["jw\\u0074".signing]\nkey = "existing"' + + +def run(*args): + return subprocess.run(args, check=True, text=True, capture_output=True).stdout + + +def keys(raw): + document = tomllib.loads(raw) + result = {} + for section in SECTIONS: + table = document + for part in section.split("."): + table = table.get(part, {}) + if "key" in table: + result[section] = table["key"] + return result + + +def fixtures(): + canonical = "\n".join(f'[{section}]\nkey = "{value}"' for section, value in KEYS.items()) + yield "canonical / final line without newline", canonical + yield "commented stale keys", canonical.replace("key =", '# key = "stale"\nkey =') + yield "commented headers / mixed line endings", "\n".join( + f'# [{section}]\r\n# key = "stale"\r\n[{section}]\nkey = "{value}"' + for section, value in KEYS.items() + ) + yield "indented headers and keys / trailing comments", "\n".join( + f' \t[ {section} ] \t# header [note]\n \tkey \t= \t"{value}" # key comment' + for section, value in KEYS.items() + ) + yield "CRLF", canonical.replace("\n", "\r\n") + yield "quoted and spaced section names", "\n".join( + f'["{section.split(".")[0]}" . \'{section.split(".")[1]}\'' + + (f' . "{section.split(".")[2]}"' if section.count(".") == 2 else "") + + f'] # original table\nkey = "{value}"' + for section, value in KEYS.items() + ) + yield "unrelated quoted header before JWT keys", '["custom section"]\nkey = "other"\n' + canonical + yield "brackets in comments", canonical.replace("key =", "# consult [notes]\nkey =") + yield "quoted values and quoted key names", "\n".join(( + '[jwt.signing]\n"key" = "brackets[inside]#value"', + "[jwt.signing.read]\n'key' = 'literal\\path[#value]'", + r'[jwt.filer_signing]' + '\n' + r'key = "escaped\"quote\\slash\u0041"', + '[jwt.filer_signing.read]\nkey = ""', + )) + yield "absent keys and sections / unrelated key", '\n'.join(( + '[jwt.signing]\nexpires_after_seconds = 10', + '[jwt.signing.read]\nkey = "read-only"', + '[unrelated]\nkey = "not-a-jwt-key"', + '# [jwt.filer_signing]\n# key = "not-active"', + )) + yield "no existing security.toml", "" + + +def check_generated(value): + decoded = base64.b64decode(value, validate=True).decode("ascii") + assert len(decoded) == 10 and decoded.isascii() and decoded.isalnum(), "invalid generated JWT key" + + +def check_helpers(helm, reference_helm): + # Exercise the real helper, not a second implementation of its matching rules. + with tempfile.TemporaryDirectory(prefix="helm-jwt-helper-") as directory: + chart = Path(directory) + (chart / "templates").mkdir() + (chart / "Chart.yaml").write_text("apiVersion: v2\nname: jwt-regression\nversion: 0.0.0\n") + shutil.copyfile(CHART / "templates/shared/_helpers.tpl", chart / "templates/_helpers.tpl") + entries = [ + json.dumps(section) + ': {{ include "seaweedfs.existingTomlKey" (list ' + + json.dumps(section) + ' .Values.raw) | toJson }}' + for section in SECTIONS + ] + template = chart / "templates/keys.yaml" + prefix = '{"apiVersion":"v1","kind":"ConfigMap","metadata":{"name":"keys"},"data":{' + helper_template = prefix + ",".join(entries) + "}}" + reference_entries = [ + json.dumps(section) + ': {{ dig ' + + " ".join(json.dumps(part) for part in section.split(".")) + + ' "key" "__ABSENT__" (fromToml .Values.raw) | toJson }}' + for section in SECTIONS + ] + reference_template = prefix + ",".join(reference_entries) + "}}" + + def render(binary, source, raw): + template.write_text(source) + values = chart / "input.json" + values.write_text(json.dumps({"raw": raw})) + output = run(binary, "template", "keys", str(chart), "-f", str(values)) + return json.loads(output[output.index("{"):])["data"] + + for name, raw in fixtures(): + expected = keys(raw) + tokens = render(helm, helper_template, raw) + actual = {section: tomllib.loads("key = " + token)["key"] + for section, token in tokens.items() if token != ""} + assert actual == expected, f"{name}: extracted keys differ from stored TOML" + if reference_helm: + reference = render(reference_helm, reference_template, raw) + reference = {section: value for section, value in reference.items() if value != "__ABSENT__"} + assert actual == reference, f"{name}: keys differ from fromToml/dig" + print(f"PASS helper: {name}") + + # A present but unsupported value must not silently become a fresh key. + try: + render(helm, helper_template, '[jwt.signing]\nkey = """multi\nline"""') + except subprocess.CalledProcessError as error: + assert "refusing to replace an existing key" in error.stderr, error.stderr + else: + raise AssertionError("multiline existing key was silently accepted or replaced") + print("PASS helper: unsupported existing value fails without rotation") + + try: + render(helm, helper_template, ESCAPED_HEADER) + except subprocess.CalledProcessError as error: + assert "unsupported quoted section header" in error.stderr, error.stderr + else: + raise AssertionError("unsupported quoted header silently rotated its key") + print("PASS helper: unsupported quoted header fails without rotation") + + +def check_upgrades(helm, context): + namespace = "jwt-key-persist-" + uuid.uuid4().hex[:8] + current = "jk-seaweedfs-security-config" + legacy = "seaweedfs-security-config" + kubectl = ["kubectl", "--context", context, "-n", namespace] + release_args = ["jk", str(CHART), "--kube-context", context, "-n", namespace] + # No workload is needed to exercise Helm's real ConfigMap lookup and update. + for setting in ( + "master.enabled=false", "volume.enabled=false", "filer.enabled=false", + "global.seaweedfs.createClusterRole=false", + "global.seaweedfs.securityConfig.jwtSigning.volumeWrite=true", + "global.seaweedfs.securityConfig.jwtSigning.volumeRead=true", + "global.seaweedfs.securityConfig.jwtSigning.filerWrite=true", + "global.seaweedfs.securityConfig.jwtSigning.filerRead=true", + ): + release_args += ["--set", setting] + + def stored(): + cm = json.loads(run(*kubectl, "get", "configmap", current, "-o", "json")) + return keys(cm["data"]["security.toml"]) + + def upgrade(): + run(helm, "upgrade", *release_args) + return stored() + + def patch(raw): + # Seed previous-release content without taking Helm 4's SSA ownership. + run(*kubectl, "patch", "configmap", current, "--type=merge", "--field-manager=helm", "-p", + json.dumps({"data": {"security.toml": raw}})) + + run(*kubectl, "create", "namespace", namespace) + try: + run(helm, "install", *release_args) + initial = stored() + assert set(initial) == set(SECTIONS), "install omitted a JWT section" + for value in initial.values(): + check_generated(value) + assert upgrade() == initial, "no-op upgrade changed an existing key" + print("PASS upgrade: all four generated keys persist") + + for name, raw in fixtures(): + patch(raw) + actual = upgrade() + expected = keys(raw) + assert set(actual) == set(SECTIONS), f"{name}: upgrade omitted a JWT section" + for section in SECTIONS: + if section in expected: + assert actual[section] == expected[section], f"{name}: changed {section}" + else: + check_generated(actual[section]) + assert upgrade() == actual, f"{name}: subsequent upgrade changed a key" + print(f"PASS upgrade: {name}") + + # Migration from the old chart name, followed by precedence of the current name. + legacy_raw = "\n".join(f'[{section}]\nkey = "legacy-{index}"' + for index, section in enumerate(SECTIONS)) + run(*kubectl, "create", "configmap", legacy, "--from-literal=security.toml=" + legacy_raw) + run(*kubectl, "delete", "configmap", current) + assert upgrade() == keys(legacy_raw), "legacy ConfigMap keys were not preserved" + print("PASS upgrade: legacy ConfigMap migration") + current_raw = next(fixtures())[1] + patch(current_raw) + assert upgrade() == keys(current_raw), "legacy ConfigMap overrode current ConfigMap" + print("PASS upgrade: current ConfigMap takes precedence") + + patch(ESCAPED_HEADER) + try: + upgrade() + except subprocess.CalledProcessError as error: + assert "unsupported quoted section header" in error.stderr, error.stderr + else: + raise AssertionError("unsupported quoted header silently rotated its key") + assert keys(run(*kubectl, "get", "configmap", current, "-o", + "jsonpath={.data.security\\.toml}"))["jwt.signing"] == "existing" + print("PASS upgrade: unsupported quoted header leaves stored key untouched") + finally: + run(*kubectl, "delete", "namespace", namespace, "--wait=false") + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--helm", default="helm") + parser.add_argument("--reference-helm", help="Helm >=3.17 binary for differential checks") + parser.add_argument("--context", help="explicit disposable Kubernetes context for live upgrade checks") + args = parser.parse_args() + print(run(args.helm, "version", "--short").strip()) + check_helpers(args.helm, args.reference_helm) + if args.context: + check_upgrades(args.helm, args.context) + + +if __name__ == "__main__": + try: + main() + except subprocess.CalledProcessError as error: + print(error.stderr, file=sys.stderr) + sys.exit(error.returncode) diff --git a/.github/workflows/helm_ci.yml b/.github/workflows/helm_ci.yml index a08b8c23a..dc5566285 100644 --- a/.github/workflows/helm_ci.yml +++ b/.github/workflows/helm_ci.yml @@ -3,10 +3,10 @@ name: "helm: lint and test charts" on: push: branches: [ master ] - paths: ['k8s/**', '.github/workflows/helm_ci.yml'] + paths: ['k8s/**', '.github/workflows/helm_ci.yml', '.github/scripts/helm_jwt_keys.py'] pull_request: branches: [ master ] - paths: ['k8s/**', '.github/workflows/helm_ci.yml'] + paths: ['k8s/**', '.github/workflows/helm_ci.yml', '.github/scripts/helm_jwt_keys.py'] permissions: contents: read @@ -20,6 +20,14 @@ jobs: with: fetch-depth: 0 + - name: Set up Helm before fromToml was available + uses: azure/setup-helm@v5 + with: + version: v3.16.3 + + - name: Record legacy Helm binary + run: echo "HELM_LEGACY=$(command -v helm)" >> "$GITHUB_ENV" + - name: Set up Helm uses: azure/setup-helm@v5 with: @@ -44,6 +52,17 @@ jobs: - name: Run chart-testing (lint) run: ct lint --target-branch ${{ github.event.repository.default_branch }} --all --validate-maintainers=false --chart-dirs k8s/charts + - name: Verify legacy Helm rendering + run: | + "$HELM_LEGACY" lint k8s/charts/seaweedfs + "$HELM_LEGACY" template test k8s/charts/seaweedfs > "$RUNNER_TEMP/legacy-default.yaml" + "$HELM_LEGACY" template test k8s/charts/seaweedfs \ + --set global.seaweedfs.securityConfig.jwtSigning.volumeRead=true \ + --set global.seaweedfs.securityConfig.jwtSigning.filerWrite=true \ + --set global.seaweedfs.securityConfig.jwtSigning.filerRead=true \ + > "$RUNNER_TEMP/legacy-jwt.yaml" + + - name: Verify template rendering run: | set -e @@ -1922,6 +1941,16 @@ jobs: kubectl delete namespace "$NS" echo "SFTP host key lifecycle tests passed" + - name: Verify JWT signing key persistence across upgrades + run: | + # chart-testing puts its pip-less venv first on PATH; use setup-python. + PYTHON="$pythonLocation/bin/python3" + "$PYTHON" -m pip install tomli==2.2.1 + CONTEXT=$(kubectl config current-context) + "$PYTHON" .github/scripts/helm_jwt_keys.py --context "$CONTEXT" + "$PYTHON" .github/scripts/helm_jwt_keys.py --helm "$HELM_LEGACY" \ + --reference-helm helm --context "$CONTEXT" + - name: Verify install into a default-deny namespace run: | set -e diff --git a/k8s/charts/seaweedfs/README.md b/k8s/charts/seaweedfs/README.md index e68d19f68..8e9812f37 100644 --- a/k8s/charts/seaweedfs/README.md +++ b/k8s/charts/seaweedfs/README.md @@ -27,6 +27,19 @@ so your deployment will be spread/HA. * cert config exists and can be enabled, but not been tested, requires cert-manager to be installed. ## Prerequisites + +The chart's templates render with Helm 3.16.3 and newer versions tested in CI. +Earlier Helm versions are not covered by this chart's compatibility checks. +When JWT signing is enabled, a live upgrade reuses keys from the existing +security ConfigMap (including the legacy ConfigMap name); a key absent from +that ConfigMap is generated. Plain `helm template` does not read cluster state; +use an install and upgrade against a cluster to check key persistence. + +The key reader supports single-line quoted TOML strings in JWT sections, +including indented assignments, quoted key names, and bare or simply quoted +section-name segments. An unsupported value or quoted header fails the upgrade +rather than silently rotating a key. + ### Database leveldb is the default database, this supports multiple filer replicas that will [sync automatically](https://github.com/seaweedfs/seaweedfs/wiki/Filer-Store-Replication), with some [limitations](https://github.com/seaweedfs/seaweedfs/wiki/Filer-Store-Replication#limitation). diff --git a/k8s/charts/seaweedfs/templates/shared/_helpers.tpl b/k8s/charts/seaweedfs/templates/shared/_helpers.tpl index 63812f091..7d73c1008 100644 --- a/k8s/charts/seaweedfs/templates/shared/_helpers.tpl +++ b/k8s/charts/seaweedfs/templates/shared/_helpers.tpl @@ -444,6 +444,46 @@ true {{- end -}} {{- end -}} +{{/* Read a JWT key from the chart's existing security.toml without fromToml + (which requires Helm >=3.17). Args: (list "
" $raw). + Return the single-line TOML string token, including its quotes, so escapes + and explicitly empty values survive unchanged. No output means absent. + Accept bare or simply quoted dotted header segments. Fail on other quoted + headers rather than risk rotating a key hidden by unsupported syntax. + This reads the chart's section/key layout, not arbitrary TOML syntax. */}} +{{- define "seaweedfs.existingTomlKey" -}} +{{- $section := index . 0 -}} +{{- $raw := index . 1 -}} +{{- $parts := list -}} +{{- range $part := splitList "." $section -}} + {{- $escaped := regexQuoteMeta $part -}} + {{- $parts = append $parts (printf `(?:%s|"%s"|'%s')` $escaped $escaped $escaped) -}} +{{- end -}} +{{- $header := printf `^\[[ \t]*%s[ \t]*\][ \t]*(#.*)?$` (join `[ \t]*\.[ \t]*` $parts) -}} +{{- $segment := `(?:[A-Za-z0-9_-]+|"[^"\\]*"|'[^']*')` -}} +{{- $simpleHeader := printf `^\[[ \t]*%s(?:[ \t]*\.[ \t]*%s)*[ \t]*\][ \t]*(#.*)?$` $segment $segment -}} +{{- $assignment := `^(key|"key"|'key')[ \t]*=[ \t]*` -}} +{{- $string := `"([^"\\]|\\.)*"|'[^']*'` -}} +{{- $active := false -}} +{{- $key := "" -}} +{{- range $rawLine := splitList "\n" $raw -}} + {{- $line := trim $rawLine -}} + {{- if hasPrefix "[" $line -}} + {{- if and (regexMatch `^\[[^]]*["']` $line) (not (regexMatch $simpleHeader $line)) (eq $key "") -}} + {{- fail (printf "security.toml has an unsupported quoted section header; refusing to replace [%s].key" $section) -}} + {{- end -}} + {{- $active = regexMatch $header $line -}} + {{- else if and $active (eq $key "") (regexMatch $assignment $line) -}} + {{- $value := regexReplaceAll $assignment $line "" -}} + {{- if not (regexMatch (printf `^(%s)[ \t]*(#.*)?$` $string) $value) -}} + {{- fail (printf "security.toml [%s].key must be a single-line quoted TOML string; refusing to replace an existing key" $section) -}} + {{- end -}} + {{- $key = regexFind $string $value -}} + {{- end -}} +{{- end -}} +{{- $key -}} +{{- end -}} + {{/* True when the post-install bucket hook Job renders: an S3 endpoint, plus buckets to create on it. Read by the Job itself and by its NetworkPolicy, which has to appear exactly when the Job does - a Job without its policy diff --git a/k8s/charts/seaweedfs/templates/shared/security-configmap.yaml b/k8s/charts/seaweedfs/templates/shared/security-configmap.yaml index 17b82cca0..e840e62f8 100644 --- a/k8s/charts/seaweedfs/templates/shared/security-configmap.yaml +++ b/k8s/charts/seaweedfs/templates/shared/security-configmap.yaml @@ -18,7 +18,7 @@ data: {{- $legacyName := printf "%s-%s" (include "seaweedfs.name" .) "security-config" }} {{- $existing = lookup "v1" "ConfigMap" .Release.Namespace $legacyName }} {{- end }} - {{- $securityConfig := fromToml (dig "data" "security.toml" "" $existing) }} + {{- $existingToml := dig "data" "security.toml" "" $existing }} {{- $securityConfigValues := .Values.global.seaweedfs.securityConfig | default dict }} {{- $jwtSigning := $securityConfigValues.jwtSigning | default dict }} {{- $expiresAfterSeconds := $jwtSigning.expiresAfterSeconds | default dict }} @@ -29,7 +29,7 @@ data: # the jwt signing key is read by master and volume server # the jwt defaults to expire after 10 seconds [jwt.signing] - key = "{{ dig "jwt" "signing" "key" (randAlphaNum 10 | b64enc) $securityConfig }}" + key = {{ include "seaweedfs.existingTomlKey" (list "jwt.signing" $existingToml) | default (randAlphaNum 10 | b64enc | quote) }} {{- if gt (int $expiresAfterSeconds.volumeWrite) 0 }} expires_after_seconds = {{ int $expiresAfterSeconds.volumeWrite }} {{- end }} @@ -41,7 +41,7 @@ data: # - the Volume server validates the JWT on reading # the jwt defaults to expire after 60 seconds [jwt.signing.read] - key = "{{ dig "jwt" "signing" "read" "key" (randAlphaNum 10 | b64enc) $securityConfig }}" + key = {{ include "seaweedfs.existingTomlKey" (list "jwt.signing.read" $existingToml) | default (randAlphaNum 10 | b64enc | quote) }} {{- if gt (int $expiresAfterSeconds.volumeRead) 0 }} expires_after_seconds = {{ int $expiresAfterSeconds.volumeRead }} {{- end }} @@ -53,7 +53,7 @@ data: # - the Filer server validates the JWT on writing # the jwt defaults to expire after 10 seconds [jwt.filer_signing] - key = "{{ dig "jwt" "filer_signing" "key" (randAlphaNum 10 | b64enc) $securityConfig }}" + key = {{ include "seaweedfs.existingTomlKey" (list "jwt.filer_signing" $existingToml) | default (randAlphaNum 10 | b64enc | quote) }} {{- if gt (int $expiresAfterSeconds.filerWrite) 0 }} expires_after_seconds = {{ int $expiresAfterSeconds.filerWrite }} {{- end }} @@ -65,7 +65,7 @@ data: # - the Filer server validates the JWT on reading # the jwt defaults to expire after 60 seconds [jwt.filer_signing.read] - key = "{{ dig "jwt" "filer_signing" "read" "key" (randAlphaNum 10 | b64enc) $securityConfig }}" + key = {{ include "seaweedfs.existingTomlKey" (list "jwt.filer_signing.read" $existingToml) | default (randAlphaNum 10 | b64enc | quote) }} {{- if gt (int $expiresAfterSeconds.filerRead) 0 }} expires_after_seconds = {{ int $expiresAfterSeconds.filerRead }} {{- end }}