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.

Sign up for GitHub

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

Fix false positives on pods with network policies #367

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix false positives on pods with network policies #367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions internal/lint/pod.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if np.Spec.Ingress != nil {
matches[0]++
}
if s.checkEgresses(ctx, pod, np.Spec.Egress) {
if np.Spec.Egress != nil {
matches[1]++
}
}
Expand Down
37 changes: 16 additions & 21 deletions internal/lint/pod_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,17 @@ func TestPodNPLint(t *testing.T) {

po := NewPod(test.MakeCollector(t), dba)
assert.Nil(t, po.Lint(test.MakeContext("v1/pods", "pods")))
assert.Equal(t, 2, len(po.Outcome()))
assert.Equal(t, 3, len(po.Outcome()))

ii := po.Outcome()["ns1/p1"]
assert.Equal(t, 1, len(ii))
assert.Equal(t, `[POP-1204] Pod Egress is not secured by a network policy`, ii[0].Message)

ii = po.Outcome()["ns2/p2"]
assert.Equal(t, 0, len(ii))

ii = po.Outcome()["ns3/p3"]
assert.Equal(t, 0, len(ii))
}

func TestPodCheckSecure(t *testing.T) {
Expand Down Expand Up @@ -133,25 +136,21 @@ func TestPodLint(t *testing.T) {
assert.Equal(t, 0, len(ii))

ii = po.Outcome()["default/p2"]
assert.Equal(t, 6, len(ii))
assert.Equal(t, 4, len(ii))
assert.Equal(t, `[POP-207] Pod is in an unhappy phase ()`, ii[0].Message)
assert.Equal(t, `[POP-208] Unmanaged pod detected. Best to use a controller`, ii[1].Message)
assert.Equal(t, `[POP-1204] Pod Ingress is not secured by a network policy`, ii[2].Message)
assert.Equal(t, `[POP-1204] Pod Egress is not secured by a network policy`, ii[3].Message)
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"]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

assert.Equal(t, 6, len(ii))
assert.Equal(t, 4, len(ii))
assert.Equal(t, `[POP-105] Liveness uses a port#, prefer a named port`, ii[0].Message)
assert.Equal(t, `[POP-105] Readiness uses a port#, prefer a named port`, ii[1].Message)
assert.Equal(t, `[POP-1204] Pod Ingress is not secured by a network policy`, ii[2].Message)
assert.Equal(t, `[POP-1204] Pod Egress is not secured by a network policy`, ii[3].Message)
assert.Equal(t, `[POP-301] Connects to API Server? ServiceAccount token is mounted`, ii[4].Message)
assert.Equal(t, `[POP-109] CPU Current/Request (2000m/1000m) reached user 80% threshold (200%)`, ii[5].Message)
assert.Equal(t, `[POP-301] Connects to API Server? ServiceAccount token is mounted`, ii[2].Message)
assert.Equal(t, `[POP-109] CPU Current/Request (2000m/1000m) reached user 80% threshold (200%)`, ii[3].Message)

ii = po.Outcome()["default/p4"]
assert.Equal(t, 15, len(ii))
assert.Equal(t, 13, len(ii))
assert.Equal(t, `[POP-204] Pod is not ready [0/1]`, ii[0].Message)
assert.Equal(t, `[POP-204] Pod is not ready [0/2]`, ii[1].Message)
assert.Equal(t, `[POP-100] Untagged docker image in use`, ii[2].Message)
Expand All @@ -163,20 +162,16 @@ func TestPodLint(t *testing.T) {
assert.Equal(t, `[POP-113] Container image "zorg:latest" is not hosted on an allowed docker registry`, ii[8].Message)
assert.Equal(t, `[POP-107] No resource limits defined`, ii[9].Message)
assert.Equal(t, `[POP-208] Unmanaged pod detected. Best to use a controller`, ii[10].Message)
assert.Equal(t, `[POP-1204] Pod Ingress is not secured by a network policy`, ii[11].Message)
assert.Equal(t, `[POP-1204] Pod Egress is not secured by a network policy`, ii[12].Message)
assert.Equal(t, `[POP-300] Uses "default" ServiceAccount`, ii[13].Message)
assert.Equal(t, `[POP-301] Connects to API Server? ServiceAccount token is mounted`, ii[14].Message)
assert.Equal(t, `[POP-300] Uses "default" ServiceAccount`, ii[11].Message)
assert.Equal(t, `[POP-301] Connects to API Server? ServiceAccount token is mounted`, ii[12].Message)

ii = po.Outcome()["default/p5"]
assert.Equal(t, 7, len(ii))
assert.Equal(t, 5, len(ii))
assert.Equal(t, `[POP-113] Container image "blee:v1.2" is not hosted on an allowed docker registry`, ii[0].Message)
assert.Equal(t, `[POP-106] No resources requests/limits defined`, ii[1].Message)
assert.Equal(t, `[POP-102] No probes defined`, ii[2].Message)
assert.Equal(t, `[POP-1204] Pod Ingress is not secured by a network policy`, ii[3].Message)
assert.Equal(t, `[POP-1204] Pod Egress is not secured by a network policy`, ii[4].Message)
assert.Equal(t, `[POP-209] Pod is managed by multiple PodDisruptionBudgets (pdb4, pdb4-1)`, ii[5].Message)
assert.Equal(t, `[POP-301] Connects to API Server? ServiceAccount token is mounted`, ii[6].Message)
assert.Equal(t, `[POP-209] Pod is managed by multiple PodDisruptionBudgets (pdb4, pdb4-1)`, ii[3].Message)
assert.Equal(t, `[POP-301] Connects to API Server? ServiceAccount token is mounted`, ii[4].Message)
}

// ----------------------------------------------------------------------------
Expand Down
62 changes: 61 additions & 1 deletion internal/lint/testdata/core/pod/3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,4 +121,64 @@ items:
conditions:
phase: Running
podIPs:
- ip: 172.1.0.3
- ip: 172.1.0.3
- apiVersion: v1
kind: Pod
metadata:
name: p3
namespace: ns3
labels:
app: p3
ownerReferences:
- apiVersion: apps/v1
controller: true
kind: DaemonSet
name: rs4
spec:
serviceAccountName: sa3
tolerations:
- key: t1
operator: Exists
effect: NoSchedule
containers:
- name: c1
image: alpine:v1.0
livenessProbe:
httpGet:
path: /healthz
port: http
initialDelaySeconds: 3
periodSeconds: 3
readinessProbe:
httpGet:
path: /healthz
port: http
initialDelaySeconds: 3
periodSeconds: 3
resources:
requests:
cpu: 1
memory: 1Mi
limits:
cpu: 1
memory: 1Mi
ports:
- containerPort: 9090
name: http
protocol: TCP
env:
- name: env1
valueFrom:
configMapKeyRef:
name: cm1
key: ns
- name: env2
valueFrom:
secretKeyRef:
name: sec1
key: k1
status:
conditions:
phase: Running
podIPs:
- ip: 172.1.0.3
6 changes: 6 additions & 0 deletions internal/lint/testdata/core/sa/2.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,10 @@ items:
metadata:
name: sa2
namespace: ns2
automountServiceAccountToken: false
- apiVersion: v1
kind: ServiceAccount
metadata:
name: sa3
namespace: ns3
automountServiceAccountToken: false
24 changes: 23 additions & 1 deletion internal/lint/testdata/net/np/3.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,26 @@ items:
- {}
policyTypes:
- Ingress
- Egress
- Egress
- apiVersion: networking.k8s.io/v1
kind: NetworkPolicy
metadata:
name: pod-selecting-np
namespace: ns3
spec:
podSelector:
matchLabels:
app: p3
ingress:
- from:
- namespaceSelector:
matchLabels:
app: p1
egress:
- to:
- namespaceSelector:
matchLabels:
app: p1
policyTypes:
- Ingress
- Egress