-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
URLs with special characters don't work in the presence of URL rewriting #5576
Comments
For the above example if ingress-nginx generated following Nginx configuration maybe it would fix the problem (based on the SO answer):
Currently ingress-nginx would generate:
It seems safe to do - but I'd have to experiment more with this. I think we have to be careful to not introduce an insecure default here. @wknapik you can experiment with this by modifying
|
|
@ElvinEfendi I need a quick fix for now, so modifying ingress-nginx is not really an option for me. This appears to work, though more testing is needed:
I'm not sure why it breaks without the This can be adapted for arbitrary rewrites (see: #5576 (comment)), not just adding a path prefix. |
Today this was reported https://trac.nginx.org/nginx/ticket/1808 |
@aledbf I responded in the nginx issue with the following:
The second example is what we're discussing here, the first one is an adaptation of the same approach for redirects. |
In theory, if ingress-nginx was to adopt this approach, it would be a backwards-incompatible change, because In most cases, the regexes would not require any changes, but if either the matching, or the replacement involved special characters, the end user would have to take that into account. This might be risky not only on the functional level, but also in terms of security. All this would require testing to confirm. |
Just in case, we are not going to introduce any breaking change to rewrites. Users still complain about the change introduced in 0.20.0. |
I don't think nginx offers any other way to deal with this at the moment. IMO this has to be considered a bug in ingress-nginx, even if the root cause is on the nginx side. There is nothing wrong in the original config from this issue and it still breaks on some characters in valid URLs. |
The best solution would be for nginx to encode paths before they're passed on - to upstream servers (via I can't figure out why anyone thought it would be a good idea to pass those paths in their decoded form. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
We're currently trying to get something like this to work using annotations. It seems like people think this isn't that big of a deal, but for us, we simply want a REST endpoint where we can put a URL as part of the path. e.g. - https://www.mysite.com/myapp/api/controller/https%3A%2F%2Fexample.com%2Fsites I was able to get this to work in my local dev, which doesn't use ingress-controller, by doing the following:
We can't seem to get this to work in our k8s environment, even with us trying to use some of these workarounds. This is a snippet from our values file for k8s:
I feel like we're missing something so simple... |
The scenario I'm looking to implement:
many thanks |
If you're using load balancing with sticky sessions (i.e. |
I ran:
It's a little messy as it's a conditional statement, I used this because I had multiple paths, with the same rule needing to be applied. Also it creates the same if statement in each rule which isn't ideal, but it works. It removes the rewrite so URLs don't get decoded upstream. I would like to see an annotation added as well, this fix isn't ideal and takes away from the cleanliness of yaml. |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
/remove-lifecycle stale |
We are facing the same issue. metadata:
name: admin-service
annotations:
kubernetes.io/ingress.class: main
nginx.ingress.kubernetes.io/proxy-read-timeout: '120'
nginx.ingress.kubernetes.io/rewrite-target: /$1
nginx.ingress.kubernetes.io/x-forwarded-prefix: /admin-service
spec:
tls:
- hosts:
- somehost.som
secretName: somecert
rules:
- host: somehost.com
http:
paths:
- path: /admin-service/(.*)
pathType: ImplementationSpecific
backend:
service:
name: admin-service
port:
name: http This will expose our service under If the url has some mandatory to be encoded chars like Is my understanding correct? |
/assign |
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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
The conditional statement works for me too, thanks! Unfortunately it does stop the CORS rules working though...any ideas on how to solve that? |
@kev24uk hey maybe you or someone can help me (busting my head on it for hours)? |
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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle rotten |
/remove-lifecycle rotten |
@damiancurry friendly ping. Are you still working on that one? |
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:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
/remove-lifecycle stale |
Just encountered this issue as (having a Tomcat in backend). Unfortunately, the suggested workarounds fail due to kubernetes/kubernetes#126811 as now proxy-pass is on the blacklist. Are there any other ideas to bypass this except for modifying backend to allow the characters? |
@brutus333 would you be able to share the ingress yaml for rabbitmq UI. I'm also facing the same issue with vhost "/" |
To be fair, all bug fixes are "breaking changes". Our teams are running into the same issue, so we're wondering how big of a security hole it is to allow config snippets with location and/or proxy_pass in them. |
This apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
annotations:
nginx.ingress.kubernetes.io/rewrite-target: /$2
nginx.ingress.kubernetes.io/configuration-snippet: |
rewrite ^ $request_uri;
reqrite ^([^?]*) $1;
spec:
rules:
- host: example.com
http:
paths:
- backend:
service:
name: example-service
port:
number: 8080
path: /example(/|$)(.*)
pathType: Prefix
|
@willemm it works but not as transparent as it expected - service receives encoded string, i.e. if path ends with It may be simplified with only
about (also there is a typo in your snippet reqrite -> rewrite) |
I just wanted to share my case and see if it's the same issue. I also have it in StackOverflow here. And here is my endpoint: It works fine with a path variable like But it returns 404 for a path variable like Would there be any workaround like playing with the configuration-snippet or something? |
@wknapik There is no engaging and update on this issue and several changes have been done to the controller, including validation and hardening of paths. I will close this for now due to inactivity, but please feel free to reopen after posting updates and check based on the recent release of ingrss-nginx controller. /close |
@longwuyuan: Closing this issue. In response to this:
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. |
@longwuyuan "validation and hardening of paths" doesn't sound like it would change this. I no longer use ingress-nginx, so won't test. |
I'm still facing this issue trying to use the rabbitMQ admin interface, I'm using the managed NGINX ingress with the application routing add-on for AKS, is running version 1.9.4 This is my ingress right now: apiVersion: networking.k8s.io/v1
kind: Ingress
metadata:
name: rabbitmq-hub-ingress
labels:
helm.sh/chart: rabbitmq-hub-0.1.0
annotations:
nginx.ingress.kubernetes.io/rewrite-target: /$2
spec:
ingressClassName: nginx-internal
tls:
- hosts:
- "mycompany.com"
secretName: keyvault-rabbitmq-hub-ingress
rules:
- http:
paths:
- path: /rabbitmq-hub-mgt(/|$)(.*)
pathType: Prefix
backend:
service:
name: rabbitmq-hub
port:
number: 15672
host: "mycompany.com" And I'm still facing the same issue: I tried using this workaround nginx.ingress.kubernetes.io/configuration-snippet: |
if ($request_uri ~ "^/example(/.*)") {
proxy_pass http://upstream_balancer$1;
break;
} and this one nginx.ingress.kubernetes.io/configuration-snippet: |
rewrite ^ $request_uri;
rewrite ^([^?]*) $1; without luck Any chance this can be fixed? |
@afrancoc2000 enable rewrite-logs annotation and maybe you will see that your request turns into https://mycompany.com/myqueue or something like that because you are rewriting to regex-group 2 after the root /. Regardless of that, the rewrite annotation works because I tested it and the special-characters are dealt with as per whatever the standards are. If RabbitMQ breaks because of this rule, then I would question why you are not dedicating a host like rabbitmq.mycomaony.com, with path as just / and pathType as Prefix. That is sort of reasonable for a full-blown vendor provided app like RabbitMQ which may or may not have its own rewrites and redirections. The original issue description is not relevant as per the post of the original creator of this issue. So kindly provide all the details like ;
|
@longwuyuan we can dedicate a subdomain like rabbitmq.mycomaony.com, but the redirection should work. it looks like when using %2F the rewrite stops working. Right now, we need these conversions: https://mycompany.com/rabbitmq-hub-mgt/#/queues -> /#/queues The first one works perfectly but the second one fails with a 404 probably the wrong rewrite. I don't see how to enable rewrite-logs using the azure add on, but I will check, what I can tell you now:
So, the problem comes in the rewrite with paths that include "%2F". I will switch to use a subdomain, but I think this issue should remain open, as the problem persists. |
|
Thank you these links help, I enabled the rewrite logs and this is an example of what I received:
It looks like it ignores and removes the %2F. changing Prefix to ImplementationSpecific didn't work either the problem persists. Here are the describe logs from the ingress:
This is the curl
I don't see the need for the service describe, hope this helps, I'm moving to the subdomain implementation then. Thanks |
NGINX Ingress controller version:
0.26.1
Kubernetes version (use
kubectl version
):Environment:
uname -a
): Linux redacted 4.9.0-11-amd64 Basic structure #1 SMP Debian 4.9.189-3+deb9u2 (2019-11-11) x86_64 GNU/Linux (via kops)What happened:
If urls are being rewritten for the given ingress, requests to urls with some special characters, such as
|
(%7c
), get decoded before being passed upstream, producing an invalid path.What you expected to happen:
I expected to be able to have any (correctly encoded) characters in urls.
How to reproduce it:
Accessing https://example.com/whatever%7Cwhatever produces a proxied request to the decoded url /site/whatever|whatever, which is invalid and ends in a 400 response.
It appears there are only two ways out of it now (and possibly, as a result of this bug report, a third one in the future):
proxy_pass
argument.I haven't tried the first solution, but I don't want to do that anyway - maintaining the custom config is not something I want to do.
I will be trying the second option now (any hints would be appreciated!), hopefully this will work, but it would still be better to do it statically/declaratively in the generated config, than running custom lua code per request. If this does turn out to be a fix, I would suggest including it in official documentation.
The third option would be an annotation like
or
That should achieve a rewrite without a
rewrite
and produce valid urls for the upstream service.Anything else we need to know:
I tried dropping the regex altogether (matching on path
/
) and rewriting to/site$request_uri?
, but then I end up with double encoding.That's fixable for the query part of the url with something like:
but that still leaves the path part double-encoded, so
%7c
becomes%257c
, which is not the url being requested.This is unexpected and quite difficult to sort out. Ideally, ingress-nginx would just handle it automagically. If that's not an option, being able to override the
proxy_pass
argument with an annotation would probably be sufficient to make this work.Relevant resources:
/kind bug
The text was updated successfully, but these errors were encountered: