Skip to content

Commit 2e4783b

Browse files
myasnikovdaniilmyasnikovdaniil
committed
fix(networking): enforce the pod admission deny from both ends
Backport of #3997 to `release-1.6`, kube-ovn-webhook half. The namespaced CiliumNetworkPolicy/kube-ovn-webhook carried a cluster-spanning fromEndpoints peer on the webhook's own endpoint, so every namespace add or delete rewrote that endpoint's policy map -- the coupling #3983 reported against fail-closed admission webhooks. It also depended on nothing but that one end: while the map is being recomputed or has overflowed, the deny is the thing being rebuilt. What ships now is one cluster-scoped CiliumClusterwideNetworkPolicy/kube-ovn-webhook-c2bdbc146112 carrying two specs that fail in different directions. The source-side spec is an egressDeny on every pod endpoint naming the webhook's backend port, so it holds while the webhook endpoint's own policy is degraded, but it relies on Cilium performing the service DNAT ahead of its egress hook. The target-side spec keeps the pod deny and the world deny on the webhook endpoint, evaluated after whatever DNAT already happened, so it holds where Cilium is not the service load balancer. Neither subsumes the other, so the churn writes stop being load-bearing rather than going away -- the template says so rather than claiming the coupling is removed. `toServices` is deliberately not used to name the Service frontend, and the template carries the reason: Cilium resolves it only in allow rules (pkg/policy/k8s/service.go walks spec.Egress, never spec.EgressDeny), so an egressDeny whose only destination is an unresolved toServices reaches the datapath with an empty L3 selector that Cilium reads as a wildcard when the rule carries toPorts -- a cluster-wide egress outage on that port rather than a tighter deny. The object changes kind and name, so anything keyed on the old namespaced CiliumNetworkPolicy/kube-ovn-webhook goes quiet with no error. The template now also renders only while cilium.io/v2/CiliumClusterwideNetworkPolicy is discoverable, so `enabled: true` is no longer a promise that a policy exists; set `enabled: false` to record a deliberate opt-out. Assisted-By: LLM Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent 7c310e3 commit 2e4783b

3 files changed

Lines changed: 157 additions & 49 deletions

File tree

‎packages/system/kubeovn-webhook/templates/_helpers.tpl‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,11 +7,18 @@ kube-ovn-webhook
77
{{- end }}
88

99
{{/*
10-
The port the webhook serves on. Defined once and consumed by the container
11-
port, the Service targetPort and the CiliumNetworkPolicy, because those three
10+
The port the webhook serves on. Defined once and consumed by the container,
11+
the Service targetPort and the Cilium clusterwide policy, because those three
1212
must agree: the policy has default-deny disabled, so a policy pointed at a port
1313
nothing serves denies nothing while the real port stays open -- a drift that
1414
removes the control silently rather than breaking anything visibly.
15+
16+
The Service frontend port is deliberately NOT defined here. The policy denies
17+
the backend port only, which already covers ClusterIP-addressed traffic because
18+
Cilium translates a service to its backends before enforcing egress policy, so
19+
there is no second value that has to stay in agreement. That translation order
20+
is itself a pinned assumption -- see
21+
packages/system/cilium/tests/kube_proxy_replacement_test.yaml.
1522
*/}}
1623
{{- define "namespace-annotation-webhook.port" -}}
1724
8443

‎packages/system/kubeovn-webhook/templates/networkpolicy.yaml‎

Lines changed: 124 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -1,64 +1,146 @@
1-
{{- if .Values.webhookPolicy.enabled }}
1+
{{- $policy := .Values.webhookPolicy | default dict -}}
2+
{{- if and (dig "enabled" true $policy) (.Capabilities.APIVersions.Has "cilium.io/v2/CiliumClusterwideNetworkPolicy") }}
3+
{{- $policyHash := printf "%s/%s" .Release.Namespace .Release.Name | sha256sum | trunc 12 -}}
24
---
35
{{/*
4-
The /mutate-pods handler answers whoever asks it with namespace-derived
5-
annotations, and only the API server has any business calling it
6-
(GHSA-g883-q79m-8225). Client-certificate verification cannot close this on
7-
its own: enabling --client-ca-file requires the API server to *present* a
8-
certificate, which it does only when its AdmissionConfiguration names a
9-
kubeConfigFile for the webhook plugins. A stock Cozystack control plane ships
10-
a PodSecurity plugin block and nothing else, so no certificate is offered and
11-
RequireAndVerifyClientCert would reject the API server itself -- with
12-
failurePolicy: Fail that stops pod creation cluster-wide. Until that changes,
13-
the network is the only place the caller can actually be constrained.
6+
The /mutate-pods handler returns namespace-derived annotations to any caller,
7+
and only the API server has any business calling it (GHSA-g883-q79m-8225).
8+
Client-certificate verification cannot enforce that on the stock control plane
9+
because the API server presents no client certificate to this webhook.
1410

15-
This is a CiliumNetworkPolicy, not a core NetworkPolicy, and the distinction
16-
is the whole reason it works: packages/system/kubeovn/values.yaml sets
11+
This is a CiliumClusterwideNetworkPolicy, not a core NetworkPolicy, and the
12+
distinction is why it works: packages/system/kubeovn/values.yaml sets
1713
ENABLE_NP: false, so kube-ovn programs no ACLs and a core NetworkPolicy would
18-
be created and never enforced. Cilium enforces its own policies regardless,
19-
which is why the platform already relies on them elsewhere.
14+
be created and never enforced. Cilium enforces its own policies regardless.
2015

21-
Deny-only, with default-deny explicitly disabled. A Cilium rule carrying an
22-
ingress section puts the selected endpoints behind a default-deny, which here
23-
would mean enumerating every port these pods serve and silently breaking
24-
traffic on the first one forgotten. Subtracting one port leaves everything
25-
else exactly as it was.
16+
Deny-only, and expressed from both ends because each end fails in a direction
17+
the other does not. The source-side spec is an egressDeny on the caller's own
18+
endpoint, so it holds while this webhook's endpoint policy is being recomputed
19+
or has overflowed its map, but it relies on Cilium performing the service DNAT
20+
before egress policy runs. The target-side spec is an ingressDeny on this
21+
webhook's endpoint, evaluated after whatever DNAT already happened, so it is
22+
independent of the service-LB implementation, but denying pods there needs a
23+
cluster-spanning peer enumerated per identity on a fail-closed endpoint.
2624

27-
The API server needs no carve-out, and this was confirmed on a live kube-ovn
28-
cluster rather than reasoned about: cilium monitor on the webhook's endpoint
29-
shows the API server's connection arriving from the node InternalIP with
30-
identity "host" when it originates on the same node and "kube-apiserver" when
31-
it originates on another, never from the kube-ovn join subnet and never as
32-
"world". Neither identity is a Cilium endpoint, so fromEndpoints cannot match
33-
it and the world deny does not apply to it. Unlike the ingress-nginx admission
34-
webhook, this component exists only on the management cluster, so there is no
35-
konnectivity-tunnelled caller to exempt either.
25+
Be exact about what that means: the source-side spec was added to get a
26+
cluster-spanning selector OFF this endpoint, and the target-side spec puts one
27+
back. Shipping both does not eliminate the namespace-churn map writes here; it
28+
makes them non-load-bearing, because whichever end is degraded the other still
29+
denies, and the writes are writes rather than an outage (see the cost paragraph
30+
on the second spec). The alternative -- dropping the target-side rule -- buys
31+
the coupling back at the price of a silent fail-open wherever Cilium does not
32+
own the DNAT.
33+
34+
That coupling was not reproduced in isolation as the trigger of #3983, and the
35+
errno the issue reports cannot come from a policy denial; see the PR
36+
description. The world deny below, the other candidate that issue named, is
37+
retained on the evidence quoted further down. enableDefaultDeny stays false, so
38+
the policy subtracts one destination and port and leaves everything else
39+
exactly as it was.
40+
41+
Only the BACKEND port is denied, and that is sufficient. Cilium translates a
42+
service to its backends BEFORE egress policy is enforced, so a pod dialling
43+
this webhook's ClusterIP:443 is matched against the backend identity on the
44+
port below. Do not add a `toServices` rule to cover the ClusterIP explicitly:
45+
Cilium resolves `toServices` only inside ALLOW rules
46+
(pkg/policy/k8s/service.go walks spec.Egress, never spec.EgressDeny), and an
47+
egressDeny whose only destination is an unresolved `toServices` reaches the
48+
datapath with a non-nil-but-empty L3 selector, which Cilium reads as a
49+
WILDCARD when the rule carries toPorts (pkg/policy/types/policyentry.go). That
50+
denies EVERY destination on that port for every selected endpoint -- a
51+
cluster-wide egress outage rather than a tighter deny.
52+
53+
That ordering is Cilium's own and it is conditional: it holds while Cilium owns
54+
service load-balancing, which on every cozystack variant it does because
55+
packages/system/cilium/values.yaml sets kubeProxyReplacement: true against an
56+
upstream chart default of "false". Set that false and the ClusterIP DNAT moves
57+
downstream of Cilium's egress hook, the packet reaches policy_can_egress4 with
58+
dst = ClusterIP, matches no endpoint identity, and -- default-deny being off --
59+
is allowed, restoring pod access to /mutate-pods with nothing to look at.
60+
packages/system/cilium/tests/kube_proxy_replacement_test.yaml pins that value
61+
and packages/system/cilium/templates/kube-proxy-replacement-beacon.yaml renders
62+
a status ConfigMap whenever it is not true, which is the only signal an operator
63+
override in the field can produce. The second spec below then keeps enforcing
64+
whoever performs the DNAT.
65+
66+
The API server and host-network workloads are not selected: a rule carrying an
67+
endpointSelector can never match the host endpoint, because Cilium routes host
68+
policy through nodeSelector instead (pkg/policy/rule.go, matchesSubject). This
69+
component exists only on the management cluster, so there is no
70+
konnectivity-tunnelled caller to exempt.
71+
72+
The second spec is target-side, on the webhook endpoint, and carries two denies
73+
kept for different reasons.
74+
75+
Its pod deny is deliberate belt and braces. Being on the destination's ingress
76+
path it is evaluated after whatever DNAT has already happened, so it holds even
77+
where Cilium is not the service load balancer -- exactly the exposure the
78+
paragraph above describes. It cannot catch the API server: for a clusterwide
79+
policy Cilium rewrites every fromEndpoints selector to also require
80+
k8s:io.kubernetes.pod.namespace to Exist, plus the cluster label (Cilium
81+
v1.19.5, pkg/k8s/apis/cilium.io/utils/utils.go:120-154 getEndpointSelector,
82+
whose own comment at :141 says that even a wildcard must not become a truly
83+
empty allow-all selector), and reserved identities carry neither label. What
84+
WOULD catch it is `fromEntities: [all]` or an omitted peer, so the shape is
85+
pinned in tests/networkpolicy_test.yaml rather than left to review.
86+
87+
Its cost, stated rather than denied: a cluster-spanning fromEndpoints peer is
88+
enumerated per identity, so this rule adds policy-map entries on the webhook
89+
endpoint in proportion to the number of pod identities in the cluster, and a
90+
map write on every namespace add or delete. It does not cost a drop window --
91+
pkg/endpoint/bpf.go:1168-1177 adds entries before deleting them expressly to
92+
avoid transient drops, and inverts that order only when the addition would
93+
overflow the map, warning when it does (:1180-1182). The residual risk is
94+
therefore cardinality rather than churn: an endpoint whose policy map
95+
approaches MaxEntries gets the inverted order and a possible transient drop,
96+
and cozystack leaves endpointLockdownOnMapOverflow at false
97+
(charts/cilium/values.yaml:1250), so an overflow degrades rather than locking
98+
the endpoint down.
99+
100+
Its world deny is retained rather than relied upon for the API server:
101+
`cilium monitor` on this webhook's endpoint on a live kube-ovn cluster showed
102+
the API server arriving from the node InternalIP with identity `host` when it
103+
originates on the same node and `kube-apiserver` when it originates on
104+
another, never from the kube-ovn join subnet and never as `world`.
36105

37106
Note the scope this actually has: it removes in-cluster pods and external
38107
traffic, not every possible caller. Any host-network workload on the
39108
management cluster carries the same host / remote-node identity that exempts
40109
the API server and keeps its reach.
41110
*/}}
42111
apiVersion: cilium.io/v2
43-
kind: CiliumNetworkPolicy
112+
kind: CiliumClusterwideNetworkPolicy
44113
metadata:
45-
name: {{ include "namespace-annotation-webhook.name" . }}
46-
namespace: {{ .Release.Namespace }}
47-
spec:
48-
endpointSelector:
114+
name: kube-ovn-webhook-{{ $policyHash }}
115+
specs:
116+
- endpointSelector: {}
117+
enableDefaultDeny:
118+
ingress: false
119+
egress: false
120+
egressDeny:
121+
- toEndpoints:
122+
- matchLabels:
123+
"k8s:io.kubernetes.pod.namespace": {{ .Release.Namespace | quote }}
124+
"k8s:app.kubernetes.io/name": {{ include "namespace-annotation-webhook.name" . | quote }}
125+
"k8s:app.kubernetes.io/instance": {{ .Release.Name | quote }}
126+
toPorts:
127+
- ports:
128+
- port: {{ include "namespace-annotation-webhook.port" . | quote }}
129+
protocol: TCP
130+
- endpointSelector:
49131
matchLabels:
50-
app.kubernetes.io/name: {{ include "namespace-annotation-webhook.name" . }}
51-
app.kubernetes.io/instance: {{ .Release.Name }}
132+
"k8s:io.kubernetes.pod.namespace": {{ .Release.Namespace | quote }}
133+
"k8s:app.kubernetes.io/name": {{ include "namespace-annotation-webhook.name" . | quote }}
134+
"k8s:app.kubernetes.io/instance": {{ .Release.Name | quote }}
52135
enableDefaultDeny:
53136
ingress: false
137+
egress: false
54138
ingressDeny:
55-
{{- /*
56-
Every pod in the cluster. The namespace key is matched with Exists so the rule
57-
spans all namespaces: a plain fromEndpoints in a namespaced
58-
CiliumNetworkPolicy would only ever select pods sharing this policy's
59-
namespace, which would leave the webhook open to every other one.
60-
*/}}
61139
- fromEndpoints:
140+
# Every pod in the cluster. The namespace key is matched with Exists so the
141+
# rule spans all namespaces rather than only this policy's own, and so the
142+
# selector is explicit about the requirement Cilium would otherwise add for
143+
# it (see the comment above).
62144
- matchExpressions:
63145
- key: k8s:io.kubernetes.pod.namespace
64146
operator: Exists

