Custom headers via annotation (make it bulletproof instead of using configuration-snippet)

[mblaschke]

It would be helpful to have support for custom headers via annotation instead of using the snippet annotation

metadata:
  annotations:
    headers.nginx.ingress.kubernetes.io/Content-Security-Policy: |-
      default-src 'self'; img-src https://*; child-src 'none';
    headers.nginx.ingress.kubernetes.io/referrer-policy: no-referrer
    headers.nginx.ingress.kubernetes.io/x-content-type-options: nosniff
    headers.nginx.ingress.kubernetes.io/feature-policy: |-
      camera 'none'; geolocation 'none'; microphone 'none'; payment 'none'
    headers.nginx.ingress.kubernetes.io/permissions-policy: |-
      camera=(), geolocation=(), microphone=(), payment=()

this is more failproof then using more_set_headers as this could break nginx configuration. a dedicated header annotation would also be easier for normal developers without nginx knowledge.

ingress-nginx should be able to fetch all headers.nginx.ingress.kubernetes.io/* annotations and transform them into more_set_headers statements and escape the header name and value properly.

/kind feature

[k8s-ci-robot]

@mblaschke: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

[sherifabdlnaby]

I think this is becoming very important after kubernetes/kubernetes#126811 - CVE-2021-25742 because we can't mitigate it without disabling the *-snippet annotations and the only method to add custom headers per ingress Object is using configuration-snippet...

nginx.ingress.kubernetes.io/configuration-snippet: |
  more_set_headers "Request-Id: $req_id";

I do not necessarily think the suggested implementation is the best "Interface" and I believe we as a community might wanna discuss the how-to.

But an Annotation/(s) for adding headers, in general, is really necessary to mitigate kubernetes/kubernetes#126811 without losing a critical feature we rely on heavily(and I believe many do).

[k8s-triage-robot]

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

[sherifabdlnaby]

I believe this issue should be a priority after CVE-2021-25742.
(commenting to keep issue open)

[k8s-triage-robot]

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

[mblaschke]

/remove-lifecycle rotten

[mblaschke]

bump

[mblaschke]

bump

[mblaschke]

bump

[mblaschke]

bump

[pierluigilenoci]

I'm afraid that hopes of seeing this thing implemented are in vain. 😢

[satyamz]

Hi all 👋🏾
We have worked on the adding custom-response-header annotation support for the ingress-nginx, using which users can specify the custom response headers via annotation. We will soon open a PR for this, we are in process of signing corporate CLA. Thanks.

[mblaschke]

@satyamz
any progress here?

[satyamz]

Hi @mblaschke I opened the PR #9779 today for this.

[pierluigilenoci]

Almost 900 days without seeing a leaf move, and now, in one week, two PRs!
Beyond belief. 🚀

[mblaschke]

I would prefer a solution with only annotations and not an additional configmap to make it easier.

@satyamz
why not split by newline?

annotations:
  nginx.ingress.kubernetes.io/custom-response-headers:
    Cache-Control: no-cache 
    Strict-Transport-Security: max-age=31536000; includeSubDomains

[cgroschupp]

@mblaschke Before I implemented my PR, I looked at how other annotations solve this problem. And I saw that the authreq annotation uses the configmap method. So I chose this way.

https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/authreq/main.go#L290

[satyamz]

@mblaschke the standard followed by ingress controllers like Traefik followed the notion of separation based on operator like ||.

[jacekn]

I also think simple string would be easier to use than a ConfigMap. Looking at other annotations most of them are simple datatypes like strings or integers.
ConfigMaps also introduce some operational problems. For example it would be impossible, or at least very hard, to write admission controller rules to validate headers or enforce certain headers are specified.

[jacekn]

@jacekn this kind of validation can be easily achieved using something like Kyverno.

Can you write kyverno policy to verify object that is not being updated?
Consider the following scenario:

1. User adds ConfigMap with headers defined. The ConfigMap contains headers we don't want to allow or lacks headers we want to enforce. We can't have kyverno policy verify ConfigMaps against nginx rules because there can be non-nginx ConfigMaps.
2. User then adds Ingress object refering the configMap. Can kyverno resolve the ConfigMap name, get the ConfigMap from k8s API and verify against rules?

[pierluigilenoci]

@jacekn, let's assume we have a ConfigMap that looks like this:

apiVersion: v1
kind: ConfigMap
metadata:
  name: nginx-headers
  labels:
    nginx-header-validation: true # label of your choice
data:
  headers:
    - "Cache-Control: no-cache" 
    - "Strict-Transport-Security: max-age=31536000; includeSubDomains"

And then you can validate it with something like this:

apiVersion: kyverno.io/v1
kind: ClusterPolicy
metadata:
  name: check-nginx-headers
spec:
  validationFailureAction: Enforce
  rules:
    - name: check-nginx-headers
      match:
        any:
        - resources:
            kinds:
            - ConfigMap
            selector:
              matchLabels:
                nginx-header-validation: true # label of your choice         
      validate:
        message: "The header `Cache-Control: no-cache` is required."
        pattern:
          data:
            ^(headers):
              "Cache-Control: no-cache" 

I rushed it down so I don't know if it already works 100% but I think it's enough to understand the meaning.

[mblaschke]

For our teams a single annotation based solution would be much easier and for our SRE team it would be easier to support such scenarios.

@pierluigilenoci
In a shared cluster with multiple teams we cannot rely on labels on configmaps. How to control and validate it in such situations?
We're currently using OPA gatekeeper in AKS (because we get Azure support for it).

[jacekn]

@pierluigilenoci the policy you provided requires user to "cooperate" with the admission controller. They could just remove the annotation and bypass the policy.
But more importantly the fact that a ConfigMap exists does not mean that users will use it in their ingresses. They could create new ConfigMap and just refer to it in the ingress annotation bypassing your restrictions

[pierluigilenoci]

@mblaschke @jacekn, the NGINX controller, to import the ConfigMap into the ingresses, must necessarily have to use some mechanism to understand which ConfigMaps are the ones connected to the headers and which are not. This mechanism could be a trivial label. Without the label, the headers are not imported. This label could then be used by Kyverno or OPA (it changes little, both tools work similarly) for the abovementioned checks.

The solution seems straightforward to me. Please write more clearly which cases could not be covered by such a solution.

[jacekn]

@mblaschke @jacekn, the NGINX controller, to import the ConfigMap into the ingresses, must necessarily have to use some mechanism to understand which ConfigMaps are the ones connected to the headers and which are not.

Could you expand on why there must be such mechanism? Currently there isn't one in the nginx controller as far as I can tell. Adding such mechanism would make things harder to implement and it would also reduce user experience.
For example let's assume a user wants to add "X-my-header" with value of "foobar". With annotation we propose all they need to do is add the following annotation:
nginx.ingress.kubernetes.io/custom-response-headers: "X-my-header: foobar"

It's simple, only one object needs to be modified and it's immediately obvious what headers we inject.

If I understand your proposal correctly you'd like for the process to look like this:

1. User adds new ConfigMap with the header value
2. The ConfigMap needs to contain special label, for example nginx-header-validation: true
3. The user then references the ConfigMap in the Ingress like this:
   nginx.ingress.kubernetes.io/custom-headers: myconfigmap

Your version seems significantly more involved and there are more places where mistakes can be made. Users have to remember to add the label, otherwise they'll end up with invalid ingress. It's also harder to write admission controller rules as now we can't parse the ingress itself, we have to parse ConfigMaps instead.

Some admission controller rules would likely be impossible to write even. What if we wanted to have different admission controller rules for Ingresses for the domain example.com and different for example2.com. How would you validate the ConfigMaps if you don't know which Ingress they would be attached to?

The solution seems straightforward to me. Please write more clearly which cases could not be covered by such a solution

I respectfully disagree that the Configmap approach is straightforward. Simple string annotation, where headers are embedded in the Ingress object itself, seems much simpler to me.

For clarity, I think the downsides of the ConfigMap approach that I can think of are:

1. It requires new object
2. User have to remember to add relevant labels for security
3. Headers are not immediately visible in the Ingress definition
4. Some admission controller rules would be impossible to write. Gor example different headers rules depending on the domain in the Ingress

@pierluigilenoci are there any cases that would not be covered by the simple "string" approach?

[mblaschke]

The best approach would be if headers would be included in ingress manifest for example:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: minimal-ingress
  annotations:
    nginx.ingress.kubernetes.io/rewrite-target: /
spec:
  ingressClassName: nginx-example
  rules:
  - http:
      headers:
        x-foobar: barfoo
      paths:
      - path: /testpath
        pathType: Prefix
        backend:
          service:
            name: test
            port:
              number: 80

but that would mean a change in the ingress manifest

[sherifabdlnaby]

I dislike the configmap approach for setting headers. The Annotations have always been the way to pass Ingress-controller-specific customizations and based on that you'll find that most -if not all- Helm Charts support passing extra annotations.

The configmap approach means that if you're using a 3rd Party Helm Chart then you won't be able to customize it and pass custom headers unless you extend the chart, deploy your own separate resource, or if the chart itself supports creating an extra configmap.

IMO This will add friction to use the feature, something that we should avoid since the alternative is to enable configuration-snippet which is dangerous (#7811 (comment))

The Ingress API purpose was to make defining an Ingress as portable as possible, of-course it turns out to be limiting (hence Gateway API) but I think for the time being ingress-nginx should stick with defining customizations in Ingress Object itself and hence allow all already existing templates, charts, etc to use the feature.

[pierluigilenoci]

@jacekn I just explained that an approach with ConfigMaps was also possible.
There's no specific reason to prefer one method over the other.
It all depends on the goal you want to achieve.
The annotation approach is more straightforward; the one with the ConfigMap allows to reuse of the configurations in multiple ingresses and, therefore to reduce the duplication of the code.

In fact, two PRs are currently trying to solve the problem, one for each method.

• Annotation: Add annotation for custom response headers  #9779 by @satyamz
• ConfigMap: feat: add annotation to allow to add custom response headers #9742 by @cgroschupp

[pierluigilenoci]

@sherifabdlnaby many of the more common component charts within clusters almost always have the option of defining extra manifests. So I don't see this as a problem either.

For instance: Grafana, Prometheus, OAuth2 Proxy and so on and so fort.

[jacekn]

@jacekn I just explained that an approach with ConfigMaps was also possible.
There's no specific reason to prefer one method over the other.
It all depends on the goal you want to achieve.

The reason to use the annotation route that me, and a few others, gave is simplicity. The ConfigMap route is more complex to use and it depends on ConfigMaps having specific annotations or labels to allow them to be secured.
On top of that, in its current form, the ConfigMap PR does not actually enforce any label or annotations. This means that the admission controller rules you provided as examples wouldn't work. If that PR is merged it would be impossible for users to write validating hooks that can't be easily bypassed. I'll add a note about this to the PR, it's good info for reviewers to know about.

The annotation approach is more straightforward; the one with the ConfigMap allows to reuse of the configurations in multiple ingresses and, therefore to reduce the duplication of the code.

The ConfigMap approach could allow reuse but only in specific use cases. It would help for scenarios where multiple ingresses are defined within one namespace. However if multiple namespaces are used users would still need to duplicate ConfigMaps. I understand it could be a win in some cases but given the admission controller complexity and the fact that we need multiple objects I'd say cons outweigh pros

[jacekn]

@sherifabdlnaby many of the more common component charts within clusters almost always have the option of defining extra manifests. So I don't see this as a problem either.

The problem is that not all charts do this though. This means that some users will be caught in a situation where they can't add extra headers easily. It's hard to judge how many users will be affected but having a solution that does not require extra manifests will increases chances of success thus increasing usability and user experience.

[cgroschupp]

However if multiple namespaces are used users would still need to duplicate ConfigMaps.

@jacekn Why is this necessary? you can specify a configmap in another ns, with "namespace/configmap".

On top of that, in its current form, the ConfigMap PR does not actually enforce any label or annotations.

I can implement this, how should I name this label/annotation?

One way to make the configmap way safer is to implement a namespace whitelist from which you can fetch the cm. but that's just an idea.

[jacekn]

However if multiple namespaces are used users would still need to duplicate ConfigMaps.

@jacekn Why is this necessary? you can specify a configmap in another ns, with "namespace/configmap".

That assumes users have access to see ConfigMaps in other namespaces. This may not be the case in secure environments. Also if you use somebody else's ConfigMap don't you risk config changing from underneath your Ingress without you knowing?

On top of that, in its current form, the ConfigMap PR does not actually enforce any label or annotations.

I can implement this, how should I name this label/annotation?

Personally I think it would be best to agree on which approach is better and take it from there.

One way to make the configmap way safer is to implement a namespace whitelist from which you can fetch the cm. but that's just an idea.

The purpose of this feature is to allow users to add headers to their Ingresses and the ingresses are defined in the users' namespaces. I think realistically we'd have to allow them to use ConfigMaps in their own namespaces so the whitelist would need to contain all namespaces in many cases. If we require whitelist this would add to the complexity of the solution and add maintenance burden too.

Using simple string annotation removes those problems (no need for custom labels, no need for RBAC rules, no need for whitelists, no need to read up on special labels needed in the ConfigMap). It also improves security - it's super easy to validate headers and enforce rules. The only downside I can see is that in some cases there will be some duplication in annotations. However some duplication will actually still be there even with the ConfigMap approach - users will still need add annotation to specify which ConfigMap to use

[cgroschupp]

how do we get on here? in my use case the ConfigMap way fits better. but we need a decision.

[mc-zact]

Wonder if there was some final decision on this? not having this is a bit of a blocker for using ingress-nginx in prod because we have no easy way or secure to set Content-Security-Policy headers... with k8s microservices its not ideal to have that in the actual apps either

[jacekn]

We've been trying to clarify the direction in this PR but there isn't any progress so far. We're trying to organize video call or possibly start thread in slack with maintainers to explain what issues we forsee with the ConfigMap approach and justify UX advantages or a simple string but this hasn't happened yet.

[jacekn]

FYI we published image using code from this PR in dockerhub if anybody is interested in testing it:
stellar/ingress-nginx-controller:v1.8.0-722170c32fc

[mblaschke]

Maybe the better solution would be to introduce a CRD like Traefik is doing it instead of misusing ConfigMaps as they cannot be validated.

apiVersion: traefik.io/v1alpha1
kind: Middleware
metadata:
  name: test-header
spec:
  headers:
    customRequestHeaders:
      X-Script-Name: "test" # Adds
      X-Custom-Request-Header: "" # Removes
    customResponseHeaders:
      X-Custom-Response-Header: "" # Removes

see https://doc.traefik.io/traefik/middlewares/http/headers/

[sherifabdlnaby]

I again vote to start with an Annotation based approach first and consider adding an option to use ConfigMap or a CRD in the future. Annotations are the most "Accessible" way to enable this feature since most Helm Charts and Templating tools support adding extra annotations. This makes migration to the new annotation and away from Configuration-Snippet (which is dangerous) easy!

[mc-zact]

agree with @sherifabdlnaby, same vote here, also there is currently no way to do this which is also concerning for cases like "content-security-policy" headers etc, this can't be defined in any way currently and sure it could be handled in he applications, but in the world of k8s that's very impractical, currently I'm left with having and actually "proxy" nginx pod running to handle it.

[mblaschke]

this feature could also solve kubernetes/kubernetes#126816 for most users.

[mblaschke]

any progress here?

[pierluigilenoci]

@mblaschke there is a PR #9742 to solve this.

[JoniJnm]

Is there any way to do the same (use the annotation nginx.ingress.kubernetes.io/custom-headers) but for upstream? I mean, using proxy_set_header instead of more_set_headers?

Because custom-headers could be for response or for request

[pierluigilenoci]

@JoniJnm I think the only possibility is to open a new PR to introduce this feature.

[satyamz]

@JoniJnm Yes, that seems possible but project maintainers made it clear they won't accept any change having headers via annotations.