Add opt-in NetworkPolicies for the application namespace - #51
Conversation
The namespace previously had no NetworkPolicies - every pod could talk to every other pod and the whole cluster network, which matters doubly with the unauthenticated redis. Off by default for parity (networkPolicy.enabled). When enabled, the infra chart (installed first) renders the namespace-wide default ingress deny, the redis (6379 in-namespace, 9121 monitoring) and rabbitmq (5672 in-namespace, 15672 ingress controller, 15692 monitoring) allows, an extra-ingress escape hatch and an optional egress lockdown (DNS + in-namespace + explicit rules); the app chart allows the ingress controller and in-namespace pods to reach the webserver (8080) and storefront (3000). The ingress-controller and monitoring namespace selectors are configurable. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds opt-in Kubernetes NetworkPolicy resources to harden the application namespace without impacting default renders (networkPolicy.enabled: false by default). This fits the repo’s Helm/helmfile-based deployment by letting environments progressively roll out network isolation while keeping legacy-parity defaults.
Changes:
- Introduces namespace-wide default-deny ingress plus Redis/RabbitMQ allow policies (infra chart), with optional egress lockdown support.
- Adds app workload allow policies for webserver (8080) and storefront (3000) (app chart).
- Documents the new values and adds helm-unittest suites covering default-off and enabled behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| docs/values.md | Documents networkPolicy values and rollout/egress caveats. |
| charts/shopsys-infra/values.yaml | Adds defaulted networkPolicy configuration (shared values section). |
| charts/shopsys-infra/templates/networkpolicy.yaml | Renders infra-owned NetworkPolicies (default-deny ingress, Redis/RabbitMQ allows, optional egress lockdown, extra ingress escape hatch). |
| charts/shopsys-infra/tests/networkpolicy_test.yaml | Adds unit tests for infra NetworkPolicy rendering and egress-lockdown behavior. |
| charts/shopsys-app/values.yaml | Adds documented networkPolicy configuration section (shared between charts). |
| charts/shopsys-app/templates/networkpolicy.yaml | Renders app-owned NetworkPolicies allowing ingress to webserver/storefront. |
| charts/shopsys-app/tests/networkpolicy_test.yaml | Adds unit tests for app NetworkPolicy rendering and selector customization. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # application pods in this namespace | ||
| - from: | ||
| - podSelector: {} | ||
| ports: | ||
| - port: 5672 | ||
| # management UI through the ingress controller | ||
| - from: | ||
| - namespaceSelector: | ||
| {{- toYaml .Values.networkPolicy.ingressControllerNamespace | nindent 12 }} | ||
| ports: | ||
| - port: 15672 | ||
| # prometheus scraping of the built-in exporter |
There was a problem hiding this comment.
Not applicable to this chart as it stands: RabbitMQ clustering is not supported here at all. The StatefulSet configures no peer-discovery mechanism and no shared Erlang cookie (each replica gets its own PVC with a node-generated random cookie), so rabbitmq.replicas > 1 yields independent brokers that never attempt inter-node Erlang distribution - there is no EPMD/25672 traffic for the NetworkPolicy to block, with or without this PR. The default is replicas: 1 (legacy parity). If clustering support ever lands (peer discovery + shared cookie), the 4369/25672 pod-to-pod allow will be added as part of that change rather than as a dead rule now.
henzigo
left a comment
henzigo
left a comment
There was a problem hiding this comment.
Reviewed the #51 diff against its stacked base default-security-contexts (charts/shopsys-app + charts/shopsys-infra networkpolicy templates, values, tests, docs/values.md).
Verified locally on the branch:
./tests/run-golden-tests.sh— 15/15 PASS (default-off confirmed: zero golden impact)helm unittest charts/shopsys-app charts/shopsys-infra— 97/97 PASShelm lintboth charts withnetworkPolicy.enabled=true(+ egress lockdown) — clean- Rendered with policies enabled (incl.
extraIngress+egress.rules) and validated with kubeconform-strict(K8s 1.31) — 8/8 valid - Cross-checked every
podSelectoragainst the actual immutableapp:selector labels (webserver-php-fpm,storefront,redis,rabbitmq) and every policy port against the real container ports (8080, 3000, 6379/9121, 5672/15672/15692) — all correct. Both ingresses (ingress-domains,ingress-mcp) target onlywebserver-php-fpm:8080, which is covered; storefront↔webserver and hook-job traffic is in-namespace and covered; DNS egress correctly allows both UDP and TCP 53.
The overall model is sound: namespace-wide deny + per-workload allows in the infra chart (installed first), workload allows in the app chart, opt-in default-off for a generic reusable package, egress lockdown as a second opt-in with the PostgreSQL/Elasticsearch/S3/SMTP/K8s-API caveat documented in both values comments and docs/values.md. The CNI-enforcement and kubelet-probe caveats are documented. Nice work on the split and the escape hatches.
Findings (severity-ranked):
- High — cert-manager HTTP01 solver pods are blocked by
default-deny-ingress(inline). Certificate issuance/renewal will silently break after enabling the policies, and only surface at renewal time. - Medium —
networkPolicyis not declared in eithervalues.schema.json(inline). A typo in this security toggle fails open silently. - Low — DNS egress is allowed to any destination (inline) — DNS-tunnel exfiltration remains possible under the lockdown.
- Low —
allow-storefrontgrants ingress from the ingress-controller namespace, but no shipped ingress targets storefront:3000 (inline) — unused surface with a misleading comment. - Low — RabbitMQ 15672 is opened from the ingress-controller namespace even when the management ingress does not render (inline).
- Info — no golden scenario exercises the enabled state. The unit tests cover chart-level rendering, but a small golden scenario (or a variant) with
networkPolicy.enabled: truewould lock the rendering under the full helmfile state-values path, consistent with how other opt-in features are guarded here. Also info:allow-redis/allow-rabbitmqrender even whenredis.enabled/rabbitmq.enabledare false — harmless (they select nothing) but you may want to gate them for render cleanliness.
— Claude Agent (CR)
| apiVersion: networking.k8s.io/v1 | ||
| kind: NetworkPolicy | ||
| metadata: | ||
| name: default-deny-ingress |
There was a problem hiding this comment.
High: This default deny selects every pod in the namespace — including cert-manager's HTTP01 solver pods. Both ingress-domains and ingress-rabbitmq carry cert-manager.io/cluster-issuer, and with an ACME HTTP01 issuer cert-manager spawns temporary solver pods in this namespace, reached via ingress-nginx on pod port 8089 (/.well-known/acme-challenge/...). No allow policy selects them (their labels are acme.cert-manager.io/http01-solver: "true", not app: ...), so once networkPolicy.enabled: true every certificate issuance/renewal fails — and it only surfaces weeks later at renewal time.
Suggestion: add a built-in allow policy (podSelector on acme.cert-manager.io/http01-solver: "true", port 8089, from the ingress-controller namespace — cert-manager's docs describe exactly this), or at minimum document the gotcha in docs/values.md with a ready-made extraIngress example. DNS01 issuers are unaffected, but the charts can't know which issuer type the cluster uses. — Claude Agent (CR)
There was a problem hiding this comment.
Implemented in ffc27ad - this was a real trap, thanks. Added a built-in allow-acme-solver policy (podSelector acme.cert-manager.io/http01-solver: "true", port 8089), rendered whenever networkPolicy.enabled is on; it selects no pods on DNS01 clusters, so it is a safe default. One deliberate deviation from the suggestion: the rule is not restricted to the ingress-controller namespace, because cert-manager's pre-issuance self-check may not arrive with the ingress-controller namespace as its traffic source (load-balancer hairpin / externalTrafficPolicy specifics vary per cluster), and the solver serves nothing but the public challenge token anyway. Documented in the values comments and docs/values.md, and covered by a unit test plus the new network-policies golden scenario.
| # default-deny ingress for the whole namespace, allows for the chart workloads, and an | ||
| # optional egress lockdown. Roll out on a dev environment first and verify that your CNI | ||
| # enforces policies and that kubelet probes still pass. | ||
| networkPolicy: |
There was a problem hiding this comment.
Medium: Neither chart's values.schema.json declares networkPolicy (roots keep additionalProperties: true, so nothing validates it). For a security feature this fails open: networkPolicy.enable: true, a misspelled ingressControllerNamespace, or a non-boolean enabled all silently render nothing / wrong selectors while the operator believes the namespace is locked down. A small schema block (enabled boolean, egress.enabled boolean, object shapes for the selectors, arrays for extraIngress/rules) in both charts would catch typos at deploy time. I know #50 also skipped schema entries for its new keys, but here the failure mode is a missing security control rather than a missing hardening default. — Claude Agent (CR)
There was a problem hiding this comment.
Implemented in ffc27ad. Both charts' values.schema.json now declare the full networkPolicy shape with additionalProperties: false on the block (and on egress), so networkPolicy.enable: true, a non-boolean enabled or any misspelled sub-key fails the deploy instead of silently rendering nothing. Verified: --set networkPolicy.enable=true now errors with "additional properties 'enable' not allowed". The namespace selectors stay loose object (they legitimately accept matchLabels or matchExpressions), and the roots keep additionalProperties: true as required by the shared helmfile state values.
| egress: | ||
| # DNS anywhere (cluster DNS location differs per cluster) | ||
| - ports: | ||
| - port: 53 |
There was a problem hiding this comment.
Low (hardening): This rule allows egress on port 53 to any destination, including arbitrary internet resolvers — so DNS tunneling remains a viable exfiltration path even under the lockdown, which is one of the main threats an egress lockdown addresses. Consider making the DNS destination configurable, e.g. networkPolicy.egress.dnsSelector defaulting to namespaceSelector: {matchLabels: {kubernetes.io/metadata.name: kube-system}} (+ podSelector: {k8s-app: kube-dns}), with an explicit empty value falling back to the current "anywhere" for clusters with node-local DNS or non-standard resolver locations. Keeping "anywhere" as the default is defensible for a generic package — but then it deserves a sentence in docs/values.md so operators know the residual risk. — Claude Agent (CR)
There was a problem hiding this comment.
Documented in ffc27ad, keeping "anywhere" as the default: the residual DNS-tunneling risk is now called out in the template comment, the values comments and docs/values.md. I did not add a dnsSelector knob for now - a generic package cannot pick a safe default (kube-system/kube-dns vs CoreDNS labels vs node-local caches on link-local IPs that a namespaceSelector cannot express at all), and a wrong default here bricks all name resolution in the namespace. If a consuming project wants a pinned resolver destination, that is a reasonable follow-up feature once there is a concrete cluster to design it against.
| ingress: | ||
| - from: | ||
| # e-shop ingresses | ||
| - namespaceSelector: |
There was a problem hiding this comment.
Low: The comment says "e-shop ingresses", but no ingress shipped by these charts targets storefront:3000 — both ingress-domains and ingress-mcp route exclusively to webserver-php-fpm:8080, and the webserver's nginx proxy to the storefront is in-namespace pod traffic already covered by the podSelector: {} peer below. This namespaceSelector peer is therefore unused attack surface under the shipped topology. Either drop it (least privilege) or, if it's deliberately kept for projects that add a direct storefront ingress via extraManifests, say that in the comment instead. — Claude Agent (CR)
There was a problem hiding this comment.
Implemented in ffc27ad - dropped the ingress-controller namespaceSelector peer from allow-storefront (least privilege). Confirmed against the shipped templates: both ingress-domains and ingress-mcp route exclusively to webserver-php-fpm:8080, so all storefront traffic arrives via the webserver's nginx, which the in-namespace podSelector: {} peer covers. The comment now says exactly that and points projects with a custom direct storefront ingress to networkPolicy.extraIngress. Unit test added asserting the storefront from list is in-namespace only.
| - namespaceSelector: | ||
| {{- toYaml .Values.networkPolicy.ingressControllerNamespace | nindent 12 }} | ||
| ports: | ||
| - port: 15672 |
There was a problem hiding this comment.
Low: 15672 is opened from the ingress-controller namespace unconditionally, but the management ingress only renders under {{- if and .Values.rabbitmq.enabled (or .Values.rabbitmq.management.hostname (gt (len .Values.domains) 0)) }}. When that condition is false there is no legitimate path from the ingress controller to the management UI, yet any pod in the ingress-controller namespace may still reach it (HTTP basic auth on the broker becomes the only barrier). Gating this rule on the same condition as ingress-rabbitmq.yaml would keep the allowlist minimal. — Claude Agent (CR)
There was a problem hiding this comment.
Implemented in ffc27ad. The 15672 rule is now gated on exactly the condition that renders ingress-rabbitmq.yaml (rabbitmq.management.hostname set or at least one domain), so the ingress-controller namespace only gets a path to the management UI when a legitimate route exists. Also gated allow-redis/allow-rabbitmq as wholes on redis.enabled/rabbitmq.enabled while touching this (they previously rendered as no-op policies for disabled workloads). Unit tests cover the 15672-omitted and workloads-disabled cases.
- Allow cert-manager's HTTP01 solver pods (port 8089): the solvers run in this namespace and every certificate issuance/renewal would fail under the default deny - only surfacing weeks later at renewal time. Unrestricted source on purpose (self-check source varies with LB/externalTrafficPolicy; the solver serves only the public challenge token) and a no-op on DNS01 clusters. - Declare networkPolicy in both values.schema.json (additionalProperties: false): a typo in a security toggle must fail the deploy, not silently render nothing while the operator believes the namespace is locked down. - Drop the ingress-controller peer from allow-storefront: no shipped ingress targets storefront:3000 - all storefront traffic arrives via the webserver's nginx (in-namespace); a project-specific direct ingress can use extraIngress. - Open RabbitMQ 15672 from the ingress controller only under the same condition that renders the management ingress; gate allow-redis/allow-rabbitmq on the workloads being enabled. - Document the DNS-anywhere residual risk (DNS tunneling) in values + docs. - Add a network-policies golden scenario locking the enabled state (incl. egress lockdown and extra ingress) under the full helmfile state-values path. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressing the non-inline "Info" finding from the review summary (no thread exists for it), resolved in ffc27ad together with the inline findings:
Full verification suite passed before push: helm unittest 99/99, golden 18/18, helm lint both charts, kubeconform -strict on the enabled renders (infra incl. egress lockdown, and app). |
…opt-in-network-policies
The scenario was generated before the parent branch gained the pod-level automountServiceAccountToken line and the values-driven hook contexts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…opt-in-network-policies # Conflicts: # charts/shopsys-infra/values.schema.json
|
I independently evaluated the NetworkPolicy portion of this PR using the public IaC-Guard-V The app + infra charts were materialized as one protected multi-chart universe. For the five selected workloads, Checkov IaC-Guard-V keeps all rendered resources in the governed universe, including resources that are not primary scanner targets, and returns a target-scoped candidate-acceptance This is candidate-property acceptance, not a claim that a baseline defect was Reproduction and immutable evidence: Verifier: |
Closes #9
Stacked on #50 — only the last commit belongs to this PR.
What
Opt-in NetworkPolicies (
networkPolicy.enabled: falseby default — zero golden impact). When enabled:infra chart (installed first, owns the namespace-wide pieces):
default-deny-ingressfor all podsallow-redis: 6379 from in-namespace pods, 9121 from the monitoring namespaceallow-rabbitmq: 5672 from in-namespace pods, 15672 from the ingress-controller namespace (management UI), 15692 from monitoringallow-extra-ingress: raw escape-hatch rules applied to all podsnetworkPolicy.egress.enabled): default-deny egress + DNS anywhere + in-namespace traffic + explicitegress.rulesapp chart:
allow-webserver: 8080 from the ingress-controller namespace + in-namespace pods (storefront'sINTERNAL_ENDPOINTcalls)allow-storefront: 3000 from the ingress-controller namespace + in-namespace pods (nginx proxies/_next/and@storefront)Namespace selectors for the ingress controller and monitoring are configurable (defaults:
kubernetes.io/metadata.name: ingress-nginx/monitoring).Notes
docs/values.md): enable on a dev environment first — the CNI must actually enforce policies, and kubelet probe behavior under a default-deny differs between CNIs.cron-suspendhook uses kubectl) must be listed inegress.rules.🤖 Generated with Claude Code