Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regex in proxy-redirect-from and proxy-redirect-to #10698

Open
suvl opened this issue Nov 29, 2023 · 11 comments
Open

Regex in proxy-redirect-from and proxy-redirect-to #10698

suvl opened this issue Nov 29, 2023 · 11 comments
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. triage/needs-information Indicates an issue needs more information in order to work on it.

Comments

@suvl
Copy link

suvl commented Nov 29, 2023

I'm using a complex proxy_redirect instruction directly in nginx and it works, it changes the Location header so that clients are redirected to an ingress for direct connection to a specific statefulset instance. The instruction is this:

proxy_redirect ~^.*\/\/pod-(?<pod_nr>\d+?)(\..*\.svc\.cluster\.local)?(:\d*)?(?<pod_path>\/.*)?$ https://p-$pod_nr.project.example.com$pod_path;

This worked for years but we're moving to using ingress-nginx instead of managing nginx configs. I'm now trying to create the same thing using annotations, so tried the following:

metadata:
  annotations:
    nginx.ingress.kubernetes.io/proxy-redirect-from: '~*^.*\/\/pod-(?<pod_nr>\d+?)(\..*\.svc\.cluster\.local)?(:\d*)?(?<pod_path>\/.*)?$'
    nginx.ingress.kubernetes.io/proxy-redirect-to: "https://p-$pod_nr.project.example.com$pod_path"

However I get these two errors in the logs:

ingress-nginx-controller-59d9446fd6-fv265 controller W1129 16:43:09.772362       7 validators.go:221] validation error on ingress ns/pod-ingress: annotation proxy-redirect-from contains invalid value ~*^.*\/\/pod-(?<pod_nr>\d+?)(\..*\.svc\.cluster\.local)?(:\d*)?(?<pod_path>\/.*)?$
ingress-nginx-controller-59d9446fd6-fv265 controller W1129 16:43:09.773233       7 validators.go:221] validation error on ingress ns/pod-ingress: annotation proxy-redirect-to contains invalid value https://p-$pod_nr.project.example.com$pod_path

Now, I need guidance on why this is being refused while working perfectly in nginx and how to fix it. Tried to use a nginx.ingress.kubernetes.io/configuration-snippet directly but then I got:

nginx: [emerg] "proxy_redirect" directive is duplicate in /tmp/nginx/nginx-cfg3481804691:726

as the ingress controller always inserts a proxy_redirect off; instruction and that is not possible to disable.

Without editing the ingress template directly, how can I implement this?

@suvl suvl added the kind/bug Categorizes issue or PR as related to a bug. label Nov 29, 2023
@k8s-ci-robot k8s-ci-robot added the needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. label Nov 29, 2023
@k8s-ci-robot
Copy link
Contributor

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.

@suvl
Copy link
Author

suvl commented Nov 30, 2023

I can see from https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/parser/validators.go#L62 and https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/parser/validators.go#L44 , https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/parser/validators.go#L47 that

only -._~a-zA-Z0-9/:?&= chars are allowed in this snippet. Why is that? The nginx configuration has no such restriction.

The nginx documentation for the ngx_http_proxy_module states in here that

The directive can be specified (1.1.11) using regular expressions. In this case, redirect should either start with the “~” symbol for a case-sensitive matching, or with the “~*” symbols for case-insensitive matching. The regular expression can contain named and positional captures, and replacement can reference them

So, is there a reason we're expecting a URL in here? Can we also accept a full regex with capture groups?

@longwuyuan
Copy link
Contributor

@longwuyuan
Copy link
Contributor

/triage needs-information

@k8s-ci-robot k8s-ci-robot added the triage/needs-information Indicates an issue needs more information in order to work on it. label Dec 1, 2023
@longwuyuan
Copy link
Contributor

/remove-kind bug

@k8s-ci-robot k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. and removed kind/bug Categorizes issue or PR as related to a bug. labels Dec 1, 2023
@suvl
Copy link
Author

suvl commented Dec 1, 2023

@longwuyuan Sorry, not sure what you meant. If I understood correctly, you want me to close this issue and open a new one using a template?

Also, do not agree that this is not a bug. I still believe it is a bug, as the nginx configuration allows for something that is not being allowed via these annotations.

The "allow snippet annotations" is not helpful, as one cannot disable via configuration that a proxy_redirect off; instruction always gets printed for every location, so inserting a configuration snippet will not work for this purpose.

@opencmit2
Copy link

opencmit2 commented Dec 7, 2023

Hi @suvl
I can't reproduce the problem you stated using your method.

  annotations:
    nginx.ingress.kubernetes.io/proxy-redirect-from: '~*^.*\/\/pod-(?<pod_nr>\d+?)(\..*\.svc\.cluster\.local
)?(:\d*)?(?<pod_path>\/.*)?$'
    nginx.ingress.kubernetes.io/proxy-redirect-to: "https://p-$pod_nr.project.example.com$pod_path"

nginx.config
image

image version: ingress-nginx/controller:v1.6.4

@suvl
Copy link
Author

suvl commented Dec 7, 2023

Hey there @opencmit2, do you have

controller:
  extraArgs:
    enable-annotation-validation: "true"
  config:
    strict-validate-path-type: "true"

in your install values? As required to fix the latest CVEs, right?

@suvl
Copy link
Author

suvl commented Apr 29, 2024

@opencmit2 just tested it with version 4.8.3, this is what I get in the logs:

public-nginx-ingress-nginx-controller-799f9bcd97-8695t controller W0429 16:08:10.821757       6 validators.go:221] validation error on ingress webpa-staging/petasos-external: annotation proxy-redirect-from contains invalid value ~.*talaria-(?<talaria_host>\d*)(\.talaria.webpa-staging)?(\:\d+)?(?<talaria_path>\/.*)?$
public-nginx-ingress-nginx-controller-799f9bcd97-8695t controller W0429 16:08:10.821790       6 validators.go:221] validation error on ingress webpa-staging/petasos-external: annotation proxy-redirect-to contains invalid value https://talaria-$talaria_host.webpa-staging.oizys.clg.nos.pt$talaria_path

Only when I set enable-annotation-validation: "false" does this work.

So I call this a bug. The annotation has a valid value, the regex compiles and is somewhat sane. I cannot feel safe setting this to false in this, multi-tenant, cluster, so I really think this validation should be fixed.

edit: latest 4.10.1 version also has this problem.

@suvl
Copy link
Author

suvl commented Apr 29, 2024

Just checked https://github.com/kubernetes/ingress-nginx/blob/main/internal/ingress/annotations/parser/validators.go#L64

It now accepts a regex like

^[\-\.\_\~a-zA-Z0-9\/:?&=]*$

For this to work none of the regexes in the file would do.

The closest is the one from IsValidRegex but not quite close. For instance, it fails to have the ~ character for specifying case sensitive or insensitive matching. But we could maybe do without it, probably. For named capture groups (that are reused later in proxy-redirect-to) it is still missing the <> characters, though.

So the working regex would be:

^[\-\.\_\~a-zA-Z0-9\/:^$\[\]\(\)\{\}*+?|&=\\<>]+$

@longwuyuan
Copy link
Contributor

cc @rikatz @tao12345666333 corner-case combo of regex in proxy-redirect-from and proxy-redirect-ro annotations

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. triage/needs-information Indicates an issue needs more information in order to work on it.
Projects
Development

No branches or pull requests

4 participants