feat: add annotation to allow to add custom response headers

[cgroschupp]

What this PR does / why we need it:

Because of the CVE-2021-25742, we need a better way to define response headers, when snippets are disabled.

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [x] 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

fixes #7811

How Has This Been Tested?

Checklist:

• [x] My change requires a change to the documentation.
• [x] I have updated the documentation accordingly.
• [x] I've read the CONTRIBUTION guide
• [x] I have added unit and/or e2e tests to cover my changes.
• [x] All new and existing tests passed.
• [x] 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

Allow to set custom response headers by annotation

[k8s-ci-robot]

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

[strongjz]

@cgroschupp This PR looks good so far, can you add an e2e test as well

Here is the annotations e2e https://github.com/kubernetes/ingress-nginx/tree/main/test/e2e/annotations

[strongjz]

/triage accepted /priority backlog /kind feature

[cgroschupp]

@strongjz i added the e2e tests.

[jacekn]

FYI we identified some downsides of this approach in this thread.

In summary if we use ConfigMaps to configure headers it would be impossible to write admission controller rules to validate headers since it's the Ingress object that would get validate and the object wouldn't not have annotations for us to check.

Alternative approach is to use simple string annotation, PR to do that is here

[github-actions[bot]]

This is stale, but we won't close it automatically, just bare in mind the maintainers may be busy with other tasks and will reach your issue ASAP. If you have any question or request to prioritize this, please reach #ingress-nginx-dev on Kubernetes Slack.

[k8s-triage-robot]

The lifecycle/frozen label can not be applied to PRs.

This bot removes lifecycle/frozen from PRs because:

• Commenting /lifecycle frozen on a PR has not worked since March 2021
• PRs that remain open for >150 days are unlikely to be easily rebased

You can:

• Rebase this PR and attempt to get it merged
• Close this PR with /close

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

/remove-lifecycle frozen

[tao12345666333]

/assign

[jacekn]

Just FYI - there are concerns about using ConfigMaps for headers, some are captured here. Using ConfigMaps will cause problems to other parts of the ecosystem, for example to helm chart authors. We're planning to chat with nginx maintainers to explain rationale behind choosing string annotation for the implementation we used. Maybe, if enough users feel ConfigMaps are good to have we could have two methods to set headers?

[cgroschupp]

Just FYI - there are concerns about using ConfigMaps for headers, some are captured here. Using ConfigMaps will cause problems to other parts of the ecosystem, for example to helm chart authors. We're planning to chat with nginx maintainers to explain rationale behind choosing string annotation for the implementation we used. Maybe, if enough users feel ConfigMaps are good to have we could have two methods to set headers?

I'm not a big fan of having multiple ways to do the same thing.

[jacekn]

I'm not a big fan of having multiple ways to do the same thing.

100% Agreed! The reason why it may be acceptable is because the ConfigMap approach is quite non-standard in the Ingress world from what I can tell, it poses some usability challenges and it can be insecure to use for those who use admission controllers to control security in k8s. It would be very nice if we could allow users to use KISS string approach. But given that some maintainers would like to see ConfigMaps maybe allowing two methods is acceptable to keep maintainers happy while also allowing simple use cases for many users.

[tao12345666333]

please resolve the conflicts, thanks

[cgroschupp]

ConfigMap approach is quite non-standard in the Ingress world

@jacekn I don't know if there is a standard, I know several ways in the ingress world to define headers, for example custom crd(contour, istio), annotation(traefik) and configmap like the ingress-nginx authreq feature.

To me it looks like there is no standard, the solution should match the project.

[longwuyuan]

I am not a developer so should avoid opining here. But after reading content here, I was inclined to opine. Allowing freehand (apologies for lack of more appropriate word) characters in a annotation value, is not a option, even after considering the benefit of ease to spec out headers (from the vast scope of possible headers).

Much much incidental feedback and reasoning has gone into taking the direction of validating input. So even if painful to implement and maintain tons of configMap key:value pairs, that is is the only option to have acceptable risks. Regards.

On Wed, 14 Jun, 2023, 2:03 pm Christian Groschupp, @.***> wrote:

ConfigMap approach is quite non-standard in the Ingress world

@jacekn https://github.com/jacekn I don't know if there is a standard, I know several ways in the ingress world to define headers, for example custom crd(contour, istio), annotation(traefik) and configmap like the ingress-nginx authreq feature.

To me it looks like there is no standard, the solution should match the project.

