feat: Add cluster age key and document #46
clank-bot
martin
Loading…
Reference in a new issue
No description provided.
Delete branch "secrets"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Review Summary
This PR introduces the full SOPS secret-management workflow for the cluster: a new
clusterage key added as a recipient in.sops.yaml, thesops-secrets-operatorindevis wired to asops-age-key-fileSecret viasecretsAsFiles+SOPS_AGE_KEY_FILEenv, thejustfilegains parameterizedsops-edit/sops-encrypt/sops-decryptand asops-newscaffold (backed byscripts/sops-new.sh), and AGENTS.md/README are updated to document it. I verified the generatedmanifests/files match the sources (env and manifestsargocd-secrets.sops.yamlare byte-identical, both re-encrypted to the same blob), and that the chart values (secretsAsFiles,extraEnv) render correctly into the Deployment.Overall the change is sound and consistent:
--indent 2keeps sops output 2-space formatted, the re-encryption adds the new recipient without leaking values, and the workflow docs are accurate.Non-blocking items worth addressing:
SOPS_AGE_KEY_FILE=/etc/sops-age-key-file/keyonly works if the out-of-bandsops-age-key-fileSecret has a data key named exactlykey. This cannot be verified from the repo; if the name differs or the Secret is missing, the operator pod fails to start or silently can't decrypt. Worth documenting the required key name and adding asops-statuscheck for it..sops.yamlcreation rules apply to allenv/**/*.sops.yaml, but theclusterprivate key only lives indev. Futureinfra/prodsecrets would therefore be decryptable by the dev cluster. Consider per-env recipients/path-scoped rules.just sops-statuslists every pod indev(noise); filter by the operator'sapp.kubernetes.io/namelabel.sops-new.shperforms no K8s-name validation.*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: comment
Code review findings from opencode.
@ -5,3 +6,4 @@encrypted_regex: ^(data|stringData)$age:- *nidaros- *cluster[security] The
clusterrecipient's private key is stored only in thedevcluster (per AGENTS.md), but this creation rule (path_regex: \.sops\.yaml$) applies to everyenv/**/*.sops.yaml. Any future secret added underenv/infra/orenv/prod/will be encrypted for the dev-cluster key, meaning a compromise of dev would expose infra/prod secrets. Consider per-environment recipients with path-scoped rules (e.g. separate rules forenv/dev/vsenv/prod/).@ -19,0 +20,4 @@{name = "sops-age-key-file";mountPath = "/etc/sops-age-key-file";secretName = "sops-age-key-file";[bug]
SOPS_AGE_KEY_FILE=/etc/sops-age-key-file/keyrequires the out-of-bandsops-age-key-fileSecret to contain a data key literally namedkey(the mount exposes the Secret's data keys as files). This can't be verified from the repo, and if the key is named differently the operator pod won't start (mount error) or will silently fail to decrypt, breaking the entire secret workflow this PR introduces. Please confirm/document the exact data key name, and consider adding an existence check (e.g. tojust sops-status).@ -84,3 +84,3 @@[group("Secrets")]sops-status:kubectl get pods -n sops-secrets-operatorkubectl get pods -n dev[style]
kubectl get pods -n devreturns every pod in the dev namespace, which can drown out the operator status and produce false alarms when unrelated apps are down. Filter by the operator's label instead, e.g.kubectl get pods -n dev -l app.kubernetes.io/name=sops-secrets-operator(the Deployment carries that label).@ -0,0 +3,4 @@path="$1"namespace="$2"secret_name="$3"[style] No validation is done on
namespace/secret_nameagainst Kubernetes naming rules, so a typo produces an invalid SopsSecret that fails later in the pipeline. A simple regex check (or akubectl create --dry-run=clientvalidation) in the scaffold would fail fast. Also note the scaffold writes a plaintextreplace-mevalue — fine as a template, but make sure the file is encrypted viajust sops-edit/just sops-encryptbefore committing.Review Summary
This PR adds first-class SOPS secret management to the cluster: a cluster-wide age recipient key (
age1zy4…cecdy), wires thesops-age-key-fileSecret +SOPS_AGE_KEY_FILEenv var into the sops-secrets-operator (viasecretsAsFiles/extraEnv), re-encrypts existing secrets for the new recipient, and adds thejust sops-*command suite with ascripts/sops-new.shscaffolder. Documentation (AGENTS.md/README.md) is thorough and consistent.Strengths:
/etc/sops-age-key-file/key) matchesSOPS_AGE_KEY_FILE, and the bootstrap recipe always uses data keykey.sops-statuscorrectly uses theapp.kubernetes.io/nameselector (matches the Helm-generated pod labels) and validates the secret's data key.sops-new.shvalidates k8s name rules (63 chars, DNS-1123), refuses existing files, requires the.sops.yamlsuffix, and cleans up on failed validation.Main concern — security:
sops-new.shscaffolds a plaintext SopsSecret containing the placeholder valuereplace-me, and encryption depends entirely on the user remembering to runjust sops-edit. Nothing (build,just switch, CI, git) guards against committing/syncing the plaintext placeholder; since nixidy copies the file verbatim intomanifests/, it would be deployed to the cluster as a literalreplace-mesecret with no failure. Recommend hardening the workflow.Minor notes: kubectl's "no matches for kind" check in sops-new.sh is a fragile message-based match (CRD-not-installed and API-down both pass), and the
sops-status/bootstrap namespace is hardcoded todev(documented, so acceptable).*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes
Code review findings from opencode.
@ -0,0 +46,4 @@- name: ${secret_name}type: OpaquestringData:${secret_name}: replace-me[security] The scaffolder writes a plaintext SopsSecret with a dummy value
replace-me. Encryption only happens later viajust sops-edit, so the file is a live secret-in-plaintext until then. Because nixidy copies the file verbatim intomanifests/(the operator decrypts in-cluster), runningjust switchbefore encrypting would deploy a literalreplace-meSecret with no error. Nothing guards against this (no build/CI check that the file is actually encrypted). Suggest encrypting the placeholder immediately withsops -e -iinside sops-new.sh, and/or adding a CI/build check that every*.sops.yamlcommitted is encrypted (e.g. grep for anENC[AES256_GCMdata value or a validmac:).@ -0,0 +54,4 @@echo "kubectl validation: OK"elseerr=$(kubectl create --dry-run=client -f "$path" -o yaml 2>&1 || true)if echo "$err" | grep -q 'no matches for kind'; then[style] Determining whether kubectl validation should be skipped by grepping stderr for the literal 'no matches for kind' is fragile: an unreachable/unauthorized API server, expired context, or other schema errors that still contain that substring will silently downgrade to a warning and keep a file that failed validation. Consider checking the error for other CRD/API failures, or running the validation against a local schema (e.g.
kubectl apply --dry-run=client --validate=falsewon't help either; simplest is to require the CRD before relying on the check).Review Summary
This PR wires the
sops-secrets-operatorto decrypt secrets in-cluster using a dedicated cluster age key:What's good: The
.sops.yamlrecipient change and the re-encryptedargocd-secrets.sops.yamlare consistent acrossenv/andmanifests/(identical ciphertexts, MAC,version: 3.13.3). ThesecretsAsFiles+extraEnvHelm values render correctly into the Deployment (volume, read-only mount,SOPS_AGE_KEY_FILE=/etc/sops-age-key-file/key). Thejustfileadditions are solid:sops-bootstrap-age-keyvalidates the keyfile (AGE-SECRET-KEY-1) and always names the data keykey, matching whatSOPS_AGE_KEY_FILErequires. Docs (README/AGENTS) are thorough and consistent, and thesops-statusnamespace fix (devvs the oldsops-secrets-operator) matches the actual Deployment namespace. The chicken-and-egg bootstrap ordering is correctly documented.Concerns: (1) The new docs state a SopsSecret's
namespacemust match the application's namespace, but the flagship templateargocd-secrets.sops.yamldeclaresnamespace: devwhile ArgoCD runs inargocd— the resultingargocd-secretwould land indevwhere ArgoCD won't read it (notargetNamespace). Worth verifying. (2) The mounted age private key defaults to mode 0644 in the container; considerdefaultMode: 0600. (3) The cluster key is now a recipient for every environment's.sops.yaml(globalcreation_rules), and its private key sits in a plaintext Secret indev— broadens blast radius ifdevis compromised. (4) The operator Deployment hard-depends on the out-of-band secret; until bootstrapped the pod stays inCreateContainerConfigErrorand nothing decrypts, and a key rotation won't trigger a Deployment roll (no checksum annotation).Net: the core change is correct, well-tested by the rendered manifests, and well-documented. Remaining items are pre-existing concerns or hardening suggestions rather than blockers.
*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: comment
Code review findings from opencode.
@ -5,3 +6,4 @@encrypted_regex: ^(data|stringData)$age:- *nidaros- *cluster[security] Adding the cluster key to the global
creation_rulesmeans every.sops.yamlin every environment is now encrypted to the in-cluster key, whose private key lives in a plaintext Secret in thedevnamespace. A compromise ofdev(anyone withsecretsread there) yields the master key for all environments, includingprod. This is the inherent sops-secrets-operator tradeoff, but consider documenting the blast radius / restricting RBAC onsops-age-key-file, or using per-environment recipients if prod must not be decryptable by the dev cluster.@ -312,0 +329,4 @@- Files must end in `.sops.yaml` so the creation rule in `.sops.yaml`(`encrypted_regex: ^(data|stringData)$`) applies.- The `namespace:` field of the `SopsSecret` decides where the `Secret` is created — it must match the[bug] The new guidance says the SopsSecret
namespacefield "must match the application's namespace", but the template/example referenced everywhere isenv/dev/argocd-secrets.sops.yaml, which declaresnamespace: devwhile the ArgoCD application runs inargocd(env/dev/argocd.nix:11). Since the operator creates the resulting Secret in the SopsSecret's own namespace (notargetNamespace),argocd-secretwould be created indevand ArgoCD would never readadmin.password/oidc.keycloak.clientSecretfrom it. This appears pre-existing, but this PR's docs codify it as the reference workflow — please verify the secret actually reaches theargocdnamespace (or fix the example's namespace), otherwise the documented "must match" rule contradicts the very template it recommends.@ -19,0 +19,4 @@# Requires the out-of-band Secret `sops-age-key-file` in `dev` to contain a data key# literally named `key` (exposed as the file mounted at `/etc/sops-age-key-file/key`).# Bootstrap/replace with: just sops-bootstrap-age-key <keyfile>secretsAsFiles = [[security] The mounted
sops-age-key-filevolume renders without adefaultMode(see the generated Deployment), so the private age key file is world-readable (0644) inside the container. Consider settingdefaultMode: 0600if the chart'ssecretsAsFilesitems support it (or via a post-render/kustomize transform).[style] Operational ordering: the Deployment now hard-depends on the out-of-band
sops-age-key-fileSecret. Until it is bootstrapped (manually), the operator pod is stuck inCreateContainerConfigErrorand no SopsSecret decrypts — includingargocd-secrets, so bootstrap must happen before/at the firstjust switch. Also, rotating the key viajust sops-bootstrap-age-keywon't trigger a Deployment rollout (no checksum annotation on the secret); kubelet will eventually refresh the mounted file, but if the operator caches the key at startup a manualkubectl rollout restartmay be needed. The docs hint at this; making it explicit would help.