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

bug: redirect http to https but port not change #7011

Closed
tangzhenhuang opened this issue May 9, 2022 · 10 comments · Fixed by #7065
Closed

bug: redirect http to https but port not change #7011

tangzhenhuang opened this issue May 9, 2022 · 10 comments · Fixed by #7065

Comments

@tangzhenhuang
Copy link
Contributor

Current Behavior

Recently, I upgraded apisix with the latest code, and found that there was an error in converting http to https. When redirecting, I always bring a port 80. It should be caused by this change:https://github.com/apache/apisix/pull/6686/files#r867816800

Maybe set $var_x_forwarded_port $server_port; in ngx_tpl.lua conflicts with local ret_port = tonumber(ctx.var["var_x_forwarded_port"]) in redirect.lua

Expected Behavior

port changed from 80 to 443

Error Logs

No response

Steps to Reproduce

add redirect and set http_to_https to true

Environment

  • APISIX version (run apisix version): master
  • Operating system (run uname -a):
  • OpenResty / Nginx version (run openresty -V or nginx -V):
  • etcd version, if relevant (run curl http://127.0.0.1:9090/v1/server_info):
  • APISIX Dashboard version, if relevant:
  • Plugin runner version, for issues related to plugin runners:
  • LuaRocks version, for installation issues (run luarocks --version):
@kwanhur
Copy link
Contributor

kwanhur commented May 9, 2022

Yep, it's a big problem :-(

@tzssangglass
Copy link
Member

tzssangglass commented May 9, 2022

as: https://github.com/apache/apisix/pull/6686/files#r868034475

Can we set a default value for this case?
If there is no X-Forwarded-Port in clinet reuqest headers, then the redirect plugin use 443 as the default https port?

@tokers
Copy link
Contributor

tokers commented May 10, 2022

as: https://github.com/apache/apisix/pull/6686/files#r868034475

Can we set a default value for this case? If there is no X-Forwarded-Port in clinet reuqest headers, then the redirect plugin use 443 as the default https port?

@tzssangglass What if the client doesn't carry X-Forwarded-For header and APISIX listening on non-443 ports for HTTPS (say 9443)?

@tzssangglass
Copy link
Member

What if the client doesn't carry X-Forwarded-For header and APISIX listening on non-443 ports for HTTPS (say 9443)?

In this case the user needs to make the request headers carry X-Forwarded-Port, which is also in line with the docs that describes:

When enabling http_to_https, the port in the redirect URL will be the value of header X-Forwarded-Port or the port of the server.

Just an idea.

@tokers
Copy link
Contributor

tokers commented May 10, 2022

That's not so generic. APISIX should have a generic solution. Such as using the HTTPS port (we can know it from config.yaml even if XFF header missing.

@tzssangglass
Copy link
Member

we can know it from config.yaml even if XFF header missin

I agree with this way

@kwanhur
Copy link
Contributor

kwanhur commented May 10, 2022

That's not so generic. APISIX should have a generic solution. Such as using the HTTPS port (we can know it from config.yaml even if XFF header missing.

It means place one initial variable in config.yaml and then fetch the custom value from XFF?

@tokers
Copy link
Contributor

tokers commented May 11, 2022

That's not so generic. APISIX should have a generic solution. Such as using the HTTPS port (we can know it from config.yaml even if XFF header missing.

It means place one initial variable in config.yaml and then fetch the custom value from XFF?

Yes. But we can have a custom logic that just get the HTTPS port from the config.yaml.

if X-Forwarded-Port exists then
    using X-Forwarded-Port
else
    using HTTPS port from config.yaml
end

@kwanhur
Copy link
Contributor

kwanhur commented May 11, 2022

If the https redirect port from config.yaml, seems update it has to restart apisix manually? like changing 443 to 8443.

Why not from plugin's attribute? It can be updated automatically.

@jwrookie
Copy link
Contributor

After discussion, we adopted the following solution

When enabling http_to_https, the ports in the redirect URL will pick a value in the following order (in descending order of priority)

  • Read plugin_attr.redirect.https_port from the configuration file (conf/config.yaml).
  • If apisix.ssl is enabled, read apisix.ssl.listen_port first, and if it does not exist, read apisix.ssl.listen and select a port randomly from it.
  • Use 443 as the default https port.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants