Nginx panics when `cors-allow-origin` contains invalid input #8168

L1ghtman2k · 2022-01-19T23:20:24Z

NGINX Ingress controller version : v1.1.0

Kubernetes version (use kubectl version):

Client Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.2", GitCommit:"092fbfbf53427de67cac1e9fa54aaa09a28371d7", GitTreeState:"clean", BuildDate:"2021-06-16T12:59:11Z", GoVersion:"go1.16.5", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"20+", GitVersion:"v1.20.7-eks-d88609", GitCommit:"d886092805d5cc3a47ed5cf0c43de38ce442dfcb", GitTreeState:"clean", BuildDate:"2021-07-31T00:29:12Z", GoVersion:"go1.15.12", Compiler:"gc", Platform:"linux/amd64"}

Environment: EKS

Cloud provider or hardware configuration: AWS
OS (e.g. from /etc/os-release): Amazon Linux 2
How was the ingress-nginx-controller installed: https://raw.githubusercontent.com/kubernetes/ingress-nginx/controller-v1.1.0/deploy/static/provider/cloud/deploy.yaml
Current State of the controller:

Name:         nginx
Labels:       app.kubernetes.io/name=ingress-nginx
              app.kubernetes.io/part-of=ingress-nginx
              cluster=eksctlci
Annotations:  ingressclass.kubernetes.io/is-default-class: true
Controller:   k8s.io/ingress-nginx
Events:       <none>


Name:         nlb-switch-nginx
Labels:       app.kubernetes.io/name=ingress-nlb-switch-nginx
              app.kubernetes.io/part-of=ingress-nginx
              cluster=eksctlci
Annotations:  ingressclass.kubernetes.io/is-default-class: false
Controller:   k8s.io/ingress-nlb-switch-nginx
Events:       <none>

What happened:

Applying the Ingress to Nginx v1.1.0 that has invalid cors-allow-origin will cause the Nginx validation webhook to panic(the panic during validation webhook is recovered, so the actual Nginx instance is still operational):

This is what the client sees when trying to create an Ingress with invalid cors-allow-origin

Error from server (InternalError): error when creating "/home/user/nginx-test.yaml": Internal error occurred: failed calling webhook "validate.nginx.ingress.kubernetes.io": Post "https://ingress-nginx.svc:443/networking/v1/ingresses?timeout=10s": EOF

Logs on the Nginx controller yield the following:

E0119 21:03:47.086234       6 main.go:134] Error parsing cors-allow-origin parameters. Supplied incorrect origin: example1.com. Skipping.
2022/01/19 21:03:47 http: panic serving 192.168.142.65:54954: runtime error: slice bounds out of range [3:1]
goroutine 191768 [running]:
net/http.(*conn).serve.func1()
        net/http/server.go:1801 +0xb9
panic({0x18588a0, 0xc001c24de0})
        runtime/panic.go:1047 +0x266
k8s.io/ingress-nginx/internal/ingress/annotations/cors.cors.Parse({{0xc0001f6958, 0xc00101fa10}}, 0xc0001f6950)
        k8s.io/ingress-nginx/internal/ingress/annotations/cors/main.go:135 +0x78d
k8s.io/ingress-nginx/internal/ingress/annotations.Extractor.Extract({0x1bbbc58}, 0xc00033e300)
        k8s.io/ingress-nginx/internal/ingress/annotations/annotations.go:180 +0x147
k8s.io/ingress-nginx/internal/ingress/controller.(*NGINXController).CheckIngress(0xc00040e4b0, 0xc00033e300)
        k8s.io/ingress-nginx/internal/ingress/controller/controller.go:288 +0x905
k8s.io/ingress-nginx/internal/admission/controller.(*IngressAdmission).HandleAdmission(0xc0000bf9f0, {0x1b87c20, 0xc00101e870})
        k8s.io/ingress-nginx/internal/admission/controller/main.go:89 +0x3e2
k8s.io/ingress-nginx/internal/admission/controller.(*AdmissionControllerServer).ServeHTTP(0xc0000bfa00, {0x1b99b50, 0xc00110b6c0}, 0x10)
        k8s.io/ingress-nginx/internal/admission/controller/server.go:80 +0x214
net/http.serverHandler.ServeHTTP({0x1b8cea0}, {0x1b99b50, 0xc00110b6c0}, 0xc00117ea00)
        net/http/server.go:2878 +0x43b
net/http.(*conn).serve(0xc00060caa0, {0x1b9ef88, 0xc000c8b410})
        net/http/server.go:1929 +0xb08
created by net/http.(*Server).Serve
        net/http/server.go:3033 +0x4e8

What you expected to happen:

I expected nginx validation webhook to return back an error saying that provided cors-allow-origin contains illegal words. Perhaps provide that format that cors-allow-origin must follow: http(s)://example1.com,http(s)://example2.com

How to reproduce it:

Apply the following Ingress:

apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/cors-allow-headers: Keep-Alive,User-Agent
    nginx.ingress.kubernetes.io/cors-allow-origin: example1.com,example2.com,example2.com
    nginx.ingress.kubernetes.io/enable-cors: "true"
  name: nginx-err
  namespace: default
spec:
  rules:
  - http:
      paths:
      - path: /testpath
        pathType: Prefix
        backend:
          service:
            name: test
            port:
              number: 80

Anything else we need to know:

If someone is upgrading from versions v0 to >=v1.0.5, and they already have an invalid Ingress object in the environment prior to the upgrade, the upgrade will render nginx unusable since:

The newly rolling v1 Nginx instances are in a constant Crash Loop (since panic isn't recovered in this case)
The older v0 instances that are waiting for eviction(that will never happen because v1 is in Crash Loop) will not be able to elect the leader since the service account in v1 uses a different configmap name for leader election. So, v0 instances get permission denied when trying to access configmap that followed v0 naming convention

The text was updated successfully, but these errors were encountered:

longwuyuan · 2022-01-20T03:05:38Z

@L1ghtman2k , thanks for reporting this.
(1) Are you able to reproduce this with the latest release of the controller.
(2) Can you please fill up the issue description with more details like the request you sent to the cluster etc

@theunrealgeek , I thought we blocked bad stuff only. Wonder if you also can reproduce this panic

/triage accepted

longwuyuan · 2022-01-20T03:06:24Z

/assign theunrealgeek

k8s-ci-robot · 2022-01-20T03:06:25Z

@longwuyuan: GitHub didn't allow me to assign the following users: theunrealgeek.

Note that only kubernetes members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign theunrealgeek

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.

L1ghtman2k · 2022-01-20T03:54:59Z

@L1ghtman2k , thanks for reporting this. (1) Are you able to reproduce this with the latest release of the controller. (2) Can you please fill up the issue description with more details like the request you sent to the cluster etc

@theunrealgeek , I thought we blocked bad stuff only. Wonder if you also can reproduce this panic

/triage accepted

Hello, Yes, I am able to reproduce the issue on v1.1.1.

You can test it in a small local kind cluster too:

kind create cluster --image kindest/node:v1.23.0

Apply nginx helm-chart:

helm upgrade --install ingress-nginx ingress-nginx \
  --repo https://kubernetes.github.io/ingress-nginx \
  --namespace ingress-nginx --create-namespace

Wait for the ingress controller to become available, and then:

cat <<EOF | kubectl create -f -
apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
  annotations:
    nginx.ingress.kubernetes.io/cors-allow-headers: Keep-Alive,User-Agent
    nginx.ingress.kubernetes.io/cors-allow-origin: example1.com,example2.com,example2.com
    nginx.ingress.kubernetes.io/enable-cors: "true"
  name: nginx-err
  namespace: default
spec:
  rules:
  - http:
      paths:
      - path: /testpath
        pathType: Prefix
        backend:
          service:
            name: test
            port:
              number: 80
EOF

longwuyuan · 2022-01-20T05:32:47Z

ok. I saw the error in the logs and I also see that specificing the correct format created the ingress

But the controller is not dead after that error. So it seems we should handle that invalid input better. At least just log one more line of a friendlier message to user as root cause is just missing format (required protocol like http or https there)

L1ghtman2k · 2022-01-20T05:49:20Z

@longwuyuan correct, however, if someone has that invalid object in the environment, prior to upgrading, and then migrates to v1 nginx from v0, that will crash the controller, since (my guess) panics are not recovered outside of the validation step.

In any case, like you mentioned, handling invalid input should fix the above too

longwuyuan · 2022-01-20T06:17:11Z

I'll wait for @theunrealgeek or others to comment as I am not a developer. But I saw just now that if the value for that annotation is invalid, then the ingress object can not even be created. The admission webhook is catching that already or so it seems. Thanks, ; Long Wu Yuan

…

On 1/20/22 11:19 AM, Aibek wrote: @longwuyuan <https://github.com/longwuyuan> correct, however, note that |Anything else we need to know| section in the first message. If someone has that invalid object in the environment, prior to upgrading, and then migrates to v1 nginx from v0, that will crash the controller, since my guess is that panics are not recovered outside of validation step — Reply to this email directly, view it on GitHub <#8168 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABGZVWQ6SEXMADDYPCMP5MLUW6O63ANCNFSM5MLH5SAQ>. Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

larivierec · 2022-01-24T23:28:38Z

Hey there, so having a quick look this is an issue directly during parsing of annotations.
https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/cors/main.go#L135

It happens when everything in the slice is invalid. If just one turns out to be ~~invalid~~ valid then it works fine.
Probably find a better way to do this.

longwuyuan · 2022-01-25T03:24:26Z

@larivierec I did not test yet but did you mean , the below works without panic ;

nginx.ingress.kubernetes.io/cors-allow-origin: http://example1.com,http://example2.com,example2.com

I have to check but there is no protocol in the last FQDN in thsi example and secondly the last FQDN is actually a repeated one.

So what invalidates and causes the panic. The missing protocol or the repeat

L1ghtman2k · 2022-01-25T06:34:12Z

@longwuyuan, I think @larivierec meant: If one turns out to be ~~invalid~~ valid then it works fine.
Then to answer your question:
If protocol is missing in ALL of the FQDNs(no matter what FQDNs are), then it will cause panic.

And from your example above:
nginx.ingress.kubernetes.io/cors-allow-origin: http://example1.com,http://example2.com,example2.com
It should not cause panic since not all of the elements are invalid

longwuyuan · 2022-01-25T06:45:56Z

Thanks @L1ghtman2k . I am not a developer and these are times I wish I was.
So this still comes down to checking & better handling of the value.
Have to wait for developer time on this, unless someone here wants to send a PR to do at least a validity check. And now that you think of it, a test is also needed in the CI.

larivierec · 2022-01-25T12:53:08Z

@longwuyuan, I think @larivierec meant: If one turns out to be ~~invalid~~ valid then it works fine.
The to answer your question:
If protocol is missing in ALL of the FQDNs(no matter what FQDNs are), then it will cause panic.

And from your example above:
nginx.ingress.kubernetes.io/cors-allow-origin: http://example1.com,http://example2.com,example2.com
It should not cause panic since not all of the elements are invalid

Yes exactly, my fault. Mobile keyboards cause more confusion sometimes then help. 😅

Thanks for the clarification

iamNoah1 · 2022-01-27T08:52:22Z

/priority important-soon

…8185) * fixes #8168 by appending elements on match, instead of removing * refactor the corsOriginRegex comparison, and initialize CorsAllowOrigin

…ubernetes#8185) * fixes kubernetes#8168 by appending elements on match, instead of removing * refactor the corsOriginRegex comparison, and initialize CorsAllowOrigin

L1ghtman2k added the kind/bug Categorizes issue or PR as related to a bug. label Jan 19, 2022

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority labels Jan 19, 2022

L1ghtman2k changed the title ~~Nginx panics when cors-allow-origin contains invalid~~ Nginx panics when cors-allow-origin contains invalid input Jan 19, 2022

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 20, 2022

L1ghtman2k mentioned this issue Jan 25, 2022

Append elements on match, instead of removing for cors-annotations #8185

Merged

9 tasks

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority labels Jan 27, 2022

k8s-ci-robot closed this as completed in #8185 Feb 13, 2022

SeanBenoit mentioned this issue Jun 29, 2022

Support cors-allow-origin with multiple origins #7614

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nginx panics when `cors-allow-origin` contains invalid input #8168

Nginx panics when `cors-allow-origin` contains invalid input #8168

L1ghtman2k commented Jan 19, 2022 •

edited

Loading

longwuyuan commented Jan 20, 2022

longwuyuan commented Jan 20, 2022

k8s-ci-robot commented Jan 20, 2022

L1ghtman2k commented Jan 20, 2022 •

edited

Loading

longwuyuan commented Jan 20, 2022 •

edited

Loading

L1ghtman2k commented Jan 20, 2022 •

edited

Loading

longwuyuan commented Jan 20, 2022 via email

larivierec commented Jan 24, 2022 •

edited

Loading

longwuyuan commented Jan 25, 2022

L1ghtman2k commented Jan 25, 2022 •

edited

Loading

longwuyuan commented Jan 25, 2022

larivierec commented Jan 25, 2022 •

edited

Loading

iamNoah1 commented Jan 27, 2022

Nginx panics when cors-allow-origin contains invalid input #8168

Nginx panics when cors-allow-origin contains invalid input #8168

Comments

L1ghtman2k commented Jan 19, 2022 • edited Loading

longwuyuan commented Jan 20, 2022

longwuyuan commented Jan 20, 2022

k8s-ci-robot commented Jan 20, 2022

L1ghtman2k commented Jan 20, 2022 • edited Loading

longwuyuan commented Jan 20, 2022 • edited Loading

L1ghtman2k commented Jan 20, 2022 • edited Loading

longwuyuan commented Jan 20, 2022 via email

larivierec commented Jan 24, 2022 • edited Loading

longwuyuan commented Jan 25, 2022

L1ghtman2k commented Jan 25, 2022 • edited Loading

longwuyuan commented Jan 25, 2022

larivierec commented Jan 25, 2022 • edited Loading

iamNoah1 commented Jan 27, 2022

Nginx panics when `cors-allow-origin` contains invalid input #8168

Nginx panics when `cors-allow-origin` contains invalid input #8168

L1ghtman2k commented Jan 19, 2022 •

edited

Loading

L1ghtman2k commented Jan 20, 2022 •

edited

Loading

longwuyuan commented Jan 20, 2022 •

edited

Loading

L1ghtman2k commented Jan 20, 2022 •

edited

Loading

larivierec commented Jan 24, 2022 •

edited

Loading

L1ghtman2k commented Jan 25, 2022 •

edited

Loading

larivierec commented Jan 25, 2022 •

edited

Loading