cilium: chaining mode skb->mark can be mangled by iptables allow opt-out #12185
Conversation
|
Please set the appropriate release note label. |
jrfastab
requested review from
joestringer,
aanm and
tgraf
and removed request for
joestringer
|
test-me-please |
|
fixes #12136 |
| @@ -307,6 +307,8 @@ data: | |||
| # - portmap (Enables HostPort support for Cilium) | |||
| cni-chaining-mode: {{ .Values.global.cni.chainingMode }} | |||
|
|
|||
| enable-identity-map: {{ .Values.global.enableIdentityMap }} | |||
There was a problem hiding this comment.
I think this needs to be:
{{- if hasKey .Values "enableIdentityMap" }}
enable-identity-map: {{ .Values.enableIdentityMap | quote }}
{{- else if (ne $enableIdentityMap "true") }}
enable-identity-map: "false"
{{- end }}
and on top of this file:
...
{{- /* Default values when 1.8 was initially deployed */ -}}
...
{{- $enableIdentityMap = "true" -}}
| @@ -186,6 +186,8 @@ global: | |||
| endpointRoutes: | |||
| enabled: false | |||
|
|
|||
| enableIdentityMark: true | |||
There was a problem hiding this comment.
this should be in the cilium/charts/config/values.yaml like the other flags are
The skb->mark field may not reliable in chaining modes because the host stack may have conflicting users of the mark field. For example in one case we observe a set mark rule 'MARK and 0xfff1ffff' which mangles the identity stored in skb->mark. If Cilium user also has ingress policy logic the identity is no longer correct. This likely will result in policy denied hits, -> stack flow 0xa0688256 identity 54898->7179 state established ifindex 0 orig-ip 0.0.0.0: 192.168.187.136:50344 -> 192.168.187.139:80 tcp SYN xx drop (Policy denied) flow 0xa0688256 to endpoint 149, identity 54897->7179: 192.168.187.136:50344 -> 192.168.187.139:80 tcp SYN To fix this create a flag EnableIdentityMark to allow setting the identity. In cases that have conflicting mark values this can then be disabled. The trace on ingress will no longer have a listed identity but because when 'identity < UNMANAGED' we do the lookup using the src ip and policy will work correctly. Fixes: f25d8b9 ("bpf: Preserve source identity for hairpin via stack") Signed-off-by: John Fastabend <john.fastabend@gmail.com>
|
test-me-please |
| # enables passing identity on local routes by using the mark fields. However, | ||
| # in cases where this conflicts with a chained CNI plugin it may be disabled. | ||
| #enableIdentityMark: true |
There was a problem hiding this comment.
I don't see the corresponding chaining docs changes for this, did you intend to add them here?
There was a problem hiding this comment.
We should set it in the Calico guide
| {{- if hasKey .Values "enableIdentityMap"}} | ||
| enable-identity-map: {{ .Values.global.enableIdentityMap | quote }} | ||
| {{- else if (ne $enableIdentityMap "true") }} | ||
| enable-identity-map: "false" | ||
| {{- end }} |
There was a problem hiding this comment.
@tgraf has the freshest understanding of exactly how to format these options, would be good to get a quick look from him whether it's actually matching our latest approach.
joestringer
added
the
release-note/bug
| # enables passing identity on local routes by using the mark fields. However, | ||
| # in cases where this conflicts with a chained CNI plugin it may be disabled. | ||
| #enableIdentityMark: true |
There was a problem hiding this comment.
We should set it in the Calico guide
joestringer
added
the
priority/high
|
Just docs fixes remaining, those can be handled in a follow-up PR. |
maintainer-s-little-helper
bot
moved this from Needs backport from master
to Backport pending to v1.8
in 1.8.0
maintainer-s-little-helper
bot
moved this from Backport pending to v1.8
to Backport done to v1.8
in 1.8.0
| @@ -349,6 +350,12 @@ data: | |||
| # - portmap (Enables HostPort support for Cilium) | |||
| cni-chaining-mode: {{ .Values.global.cni.chainingMode }} | |||
|
|
|||
| {{- if hasKey .Values "enableIdentityMap"}} | |||
| enable-identity-map: {{ .Values.global.enableIdentityMap | quote }} | |||
There was a problem hiding this comment.
Don't we have a mismatch here? The new argument in the daemon is enable-identity-mark, but here we define enable-identity-map. Shouldn't we do s/map/mark/ in helm charts?
Am I missing something here?
I noticed that when backporting to 1.7.
There was a problem hiding this comment.
Ah, sorry, it got fixed here #12194
joestringer
moved this from Needs backport from master
to Backport pending to v1.7
in 1.7.7
The skb->mark field may not reliable in chaining modes because the host
stack may have conflicting users of the mark field. For example in one
case we observe a set mark rule 'MARK and 0xfff1ffff' which mangles the
identity stored in skb->mark. If Cilium user also has ingress policy
logic the identity is no longer correct. This likely will result in policy
denied hits,
-> stack flow 0xa0688256 identity 54898->7179 state established ifindex 0 orig-ip 0.0.0.0: 192.168.187.136:50344 -> 192.168.187.139:80 tcp SYN
xx drop (Policy denied) flow 0xa0688256 to endpoint 149, identity 54897->7179: 192.168.187.136:50344 -> 192.168.187.139:80 tcp SYN
To fix this create a flag EnableIdentityMark to allow setting the identity.
In cases that have conflicting mark values this can then be disabled. The
trace on ingress will no longer have a listed identity but because when
'identity < UNMANAGED' we do the lookup using the src ip and policy will
work correctly.
Fixes: f25d8b9 ("bpf: Preserve source identity for hairpin via stack")
Signed-off-by: John Fastabend john.fastabend@gmail.com