kubernetes / ingress-nginx

Ingress NGINX Controller for Kubernetes
https://kubernetes.github.io/ingress-nginx/
Apache License 2.0
17.54k stars 8.26k forks source link

Regex "." does not match escaped newlines in URIs #11607

Open dkellner opened 4 months ago

dkellner commented 4 months ago

What happened:

The documented example for rewrite-target (https://kubernetes.github.io/ingress-nginx/examples/rewrite/#rewrite-target) truncates URIs containing correctly escaped newlines \n = %0A.

Requesting /something/foo%0Abar will result in the URI being rewritten to /foo. This is due to the generated nginx rewrite rule: rewrite "(?i)/something(/|$)(.*)" /$2 break;.

What you expected to happen:

I expected . to match all characters of a valid URI.

In other words, I've expected the rewrite rule to be: rewrite "(?is)/something(/|$)(.*)" /$2 break;. s enables . to match all characters including newlines.

NGINX Ingress controller version (exec into the pod and run nginx-ingress-controller --version.):

/etc/nginx $ /nginx-ingress-controller --version
-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.9.6
  Build:         6a73aa3b05040a97ef8213675a16142a9c95952a
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.21.6

-------------------------------------------------------------------------------

Kubernetes version (use kubectl version):

> kubectl version
Client Version: v1.29.4
Kustomize Version: v5.0.4-0.20230601165947-6ce0bf390ce3
Server Version: v1.29.4-gke.1043004

Environment:

I don't think our cluster configuration is relevant for this issue. I will provide more details if needed.

How to reproduce this issue:

Install minikube/kind

(I've only tried Kind.)

Install the ingress controller

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/deploy/static/provider/baremetal/deploy.yaml

Install an application that will act as default backend (is just an echo app)

kubectl apply -f https://raw.githubusercontent.com/kubernetes/ingress-nginx/main/docs/examples/http-svc.yaml

Create an ingress

From https://kubernetes.github.io/ingress-nginx/examples/rewrite/#rewrite-target :

$ echo '
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/use-regex: "true"
    nginx.ingress.kubernetes.io/rewrite-target: /$2
  name: rewrite
  namespace: default
spec:
  ingressClassName: nginx
  rules:
  - host: rewrite.bar.com
    http:
      paths:
      - path: /something(/|$)(.*)
        pathType: ImplementationSpecific
        backend:
          service:
            name: http-svc
            port:
              number: 80
' | kubectl create -f -

For comparison we can use this ingress without regex:

$ echo '
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  name: simple
  namespace: default
spec:
  ingressClassName: nginx
  rules:
  - host: simple.bar.com
    http:
      paths:
      - path: /
        pathType: ImplementationSpecific
        backend:
          service:
            name: http-svc
            port:
              number: 80
' | kubectl create -f -

Make a request

$ POD_NAME=$(kubectl get pods -n ingress-nginx -l app.kubernetes.io/name=ingress-nginx -l app.kubernetes.io/component=controller -o NAME)
$ kubectl exec -it -n ingress-nginx $POD_NAME -- curl -H 'Host: rewrite.bar.com' "localhost/something/foo%0Abar"

Hostname: http-svc-6578d78d59-ws49s

Pod Information:
    node name:  kind-control-plane
    pod name:   http-svc-6578d78d59-ws49s
    pod namespace:  default
    pod IP: 10.244.0.7

Server values:
    server_version=nginx: 1.14.2 - lua: 10015

Request Information:
    client_address=10.244.0.8
    method=GET
    real path=/foo
    query=
    request_version=1.1
    request_scheme=http
    request_uri=http://rewrite.bar.com:8080/foo

Request Headers:
    accept=*/*
    host=rewrite.bar.com
    user-agent=curl/8.8.0
    x-forwarded-for=::1
    x-forwarded-host=rewrite.bar.com
    x-forwarded-port=80
    x-forwarded-proto=http
    x-forwarded-scheme=http
    x-real-ip=::1
    x-request-id=9940b190f3d018691e099ff225ac6a07
    x-scheme=http

Request Body:
    -no body in request-

Sending a similar request to simple.bar.com lets the backend receive the complete URI:

$ POD_NAME=$(kubectl get pods -n ingress-nginx -l app.kubernetes.io/name=ingress-nginx -l app.kubernetes.io/component=controller -o NAME)
$ kubectl exec -it -n ingress-nginx $POD_NAME -- curl -H 'Host: simple.bar.com' "localhost/foo%0Abar"

Hostname: http-svc-6578d78d59-ws49s

Pod Information:
    node name:  kind-control-plane
    pod name:   http-svc-6578d78d59-ws49s
    pod namespace:  default
    pod IP: 10.244.0.7

Server values:
    server_version=nginx: 1.14.2 - lua: 10015

Request Information:
    client_address=10.244.0.8
    method=GET
    real path=/foo%0Abar
    query=
    request_version=1.1
    request_scheme=http
    request_uri=http://simple.bar.com:8080/foo%0Abar

Request Headers:
    accept=*/*
    host=simple.bar.com
    user-agent=curl/8.8.0
    x-forwarded-for=::1
    x-forwarded-host=simple.bar.com
    x-forwarded-port=80
    x-forwarded-proto=http
    x-forwarded-scheme=http
    x-real-ip=::1
    x-request-id=100965d8fcd21aa88fed5365f3d17582
    x-scheme=http

Request Body:
    -no body in request-

Anything else we need to know:

path: /something(/|$)([\s\S]*) works and does what I want.

k8s-ci-robot commented 4 months 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 4 months ago

/remove-kind bug /kind support

It may be more precise description if the label bug is added after a complete triage that shows the info leading to the bug.

I think much digging is needed but regex involves lot of complications and the project has started validating paths.

Can you please try v1.10.1 of the econtroller and update.

Also, please comment on the uncommon use of a newline char in the path of a requested url, with some more elaborate detail.

dkellner commented 4 months ago

My local test with Kind already had v1.11.0 installed. The issue is reproducible with this version as well.

/etc/nginx $ /nginx-ingress-controller --version
-------------------------------------------------------------------------------
NGINX Ingress controller
  Release:       v1.11.0
  Build:         96dea883d6ee3c6261b722896e8638758b7cc4cb
  Repository:    https://github.com/kubernetes/ingress-nginx
  nginx version: nginx/1.25.5

-------------------------------------------------------------------------------

I agree newlines in URLs are uncommon and we don't use them purposely. They occured when clients tried to make our systems misbehave.

For example, consider an internal backend service (behind ingress-nginx with regex paths) with an endpoint https://things.example.com/things/<thing-id> and a public-facing site https://example.com/things/<thing-id> issuing GET requests to the backend service. You'd expect a request to https://things.example.com/things/%0A to yield a HTTP 404 as there's no thing with ID \n. Instead you'll get the response for GET /things/ due to the rewrite, which might be a listing of many things. This probably causes an HTTP 500 in your frontend as the response schema is different.

One can imagine scenarios with security implications, as you can use injecting %0A to effectively cut off segments or query parameters that are appended in application code.

Note that this is not an issue due to lack of escaping user input. You're correctly escaping the ID received from the client during construction of your backend URL. You'd need internal knowledge of the rewrite rules to know that you have to watch out for newlines.

longwuyuan commented 4 months ago

Can you test any URL that has percentage char in URL but not newline

dkellner commented 4 months ago

Sure! URL-encoded % is %25, which is matched by . in the regex rule and forwarded to the backend.

GET /foo%25bar:

$ kubectl exec -it -n ingress-nginx $POD_NAME -- curl -H 'Host: rewrite.bar.com' "localhost/something/foo%25bar"

Hostname: http-svc-6578d78d59-z9jlg

[...]

Request Information:
    client_address=10.244.0.7
    method=GET
    real path=/foo%25bar
    query=
    request_version=1.1
    request_scheme=http
    request_uri=http://rewrite.bar.com:8080/foo%25bar

[...]

If there's a % in the URL that's not part of a valid URL-encoded character, ingress-nginx will return an HTTP 400.

GET /foo%zz:

$ kubectl exec -it -n ingress-nginx $POD_NAME -- curl -H 'Host: rewrite.bar.com' "localhost/something/foo%zz"
<html>
<head><title>400 Bad Request</title></head>
<body>
<center><h1>400 Bad Request</h1></center>
<hr><center>nginx</center>
</body>
</html>
longwuyuan commented 4 months ago

Do I understand correctly that you see percent/% character in path works if there is no rewrite annotation used and percent/% character breaks the functionality if rewrite annotation is used ?

dkellner commented 4 months ago

Yes, but this only applies to newlines (%0A). All other percent-escaped characters work in both configurations. The reason is that . in an nginx regex does not match newlines by default.

I found a similar ticket for nginx itself: https://trac.nginx.org/nginx/ticket/2452 . The suggested solution there is to "[enable] single line mode explicitly with (?s) in the regular expression if needed".

longwuyuan commented 4 months ago

If its only a problem with newline character, then I think it will not be fixed. Please wait for comments from others on this.

There is acute shortage of resources and there is a clear direction the project is heading (fundamental need to implement what is called "Kubernetes Gatway API") and other very basic ground realities.

So please see if others will comment and if it will help but the project is not likely to to change code to allow newlines bot for security reasons and as well for general direction the project is heading in.

github-actions[bot] commented 3 months ago

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.