Implement support for named ports in NetPols #134

Phil1602 · 2021-01-15T22:51:14Z

Fixes #131

Changes

Illuminatio will lookup the port name during test case orchestration and will use numerical port by then. Currently the first matched pod/container's port will be used, which should be sufficient for the most cases. Note that there are some cases, which are still not yet covered and could lead to false positives for example (see 2. point in TODOs).

While testing the implementation using https://github.com/cloudogu/k8s-security-demos I have discovered some more issues.
Most of them need further investigation, but I have created some sanity checks (e.g. if =! none) to handle the errors and get at least a result. I will try to split those different related issues into actual issues to have them addressed separately.

Implementation

The named ports are printed out as is for the generated test cases, but are resolved later one to numerical ports. This is basically the same approach as for the pod label selectors, which are translated to actual pods or dummy pods.

TODOs

Cleanup commits
Think about the problem that different pods with the same labelset could have ports with different number but equal names. It be nice if we would be able to actually test it, because there could be also some differences between the CNIs in their implementation/support of this "edge case".
-> I will create a followup issue therefore since this requires architectural redesign in some places.
Unit tests
e2e test cases
Create follow-up issues for the discovered problems
Check if egress ports can also be specified using a name of a label selected pod + implement if necessary
-> Egress is currently not (fully) supported anway(?), see Support egress policies #65. Since namedports are rarely used in egress policies, I decided to skip this feature for now.

codecov · 2021-01-17T13:31:46Z

Codecov Report

Merging #134 (a2e94de) into master (dd57599) will decrease coverage by 1.00%.
The diff coverage is 12.00%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
- Coverage   39.21%   38.20%   -1.01%     
==========================================
  Files          11       11              
  Lines        1321     1369      +48     
  Branches      264      276      +12     
==========================================
+ Hits          518      523       +5     
- Misses        754      798      +44     
+ Partials       49       48       -1

