Content-Length header set to empty string in auth subrequests causes compatibility issues

Schmitze333 commented 5 days ago

What happened:

When using ingress-nginx for handling auth subrequests, the controller sets the Content-Length header to an empty string ("") for these subrequests. This leads to problems when other downstream proxies are involved.

We run ingress-nginx in a cluster with cilium as CNI and kube-proxy-replacement enabled. The downstream proxy sanitizes (drops) the ‘Content-Length’ header, since it enforces strict compliance with the latest HTTP specification. This in turn leads to problems in the authentication service itself.

What you expected to happen:

For an auth subrequest the Content-Length header should either be omitted or explicitly set to 0 in compliance with RFC 9110 (HTTP/1.1).

However, the problem can easily be mitigated by annotating the ingress definition with:

    nginx.ingress.kubernetes.io/auth-snippet: proxy_set_header Content-Length 0;

But IMHO making the header RFC-compliant is a cleaner solution

How to reproduce this issue:

Deploy ingress-nginx.

Create an ingress definition with auth subrequest:

---
ingress:
ingressDefinitions:
example:
  name: example
  annotations:
    nginx.ingress.kubernetes.io/auth-method: POST
    nginx.ingress.kubernetes.io/auth-url: http://auth.default.svc.cluster.local/validate/session
      ssi_silent_errors on;
  ingressClassName: public
  tls:
    - secretName: example
      hosts:
        - test.example.local
  rules:
    - host: test.example.local
      http:
        paths:
          - path: /
            pathType: Prefix
            backend:
              service:
                name: some-service
                port:
                  name: http

Inspect the auth subrequests sent by ingress-nginx and observe that the Content-Length header is set to "".

Anything else we need to know:

I think if desired, this is rather an easy fix in the nginx template here. There’s another spot in the same template file where the ‘Content-Type’ header is correctly set to 0.

I’d happily create a PR for this if wanted.

KR

k8s-ci-robot commented 5 days ago

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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.

longwuyuan commented 5 days ago

We only test on a kind cluster so implies what is called as "kindnet".

We need to be certain of what the implications of your PR will be.

Such contributions are most welcome as it enhances user experiences.

But the very first thought that comes to me is will you implement the setting of this header as a optional feature, or will you force set it for non-cilium users as well.

/remove-kind bug /kind feature

Feature because it is not implied by the K8S Ingress-API KEP

Schmitze333 commented 5 days ago

The header is already set in the current implementation, but not RFC-compliant. I'd change the header in the template from Content-Length: "" to Content-Length: 0 for also for non-cilium users so that it's consistent across the template and RFC-compliant.

In order to stay non-breaking, it could also be put behind a setting, although I think this would be overly complex for such a small change. But in the end, this is the decision of the ingress-nginx team.

longwuyuan commented 5 days ago

Thanks for repeat stating the RFC compliance aspect.

If you sort of abundantly make it clear, with explicit description of the PR, that the PR brings improvement as a RFC compliance, I think it will help a lot.

Thanks for the proposed contribution.

strongjz commented 5 days ago

@Schmitze333 This seems like a relatively straightforward change to the template.

@rikatz I wonder why we were allowed to use " and not 0? would something like this be caught with a cross-plane?

Schmitze333 commented 4 days ago

I did a more carefully inspection of the situation and all involved parts.

Here are some findings:

The template follows the auth-request description from the official nginx docs which also sets the Content-Length header to "".
Already nginx kicks out the RFC-incompliant header. So, despite the proxy_set_header Content-Length "" directive in the template, the Content-Length header is not sent with the request.
Hence, ingress-nginx behaves RFC-compliant (see here), since sending a Content-Length header in requests without content is optional.

Given that a user must set the HTTP method for the auth sub-request, it totally makes sense that the Content-Length is not sent per default, since it would not make any sense for GET requests. If I make the auth-request a POST request, it's up to me to take care of the Content-Length header.

I apologise for wrongly blaming ingress-nginx and opening an issue without having investigated the situation sufficiently. I'm sorry for creating noise here.

Thank you for being so responsive!

kubernetes / ingress-nginx