Inherit NET_BIND_SERVICE from IC to Nginx #3722

sigv · 2023-04-03T18:00:12Z

Proposed changes

8be0144: Rework port binding logic without privileges caused issues for host networking configurations. The Kubernetes documentation states that the net.* sysctls can be used with container networking, which was misinterpreted.

This commit reverts the change, bringing back NET_BIND_SERVICE to the Nginx process, as well as reverts the libcap package removal done in a later commit.

In order to avoid privilege escalation being re-introduced, the IC process is also receiving NET_BIND_SERVICE, so that it can be inherited over to Nginx.

This change aims to restore host networking as functional for the Helm chart. A future change is recommended to harden security for the IC process (to drop the capability after executing Nginx) as well as Nginx itself (to drop the capability after binding).

OBS! To use a 3.1.0 image, you should manually install the setcap binary and add +ep on /nginx-ingress and +eip on nginx binary.

Resolves #3714.
Effectively reverts #3573 and #3616.

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

tstraley · 2023-04-03T18:08:54Z

IMO this is a better solution anyway because the Kubernetes Pod Security Standards allows for adding the NET_BIND_SERVICE capability within the restricted policy, whereas the sysctls net.ipv4.ip_unprivileged_port_start is only permitted in the baseline policy.

I think this means NIC will be fully compatible with the restricted policy after this, if I'm not missing anything.

sigv · 2023-04-03T18:12:22Z

IMO this is a better solution anyway because the Kubernetes Pod Security Standards allows for adding the NET_BIND_SERVICE capability within the restricted policy, whereas the sysctls net.ipv4.ip_unprivileged_port_start is only permitted in the baseline policy.

I think this means NIC will be fully compatible with the restricted policy after this, if I'm not missing anything.

From what I see, net.ipv4.ip_unprivileged_port_start is allowed in Baseline policy as it is categorized as a safe sysctl.
However I am not seeing any mention of it being blocked in Restricted policy.

Keep in mind that Restricted policy inherits Baseline policy.
Am I missing something here?

But yes - this is generally a cleaner approach.

tstraley · 2023-04-03T18:14:00Z

From what I see, net.ipv4.ip_unprivileged_port_start is allowed in Baseline policy as it is categorized as a safe sysctl. However I am not seeing any mention of it being blocked in Restricted policy.

Keep in mind that Restricted policy inherits Baseline policy.

Oh, I see - that wasn't clear to me. Thanks.

sigv · 2023-04-03T18:16:11Z

Oh, I see - that wasn't clear to me. Thanks.

The first line of Controls states Everything from the baseline profile.
But I do agree the Documentation can be a tricky beast.

As to why this solution is objectively cleaner, it allows dropping the capability in turn locking down ports 80/443.

sigv · 2023-04-03T19:40:11Z

Locally tested it comes online for container build:

sudo snap install microk8s --classic
microk8s status --wait-ready

sudo microk8s reset
microk8s enable dashboard dns registry istio

make debian-image PREFIX=myregistry.example.com/nginx-ingress TARGET=container
docker save myregistry.example.com/nginx-ingress:3.0.0-SNAPSHOT-aa3f2ab > nic.tar
microk8s ctr images import nic.tar
rm nic.tar

microk8s helm install --create-namespace --namespace nginx-ingress nginx-ingress deployments/helm-chart \
  --set controller.image.repository=myregistry.example.com/nginx-ingress \
  --set controller.image.tag=3.0.0-SNAPSHOT-aa3f2ab \
  --set controller.image.pullPolicy=Never \
  --set controller.hostNetwork=true

microk8s kubectl get pods -n nginx-ingress
# NAME                                        READY   STATUS    RESTARTS   AGE
# nginx-ingress-controller-7f756c56c5-jzmww   1/1     Running   0          10s

microk8s helm uninstall --namespace nginx-ingress nginx-ingress
microk8s ctr images rm myregistry.example.com/nginx-ingress:3.0.0-SNAPSHOT-aa3f2ab

The download target should be safe as it inherits from a different image, where setcap will already have been run.

For local, goreleaser and aws, we have to assume local user that builds cannot run setcap.
I am currently proposing to shuffle USER down to 0 and then back up to 101.

sigv · 2023-04-03T20:50:28Z

