Rework port binding logic without privileges

[sigv]

Proposed changes

This is a breaking change! To start Nginx Ingress Controller with this commit applied, you must use an updated image.

Historically, the Ingress Controller entrypoint launched with restricted capabilities. Then Nginx process escalated privileges (NET_BIND_SERVICE) to bind ports 80 and 443 as non-root user. Allowing privilege escalation is generally frowned upon in various policies.

The Nginx binary in old images was adding NET_BIND_SERVICE to its Permitted capability set and also setting the Effective bit, to enforce the Permitted capability during launch. (That's the escalation there.)

With this change, privilege escalation is no longer allowed and the NET_BIND_SERVICE capability is removed. To allow the binary to start, the capabilities are no longer being adjusted on the binary file.

This works because Kubernetes v1.22+ allows Pods to independently lower unprivileged port range to start with zero without affecting other pods (namespaced/"safe" sysctls).

OBS! An old image may be used if the binary's Effective bit is removed:

FROM nginx/nginx-ingress:3.0.2
USER root
RUN setcap 'cap_net_bind_service=-e' /usr/sbin/nginx 'cap_net_bind_service=-e' /usr/sbin/nginx-debug \
    && setcap -v 'cap_net_bind_service=-e' /usr/sbin/nginx 'cap_net_bind_service=-e' /usr/sbin/nginx-debug
# 101 is nginx
USER 101

Improves #3544. Privilege Escalation is the initial scope of the linked issue. However, as indicated by the issue creator, there may be follow-up tasks to fully close it.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

• I have read the CONTRIBUTING doc
• I have added tests that prove my fix is effective or that my feature works
• I have checked that all unit tests pass after adding my changes
• I have updated necessary documentation
• I have rebased my branch onto main
• I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

[sigv]

I have left the NET_BIND_SERVICE capability in the Permitted set on the binary file. But I'm thinking now that there is no scenario where Kubernetes ends up using it.

Is there some strange Docker native scenario where it's helpful? My understanding is: having the capability as Permitted for file allows the capability to be added to the thread, if the bounding set includes NET_BIND_SERVICE.
If we don't plan on the image being run natively, then we should probably remove the Permitted.

[sigv]

As mentioned in description, to test the chart, you would need to build the image similar to:

make debian-image PREFIX=myregistry.example.com/nginx-ingress TARGET=container

If using Minikube, you can pull the image into local cluster:

minikube image load myregistry.example.com/nginx-ingress:untagged-EXAMPLE

When installing the chart:

--set controller.image.repository=myregistry.example.com/nginx-ingress --set controller.image.tag=untagged-EXAMPLE

[codecov-commenter]

Codecov Report

Merging #3573 (76d690a) into main (9621336) will increase coverage by 0.05%.
The diff coverage is 0.00%.

❗ Current head 76d690a differs from pull request most recent head da15524. Consider uploading reports for the commit da15524 to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #3573      +/-   ##
==========================================
+ Coverage   52.24%   52.29%   +0.05%     
==========================================
  Files          59       59              
  Lines       16873    16849      -24     
==========================================
- Hits         8816     8812       -4     
+ Misses       7762     7740      -22     
- Partials      295      297       +2     

Impacted Files	Coverage Δ
cmd/nginx-ingress/main.go	0.00% <0.00%> (ø)
internal/k8s/configuration.go	95.43% <0.00%> (-0.37%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[sigv]

Smoke Tests (debian-plus-nap, dos, 1.26.0) failure leads to this assert.

The 7th argument is checking signature_detected. Being an OSS customer, I am not familiar with the AppProtect/DOS mechanisms, so it's not clear to me currently why mitigated_by_signatures would not be detected.

=================================== FAILURES ===================================
_ TestDos.test_dos_under_attack_with_learning[crd_ingress_controller_with_dos0] _
Traceback (most recent call last):
  File "/workspace/tests/suite/test_dos.py", line 366, in test_dos_under_attack_with_learning
    assert (
AssertionError: assert (True and True and True and True and True and 84.0 < 150 and False)
 +  where 84.0 = <built-in method total_seconds of datetime.timedelta object at 0x7fb841a13bd0>()
 +    where <built-in method total_seconds of datetime.timedelta object at 0x7fb841a13bd0> = (datetime.datetime(2023, 2, 22, 9, 5, 5) - datetime.datetime(2023, 2, 22, 9, 3, 41)).total_seconds

[lucacome]

@sigv thanks for the PR! I've re-run the DoS test and it passed 🎉

I've been doing some testing and I think we can remove setcap altogether, but we need to bump the minimum required versions of k8s to 1.22 in the docs, helm chart, etc. for this to work. I think it's fine, 1.21 it's pretty old at this point. @brianehlert thoughts?

EDIT: running natively in Docker is not a use case for use, so no need to worry about that.

[sigv]

• Kubernetes v1.22 configured as lowest supported version.
• Kubernetes v1.21 removed from pipelines.
• Invocation of setcap removed outright.
• Added note about K8s support change in docs/content/technical-specifications.md.
• Branch rebased (protobuf update).

[sigv]

Taking a look at K8s Patch Releases, K8s v1.21 went EOL 2022-06-28. It should be safe to remove, as everyone running this should really upgrade. To be honest, the proposed minimum supported version (K8s v1.22) itself went EOL 2022-10-28.

[sigv]

Alternative approach I mentioned in the linked issue could be that IC process (entrypoint) is modified, so that it gets NET_BIND_SERVICE initially and executes Nginx (which could inherit the capability) and then IC drops that capability. In theory we don't need lowered unprivileged port range then - only change in IC to remove privilege escalation. If that's preferred I can propose the change instead.

It's kind of a risk assessment question of how to get rid of escalation. Either the current scope which enables the whole Pod to easily bind, or simply better handling of the NET_BIND_SERVICE capability.

[lucacome]

It's kind of a risk assessment question of how to get rid of escalation. Either the current scope which enables the whole Pod to easily bind, or simply better handling of the NET_BIND_SERVICE capability.

I like the approach of enabling the pod to easily bind. Do you see any drawbacks with this that I'm not seeing?

[sigv]

I like the approach of enabling the pod to easily bind. Do you see any drawbacks with this that I'm not seeing?

Capability being dropped would be more restrictive.

In theory, an RCE attack against nginx could result in a different process replacing nginx silently.
This new malicious process starts up and kills nginx. It then listens on ports 80 and 443, and acts as a man-in-the-middle.

In current setup, the same risk already exists. At least when taking a quick look, it seemed the nginx process does not drop NET_BIND_SERVICE (or I am just missing where it happens?).

[lucacome]

In current setup, the same risk already exists. At least when taking a quick look, it seemed the nginx process does not drop NET_BIND_SERVICE (or I am just missing where it happens?).

yeah we don't drop it

[sigv]

Build Docker Plus (ubi-plus, linux/arm64, linux/amd64, linux/s390x, goreleaser) failed to resolve the Nginx Plus package repository:

error: cannot update repo 'nginx-plus': Cannot download repomd.xml: Cannot download repodata/repomd.xml: All mirrors were tried; Last error: Curl error (58): Problem with the local SSL certificate for https://pkgs.nginx.com/plus/R28/centos/9/x86_64/repodata/repomd.xml [could not load PEM client certificate, OpenSSL error error:80000002:system library::No such file or directory, (no key found, wrong pass phrase, or wrong file format?)]

I presume this could be an intermittent issue, as nothing should have changed for this step.

Or my pipeline is not allowed to use the secret material:

Warning: nginx-repo.crt= is not a valid secret
Warning: nginx-repo.key= is not a valid secret