Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Limiting Identity-Relevant Labels breaks CiliumNetworkPolicies #18878

Closed
2 tasks done
alex-berger opened this issue Feb 22, 2022 · 14 comments · Fixed by #31178
Closed
2 tasks done

Limiting Identity-Relevant Labels breaks CiliumNetworkPolicies #18878

alex-berger opened this issue Feb 22, 2022 · 14 comments · Fixed by #31178
Labels
kind/bug This is a bug in the Cilium logic. sig/agent Cilium agent related. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.

Comments

@alex-berger
Copy link
Contributor

Is there an existing issue for this?

  • I have searched the existing issues

What happened?

Limiting Identity-Relevant Labels by setting the labels value in the cilium-config ConfigMap breaks CiliumNetworkPolicies which use cluster in fromEntities. All traffic is dropped as cilium does not recognize that it originates from in-cluster peers (cluster).

I suspect that this is caused by the fact that the k8s:io.cilium.k8s.policy.cluster identity label is not (automatically) set as soon as there are any inclusive label rules.


Given the following CiliumNetworkPolicy:

apiVersion: cilium.io/v2
kind: CiliumNetworkPolicy
metadata:
  name: coredns
  namespace: kube-system
spec:
  endpointSelector:
    matchLabels:
      k8s-app: coredns
  ingress:
    # Allow DNS access from any namespace and pod inside this Kubernetes cluster.
    - fromEntities:
        - cluster
      toPorts:
        - ports:
            - port: "53"
              protocol: UDP
        - ports:
            - port: "53"
              protocol: TCP

Scenario 1 - Identity-Relevant Labels are not constrained

Using Cilium's default settings not constraining (limiting) the set of identity-relevant labels, everything works as expected and all in-cluster peers (Pods) can successfully connect to the coredns Pods protected by the above CiliumNetworkPolicy. In that case the .status.identity.labels in the coredns' corresponding CiliumEndpoint object looks like this:

status:
  ...
  identity:
    ...
    labels:
      - k8s:app.kubernetes.io/instance=coredns
      - k8s:app.kubernetes.io/name=coredns
      - >-
        k8s:io.cilium.k8s.namespace.labels.config.linkerd.io/admission-webhooks=disabled
      - >-
        k8s:io.cilium.k8s.namespace.labels.kubernetes.io/metadata.name=kube-system
      - k8s:io.cilium.k8s.policy.cluster=default
      - k8s:io.cilium.k8s.policy.serviceaccount=coredns
      - k8s:io.kubernetes.pod.namespace=kube-system
      - k8s:k8s-app=coredns

Scenario 2 - Identity-Relevant Labels are constrained

Constraining (limiting) the set of identity-relevant labels, by setting labels in the cilium-config ConfigMap like this:

...
data:
  labels: >-
    k8s:app k8s:k8s-app k8s:kubernetes.io/metadata.name
...

Traffic from in-cluster peers (Pods) to the coredns Pods is rejected (dropped) and the .status.identity.labels in the coredns' corresponding CiliumEndpoint object looks like this:

status:
  ...
  identity:
    ...
    labels:
      - k8s:app.kubernetes.io/instance=coredns
      - k8s:app.kubernetes.io/name=coredns
      - >-
        k8s:io.cilium.k8s.namespace.labels.config.linkerd.io/admission-webhooks=disabled
      - >-
        k8s:io.cilium.k8s.namespace.labels.kubernetes.io/metadata.name=kube-system
      - k8s:io.kubernetes.pod.namespace=kube-system
      - k8s:k8s-app=coredns

The notable differences between scenarios 1 and 2 are that in 2 the labels k8s:io.cilium.k8s.policy.cluster=default and k8s:io.cilium.k8s.policy.serviceaccount=coredns are not present in the CiliumEndpoint.

Workaround

Explicitly allowing k8s:io.cilium.k8s.policy prefixed labels by adding the following additional inclusive label rules in the cilium-config ConfigMap (as outlined below) resolved the problem.

