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

False positive on Network Policies .. Sometimes #311

Closed
bpfoster opened this issue May 1, 2024 · 7 comments · May be fixed by #367
Closed

False positive on Network Policies .. Sometimes #311

bpfoster opened this issue May 1, 2024 · 7 comments · May be fixed by #367
Labels
bug Something isn't working

Comments

@bpfoster
Copy link
Contributor

bpfoster commented May 1, 2024




Describe the bug
Running popeye sometimes reports

  · foo/foo-75f84cd95b-9s62d...................................................😱
    😱 [POP-1204] Pod Ingress is not secured by a network policy.
    😱 [POP-1204] Pod Egress is not secured by a network policy.

Despite there being a Network Policy for it.

Expected behavior
No warning on missing Network Policy

Versions (please complete the following information):

  • OS: RHEL 8
  • Popeye: v0.21.3
  • K8s: v1.26.11

Additional context
I have created NetworkPolicies using the

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy

resource type.

I notice the cluster also has Calico installed providing their NetworkPolicy CRD:

$ kubectl get crd | grep networkpolicies
globalnetworkpolicies.crd.projectcalico.org
networkpolicies.crd.projectcalico.org

Installed policies:

$ kubectl get networkpolicies.crd.projectcalico.org 
No resources found in foo namespace.

$ kubectl get networkpolicies.networking.k8s.io 
NAME                                     POD-SELECTOR
foo                 app.kubernetes.io/instance=foo,app.kubernetes.io/name=foo

When I debug popeye, I see that the GVR being queried in the DB for network policies is sometimes the calico one, and sometimes the k8s.io one.

txn, it := s.db.MustITForNS(internal.Glossary[internal.NP], pod.Namespace)

When it's the Calico one, the NetworkPolicy is not found.

My guess is there's some non-determinism in the order in which the CRDs are returned/loaded, and from what I can see, the popeye DB only supports 1 per type and is overwriting whichever comes first with the second one.

popeye/internal/alias.go

Lines 92 to 116 in d09ec25

func (a *Aliases) Realize() {
for gvr, res := range a.metas {
a.aliases[res.Name] = gvr
if res.SingularName != "" {
a.aliases[res.SingularName] = gvr
}
for _, n := range res.ShortNames {
a.aliases[n] = gvr
}
if kk, ok := customShortNames[R(res.Name)]; ok {
for _, k := range kk {
a.aliases[k] = gvr
}
}
if lgvr, ok := Glossary[R(res.SingularName)]; ok {
if greaterV(gvr.V(), lgvr.V()) {
Glossary[R(res.SingularName)] = gvr
}
} else if lgvr, ok := Glossary[R(res.Name)]; ok {
if greaterV(gvr.V(), lgvr.V()) {
Glossary[R(res.Name)] = gvr
}
}
}
}

Thinking about it... should popeye know the GVR for each resource type it cares about, and ignore others? In this case it knows it wants networkpolicies.networking.k8s.io and would discard the caclico type. If there was a calico NP (or some other random one), popeye would panic when it tries to assert to netv1.NetworkPolicy.

@bpfoster
Copy link
Contributor Author

bpfoster commented May 1, 2024

This is maybe out of scope for this issue, and admittedly I'm still working on understanding Network Policies, but I'm struggling to understand the logic in Popeye on checking ingress/egress policies.

If we get to this branch of code:

popeye/internal/lint/pod.go

Lines 119 to 126 in d09ec25

if labelsMatch(&np.Spec.PodSelector, pod.Labels) {
if s.checkIngresses(ctx, pod, np.Spec.Ingress) {
matches[0]++
}
if s.checkEgresses(ctx, pod, np.Spec.Egress) {
matches[1]++
}
}

I'm not sure the logic in checkIngresses / checkEgresses is wholly correct.

popeye/internal/lint/pod.go

Lines 177 to 199 in d09ec25

func (s *Pod) checkIngresses(ctx context.Context, pod *v1.Pod, rr []netv1.NetworkPolicyIngressRule) bool {
var match int
if rr == nil {
return false
}
for _, r := range rr {
if r.From == nil {
return true
}
for _, from := range r.From {
ok, err := s.isPodTargeted(pod, from.NamespaceSelector, from.PodSelector, from.IPBlock)
if err != nil {
s.AddErr(ctx, err)
return true
}
if ok {
match++
}
}
}
return match > 0
}

If we have a NetworkPolicy which targets the pod in question, and it has ingress rules, checkIngresses appears to want the ingress's from to point to the same pod.

Similarly for checkEgresses, it's wanting the egress's to to point to the same pod.

In other words, this seems to only pass when the NetworkPolicy says the pod can talk to itself.

@derailed
Copy link
Owner

@bpfoster For the initial issue, this is indeed a problem. Will fix on the next drop.
For the later, this code checks if a given pod is protected by a networkpolicy.

@derailed derailed added the bug Something isn't working label Aug 26, 2024
@derailed derailed mentioned this issue Sep 14, 2024
@derailed
Copy link
Owner

Fixes v0.21.5

@siegy22
Copy link

siegy22 commented Oct 28, 2024

I just had a similar thing come up when denying egress:

  · about-me/about-me-5d8f856785-pb7vx.............................................................😱
    😱 [POP-1204] Pod Egress is not secured by a network policy.

This is my NetworkPolicy

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  name: deny-egress
spec:
  podSelector:
    matchLabels:
      app: about-me
  policyTypes:
    - Ingress
    - Egress
  ingress:
    - {}
  egress: []

I did verify that the policy works by looking at the drops in Cilium.

Am I missing something?

@bpfoster
Copy link
Contributor Author

bpfoster commented Oct 28, 2024

I hadn't revisited this. I too am still getting (at least consistently now!) false positives on networkpolicy linting (on latest popeye v0.21.5).

In my case, I get Pod Ingress is not secured by a network policy despite NetworkPolicies being present. checkIngresses() is still coming back with no matches so I still wonder about how that is being evaluated.
My Network Policies look like:

apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
  labels:
    app.kubernetes.io/instance: my-instance
    app.kubernetes.io/managed-by: Helm
    app.kubernetes.io/name: my-service
    app.kubernetes.io/version: 0.0.1
    helm.sh/chart: my-service-0.1.0
  name: my-networkpolicy
  namespace: my-namespace
spec:
  egress:
  - ports:
    - port: 53
      protocol: UDP
    - port: 53
      protocol: TCP
  - to:
    - namespaceSelector:
        matchLabels:
          kubernetes.io/metadata.name: my-namespace
  - ports:
    - port: 80
      protocol: TCP
  ingress:
  - from:
    - namespaceSelector:
        matchLabels:
          kubernetes.io/metadata.name: my-namespace
    ports:
    - port: http
      protocol: TCP
  podSelector:
    matchLabels:
      app.kubernetes.io/instance: my-instance
      app.kubernetes.io/name: my-service
  policyTypes:
  - Ingress
  - Egress

For @siegy22 's example, playing around with some unit tests, changing egress to look like ingress removes the finding:

ingress:
  - {}
egress:
  - {}

But I frankly don't know if the result is the same?

@siegy22
Copy link

siegy22 commented Oct 29, 2024

@bpfoster Your example denies ingress and egress. My goal is to have any ingress and no egress.

Which is defined by:

ingress:
  - {}
egress: []

But popeye says there's no egress defined (which in theory is correct) but the effect is not the same as not defining it.

@bpfoster
Copy link
Contributor Author

@derailed please see PR #367 - I think this will address the false positives @siegy22 and I are seeing, if I have not misunderstood something here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants