Config: Do not log URL parameters.

[RichardoC]

What this PR does / why we need it:

The default logging configuration will capture the url query strings, which often have sensitive information in them [1]
This PR changes that behaviour so these are no longer logged by default.

This has already been reported to [email protected], and they said to open a public PR about it.

[1] https://owasp.org/www-community/vulnerabilities/Information_exposure_through_query_strings_in_url

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

How Has This Been Tested?

Already running with this configuration on my own cluster

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.

[k8s-ci-robot]

Welcome @RichardoC!

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]

Hi @RichardoC. 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-sigs/prow repository.

[netlify]

✅ Deploy Preview for kubernetes-ingress-nginx ready!

Name	Link
🔨 Latest commit	2c7200c
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/67bc8672de7d260008e200cd
😎 Deploy Preview	https://deploy-preview-12878--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.

[RichardoC]

A bit more context

CVSS 4 score 6.8
CVSS:4.0/AV:L/AC:L/AT:N/PR:L/UI:N/VC:H/VI:N/VA:N/SC:N/SI:N/SA:N

Affected versions:
Every version since at least the following commit for the log issue. I stopped my git search at this point.

Add ability to customize upstream and stream log format
0ca3aef by Giancarlo Rubio <[email protected]>, Wed Mar 1 17:47:11 2017

Reproduction steps for logging the sensitive information

Use ingress-nginx as an ingress controller for a service that accepts sensitive values in url queries (such. as oauth)
Make a sensitive request to that ingress
See the sensitive values in the nginx logs

[RichardoC]

I can't trigger a rerun for these tests, but they're complaining with Interrupted by Other Ginkgo Process which seems unrelated to this change

[RichardoC]

It would be nice if we could add a test to prevent regressions here, but I'm not sure where best to add a test for the format of a generated log line

[Gacko]

Could you please remove changes, which are not relevant to the actual purpose of your PR? That also re-triggers CI.

[Gacko]

Also keep in mind that users, that want to change this and prevent sensitive information from being logged, can also do this by configuring their own logging format.

[RichardoC]

Could you please remove changes, which are not relevant to the actual purpose of your PR? That also re-triggers CI.

Of course, sorry about that. my IDE autoformatted 🤦

[RichardoC]

Also keep in mind that users, that want to change this and prevent sensitive information from being logged, can also do this by configuring their own logging format.

Yes, which is what I'm currently doing but logging sensitive details should be "opt out" rather than "opt in" in my opinion

[RichardoC]

Ah, the tests broke as they're asserting based on information in the logs
"log does not contains id=dummy_log_splitter_foo_bar"
How would you prefer these being fixed?

[longwuyuan]

I know you commented on the tpoic please let me ask again. Is this optional or all users are going to get to your logformat change now ?

[RichardoC]

This is optional, but all users would get it by default because the current "log sensitive information by default" isn't a great default in my opinion.
Users can override back to the previous default if they want, similar to how they now can manually configure logging to avoid this issue

[longwuyuan]

You can help current users NOT experience a change, regardless of opinions, and make the use of your change optional. Please consider that forced opt-in is something that the project ends up needing to support and answer users on. Not to mention not having resources to sustain status-quo.

[RichardoC]

You can help current users NOT experience a change, regardless of opinions, and make the use of your change optional. Please consider that forced opt-in is something that the project ends up needing to support and answer users on. Not to mention not having resources to sustain status-quo.

As already discussed with the [email protected], the workaround for this is to manually override the log format, similar to the following in a values.yaml for the helm chart. That being said, this has caught me out every single time I've used this ingress controller for my job, leading to "fun" security incidents, and I was hoping I could convince the maintainers to use a more secure by default configuration to avoid other folks having the same pain.

controller:
  config:
        log-format-upstream: $remote_addr - $remote_user [$time_local] "$request_method $uri $server_protocol" $status $body_bytes_sent "$http_referer" "$http_user_agent" $request_length $request_time [$proxy_upstream_name] [$proxy_alternative_upstream_name] $upstream_addr $upstream_response_length $upstream_response_time $upstream_status $req_id # Replaced $request with $uri to make sure we don't log url parameters

I hope this helps explain why I've opened this PR

[RichardoC]

If users want to use their own custom format, either before or after this change they can set the following in their values.yaml

Their existing custom logging configuration is unaffected by this change

controller:
  config:
        log-format-upstream:  SUPER CUSTOM LOGGING CONFIGURATION

[longwuyuan]

Thanks for your contributions as that is the only way to improve the project. My request is related to impact. If this PR makes it optional and we get user feedback, then in a later PR we can make your change the default. That way the project will not force a opt-in. There are aspects of user experience, in the context of forced-op-in, that make me request this of you.

[RichardoC]

Thanks for your contributions as that is the only way to improve the project. My request is related to impact. If this PR makes it optional and we get user feedback, then in a later PR we can make your change the default. That way the project will not force a opt-in. There are aspects of user experience, in the context of forced-op-in, that make me request this of you.

It's already optional to have this change, so this is the "later PR we can make your change the default" PR.

To repeat, users can already "opt in" to this behaviour and can if they want. That already exists. This PR is for the second stage of making it the default.

The only difference I see if how we communicate when this change is coming, which I'll need help from the maintainers for.

I hope this helps

[Gacko]

So after letting this settle a bit and finding the time to have another look, I basically agree with this change.

Ingress NGINX should be secure by default and not expose a risk for people installing it without changing any of the default values.

Of course we should also provide possibilities to still change behavior, even if this means the user's deployment gets less secure, but having such stuff configurable also makes it easier for us to change behavior in the future anyway.

So overall LGTM, but as noticed before we also need to change the tests. I therefore triggered them once again.

Regarding communication: We will handle this in the release notes of an upcoming v1.13, no worries.

[Gacko]

/triage accepted
/kind feature
/priority backlog
/lgtm
/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Gacko, RichardoC

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

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [Gacko]

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

[RichardoC]

Thanks @Gacko , I'm happy to do the work to rewrite the tests, if you can point me to an existing pattern I can use to override the log config for one specific test