Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ helmfile -e devel diff
./deploy/deploy.sh devel # full deploy incl. slack, failure recovery, website check

# Tests (run all of these before considering a change done)
./tests/run-golden-tests.sh # snapshot tests (5 scenarios × 3 variants)
./tests/run-golden-tests.sh # snapshot tests (6 scenarios × 3 variants)
./tests/run-golden-tests.sh --update # regenerate snapshots after INTENTIONAL changes
./tests/run-golden-tests.sh basic-production # single scenario
helm unittest charts/shopsys-app charts/shopsys-infra
Expand Down
46 changes: 46 additions & 0 deletions charts/shopsys-app/templates/networkpolicy.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
{{- if .Values.networkPolicy.enabled }}
{{- /* The namespace-wide default deny (and the optional egress lockdown) live in the
infra chart; this chart allows traffic to its own workloads. */}}
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: allow-webserver
labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
podSelector:
matchLabels:
app: webserver-php-fpm
policyTypes:
- Ingress
ingress:
- from:
# e-shop and MCP ingresses
- namespaceSelector:
{{- toYaml .Values.networkPolicy.ingressControllerNamespace | nindent 12 }}
# storefront server-side requests (INTERNAL_ENDPOINT) and other app pods
- podSelector: {}
ports:
- port: 8080
---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: allow-storefront
labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
podSelector:
matchLabels:
app: storefront
policyTypes:
- Ingress
ingress:
- from:
# the webserver's nginx proxies /_next/ and @storefront to the storefront; no
# shipped ingress targets the storefront directly (use networkPolicy.extraIngress
# when a project adds one)
- podSelector: {}
ports:
- port: 3000
{{- end }}
53 changes: 53 additions & 0 deletions charts/shopsys-app/tests/networkpolicy_test.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
suite: opt-in network policies (app)
values:
- ./values/required.yaml
templates:
- templates/networkpolicy.yaml
tests:
- it: renders nothing by default
asserts:
- hasDocuments:
count: 0

- it: allows the ingress controller and in-namespace pods to reach the workloads
set:
networkPolicy:
enabled: true
asserts:
- hasDocuments:
count: 2
- equal:
path: spec.podSelector.matchLabels.app
value: webserver-php-fpm
documentIndex: 0
- equal:
path: spec.ingress[0].ports[0].port
value: 8080
documentIndex: 0
- equal:
path: spec.ingress[0].from[0].namespaceSelector.matchLabels["kubernetes.io/metadata.name"]
value: ingress-nginx
documentIndex: 0
- equal:
path: spec.ingress[0].ports[0].port
value: 3000
documentIndex: 1
# no shipped ingress targets the storefront - in-namespace traffic only
- equal:
path: spec.ingress[0].from
value:
- podSelector: {}
documentIndex: 1

- it: honors a custom ingress-controller namespace selector
set:
networkPolicy:
enabled: true
ingressControllerNamespace:
matchLabels:
role: edge
asserts:
- equal:
path: spec.ingress[0].from[0].namespaceSelector.matchLabels.role
value: edge
documentIndex: 0
18 changes: 18 additions & 0 deletions charts/shopsys-app/values.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,24 @@
"existingSecret": { "type": "string" }
}
},
"networkPolicy": {
"type": "object",
"additionalProperties": false,
"properties": {
"enabled": { "type": "boolean" },
"ingressControllerNamespace": { "type": "object" },
"monitoringNamespace": { "type": "object" },
"extraIngress": { "type": "array", "items": { "type": "object" } },
"egress": {
"type": "object",
"additionalProperties": false,
"properties": {
"enabled": { "type": "boolean" },
"rules": { "type": "array", "items": { "type": "object" } }
}
}
}
},
"app": {
"type": "object",
"properties": {
Expand Down
27 changes: 27 additions & 0 deletions charts/shopsys-app/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,33 @@ serviceAccount:
name: "" # generated from the chart name when empty
automountToken: false

# Opt-in NetworkPolicies (shared values section - both charts render their part):
# default-deny ingress for the whole namespace, allows for the chart workloads (plus
# cert-manager's HTTP01 solver pods, which run in this namespace), 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:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

enabled: false
# Selector of the namespace running the ingress controller
ingressControllerNamespace:
matchLabels:
kubernetes.io/metadata.name: ingress-nginx
# Selector of the namespace running Prometheus (exporter scraping)
monitoringNamespace:
matchLabels:
kubernetes.io/metadata.name: monitoring
# Escape hatch: raw NetworkPolicyIngressRule list applied to all pods in the namespace
extraIngress: []
egress:
# When enabled, egress is denied namespace-wide except DNS, in-namespace traffic and
# the rules below. External services (PostgreSQL, Elasticsearch, S3, SMTP) and the
# Kubernetes API (needed by the cron-suspend hook) MUST be listed here.
# DNS is allowed to ANY destination (the cluster DNS location differs per cluster and
# cannot be selected generically) - DNS tunneling remains possible under the lockdown.
enabled: false
# Raw NetworkPolicyEgressRule list
rules: []

app:
# Backend environment variables (webserver, cron, consumers, migration job, cron shell).
# Values MUST be strings - quote values like "479411e7" in YAML.
Expand Down
156 changes: 156 additions & 0 deletions charts/shopsys-infra/templates/networkpolicy.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,156 @@
{{- if .Values.networkPolicy.enabled }}
{{- /* Namespace-wide policies live in the infra chart (installed first): the default
ingress deny, the redis/rabbitmq allows, the extra-ingress escape hatch and the
optional egress lockdown. The app chart adds the webserver/storefront allows. */}}
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: default-deny-ingress

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
podSelector: {}
policyTypes:
- Ingress
{{- if .Values.redis.enabled }}
---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: allow-redis
labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
podSelector:
matchLabels:
app: redis
policyTypes:
- Ingress
ingress:
# application pods in this namespace
- from:
- podSelector: {}
ports:
- port: 6379
# prometheus scraping of the redis exporter
- from:
- namespaceSelector:
{{- toYaml .Values.networkPolicy.monitoringNamespace | nindent 12 }}
ports:
- port: 9121
{{- end }}
{{- if .Values.rabbitmq.enabled }}
---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: allow-rabbitmq
labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
podSelector:
matchLabels:
app: rabbitmq
policyTypes:
- Ingress
ingress:
# application pods in this namespace
- from:
- podSelector: {}
ports:
- port: 5672
{{- /* same condition as ingress-rabbitmq.yaml - no rule when no management ingress */}}
{{- if or .Values.rabbitmq.management.hostname (gt (len .Values.domains) 0) }}
# management UI through the ingress controller
- from:
- namespaceSelector:
{{- toYaml .Values.networkPolicy.ingressControllerNamespace | nindent 12 }}
ports:
- port: 15672

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{{- end }}
# prometheus scraping of the built-in exporter
- from:
- namespaceSelector:
{{- toYaml .Values.networkPolicy.monitoringNamespace | nindent 12 }}
ports:
- port: 15692
{{- end }}
---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: allow-acme-solver
labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
# cert-manager's HTTP01 solver pods are spawned in this namespace for the ingress
# certificates and must accept the challenge request on 8089, or every issuance and
# renewal fails under the default deny. Deliberately not restricted to the
# ingress-controller namespace: cert-manager's self-check may reach the solver with a
# different source (load-balancer/externalTrafficPolicy specifics), and the solver
# serves nothing but the public challenge token. Selects no pods on DNS01 clusters.
podSelector:
matchLabels:
acme.cert-manager.io/http01-solver: "true"
policyTypes:
- Ingress
ingress:
- ports:
- port: 8089
{{- with .Values.networkPolicy.extraIngress }}
---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: allow-extra-ingress
labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
podSelector: {}
policyTypes:
- Ingress
ingress:
{{- toYaml . | nindent 4 }}
{{- end }}
{{- if .Values.networkPolicy.egress.enabled }}
---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: default-deny-egress
labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
podSelector: {}
policyTypes:
- Egress
---
apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: allow-egress
labels:
{{- include "shopsys.labels" $ | nindent 4 }}
spec:
podSelector: {}
policyTypes:
- Egress
egress:
# DNS anywhere: the cluster DNS location differs per cluster (kube-system, node-local
# caches on link-local IPs) and cannot be selected generically. Residual risk: DNS
# tunneling remains a possible exfiltration path under the lockdown.
- ports:
- port: 53

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

protocol: UDP
- port: 53
protocol: TCP
# everything inside this namespace (redis, rabbitmq, webserver, storefront)
- to:
- podSelector: {}
# project-specific external services (PostgreSQL, Elasticsearch, S3, SMTP, the
# Kubernetes API for the cron-suspend hook, ...)
{{- with .Values.networkPolicy.egress.rules }}
{{- toYaml . | nindent 4 }}
{{- end }}
{{- end }}
{{- end }}
Loading