It seems the edge image is flaky on VirtualServerRoute.k8s.nginx.org tests when I run it, and so is my local testing. Looks good otherwise.

sigv · 2023-04-04T09:01:30Z

I did a rebase, and it looks fine, but Github reports that this PR consists of three commits now. I think that's a Github web UI bug, but let me know if I should try rebasing again.

codecov · 2023-04-04T09:28:26Z

Codecov Report

Merging #3722 (6f1600b) into main (ba3ee38) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #3722   +/-   ##
=======================================
  Coverage   52.41%   52.41%           
=======================================
  Files          59       59           
  Lines       16902    16902           
=======================================
  Hits         8859     8859           
  Misses       7748     7748           
  Partials      295      295

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

sigv · 2023-04-04T11:21:27Z

Heads up - the CI failed on

debian: The repository 'https://pkgs.nginx.com/plus/R28/debian bullseye Release' does not have a Release file.
alpine: Ignoring https://pkgs.nginx.com/plus/R28/alpine/v3.17/main: Protocol error

sigv · 2023-04-26T11:35:02Z

@vepatel, heads up, the Plus repository seems to be unavailable on the CI run!

build/Dockerfile

8be0144: Rework port binding logic without privileges caused issues for host networking configurations. The Kubernetes documentation states that the `net.*` sysctls can be used with container networking, which was misinterpreted. This commit reverts the change, bringing back NET_BIND_SERVICE to the Nginx process, as well as reverts the libcap package removal done in a later commit. In order to avoid privilege escalation being re-introduced, the IC process is also receiving NET_BIND_SERVICE, so that it can be inherited over to Nginx. This change aims to restore host networking as functional for the Helm chart. A future change is recommended to harden security for the IC process (to drop the capability after executing Nginx) as well as Nginx itself (to drop the capability after binding). OBS! To use a 3.1.0 image, you should manually install the `setcap` binary and add `+ep` on `/nginx-ingress` and `+eip` on `nginx` binary. Co-authored-by: Luca Comellini <[email protected]>

sigv · 2023-04-28T11:37:06Z

Still hitting issues in CI for accessing the Nginx Plus repository. 😕

E: The repository 'https://pkgs.nginx.com/plus/R28/debian bullseye Release' does not have a Release file.

WARNING: Ignoring https://pkgs.nginx.com/plus/R28/alpine/v3.17/main: Protocol error
ERROR: unable to select packages:
  nginx-plus (no such package):
    required by: world[nginx-plus]
  nginx-plus-module-njs (no such package):
    required by: world[nginx-plus-module-njs]
  nginx-plus-module-opentracing (no such package):
    required by: world[nginx-plus-module-opentracing]

All of these jobs report warnings for nginx-repo.tls secrets at start of test run:

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

While most Debian runs have a generic "could not get a Release file" error..
There is Smoke Tests (debian-plus-nap, dos, 1.27.1) which also reports issue with reading the secret data:

  Could not load client certificate (/etc/ssl/nginx/nginx-repo.crt, SslCert option) or key (/etc/ssl/nginx/nginx-repo.key, SslKey option): Error while reading file.

sigv · 2023-04-28T11:38:44Z

@ciarams87 ping ☝🏻
(thanks for the protected branch run)

ciarams87 · 2023-04-28T11:54:46Z

Still hitting issues in CI for accessing the Nginx Plus repository. 😕

Helm Tests (debian-plus, plus)

Smoke Tests (debian-plus, vs, 1.27.1)

Smoke Tests (debian-plus, ts, 1.27.1)

Smoke Tests (debian-plus-nap, dos, 1.27.1)

Smoke Tests (debian-plus-nap, appprotect, 1.27.1)
E: The repository 'https://pkgs.nginx.com/plus/R28/debian bullseye Release' does not have a Release file.
Smoke Tests (alpine-plus, ingresses, 1.27.1)

Smoke Tests (alpine-plus, vsr, 1.27.1)
WARNING: Ignoring https://pkgs.nginx.com/plus/R28/alpine/v3.17/main: Protocol error
ERROR: unable to select packages:
  nginx-plus (no such package):
    required by: world[nginx-plus]
  nginx-plus-module-njs (no such package):
    required by: world[nginx-plus-module-njs]
  nginx-plus-module-opentracing (no such package):
    required by: world[nginx-plus-module-opentracing]
All of these jobs report warnings for nginx-repo.tls secrets at start of test run:
Warning: nginx-repo.crt= is not a valid secret
Warning: nginx-repo.key= is not a valid secret
While most Debian runs have a generic "could not get a Release file" error.. There is Smoke Tests (debian-plus-nap, dos, 1.27.1) which also reports issue with reading the secret data:
  Could not load client certificate (/etc/ssl/nginx/nginx-repo.crt, SslCert option) or key (/etc/ssl/nginx/nginx-repo.key, SslKey option): Error while reading file.

@sigv Running the pipeline again now. Sorry for the inconvenience!

(As an explanation, PRs from external contributors cannot access the build secrets. If there are no changes to the base NGINX Plus images, this normally does not cause a problem as we can use cached layers from other builds, but when changes are made which require rebuilding the whole image, as here, a maintainer needs to open a separate branch which does have secret-read access)

lucacome

Thanks @sigv !

8be0144: Rework port binding logic without privileges caused issues for host networking configurations. The Kubernetes documentation states that the `net.*` sysctls can be used with container networking, which was misinterpreted. This commit reverts the change, bringing back NET_BIND_SERVICE to the Nginx process, as well as reverts the libcap package removal done in a later commit. In order to avoid privilege escalation being re-introduced, the IC process is also receiving NET_BIND_SERVICE, so that it can be inherited over to Nginx. This change aims to restore host networking as functional for the Helm chart. A future change is recommended to harden security for the IC process (to drop the capability after executing Nginx) as well as Nginx itself (to drop the capability after binding). OBS! To use a 3.1.0 image, you should manually install the `setcap` binary and add `+ep` on `/nginx-ingress` and `+eip` on `nginx` binary. (cherry picked from commit 5d56f71)

8be0144: Rework port binding logic without privileges caused issues for host networking configurations. The Kubernetes documentation states that the `net.*` sysctls can be used with container networking, which was misinterpreted. This commit reverts the change, bringing back NET_BIND_SERVICE to the Nginx process, as well as reverts the libcap package removal done in a later commit. In order to avoid privilege escalation being re-introduced, the IC process is also receiving NET_BIND_SERVICE, so that it can be inherited over to Nginx. This change aims to restore host networking as functional for the Helm chart. A future change is recommended to harden security for the IC process (to drop the capability after executing Nginx) as well as Nginx itself (to drop the capability after binding). OBS! To use a 3.1.0 image, you should manually install the `setcap` binary and add `+ep` on `/nginx-ingress` and `+eip` on `nginx` binary. (cherry picked from commit 5d56f71) Co-authored-by: Valters Jansons <[email protected]>

muhammadwasay · 2023-11-29T07:32:26Z

@sigv I am confused after reading all the comments. Just want to confirm that can nginxinc/kubernetes-ingress avoid the privilege escalation of NET_BIND_SERVICE or not?

sigv requested a review from a team as a code owner April 3, 2023 18:00

github-actions bot added the helm_chart Pull requests that update the Helm Chart label Apr 3, 2023

sigv force-pushed the inherit-netbindservice branch 4 times, most recently from 184f937 to aa3f2ab Compare April 3, 2023 19:32

sigv force-pushed the inherit-netbindservice branch from aa3f2ab to 1db6c6f Compare April 3, 2023 20:04

sigv force-pushed the inherit-netbindservice branch from 1db6c6f to 2cbfdf0 Compare April 3, 2023 21:48

github-actions bot added the tests Pull requests that update tests label Apr 4, 2023

sigv force-pushed the inherit-netbindservice branch from 3419fde to 6c16513 Compare April 4, 2023 18:24

github-actions bot added documentation Pull requests/issues for documentation and removed tests Pull requests that update tests labels Apr 4, 2023

sigv force-pushed the inherit-netbindservice branch from 4b952a3 to a8bd2d3 Compare April 5, 2023 07:47

github-actions bot removed the documentation Pull requests/issues for documentation label Apr 5, 2023

vepatel requested a review from lucacome April 5, 2023 10:59

sigv force-pushed the inherit-netbindservice branch 2 times, most recently from b65e804 to f27c32a Compare April 6, 2023 10:15