— Reply to this email directly, view it on GitHub https://github.com/kubernetes/ingress-nginx/pull/9742#issuecomment-1590732651, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABGZVWSRTAA65R3WWUNNU3DXLFZOZANCNFSM6AAAAAAV342S2E . You are receiving this because you are subscribed to this thread.Message ID: @.***>

[jacekn]

I am not a developer so should avoid opining here. But after reading content here, I was inclined to opine. Allowing freehand (apologies for lack of more appropriate word) characters in a annotation value, is not a option, even after considering the benefit of ease to spec out headers (from the vast scope of possible headers). Much much incidental feedback and reasoning has gone into taking the direction of validating input.

Yes I 100% agree! What I'd like to confirm, to avoid confusion is that data needs to be validated regardless of whether it comes from a ConfigMap or from a string. In fact header values inside the ConfigMap will be strings that will need to be validated.

So even if painful to implement and maintain tons of configMap key:value pairs, that is is the only option to have acceptable risks. Regards.

Based on my above comment I disagree that the ConfigMap is the only way. Exact implementation detail of how headers are set does not directly impact whether the values (and header names) are validated or not. We can and should validate them even if we use freeform string annotation, just just they will be validated if we allow arbitrary ConfigMaps to be source of such annotations. Source of the data doesn't change this. At this point we believe that the string implementation is secure but we asked reviewers in the PR to let us know if they see any problems with the code. Whatever validation we think is necessary can be implemented with strings.

The difference between approaches is about UX, simplicity and impact on the wider ecosystem and I believe that simple string annotation is more standard, easier to secure with admission controllers, easier to understand for end users and will have smaller impact on the wider ecosystem (such as helm charts)

[netlify[bot]]

✅ Deploy Preview for kubernetes-ingress-nginx canceled.

Name	Link
🔨 Latest commit	0d2ea1afb19ded188bf187d89aa31105cbd178d4
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-ingress-nginx/deploys/66150850edfcbb00086a9062

[tao12345666333]

please rebase, thanks

[pierluigilenoci]

@tao12345666333, could you please take a look?

[pierluigilenoci]

Two PRs (#9779 and #9742) do the same thing differently in the pipeline. Who knows who will win this race? 😆

[tao12345666333]

https://github.com/kubernetes/ingress-nginx/pull/9779#issuecomment-1679264291

Thanks for your contributions! As I said, the two PRs are both very good. I didn't notice that these two PRs were doing the same thing and there was some delay. Have you joined the Kubernetes Slack? Please contact me.

[rikatz]

/assign /priority important-soon

[linux-foundation-easycla[bot]]

The committers listed above are authorized under a signed CLA.

• :white_check_mark: login: cgroschupp / name: Christian Groschupp (8c34a2d4377ebc6e02498337ac19de74559d8fc0, 3673b3668b3c25890d37ebeefc2834d72fb59075, 21fc50f15b982f9b674bf70098892e30765e499e, 0f5c9fa92032d495d6d0b5be9d97c097d8726907, e82f1cf30947220b20a993afe8b4ca1717da861d, 15bbdf91868442cf0fadd08dd6b9d06cca287e49, 6836b7103d5ccfe83dd75fa243cb0dda366af299, 87f27d98ab7403cb6ae72a15b2ba365b8ef91133, 47cb871d1062584a627ea06e5e23f2db60d1687f, ba3525bf88acb825e46856f3b24046faf0979060, ccb990e349ce69f30d449a4774a0dc9280779449, 196d49d92b18f1f609238d12a5c2eaa21a796f8d, d2a2a111894bf0b76c098e296393602e8c38f639, e6599881670a74192fc6a35d95a471c9be4bc95d, 42dd6b643b71f5851d9342bfff1485a56c85588a, 0d2ea1afb19ded188bf187d89aa31105cbd178d4)

[pierluigilenoci]

@cgroschupp, the PR now has conflicts 😢

[tao12345666333]

Need rebase, thanks

[pierluigilenoci]

@cgroschupp, the PR now has conflicts 😢 @rikatz, could you please take the time to help this PR move forward?

[pierluigilenoci]

@cgroschupp, could you please rebase? 🙏🏻 This PR is turning into a joke! 🤦🏻

[satyamz]

Any update on this PR?

[nguyenthai0107]

Hello everyone. Is this PR still process ? thanks.

[cgroschupp]

@nguyenthai0107 I have updated the PR. From my side is the PR ready. i need feedback from the maintainers.

[tao12345666333]

Thank you! I have re-run all CI jobs.