cilium: chaining mode skb->mark can be mangled by iptables allow opt-out#12185
Merged
joestringer merged 1 commit intomasterfrom Jun 18, 2020
Merged
cilium: chaining mode skb->mark can be mangled by iptables allow opt-out#12185joestringer merged 1 commit intomasterfrom
joestringer merged 1 commit intomasterfrom
Conversation
|
Please set the appropriate release note label. |
jrfastab
requested review from
aanm,
joestringer and
tgraf
and removed request for
joestringer
Contributor
Author
|
test-me-please |
Contributor
Author
|
fixes #12136 |
aanm
requested changes
Jun 18, 2020
Member
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" -}}
Member
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 <[email protected]>
Contributor
Author
|
test-me-please |
joestringer
approved these changes
Jun 18, 2020
Comment on lines
+50
to
+52
| # 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 |
Member
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?
Contributor
There was a problem hiding this comment.
We should set it in the Calico guide
Comment on lines
+353
to
+357
| {{- if hasKey .Values "enableIdentityMap"}} | ||
| enable-identity-map: {{ .Values.global.enableIdentityMap | quote }} | ||
| {{- else if (ne $enableIdentityMap "true") }} | ||
| enable-identity-map: "false" | ||
| {{- end }} |
Member
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
tgraf
approved these changes
Jun 18, 2020
Comment on lines
+50
to
+52
| # 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 |
Contributor
There was a problem hiding this comment.
We should set it in the Calico guide
joestringer
added
the
priority/high
vadorovsky
reviewed
Jul 8, 2020
| cni-chaining-mode: {{ .Values.global.cni.chainingMode }} | ||
|
|
||
| {{- if hasKey .Values "enableIdentityMap"}} | ||
| enable-identity-map: {{ .Values.global.enableIdentityMap | quote }} |
Member
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.
julianwiedmann
added a commit
to julianwiedmann/cilium
that referenced
this pull request
Apr 4, 2025
…ENTITY For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see cilium#12185). Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 4, 2025
When AWS-CNI is in control of orchestrating the connectivity on the node, we shouldn't assume that usage of skb->mark for Cilium is safe. #12185 introduced the `--enable-identity-mark` option for this scenario, so that Cilium doesn't use the skb->mark for identity propagation. Set this flag in the AWS-CNI workflow accordingly. Signed-off-by: Julian Wiedmann <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 4, 2025
…ENTITY For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see #12185). Signed-off-by: Julian Wiedmann <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 4, 2025
When AWS-CNI is in control of orchestrating the connectivity on the node, we shouldn't assume that usage of skb->mark for Cilium is safe. #12185 introduced the `--enable-identity-mark` option for this scenario, so that Cilium doesn't use the skb->mark for identity propagation. Set this flag in the AWS-CNI workflow accordingly. Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 4, 2025
…ENTITY [ upstream commit 2566ce2 ] For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see #12185). Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 4, 2025
[ upstream commit c73b5f0 ] When AWS-CNI is in control of orchestrating the connectivity on the node, we shouldn't assume that usage of skb->mark for Cilium is safe. #12185 introduced the `--enable-identity-mark` option for this scenario, so that Cilium doesn't use the skb->mark for identity propagation. Set this flag in the AWS-CNI workflow accordingly. Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 7, 2025
…ENTITY [ upstream commit 2566ce2 ] For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see #12185). Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 7, 2025
[ upstream commit c73b5f0 ] When AWS-CNI is in control of orchestrating the connectivity on the node, we shouldn't assume that usage of skb->mark for Cilium is safe. #12185 introduced the `--enable-identity-mark` option for this scenario, so that Cilium doesn't use the skb->mark for identity propagation. Set this flag in the AWS-CNI workflow accordingly. Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 7, 2025
…ENTITY [ upstream commit 2566ce2 ] For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see #12185). Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 7, 2025
[ upstream commit c73b5f0 ] When AWS-CNI is in control of orchestrating the connectivity on the node, we shouldn't assume that usage of skb->mark for Cilium is safe. #12185 introduced the `--enable-identity-mark` option for this scenario, so that Cilium doesn't use the skb->mark for identity propagation. Set this flag in the AWS-CNI workflow accordingly. Signed-off-by: Julian Wiedmann <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 7, 2025
…ENTITY [ upstream commit 2566ce2 ] For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see #12185). Signed-off-by: Julian Wiedmann <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 7, 2025
[ upstream commit c73b5f0 ] When AWS-CNI is in control of orchestrating the connectivity on the node, we shouldn't assume that usage of skb->mark for Cilium is safe. #12185 introduced the `--enable-identity-mark` option for this scenario, so that Cilium doesn't use the skb->mark for identity propagation. Set this flag in the AWS-CNI workflow accordingly. Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 8, 2025
…ENTITY [ upstream commit 2566ce2 ] For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see #12185). Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 8, 2025
[ upstream commit c73b5f0 ] When AWS-CNI is in control of orchestrating the connectivity on the node, we shouldn't assume that usage of skb->mark for Cilium is safe. #12185 introduced the `--enable-identity-mark` option for this scenario, so that Cilium doesn't use the skb->mark for identity propagation. Set this flag in the AWS-CNI workflow accordingly. Signed-off-by: Julian Wiedmann <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 8, 2025
…ENTITY [ upstream commit 2566ce2 ] For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see #12185). Signed-off-by: Julian Wiedmann <[email protected]>
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 8, 2025
[ upstream commit c73b5f0 ] When AWS-CNI is in control of orchestrating the connectivity on the node, we shouldn't assume that usage of skb->mark for Cilium is safe. #12185 introduced the `--enable-identity-mark` option for this scenario, so that Cilium doesn't use the skb->mark for identity propagation. Set this flag in the AWS-CNI workflow accordingly. Signed-off-by: Julian Wiedmann <[email protected]>
julianwiedmann
added a commit
that referenced
this pull request
Apr 9, 2025
…ENTITY [ upstream commit 2566ce2 ] For environments where option.EnableIdentityMark is not set (and thus from-container doesn't set the source identity in the skb->mark), we shouldn't attempt to retrieve the identity. Which leaves the question whether all the *other* use of skb->mark in this program is safe. Or all those features are simply incompatible with running in a CNI-chained environment, where skb->mark is not owned by Cilium (see #12185). Signed-off-by: Julian Wiedmann <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 [email protected]