vepatel requested a review from jasonwilliams14 April 6, 2023 11:41

sigv force-pushed the inherit-netbindservice branch 3 times, most recently from 99e8aba to 3c5a700 Compare April 11, 2023 07:24

sigv force-pushed the inherit-netbindservice branch 2 times, most recently from f0d1e0f to 355a46e Compare April 19, 2023 08:18

vepatel added this to the v3.1.1 milestone Apr 19, 2023

danielnginx modified the milestone: v3.1.1 Apr 19, 2023

sigv force-pushed the inherit-netbindservice branch 3 times, most recently from 0aacbfd to 9e2f0a1 Compare April 26, 2023 11:08

sigv force-pushed the inherit-netbindservice branch 2 times, most recently from 4f4cda9 to ede9268 Compare April 26, 2023 21:37

ciarams87 approved these changes Apr 27, 2023

View reviewed changes

jjngx approved these changes Apr 27, 2023

View reviewed changes

sigv force-pushed the inherit-netbindservice branch from ede9268 to c5882fa Compare April 27, 2023 19:10

lucacome requested changes Apr 28, 2023

View reviewed changes

build/Dockerfile Outdated Show resolved Hide resolved

build/Dockerfile Outdated Show resolved Hide resolved

sigv force-pushed the inherit-netbindservice branch 2 times, most recently from 81edd48 to cf92c83 Compare April 28, 2023 09:46

sigv force-pushed the inherit-netbindservice branch from cf92c83 to 6f1600b Compare April 28, 2023 09:50

lucacome approved these changes Apr 28, 2023

View reviewed changes

lucacome merged commit 5d56f71 into nginx:main Apr 28, 2023

sigv deleted the inherit-netbindservice branch April 29, 2023 13:14

brian-provenzano mentioned this pull request May 22, 2023

Pods restart loop with error "[emerg] 23#23: bind() to 0.0.0.0:80 failed (13: Permission denied)" in latest chart/version for daemonset #3932

Closed

lucacome added the enhancement Pull requests for new features/feature enhancements label Jun 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inherit NET_BIND_SERVICE from IC to Nginx #3722

Inherit NET_BIND_SERVICE from IC to Nginx #3722

sigv commented Apr 3, 2023 •

edited

Loading

tstraley commented Apr 3, 2023

sigv commented Apr 3, 2023 •

edited

Loading

tstraley commented Apr 3, 2023

sigv commented Apr 3, 2023

sigv commented Apr 3, 2023

sigv commented Apr 3, 2023 •

edited

Loading

sigv commented Apr 4, 2023

codecov bot commented Apr 4, 2023 •

edited

Loading

sigv commented Apr 4, 2023

sigv commented Apr 26, 2023

sigv commented Apr 28, 2023 •

edited

Loading

sigv commented Apr 28, 2023 •

edited

Loading

ciarams87 commented Apr 28, 2023

lucacome left a comment

muhammadwasay commented Nov 29, 2023

Inherit NET_BIND_SERVICE from IC to Nginx #3722

Inherit NET_BIND_SERVICE from IC to Nginx #3722

Conversation

sigv commented Apr 3, 2023 • edited Loading

Proposed changes

Checklist

tstraley commented Apr 3, 2023

sigv commented Apr 3, 2023 • edited Loading

tstraley commented Apr 3, 2023

sigv commented Apr 3, 2023

sigv commented Apr 3, 2023

sigv commented Apr 3, 2023 • edited Loading

sigv commented Apr 4, 2023

codecov bot commented Apr 4, 2023 • edited Loading

Codecov Report

sigv commented Apr 4, 2023

sigv commented Apr 26, 2023

sigv commented Apr 28, 2023 • edited Loading

sigv commented Apr 28, 2023 • edited Loading

ciarams87 commented Apr 28, 2023

lucacome left a comment

Choose a reason for hiding this comment

muhammadwasay commented Nov 29, 2023

sigv commented Apr 3, 2023 •

edited

Loading

sigv commented Apr 3, 2023 •

edited

Loading

sigv commented Apr 3, 2023 •

edited

Loading

codecov bot commented Apr 4, 2023 •

edited

Loading

sigv commented Apr 28, 2023 •

edited

Loading

sigv commented Apr 28, 2023 •

edited

Loading