‎packages/system/kubeovn-webhook/values.yaml‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,16 +12,35 @@ image: ghcr.io/cozystack/cozystack/kubeovn-webhook:v1.6.2@sha256:8abc089c4469b65
1212
# ships only a PodSecurity block. Until that changes, this policy is the
1313
# enforcement.
1414
#
15-
# Rendered as a CiliumNetworkPolicy on purpose: kube-ovn runs with
15+
# Rendered as a CiliumClusterwideNetworkPolicy on purpose: kube-ovn runs with
1616
# ENABLE_NP: false, so a core NetworkPolicy object would be created and never
17-
# enforced. Set enabled: false on a cluster whose networking variant ships no
18-
# Cilium, where the object would have no controller to act on it.
17+
# enforced. The template renders only when that Cilium API is discoverable; set
18+
# enabled: false to opt out explicitly.
1919
#
2020
# The port is deliberately not configurable here. It has to agree with the
2121
# container port and the Service targetPort, and because the policy runs with
2222
# default-deny disabled, a disagreement fails open: the policy would deny a port
2323
# nothing serves while the real one stayed reachable, removing this control
24-
# without breaking anything visible. All three read one definition instead --
25-
# see namespace-annotation-webhook.port in templates/_helpers.tpl.
24+
# without breaking anything visible. The source-side endpoint deny and the
25+
# target-side world deny both read that one definition -- see
26+
# namespace-annotation-webhook.port in templates/_helpers.tpl. Only the backend
27+
# port is denied; Cilium translates a service to its backends before enforcing
28+
# egress policy, so ClusterIP-addressed traffic is matched there too. That last
29+
# step depends on Cilium owning service load-balancing --
30+
# packages/system/cilium/values.yaml kubeProxyReplacement, true there against an
31+
# upstream default of "false", pinned by
32+
# packages/system/cilium/tests/kube_proxy_replacement_test.yaml, which carries
33+
# the argument. Flip it and the ClusterIP path to /mutate-pods reopens while the
34+
# backend deny keeps working -- except that the policy's target-side rule does
35+
# not depend on that value at all, being evaluated on the webhook's own ingress
36+
# path after whatever DNAT already happened, so the two together degrade rather
37+
# than fail open.
38+
#
39+
# Residual risk worth stating: no test in this repo can observe a live cluster's
40+
# Cilium values, so an operator setting kubeProxyReplacement: false in the field
41+
# is caught only by the status ConfigMap that
42+
# packages/system/cilium/templates/kube-proxy-replacement-beacon.yaml renders.
43+
# Whether that configuration is supported at all is a platform question this
44+
# chart does not answer.
2645
webhookPolicy:
2746
enabled: true

0 commit comments

Comments
 (0)