...
data:
  labels: >-
    k8s:app k8s:k8s-app k8s:kubernetes.io/metadata.name k8s:io.cilium.k8s.policy
...

With that setting, bot missing identity labels (k8s:io.cilium.k8s.policy.cluster=default and k8s:io.cilium.k8s.policy.serviceaccount=coredns) are set in the corresponding CiliumEndpoint and in-cluster clients can successfully connect to the coredns Pods.

Expected Behavior

The prefix k8s:io.cilium.k8s.policy (or maybe even k8s:io.cilium.k8s) should always be automatically added to the set of identity-relevant labels.

Cilium Version

1.11.1

Kernel Version

  • 5.4.156
  • 5.10.75

Kubernetes Version

1.21 (EKS)

Sysdump

No response

Relevant log output

No response

Anything else?

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@alex-berger alex-berger added kind/bug This is a bug in the Cilium logic. needs/triage This issue requires triaging to establish severity and next steps. labels Feb 22, 2022
@aanm
Copy link
Member

aanm commented Mar 3, 2022

Hello @alex-berger thanks for opening the GH issue, can you provide a Cilium sysdump for the working and non-working case of both source and destination nodes? Thank you!

@aanm aanm added need-more-info More information is required to further debug or fix the issue. sig/agent Cilium agent related. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies. labels Mar 3, 2022
@aanm aanm self-assigned this Mar 3, 2022
@aanm
Copy link
Member

aanm commented Mar 3, 2022

@alex-berger no need for the Cilium sysdump. After giving a little bit of consideration we think it's probably better to document this corner case in Limiting Identity-Relevant Labels instead of automatically allowing k8s:io.cilium.k8s.policy. Would you care to submit a PR with such changes? Thank you!

@aanm aanm assigned alex-berger and unassigned aanm Mar 3, 2022
@alex-berger
Copy link
Contributor Author

@aanm I don't think that this is a corner case. Entities like for example cluster and kube-apiserver should work out-of-the-box, not needing any very specific configuration quirks.

IMHO the relevant labels (or their prefixes) should always be added to the set of identity-relevant labels. Otherwise this will just cause a lot of (bug) tickets on GitHub as people run into problems when using those entities in their CiliumNetworkPolicies.

@github-actions
Copy link

This issue has been automatically marked as stale because it has not
had recent activity. It will be closed if no further activity occurs.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Mar 18, 2023
@github-actions
Copy link

github-actions bot commented Apr 2, 2023

This issue has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 2, 2023
@soggiest
Copy link
Contributor

soggiest commented Feb 16, 2024

I want to re-open this issue, I ran into this exact corner case. Since Entities are special selectors I would expect them to be applied despite whatever is configured for Security-Relevant Identities. If someone wants to be more specific in their network policies than what is provided by the Entities they should use one of the other policy rule constructs such as toEndpoints

@ruslanys
Copy link

I agree with @soggiest.

I found an issue that I cannot use pod-index, because Cilium ignores it even when I declare implicitly that label in cilium-config.

We have a use-case where a StatefulSet from 10 pods are utilising different egress gateways. The idea was to use pod-index label, and route traffic from a 10 different gateways, however as Cilium doesn't respect a "labels" declaration and removes pod-index we could not do this way.

@joestringer joestringer reopened this Feb 16, 2024
@joestringer joestringer removed need-more-info More information is required to further debug or fix the issue. needs/triage This issue requires triaging to establish severity and next steps. stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Feb 16, 2024
@soggiest
Copy link
Contributor

Thank you @joestringer !

@pjablonski123
Copy link

Apart from the standard list what about adding a new label "the initial identity of the pod" to the list of identity-relevant labels? This could help to track changes and also built dynamic network policies which prevent or make spoofing more difficult.

@joestringer
Copy link
Member

@pjablonski123 could you expand on your suggestion a bit more? An Identity is a list of labels, which are derived from Pods. The configurable daemon argument --labels decides which labels to either include or exclude in the Identities. I'm not sure I understand how the Identity could be part of a label inclusion/exclusion set.

@ruslanys
Copy link