Impacted Files	Coverage Δ
src/illuminatio/illuminatio.py	`0.00% <0.00%> (ø)`
src/illuminatio/illuminatio_runner.py	`28.12% <0.00%> (-0.45%)`	⬇️
src/illuminatio/test_orchestrator.py	`23.22% <10.00%> (-0.95%)`	⬇️
src/illuminatio/k8s_util.py	`20.00% <16.66%> (-1.74%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd57599...e4c283c. Read the comment docs.

maxbischoff

Some minor comments and one potential issues with named ports containing a -. Otherwise LGTM, thanks for contributing :)

maxbischoff · 2021-02-02T06:43:43Z

local_dev/start_docker.sh

@@ -11,7 +11,8 @@ minikube start \
    --cni=calico \
    --container-runtime=docker \
    --host-only-cidr=172.17.17.1/24 \
-    --kubernetes-version="${KUBERNETES_VERSION}"
+    --kubernetes-version="${KUBERNETES_VERSION}" \
+    --insecure-registry=localhost:5000


This seems to have not been needed up until now, do you know why?

Kubelet was not able to pull the images from the internal minikube registry addon.
After adding this, it worked for me. But I did not further investigate logs besides the kubelet event, so I'm not sure whats the exact cause and why the pipeline does not have this issue.

maxbischoff · 2021-02-02T06:47:28Z

src/illuminatio/illuminatio.py

@@ -264,6 +264,14 @@ def transform_results(
                LOGGER.debug("mapped_sender_pod: %s", mapped_sender_pod)
                LOGGER.debug("mapped_receiver_pod: %s", mapped_receiver_pod)
                LOGGER.debug("raw_results: %s", json.dumps(raw_results))
+                if mapped_receiver_pod not in raw_results[mapped_sender_pod]:
+                    LOGGER.error(


I think I would prefer the results to contain an entry for these failures, where the state is ERR. This way other output formats would also show the error without checking logs and test results would not vanish "silently" (when only looking at the result).

Sounds good, yes.

I thought about refactoring this whole error handling in another gh issue (basically differentiate between success, failure(case failed) and error (prerequisites etc failed). But you are right, should at least be consistent for now.

maxbischoff · 2021-02-02T06:47:42Z

src/illuminatio/illuminatio_runner.py

@@ -151,6 +151,9 @@ def run_tests_for_sender_pod(sender_pod, cases):
    # TODO check if network ns is None -> HostNetwork is set
    results = {}
    for target, ports in cases[from_host_string].items():
+        if target is None:
+            LOGGER.error("Target is none. Skipping")


Again, I think I would prefer the results to contain an entry for these failures, where the state is ERR. This way other output formats would also show the error without checking logs and test results would not vanish "silently" (when only looking at the result).

maxbischoff · 2021-02-02T06:51:12Z

src/illuminatio/k8s_util.py

+
+
+def resolve_port_name(namespace, pod_label_selector_string, portname):
+    api = k8s.client.CoreV1Api()


I would prefer passing the pod list. In test_orchestrator all calculations so far have been done on one momentary state of the API, so that the state is consistent across the different steps of translating test cases to the test run.

maxbischoff · 2021-02-02T06:54:41Z

src/illuminatio/test_orchestrator.py

-                p for p in service_ports if p.target_port == port_int
+                p
+                for p in service_ports
+                # TODO Services could also contain named ports. We should cross-compare here (resvoledName == numerical)


This would be done at another point, as we are iterating over numbered_ports here, wouldn't it?

The comment is related to the following issue (I guess we think about different things):

We are iterating over numbered_ports, but it could be the case, that the NetworkPolicy itself contains a numerical port (e.g. 80), but the Service uses a name (e.g. http), which is defined within the container.
In this case we are fine, since there is a service for port 80, but the comparison (e.g. if 80 == http) would fail and let the case fail, hence we would need to resolve the service ports here first to handle each edge case correctly.

I decided to skip this and only leave a comment as this is a edge case. I guess someone will use named ports in the NetPol and Svc accordingly in general.

maxbischoff · 2021-02-02T06:57:25Z

src/illuminatio/test_orchestrator.py

+
+            # Strip of the "-" and add it again after name has been resolved
+            prefix = "-" if "-" in named_port else ""
+            named_port_without_prefix = named_port.replace("-", "")


Port names can also contain a -, as far as I know.

Good catch, thanks.

maxbischoff · 2021-02-02T07:00:00Z

src/illuminatio/test_orchestrator.py

+                    str(service_ports_for_port[0].port),
+                )
+            else:
+                # TODO change to exception (explanation see above)


which is the explanation?

The same as for the numerical ports. Unfortunately the comment is not within the "review window":

"# TODO this was a hotfix for recipe 11, where ports 53 were allowed but not for any target,
# resulting in test to 53 being written despite no service matching them existing.
# That error should be handled in test generation, an exception here would be fine"

Phil1602

I will provide fixes during this week (hopefully ;) )

Phil1602 · 2021-02-02T12:40:01Z

src/illuminatio/test_orchestrator.py

-                p for p in service_ports if p.target_port == port_int
+                p
+                for p in service_ports
+                # TODO Services could also contain named ports. We should cross-compare here (resvoledName == numerical)


The comment is related to the following issue (I guess we think about different things):

We are iterating over numbered_ports, but it could be the case, that the NetworkPolicy itself contains a numerical port (e.g. 80), but the Service uses a name (e.g. http), which is defined within the container.
In this case we are fine, since there is a service for port 80, but the comparison (e.g. if 80 == http) would fail and let the case fail, hence we would need to resolve the service ports here first to handle each edge case correctly.

I decided to skip this and only leave a comment as this is a edge case. I guess someone will use named ports in the NetPol and Svc accordingly in general.

Phil1602 · 2021-02-02T12:43:13Z

src/illuminatio/illuminatio.py

@@ -264,6 +264,14 @@ def transform_results(
                LOGGER.debug("mapped_sender_pod: %s", mapped_sender_pod)
                LOGGER.debug("mapped_receiver_pod: %s", mapped_receiver_pod)
                LOGGER.debug("raw_results: %s", json.dumps(raw_results))
+                if mapped_receiver_pod not in raw_results[mapped_sender_pod]:
+                    LOGGER.error(


Sounds good, yes.

I thought about refactoring this whole error handling in another gh issue (basically differentiate between success, failure(case failed) and error (prerequisites etc failed). But you are right, should at least be consistent for now.

Phil1602 · 2021-02-02T12:49:32Z

src/illuminatio/test_orchestrator.py

+                    str(service_ports_for_port[0].port),
+                )
+            else:
+                # TODO change to exception (explanation see above)


The same as for the numerical ports. Unfortunately the comment is not within the "review window":

"# TODO this was a hotfix for recipe 11, where ports 53 were allowed but not for any target,
# resulting in test to 53 being written despite no service matching them existing.
# That error should be handled in test generation, an exception here would be fine"

Phil1602 · 2021-02-02T12:52:13Z

local_dev/start_docker.sh

@@ -11,7 +11,8 @@ minikube start \
    --cni=calico \
    --container-runtime=docker \
    --host-only-cidr=172.17.17.1/24 \
-    --kubernetes-version="${KUBERNETES_VERSION}"
+    --kubernetes-version="${KUBERNETES_VERSION}" \
+    --insecure-registry=localhost:5000


Kubelet was not able to pull the images from the internal minikube registry addon.
After adding this, it worked for me. But I did not further investigate logs besides the kubelet event, so I'm not sure whats the exact cause and why the pipeline does not have this issue.

Phil1602 · 2021-02-02T12:52:28Z

src/illuminatio/test_orchestrator.py

+
+            # Strip of the "-" and add it again after name has been resolved
+            prefix = "-" if "-" in named_port else ""
+            named_port_without_prefix = named_port.replace("-", "")


Good catch, thanks.

This reverts commit b2892d8.

Phil1602 force-pushed the gh-issue-131 branch 6 times, most recently from 56d9d00 to 5bb572d Compare January 17, 2021 13:17

Phil1602 force-pushed the gh-issue-131 branch 5 times, most recently from 7ea674c to c8f992f Compare January 24, 2021 21:13

Phil1602 added 2 commits January 25, 2021 11:49

Add support for named ports in NetPols

76b6926

Sanity checks to get it working

ccce977

Phil1602 force-pushed the gh-issue-131 branch 4 times, most recently from dc7e280 to 3e7b08d Compare January 25, 2021 22:28

Phil1602 added 2 commits January 26, 2021 13:56

Check if service port is available for named port

3bb5482

Unit test for named port test case generation

5462c0e

Phil1602 force-pushed the gh-issue-131 branch from 3e7b08d to 5462c0e Compare January 26, 2021 12:56

Phil1602 self-assigned this Jan 26, 2021

Phil1602 added the bug Something isn't working label Jan 26, 2021

Phil1602 mentioned this pull request Jan 26, 2021

Replace Travis with GitHub Actions #130

Open

Phil1602 force-pushed the gh-issue-131 branch from 789b798 to 1e991c6 Compare January 28, 2021 11:28

e2e test for named ports

a2e94de

Phil1602 force-pushed the gh-issue-131 branch from 1e991c6 to a2e94de Compare January 28, 2021 16:53

Phil1602 mentioned this pull request Jan 29, 2021

kubernetes.client.exceptions.ApiException: (422) #129

Open

Phil1602 marked this pull request as ready for review January 29, 2021 15:48

Phil1602 force-pushed the gh-issue-131 branch from 2f6f41c to e6ee650 Compare January 29, 2021 20:17

Phil1602 force-pushed the gh-issue-131 branch 4 times, most recently from 016bfb6 to 2dc5d68 Compare January 31, 2021 19:26

Unit test for port resolution

32d7c91

Phil1602 force-pushed the gh-issue-131 branch from 2dc5d68 to 32d7c91 Compare January 31, 2021 22:43

maxbischoff self-requested a review February 2, 2021 06:34

maxbischoff suggested changes Feb 2, 2021

View reviewed changes

Phil1602 commented Feb 2, 2021

View reviewed changes

Handle "-" correctly within portnames

e9489ca

maxbischoff approved these changes Feb 26, 2021

View reviewed changes

maxbischoff merged commit b2892d8 into inovex:master Feb 26, 2021

maxbischoff added a commit that referenced this pull request Feb 26, 2021

Revert "Implement support for named ports in NetPols (#134)"

5e3059f

This reverts commit b2892d8.

maxbischoff mentioned this pull request Feb 26, 2021

Revert "Implement support for named ports in NetPols" #138

Merged

maxbischoff added a commit that referenced this pull request Feb 26, 2021

Revert "Implement support for named ports in NetPols (#134)" (#138)

62af310

This reverts commit b2892d8.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement support for named ports in NetPols #134

Implement support for named ports in NetPols #134

Phil1602 commented Jan 15, 2021 •

edited

Loading

codecov bot commented Jan 17, 2021 •

edited

Loading

maxbischoff left a comment

maxbischoff Feb 2, 2021

Phil1602 Feb 2, 2021

maxbischoff Feb 2, 2021

Phil1602 Feb 2, 2021

maxbischoff Feb 2, 2021

maxbischoff Feb 2, 2021

maxbischoff Feb 2, 2021

Phil1602 Feb 2, 2021

maxbischoff Feb 2, 2021

Phil1602 Feb 2, 2021

maxbischoff Feb 2, 2021

Phil1602 Feb 2, 2021

Phil1602 left a comment

Phil1602 Feb 2, 2021

Phil1602 Feb 2, 2021

Phil1602 Feb 2, 2021

Phil1602 Feb 2, 2021

Phil1602 Feb 2, 2021



		def resolve_port_name(namespace, pod_label_selector_string, portname):
		api = k8s.client.CoreV1Api()

Implement support for named ports in NetPols #134

Implement support for named ports in NetPols #134

Conversation

Phil1602 commented Jan 15, 2021 • edited Loading

Changes

Implementation

TODOs

codecov bot commented Jan 17, 2021 • edited Loading

Codecov Report

maxbischoff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phil1602 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Phil1602 commented Jan 15, 2021 •

edited

Loading

codecov bot commented Jan 17, 2021 •

edited

Loading