Allow overriding real_ip_recursive #8366

tomaspinho · 2022-03-21T19:01:24Z

What this PR does / why we need it:

Allows control over real_ip_recursive, fixing problem shown in issues like: #4073

We are using this to fix a X-Forwarded-For spoofing issue we found while using AWS ALB as a service load balancer. Turns out they respect upstream X-Forwarded-For and don't allow us to strip/replace them, causing NGINX Ingress Controller to take the first value of the header as the defacto client IP, which can be spoofed.

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation only

Which issue/s this PR fixes

#4073

How Has This Been Tested?

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I've read the CONTRIBUTION guide
I have added tests to cover my changes.
All new and existing tests passed.

linux-foundation-easycla · 2022-03-21T19:01:29Z

The committers listed above are authorized under a signed CLA.

✅ login: tomaspinho / name: Tomás Pinho (bcab7c1, fbb2c3b, 8a268e4, ae7fee9, 93f0afc, 6d613e7, 5b21caf)

k8s-ci-robot · 2022-03-21T19:01:32Z

Welcome @tomaspinho!

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-03-21T19:01:33Z

Hi @tomaspinho. 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.

longwuyuan · 2022-03-21T23:15:27Z

@tomaspinho thanks for contributing to this long pending issue. Helps a lot.

Any chance that a e2e test can be added that simulates the complete event sequence from a client spoofing the ip with X-Forward-For header and the controller picking one the last "real" client_ip_address.

tomaspinho · 2022-03-22T11:39:50Z

It's my first time contributing to this project, so I have to learn how to run and write e2e tests.

I hope to have some time this weekend to take a look at this. Thanks @longwuyuan

tomaspinho · 2022-03-22T14:50:13Z

@longwuyuan turns out writing the e2e test was way easier than I expected! You folks have done an amazing job here 🤩

Added an e2e test exercising both cases for the flag.

k8s-triage-robot · 2022-06-20T15:20:22Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

tomaspinho · 2022-06-26T11:55:41Z

/remove-lifecycle stale

k8s-triage-robot · 2022-09-24T12:09:03Z

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle stale
Mark this issue or PR as rotten with /lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot · 2022-10-24T12:24:31Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

Mark this issue or PR as fresh with /remove-lifecycle rotten
Close this issue or PR with /close
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot · 2022-11-23T13:00:06Z

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

After 90d of inactivity, lifecycle/stale is applied
After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten
Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

michalziobro · 2023-11-29T06:43:27Z

looks like this PR was abandoned but it addresses a vital issue. Are there any chances to merge?

…sive

netlify · 2023-11-29T08:27:11Z

✅ Deploy Preview for kubernetes-ingress-nginx ready!

Name	Link
🔨 Latest commit	`6d613e7`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/65df1e5cb3e7e400087b2b2a
😎 Deploy Preview	https://deploy-preview-8366--kubernetes-ingress-nginx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

k8s-ci-robot · 2023-11-29T08:27:32Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tomaspinho
Once this PR has been reviewed and has the lgtm label, please ask for approval from strongjz. 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:

OWNERS

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

…ving unnessary Sprintfs

tomaspinho · 2023-11-29T08:50:59Z

Hey @rikatz @strongjz! 😄

Just rebased this PR and fixed the linting/formatting according to upstream. Wondering if there's anything you need from me in order to merge? Thanks!

rikatz · 2024-02-27T13:33:19Z

internal/ingress/controller/config/config.go

@@ -790,6 +795,7 @@ func NewDefault() Configuration {
 		ErrorLogLevel:                    errorLevel,
 		UseForwardedHeaders:              false,
 		EnableRealIP:                     false,
+		EnableRealIPRecursive:            true,


shouldn't this be disabled by default?

there is a "problem" with this approach of a flag defaulted to true: if you set it as false on your configmap, it may or may not represent as empty (depending on the marshaller, eg using omitempty on json flag) so because it is empty it will be defaulted to...true :)

To confirm this behavior, can you add some e2e test with both EnableRealIPRecursive = true or false on the configmap, and check if the template is rendered correctly?

Hi @rikatz ,

I've set the default to false. My initial reasoning for it to be true was that I think the more secure option should be enabled by default if it doesn't otherwise impact any functionality. Because the setting itself is dependent on EnableRealIp being set to true, the more secure configuration would be the default when EnableRealIp would be used. This is a taste thing, and not a deal breaker for getting this merged.

The nginx default is off, so let's set this to false.

test/e2e/settings/enable_real_ip_recursive.go

strongjz · 2024-04-12T15:04:29Z

/retest

strongjz · 2024-04-12T15:06:22Z

/triage accepted
/kind feature
/priority backlog

strongjz · 2024-04-12T15:06:44Z

/ok-to-test

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 21, 2022

k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. needs-priority labels Mar 21, 2022

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Mar 21, 2022

k8s-ci-robot requested a review from rikatz March 21, 2022 19:01

k8s-ci-robot added the area/docs label Mar 21, 2022

k8s-ci-robot requested a review from strongjz March 21, 2022 19:01

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Mar 21, 2022

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 22, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 20, 2022

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 26, 2022

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 24, 2022

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Oct 24, 2022

k8s-ci-robot closed this Nov 23, 2022

strongjz self-assigned this May 15, 2023

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

tomaspinho added 4 commits November 29, 2023 08:23

adds the enable-real-ip-recursive allowing control over real_ip_recur…

bcab7c1

…sive

documents new enable-real-ip-recursive configmap setting

8a268e4

add e2e test for enable-real-ip-recursive

ae7fee9

capitalizes IP in EnableRealIPRecursive to match upstream changes

93f0afc

tomaspinho force-pushed the main branch from f6ff178 to 93f0afc Compare November 29, 2023 08:27

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

appease the linter by promoting re-used header name to const and remo…

fbb2c3b

…ving unnessary Sprintfs

rikatz reviewed Feb 27, 2024

View reviewed changes

test/e2e/settings/enable_real_ip_recursive.go Outdated Show resolved Hide resolved

rikatz removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Feb 27, 2024

tomaspinho added 2 commits February 28, 2024 11:49

nit: fix copyright year

5b21caf

fix: make EnableRealIPRecursive default to false

6d613e7

strongjz added this to the release-1.11 milestone Apr 12, 2024

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow overriding real_ip_recursive #8366

Allow overriding real_ip_recursive #8366

tomaspinho commented Mar 21, 2022 •

edited

Loading

linux-foundation-easycla bot commented Mar 21, 2022 •

edited

Loading

k8s-ci-robot commented Mar 21, 2022

k8s-ci-robot commented Mar 21, 2022

longwuyuan commented Mar 21, 2022

tomaspinho commented Mar 22, 2022

tomaspinho commented Mar 22, 2022

k8s-triage-robot commented Jun 20, 2022

tomaspinho commented Jun 26, 2022

k8s-triage-robot commented Sep 24, 2022

k8s-triage-robot commented Oct 24, 2022

k8s-triage-robot commented Nov 23, 2022

michalziobro commented Nov 29, 2023

netlify bot commented Nov 29, 2023 •

edited

Loading

k8s-ci-robot commented Nov 29, 2023

tomaspinho commented Nov 29, 2023

rikatz Feb 27, 2024

rikatz Feb 27, 2024

tomaspinho Feb 28, 2024

strongjz Apr 12, 2024

strongjz commented Apr 12, 2024

strongjz commented Apr 12, 2024

strongjz commented Apr 12, 2024

Allow overriding real_ip_recursive #8366

Are you sure you want to change the base?

Allow overriding real_ip_recursive #8366

Conversation

tomaspinho commented Mar 21, 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:

linux-foundation-easycla bot commented Mar 21, 2022 • edited Loading

k8s-ci-robot commented Mar 21, 2022

k8s-ci-robot commented Mar 21, 2022

longwuyuan commented Mar 21, 2022

tomaspinho commented Mar 22, 2022

tomaspinho commented Mar 22, 2022

k8s-triage-robot commented Jun 20, 2022

tomaspinho commented Jun 26, 2022

k8s-triage-robot commented Sep 24, 2022

k8s-triage-robot commented Oct 24, 2022

k8s-triage-robot commented Nov 23, 2022

michalziobro commented Nov 29, 2023

netlify bot commented Nov 29, 2023 • edited Loading

✅ Deploy Preview for kubernetes-ingress-nginx ready!

k8s-ci-robot commented Nov 29, 2023

tomaspinho commented Nov 29, 2023

rikatz Feb 27, 2024

Choose a reason for hiding this comment

rikatz Feb 27, 2024

Choose a reason for hiding this comment

tomaspinho Feb 28, 2024

Choose a reason for hiding this comment

strongjz Apr 12, 2024

Choose a reason for hiding this comment

strongjz commented Apr 12, 2024

strongjz commented Apr 12, 2024

strongjz commented Apr 12, 2024

tomaspinho commented Mar 21, 2022 •

edited

Loading

linux-foundation-easycla bot commented Mar 21, 2022 •

edited

Loading

netlify bot commented Nov 29, 2023 •

edited

Loading