ruslanys commented Feb 20, 2024

@joestringer could you please consider an option to allow explicitly including pod-index in the mentioned --labels?

@joestringer
Copy link
Member

@ruslanys I don't understand your proposal. Is this directly related to limiting identity-relevant labels? You may also consider filing a fresh issue describing exactly the problem that you are facing, how you think that a pod-index feature could help, and exactly the behaviour that you think this flag should implement.

@pjablonski123
Copy link

@pjablonski123 could you expand on your suggestion a bit more? An Identity is a list of labels, which are derived from Pods. The configurable daemon argument --labels decides which labels to either include or exclude in the Identities. I'm not sure I understand how the Identity could be part of a label inclusion/exclusion set.

I mean an additional label derived from the current identity, eg. k8s:io.cilium.k8s.pod.identity=50427
This would help to verify immutability of pod identities. Adding this label would be needed after the identity number allocation so it would require an exception to avoid a constant loop of changing labels and generating a new identity.

How this identity label could help with the security? Upon a new deployment of pods a network policy could be generated based on the assigned label identities. Any attempt of the identity manipulation could lead to:

  • a restoration of a original immutable identity label
  • protection against spoofing of identities on the destination host which would still have a network policy with the original identity label

@joestringer
Copy link
Member

@pjablonski123 it sounds like this is a much bigger proposal that is distinct from the question in this issue, and it may be trying to mitigate some sort of potential control plane attacker or malicious node attacker scenarios. In general I would consider that Identities are mostly immutable at the moment so I'm not sure I understand the notion of "restoring immutable identity labels". Perhaps if we think that the mutability constraints around Identities are not strong enough then we could harden the control plane to provide stronger guarantees around that, in the worst case with some clear grace periods or stronger boundaries if a numeric identity needs to be returned to the pool for reallocation. Either way I'm curious to explore your idea a bit deeper, but I think we'll need a proper CFP on https://github.com/cilium/design-cfps to outline the goals, assumptions, background and proposal for such a change.

soggiest added a commit to soggiest/cilium that referenced this issue Mar 5, 2024
Entities are special selectors used by network policies. The Cluster entity relies on the `io.cilium.k8s.policy.cluster` label which is removed by Cilium if a strict identity label configuration is applied.
This PR adds the relevant Cilium policy label to the list of default labels so it will always be applied regardless of configuration, and includes this label to the associated test file.

Fixes: cilium#18878

Signed-off-by: soggiest <nicholas@isovalent.com>
soggiest added a commit to soggiest/cilium that referenced this issue Mar 7, 2024
Entities are special selectors used by network policies. The Cluster entity relies on the `io.cilium.k8s.policy.cluster` label which is removed by Cilium if a strict identity label configuration is applied.
This PR adds the relevant Cilium policy label to the list of default labels so it will always be applied regardless of configuration, and includes this label to the associated test file.

Fixes: cilium#18878

Signed-off-by: soggiest <nicholas@isovalent.com>
github-merge-queue bot pushed a commit that referenced this issue Mar 15, 2024
Entities are special selectors used by network policies. The Cluster entity relies on the `io.cilium.k8s.policy.cluster` label which is removed by Cilium if a strict identity label configuration is applied.
This PR adds the relevant Cilium policy label to the list of default labels so it will always be applied regardless of configuration, and includes this label to the associated test file.

Fixes: #18878

Signed-off-by: soggiest <nicholas@isovalent.com>
rzdebskiy pushed a commit to rzdebskiy/cilium that referenced this issue Apr 3, 2024
Entities are special selectors used by network policies. The Cluster entity relies on the `io.cilium.k8s.policy.cluster` label which is removed by Cilium if a strict identity label configuration is applied.
This PR adds the relevant Cilium policy label to the list of default labels so it will always be applied regardless of configuration, and includes this label to the associated test file.

Fixes: cilium#18878

Signed-off-by: soggiest <nicholas@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug This is a bug in the Cilium logic. sig/agent Cilium agent related. sig/policy Impacts whether traffic is allowed or denied based on user-defined policies.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants