Skip to content

Add Kyverno policy for scoped external secrets#2269

Merged
siutsin merged 3 commits intomasterfrom
fix/scoped-external-secrets-store
Apr 26, 2026
Merged

Add Kyverno policy for scoped external secrets#2269
siutsin merged 3 commits intomasterfrom
fix/scoped-external-secrets-store

Conversation

@siutsin
Copy link
Copy Markdown
Owner

@siutsin siutsin commented Apr 26, 2026

Summary

  • add Kyverno as an Argo-managed security chart
  • add a separate kyverno-policy chart for 1Password ClusterSecretStore restrictions
  • restrict shared 1Password ClusterSecretStore use to approved namespaces and item keys, including per-entry storeRef overrides
  • ignore local Air workspace metadata

Test plan

  • helm template kyverno helm-charts/kyverno -n kyverno --include-crds
  • helm template kyverno-policy helm-charts/kyverno-policy -n kyverno
  • make test

@siutsin siutsin force-pushed the fix/scoped-external-secrets-store branch from 586cfe2 to 7e0d5b9 Compare April 26, 2026 21:08
@claude
Copy link
Copy Markdown

claude Bot commented Apr 26, 2026

Claude finished @siutsin's task in 2m 54s —— View job


Review: Kyverno policy for scoped external secrets

Findings

1. Inherited dataFrom.find bypasses the key allowlist (helm-charts/kyverno-policy/templates/cluster-secret-store-policy.yaml:137-174)

restrict-inherited-datafrom-keys gates on element.extract.key != "", so dataFrom entries that use find: (regex/tags) instead of extract: are skipped entirely. With the top-level secretStoreRef pointing at the shared 1Password ClusterSecretStore (and no per-entry sourceRef), an ExternalSecret in an approved namespace can read arbitrary item keys via:

dataFrom:
  - find:
      name:
        regexp: ".*"

The explicit-override path (restrict-explicit-datafrom-keys) doesn't have this gap because its precondition keys on the sourceRef.storeRef, not on extract.key, so a find with explicit shared-store override gets denied (empty extract.key fails the AnyNotIn allowlist). But the inherited path is exploitable.

Recommend either denying find outright when the shared store is inherited, or extending the rule to also deny when element.find is set. Example for the inherited rule:

deny:
  conditions:
    any:
      - key: "{{ `{{ element.find || '' | to_string(@) }}` }}"
        operator: NotEquals
        value: ""
      - key: "{{ `{{ element.extract.key || '' }}` }}"
        operator: AnyNotIn
        value: [...]

Fix this →

2. Empty-list footgun in per-namespace policies (cluster-secret-store-policy.yaml:130-135, :168-174, :198-205, :228-235)

If a namespace is added to allowedNamespaces with an empty key list, AnyNotIn [] denies every read from the shared store in that namespace, while the top-level rule still permits the store. Net effect: silently broken namespace. Consider adding a Helm-side required/length check, or asserting in the chart that every entry has at least one key.

3. Redundant and (cluster-secret-store-policy.yaml:1)

{{- if and .Values.clusterSecretStorePolicy.enabled }}and with one argument is a no-op. Drop to {{- if .Values.clusterSecretStorePolicy.enabled }}.

4. Placeholder Chart descriptions (helm-charts/kyverno-policy/Chart.yaml:3, helm-charts/kyverno/Chart.yaml:3)

Both still say description: A Helm chart for Kubernetes. Worth replacing with something specific (e.g. "Kyverno policies restricting the shared 1Password ClusterSecretStore").

5. Operator nitAnyNotIn on the scalar request.namespace and element.remoteRef.key works but NotIn reads more obviously. Optional.

Looks good

  • Resources follow repo policy: memory request == limit, no CPU limits, ephemeral-storage set on all four kyverno controllers (helm-charts/kyverno/values.yaml:3-22).
  • Argo waves order kyverno (03) before kyverno-policy (04) (argocd/manifest.jsonnet:161-162).
  • keys ... | sortAlpha keeps rendered output stable (cluster-secret-store-policy.yaml:3).
  • background: false is the right call given foreach-heavy validation that's only meaningful on admission.
  • Top-level + per-entry storeRef checks correctly cover both inheritance and override paths for the namespace allowlist.

@claude
Copy link
Copy Markdown

claude Bot commented Apr 26, 2026

Claude finished @siutsin's task in 1m 56s —— View job


Re-review: latest commit addresses prior feedback

Verified fixes

  • find bypass: inherited dataFrom rule now denies via an any deny block that triggers if either element.find is non-empty (JMESPath to_string(@) on the ||-defaulted value) or element.extract.key is outside the allowlist (cluster-secret-store-policy.yaml:170-179). Closes the regex/tag escape hatch.
  • Empty-list footgun: chart now fails at render time when any allowedNamespaces.<ns> has no keys (cluster-secret-store-policy.yaml:4-8). Prevents the silent AnyNotIn [] denial.
  • Redundant and: dropped (cluster-secret-store-policy.yaml:1).
  • Chart descriptions: replaced placeholders with meaningful text (helm-charts/kyverno-policy/Chart.yaml:3, helm-charts/kyverno/Chart.yaml:3).
  • Operator nit: AnyNotInNotIn applied across scalar checks.

Remaining

Stray AnyNotIn (cluster-secret-store-policy.yaml:136)
The restrict-inherited-data-keys rule still uses AnyNotIn while every other rule was switched to NotIn. Behaviour is equivalent for a scalar element.remoteRef.key, but inconsistent with the rest of the file.

- key: "{{ `{{ element.remoteRef.key || '' }}` }}"
  operator: AnyNotIn  # ← change to NotIn for consistency

Fix this →

Otherwise looks good — LGTM once the nit above is addressed.


  • Diff against origin/master and against the prior review
  • Verify previous findings are addressed
  • Re-check policy logic, chart configuration
  • Post review

Branch: fix/scoped-external-secrets-store ·

@claude
Copy link
Copy Markdown

claude Bot commented Apr 26, 2026

Claude finished @siutsin's task in 47s —— View job


Re-review: operator alignment ✅

Verified

  • restrict-inherited-data-keys now uses NotIn (cluster-secret-store-policy.yaml:136), matching every other scalar check in the file. Closes the inconsistency flagged in the previous review.
  • All seven scalar-allowlist conditions (request.namespace x3, element.remoteRef.key x2, element.extract.key x2) now use NotIn uniformly.
  • The find-bypass guard on the explicit-override path is still safe under NotIn: a find entry has empty extract.key, which fails NotIn [allowed keys] and gets denied (cluster-secret-store-policy.yaml:235-240).

LGTM — nothing else to flag.


  • Diff against origin/master and against the previous review
  • Verify the AnyNotInNotIn nit was addressed
  • Re-check policy logic and chart configuration
  • Post review

Branch: fix/scoped-external-secrets-store ·

@siutsin siutsin merged commit 4176cf5 into master Apr 26, 2026
7 checks passed
@siutsin siutsin deleted the fix/scoped-external-secrets-store branch April 26, 2026 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant