Update default backend to add TLS >=1.2 support #9166

JJotah · 2022-10-15T11:45:36Z

What this PR does / why we need it:

Security issue with TLS < 1.2 https://github.com//issues/9155

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
CVE Report (Scanner found CVE and adding report)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

#9155

How Has This Been Tested?

Deploy this helm chart
Point to the default backend
Execute nmap --script ssl-enum-ciphers -p PORT with the URL

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added unit and/or e2e tests to cover my changes.
All new and existing tests passed.
Added Release Notes.

Does my pull request need a release note?

Any user-visible or operator-visible change qualifies for a release note. This could be a:

CLI change
API change
UI change
configuration schema change
behavioral change
change in non-functional attributes such as efficiency or availability, availability of a new platform
a warning about a deprecation
fix of a previous Known Issue
fix of a vulnerability (CVE)

No release notes are required for changes to the following:

Tests
Build infrastructure
Fixes for unreleased bugs

For more tips on writing good release notes, check out the Release Notes Handbook

### 4.4.0
- Update Default Backend due TLS < 1.2 Security Issue

k8s-ci-robot · 2022-10-15T11:45:45Z

Welcome @JJotah!

It looks like this is your first PR to kubernetes/ingress-nginx 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/ingress-nginx has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-10-15T11:45:45Z

Hi @JJotah. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

charts/ingress-nginx/Chart.yaml

charts/ingress-nginx/README.md

charts/ingress-nginx/values.yaml

Volatus

Thanks for the PR. See comments for some suggestions.

bmv126 · 2022-10-20T14:01:44Z

@JJotah
I have few queries on the package:

Why /healthz path is being removed ? Will it not cause breaking change ?
https://kubernetes.github.io/ingress-nginx/user-guide/default-backend/
Also I do not see ssl being set in listen directive in the default.conf. How it will resolve the tls 1.2 >= issue ?
https://github.com/kubernetes/ingress-nginx/pull/9166/files#diff-86820e49f1b0e9f1c20d6f68cef87b3692f7957e44874b5fc064c5a9c8683762R5
What about custom-error via default backend ? Is it still supported ?
https://github.com/kubernetes/ingress-nginx/tree/main/images/custom-error-pages

JJotah · 2022-10-20T16:33:25Z

@JJotah I have few queries on the package:

1. Why /healthz path is being removed ? Will it not cause breaking change ?
   https://kubernetes.github.io/ingress-nginx/user-guide/default-backend/

2. Also I do not see ssl being set in listen directive in the default.conf. How it will resolve the tls 1.2 >= issue ?
   https://github.com/kubernetes/ingress-nginx/pull/9166/files#diff-86820e49f1b0e9f1c20d6f68cef87b3692f7957e44874b5fc064c5a9c8683762R5

3. What about custom-error via default backend ? Is it still supported ?
   https://github.com/kubernetes/ingress-nginx/tree/main/images/custom-error-pages

Hi @bmv126
1.- we don't need to pass healthz because the healtz is on the 80 port

2.- The issue is when you have a certificate in your load balancer (example AWS) and automatically defaultBackend enables de tls with the policy default created

3.- We can use custom error pages dockerfile for the moment in the values

longwuyuan · 2022-10-20T18:11:54Z

@JJotah, does this mean that this change is only relevant for TLS Termination in LB instead of TLS termination in controller ?

…

On Thu, Oct 20, 2022 at 10:03 PM Juan José Ruiz Romero < ***@***.***> wrote: @JJotah <https://github.com/JJotah> I have few queries on the package: 1. Why /healthz path is being removed ? Will it not cause breaking change ? https://kubernetes.github.io/ingress-nginx/user-guide/default-backend/ 2. Also I do not see ssl being set in listen directive in the default.conf. How it will resolve the tls 1.2 >= issue ? https://github.com/kubernetes/ingress-nginx/pull/9166/files#diff-86820e49f1b0e9f1c20d6f68cef87b3692f7957e44874b5fc064c5a9c8683762R5 3. What about custom-error via default backend ? Is it still supported ? https://github.com/kubernetes/ingress-nginx/tree/main/images/custom-error-pages Hi @bmv126 <https://github.com/bmv126> 1.- we don't need to pass healthz because the healtz is on the 80 port 2.- The issue is when you have a certificate in your load balancer (example AWS) and automatically defaultBackend enables de tls with the policy default created 3.- We can use custom error pages dockerfile for the moment in the values — Reply to this email directly, view it on GitHub <#9166 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWXC2IOPZURBHZ637HDWEFX6TANCNFSM6AAAAAARF4CUXY> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

-- ; Long Wu Yuan

JJotah · 2022-10-20T19:22:24Z

@JJotah, does this mean that this change is only relevant for TLS Termination in LB instead of TLS termination in controller ?
…
On Thu, Oct 20, 2022 at 10:03 PM Juan José Ruiz Romero < @.> wrote: @JJotah https://github.com/JJotah I have few queries on the package: 1. Why /healthz path is being removed ? Will it not cause breaking change ? https://kubernetes.github.io/ingress-nginx/user-guide/default-backend/ 2. Also I do not see ssl being set in listen directive in the default.conf. How it will resolve the tls 1.2 >= issue ? https://github.com/kubernetes/ingress-nginx/pull/9166/files#diff-86820e49f1b0e9f1c20d6f68cef87b3692f7957e44874b5fc064c5a9c8683762R5 3. What about custom-error via default backend ? Is it still supported ? https://github.com/kubernetes/ingress-nginx/tree/main/images/custom-error-pages Hi @bmv126 https://github.com/bmv126 1.- we don't need to pass healthz because the healtz is on the 80 port 2.- The issue is when you have a certificate in your load balancer (example AWS) and automatically defaultBackend enables de tls with the policy default created 3.- We can use custom error pages dockerfile for the moment in the values — Reply to this email directly, view it on GitHub <#9166 (comment)>, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWXC2IOPZURBHZ637HDWEFX6TANCNFSM6AAAAAARF4CUXY . You are receiving this because you are subscribed to this thread.Message ID: @.>
-- ; Long Wu Yuan

Hi @longwuyuan
Yeah, 2 things here
The current docker image from k8s registry as ingress controller we shouldn't use that

Second sure, to create new ingress controller and you didn't specify de policy in lb automatically in 443 create de default one (I only tested in AWS) and use tls 1.0 tls 1.1 that is a security issue

rikatz · 2022-10-25T22:07:38Z

charts/ingress-nginx/README.md

 | defaultBackend.image.runAsNonRoot | bool | `true` |  |
 | defaultBackend.image.runAsUser | int | `65534` |  |
-| defaultBackend.image.tag | string | `"1.5"` |  |
+| defaultBackend.image.tag | string | `"1.19.10-alpine"` |  |


can we use the last nginx version?

rikatz · 2022-10-25T22:08:57Z

charts/ingress-nginx/values.yaml

-    registry: registry.k8s.io
-    image: defaultbackend-amd64
+    repository: nginx
+    tag: 1.19.10-alpine


same here, let's use latest nginx version (not :latest, but v1.22 etc)

Lets go ahead and use
cgr.dev/chainguard/nginx:1.23.1@sha256:310f9a01fc3e7a9410ae7ea1d9cac5add66d3f95d081efa6693a829e1b6aaa70

rikatz · 2022-10-25T22:10:10Z

@strongjz what if we use https://github.com/chainguard-images/nginx ? I would love to use distroless here instead of some alpine image :)

strongjz · 2022-11-04T17:07:32Z

/kind feature
/triage accepted
/priority backlog

JJotah · 2022-11-04T21:23:46Z

@strongjz what if we use https://github.com/chainguard-images/nginx ? I would love to use distroless here instead of some alpine image :)

Ok agree let me work on it

rikatz · 2022-11-20T20:22:35Z

@JJotah let me know on this :)

k8s-ci-robot · 2023-01-09T03:19:12Z

@JJotah: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

strongjz · 2023-02-18T14:55:27Z

Please rebase and fix the merge conflicts.

longwuyuan · 2024-09-04T13:30:09Z

@JJotah are you still available to make progress on this ?

@Gacko @strongjz @rikatz we need to get this done for the users who enable default-backend.

Gacko

Do not merge. I'd rather have this stuff in the image than wrapped in the chart.

k8s-ci-robot · 2024-09-04T13:32:45Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: JJotah
Once this PR has been reviewed and has the lgtm label, please assign ubergesundheit for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

charts/ingress-nginx/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

longwuyuan · 2024-09-04T13:37:30Z

@Gacko Please elaborate "in the image". I only understand that its optional and when someone opts-in from the values file, only then the default-backend-deployment gets spec'd with the image that @rikatz and @strongjz discussed.

Gacko · 2024-09-04T13:42:10Z

We can make the default backend image safe by default. I would not implement the whole TLS stuff in the chart. This is not useful and a lot of this stuff like the default 404 response belongs into the image IMHO.

longwuyuan · 2024-09-04T13:50:23Z

ok. So are you going to use the $ROOT/images/custom-error-pages itself as the default-backend or are you going to create a new image ?

Gacko · 2024-09-04T14:08:49Z

Actually I wasn't even aware the default backend is using TLS. I mean, it only serves a 404 and plain text. No user data should be transferred to it - so why should it serve TLS after all?

longwuyuan · 2024-09-04T14:17:03Z

I am confused on that as well. Need to think through the workflow to answer your question. But one factor to note here is that this user is talking about a response from enabling default-backend, when the termination of TLS on a NLB

JJotah added 6 commits October 15, 2022 11:24

Add configmap for custom message in default backend

c4d559a

Updating default-backend-deployment to use the new configmap and images

d3f8f06

Update values yaml file due security reasons in default backend

8e9c000

Update chart.yaml with new changes

10e3e6f

Update values.yaml file with the new changes

80b5022

Update Changelog due security issue TLS < 1.2

d9ed1c4

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 15, 2022

k8s-ci-robot requested a review from ChiefAlexander October 15, 2022 11:45

k8s-ci-robot added the area/helm Issues or PRs related to helm charts label Oct 15, 2022

k8s-ci-robot requested a review from cpanato October 15, 2022 11:45

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Oct 15, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 15, 2022

JJotah added 2 commits October 15, 2022 16:19

update values with new conf for lint

ad86933

Update README with helm-docs

794bc39

Volatus reviewed Oct 17, 2022

View reviewed changes

charts/ingress-nginx/Chart.yaml Outdated Show resolved Hide resolved

Volatus reviewed Oct 17, 2022

View reviewed changes

charts/ingress-nginx/README.md Outdated Show resolved Hide resolved

Volatus reviewed Oct 17, 2022

View reviewed changes

charts/ingress-nginx/values.yaml Outdated Show resolved Hide resolved

Volatus reviewed Oct 17, 2022

View reviewed changes

charts/ingress-nginx/values.yaml Outdated Show resolved Hide resolved

Volatus suggested changes Oct 17, 2022

View reviewed changes

JJotah and others added 6 commits October 18, 2022 08:28

Merge branch 'kubernetes:main' into main

76d38c8

Update Chart.yaml file

cf4dfe3

Update README with helm-docs

cad6312

Updating default-backend-deployment to make it sure in security terms

57abe3d

Update default-backend-service with new non-root port

343fee0

Update default values file

a790354

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 19, 2022

rikatz reviewed Oct 25, 2022

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2023

Gacko requested changes Sep 4, 2024

View reviewed changes

Gacko force-pushed the main branch from 3ff42a9 to 0109163 Compare March 24, 2025 17:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update default backend to add TLS >=1.2 support #9166

Update default backend to add TLS >=1.2 support #9166

JJotah commented Oct 15, 2022 •

edited

Loading

k8s-ci-robot commented Oct 15, 2022

k8s-ci-robot commented Oct 15, 2022

Volatus left a comment

bmv126 commented Oct 20, 2022

JJotah commented Oct 20, 2022

longwuyuan commented Oct 20, 2022 via email

JJotah commented Oct 20, 2022

rikatz Oct 25, 2022

rikatz Oct 25, 2022

strongjz Nov 4, 2022

rikatz commented Oct 25, 2022

strongjz commented Nov 4, 2022

JJotah commented Nov 4, 2022

rikatz commented Nov 20, 2022

k8s-ci-robot commented Jan 9, 2023

strongjz commented Feb 18, 2023

longwuyuan commented Sep 4, 2024

Gacko left a comment

k8s-ci-robot commented Sep 4, 2024

longwuyuan commented Sep 4, 2024

Gacko commented Sep 4, 2024

longwuyuan commented Sep 4, 2024

Gacko commented Sep 4, 2024

longwuyuan commented Sep 4, 2024

Update default backend to add TLS >=1.2 support #9166

Are you sure you want to change the base?

Update default backend to add TLS >=1.2 support #9166

Conversation

JJotah commented Oct 15, 2022 • edited Loading

What this PR does / why we need it:

Types of changes

Which issue/s this PR fixes

How Has This Been Tested?

Checklist:

Does my pull request need a release note?

k8s-ci-robot commented Oct 15, 2022

k8s-ci-robot commented Oct 15, 2022

Volatus left a comment

Choose a reason for hiding this comment

bmv126 commented Oct 20, 2022

JJotah commented Oct 20, 2022

longwuyuan commented Oct 20, 2022 via email

JJotah commented Oct 20, 2022

rikatz Oct 25, 2022

Choose a reason for hiding this comment

rikatz Oct 25, 2022

Choose a reason for hiding this comment

strongjz Nov 4, 2022

Choose a reason for hiding this comment

rikatz commented Oct 25, 2022

strongjz commented Nov 4, 2022

JJotah commented Nov 4, 2022

rikatz commented Nov 20, 2022

k8s-ci-robot commented Jan 9, 2023

strongjz commented Feb 18, 2023

longwuyuan commented Sep 4, 2024

Gacko left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 4, 2024

longwuyuan commented Sep 4, 2024

Gacko commented Sep 4, 2024

longwuyuan commented Sep 4, 2024

Gacko commented Sep 4, 2024

longwuyuan commented Sep 4, 2024

JJotah commented Oct 15, 2022 •

edited

Loading