Fix false positives on pods with network policies #367
Conversation
| assert.Equal(t, `[POP-206] Pod has no associated PodDisruptionBudget`, ii[4].Message) | ||
| assert.Equal(t, `[POP-301] Connects to API Server? ServiceAccount token is mounted`, ii[5].Message) | ||
| assert.Equal(t, `[POP-206] Pod has no associated PodDisruptionBudget`, ii[2].Message) | ||
| assert.Equal(t, `[POP-301] Connects to API Server? ServiceAccount token is mounted`, ii[3].Message) | ||
|
|
||
| ii = po.Outcome()["default/p3"] |
There was a problem hiding this comment.
np2 has no podSelector (this is being reported as an invalid schema).. assuming that's the same as an empty podSelector, which should apply to all pods in the namespace then.
p3 is in the default namespace, and np2 has both ingress and egress policies, so therefore p3 is secured by ingress & egress,
| @@ -117,10 +117,10 @@ func (s *Pod) checkNPs(ctx context.Context, pod *v1.Pod) { | |||
| continue | |||
| } | |||
| if labelsMatch(&np.Spec.PodSelector, pod.Labels) { | |||
| if s.checkIngresses(ctx, pod, np.Spec.Ingress) { | |||
There was a problem hiding this comment.
The pod has been selected by the PodSelector, therefore the ingress & egress (if present) apply to it. We don't need the Ingress & Egress rules themselves to target the pod.
There was a problem hiding this comment.
@bpfoster Good catch! I agree. However I think we should perform the check if the policy does not directly selects that pod to ensure ingress/egress targets the current pod.
What do you think?
There was a problem hiding this comment.
Thanks for the review @derailed.
I am admittedly no expert here, and trying to fully comprehend both network policies and what's being tested here...
That said, I think you're trying to prove that a given pod is secured by a NetworkPolicy, limiting it's ingress and egress traffic, and not wide open.
podSelector is a required field on a NetworkPolicy. If it's an empty field, it selects all pods in the policy's namespace.
I think what we want to say is that a pod is selected by 1+ NetworkPolicies (with ingress and egress rules). The ingress and egress sections within that policy simply define the network rules we are applying to that pod... pod-x can send traffic to pod-y and receive traffic from pod-z.
I do not believe that we should count that as policies for pod-y or pod-z. If no NetworkPolicy exists that selects either of them, they are free to egress or ingress from anywhere.
| @@ -117,10 +117,10 @@ func (s *Pod) checkNPs(ctx context.Context, pod *v1.Pod) { | |||
| continue | |||
| } | |||
| if labelsMatch(&np.Spec.PodSelector, pod.Labels) { | |||
| if s.checkIngresses(ctx, pod, np.Spec.Ingress) { | |||
There was a problem hiding this comment.
@bpfoster Good catch! I agree. However I think we should perform the check if the policy does not directly selects that pod to ensure ingress/egress targets the current pod.
What do you think?
Maybe I'm misunderstanding something, but I believe the checks for network policies have some issues.
If there is a network policy whose pod selector targets a pod, we should consider that policy as applying to the pod. The existing code seems to want the network policy targeting the pod to also have ingress/egress rules matching the pod itself, which I don't think is right.
This seems to fix my false positives as well as the one reported by @siegy22 in #311.
fixes #311