-
Notifications
You must be signed in to change notification settings - Fork 617
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
Redirect http to https on the same destination #448
Comments
something like ...
|
when doing http -> https the match host needs to have the port or else you will get a redirect for sure |
@leprechau Hi. thanks for the suggestion, unfortunately i get a redirect loop using that example |
That's odd ... that's literally the exact copy/paste from our production config with the hostname changed. |
@leprechau In fabio UI:
And a curl:
If i remove the redirect accessing both http and https://some.domain.example.com works fine. |
So, I'm assuming that Secondly have you tried removing the trailing slash from the route match? Meaning |
@leprechau yes that is correct, silly me missed the curl :) |
Can you post a full log of ...
|
|
So it looks like you switch to a different host for the port 443 request than the port 80 request. Is it possible there is a difference in configuration between those two hosts? |
They are running behind an ALB in different docker-containers, 2 with go.staging.budbee.com and 2 with go.staging.budbee.com/api and the configuration is identical. Only different is the random listening port they get allocated. |
Something really strange is happening ... it's almost like there are two redirect rules defined. The first request clearly matches |
@atillamas Can you post your full routing table with |
@magiconair Sure thing, i created a completely new stack, exact copy of our staging, just running one single job, exactly the same behaviour
|
Hrm.... I wonder ... what if you try something like ...
|
So in the service definition ...
|
|
Okay, I'm stumped. The above config looks pretty simple to reproduce I'll see if I can make this happen locally. |
One last thought ... you have the AWS load balancers in-front of fabio in this case, right? I don't have much experience with AWS ... is it possible this is a case where the two load balancers are fighting? Is the AWS load balancer terminating SSL and then forwarding as plain HTTP back to fabio? Do you have a way to test direct to the fabio instance without going through the AWS load balancer? |
Yes the ALB (Application Load Balancer) is in-front of Fabio and terminating the SSL. I'm starting to believe that this is the issue also, i bet a normal Network Loadbalancer would work, i choosed the ALB to quickly get a way to be able to control access to our "internal" public exposed services with WAF, I now see that Fabio has support for IP-restriction so i might try to migrate it over to a Network Load Balancer instead. Haven't been able to test it without going through the loadbalancer in an easy way. If I run a curl with host header behind the loadbalancer the first request goes directly to Fabio, but then the next request go through the LB and into the redirect loop... |
Looks related to #450 ... is this the same issue? |
@leprechau it appears so. tcpdump of the headers
|
This code fixes the redirect problem for me but introduces the problem that i don't get any content at all :)
|
@atillamas Yes, I think this is the same problem being described in #450 for sure and you're already on the same track. I was just looking at that same spot in the code but I think the logic needs to go elsewhere. Basically we need it to not even mark the target as a redirect which would happen much earlier. |
@atillamas maybe something like this in the // find matching hosts for the request
// and add "no host" as the fallback option
hosts := t.matchingHosts(req)
hosts = append(hosts, "")
for _, h := range hosts {
if target = t.lookup(h, path, trace, pick, match); target != nil {
if target.RedirectCode != 0 {
redirectURL := target.GetRedirectURL(requestURL)
if redirectURL.Scheme == "https" && req.Header.Get("X-Forwarded-Port") != "443" {
continue
}
}
break
}
} Completely untested but I think we need to prevent the redirect route from being returned by the lookup function based on your condition. |
Maybe try something like that and post some logs with tracing enabled? |
@leprechau Thanks for the suggestion, Unfortunately I get the redirect loop when implementing the code there, I'll see if i can find some time during the weekend to troubleshoot some more. |
@atillamas gotcha ... sorry about that. I was just trying to think of how to prevent the redirect from occurring and get the "correct" target returned. Maybe it could be as simple as still returning the target but setting the redirect code to |
@leprechau No worries at all, I'm grateful for all your help! |
@atillamas understood. I'll try and take a stab at a few options using your example route table above. |
Okay, so I think I have working code but I'm not sure it's the best way as it assumes that an upstream https source is always going to be The patch ... diff --git a/route/table.go b/route/table.go
index 8e74bde..898ff1a 100644
--- a/route/table.go
+++ b/route/table.go
@@ -326,6 +326,10 @@ func (t Table) Lookup(req *http.Request, trace string, pick picker, match matche
hosts = append(hosts, "")
for _, h := range hosts {
if target = t.lookup(h, path, trace, pick, match); target != nil {
+ if target.RedirectCode != 0 && target.URL.Scheme == "https" && req.Header.Get("X-Forwarded-Port") == "443" {
+ log.Print("[DEBUG] Ignoring https redirect from https upstream")
+ continue
+ }
break
}
} The test table:
Running fabio as:
XFP 80 request:
XFP 443 request:
|
From the caps you posted above it looks like AWS is also sending |
Just confirmed this diff also works ... new tests and config below ... The patch: diff --git a/route/table.go b/route/table.go
index 8e74bde..0ac8dd2 100644
--- a/route/table.go
+++ b/route/table.go
@@ -326,6 +326,10 @@ func (t Table) Lookup(req *http.Request, trace string, pick picker, match matche
hosts = append(hosts, "")
for _, h := range hosts {
if target = t.lookup(h, path, trace, pick, match); target != nil {
+ if target.RedirectCode != 0 && target.URL.Scheme == "https" && req.Header.Get("X-Forwarded-Proto") == "https" {
+ log.Print("[DEBUG] Ignoring https redirect from https upstream")
+ continue
+ }
break
}
} The test table:
Running fabio as:
X-Forwarded-Proto http request:
X-Forwarded-Proto https:
|
Maybe the log message should be |
I'll open a PR for further discussion. |
The AWS load balancer terminates SSL and passes plain HTTP to fabio but does not offer the ability to do a schema redirect. Therefore, fabio running behind an AWS ELB with a schema redirect rule results in a redirect loop. This PR fixes fabiolb#448.
I'll comment here to keep the discussion in one place. Why only Maybe like this? target.RedirectCode != 0 && target.URL.Scheme == req.Header.Get("X-Forwarded-Proto") |
I thought about that ... but would you want to skip an upstream |
But will this change not have the same effect for |
That's a very good point. So more generically we want to prevent redirect loops. The initial case was a simple scheme redirect (http -> https) but that does not cover all use cases. How about this ...
If all the above are true ... skip to prevent a redirect loop. |
I added another commit to the PR to address these cases and make the redirect protection more generic while still satisfying the original request. |
@leprechau Thanks for this. I got it to work with your patch if i removed this check:
this is the content of all the variables when doing a request (values inside ' '):
and this is the config used:
|
Interesting ... wonder how the request hostname is empty? The path I can see that |
@leprechau this is the content of the entire req object:
Looks like it should be: and it works with:
|
Yes, I reworked it an pushed another commit to PR #466 ... can you see if this now works in your AWS environment as expected? I was trying to avoid calling |
@leprechau Nice work. Almost there now :) |
I’m assuming in this case that |
@leprechau Yes if i add the trailing / in the redirect |
@atillamas Great! Just pushed one last commit that should make it work without having to fully qualify the redirect target in your config. |
@leprechau forgot to say that it worked with $path previously also, But now I've tested your latest change and it does work as it should! 👍 great job! Thanks! |
Awesome, thanks for the patience and testing. |
I've merged the PR. Thanks a lot for the incredible patience and diligence of both of you! |
@magiconair Thank you! Without you Fabio wouldn't exist :) |
Hi, is it possible to use fabio to redirect http to https on the same destination? for example redirect http://example.com to https://example.com ? when I've tried it i just get a redirect-loop.
The text was updated successfully, but these errors were encountered: