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

HTTP -> HTTPS redirect fail if HTTPS_PORT is not 443 #2655

Closed
frafra opened this issue Mar 9, 2023 · 6 comments
Closed

HTTP -> HTTPS redirect fail if HTTPS_PORT is not 443 #2655

frafra opened this issue Mar 9, 2023 · 6 comments
Assignees
Labels
feedback: captured Feedback in issue has been captured ready Ready for implementation type: bug Something isn't working user reported Reported by a Mathesar user work: other
Milestone

Comments

@frafra
Copy link

frafra commented Mar 9, 2023

Description

HTTPS_PORT is a variable used by the compose file to map Caddy HTTPS port to HTTPS_PORT. Changing such variable does not affect the HTTP to HTTPS redirect, so the redirect is broken.

Expected behavior

Changing HTTPS_PORT should affect the HTTP to HTTPS redirect as well.

To Reproduce

  1. Clone repo
  2. cp .env.template .env
  3. Add the following lines to .env:
DOMAIN_NAME=localhost
HTTP_PORT=8080
HTTPS_PORT=8443
  1. docker compose --profile prod up --build
  2. Visit http://localhost:8080
@frafra frafra added status: triage type: bug Something isn't working labels Mar 9, 2023
@kgodey kgodey assigned silentninja and mathemancer and unassigned mathemancer Mar 9, 2023
@kgodey kgodey added work: other ready Ready for implementation and removed status: triage labels Mar 9, 2023
@kgodey kgodey added this to the v0.1.1 milestone Mar 9, 2023
@kgodey
Copy link
Contributor

kgodey commented Mar 9, 2023

Thank you for reporting this, @frafra. We will fix in our next release (next week).

@silentninja
Copy link
Contributor

@frafra When you visit the http://localhost:8080/ caddy is not issuing a redirect to 443. It’s issuing a redirect to https:// and your browser is assuming port 443. This is not a behaviour we can control. The workaround would be to make the following changes to the caddy file

  1. Add http_port and https_port as an environment variables to the docker image
  2. Add a manual redirect to the caddyfile
http://{$DOMAIN_NAME}:{$http_port} {
  redir https://{$DOMAIN_NAME}:{$https_port}
}
https://{$DOMAIN_NAME}:{$https_port} {
  ...
}

I would like to know about your use case

  • As the usage of configurable http_port and https_port is effectively intended for scenarios where your public ports remain default but your internal ports are different, e.g. your router is port forwarding from port 80 externally to port 10080 on your server and will take care of the port redirection.
  • The handshake for https certificate happens over port 443 so you need port 443 configured properly in order to get the SSL certificate.
  • Setting up a self-signed certificate is a hassle with docker and we have disabled it for our first release.

Fixing this issue complicates the setup and could lead to other bugs. So I want to avoid it unless we don't have a better workaround.

@frafra
Copy link
Author

frafra commented Mar 13, 2023

@silentninja I think that the issue is that I interpreted HTTPS_PORT as a way to set a different public port for HTTPS, while it is assumed to be changed only when using an additional reverse proxy or port forwarding mechanism.

Having two different variables to set both the public port (or public URL) and the local one could provide additional flexibility. I just tried to run a production environment on my work machine, where I do not have root access, so I cannot open port 80 or 443.

It could be that my misunderstanding is linked to using the compose and env files directly, since I opted for not using the script to generate the configuration.

@kgodey kgodey modified the milestones: v0.1.1, v0.1.2 Mar 14, 2023
@kgodey
Copy link
Contributor

kgodey commented Mar 14, 2023

Moved this to 0.1.2 since I don't want to block 0.1.1 on this issue and we'll be focusing more on installation improvements in 0.1.2.

@kgodey kgodey added the user reported Reported by a Mathesar user label Mar 21, 2023
@ghislaineguerin ghislaineguerin added the feedback: captured Feedback in issue has been captured label Mar 29, 2023
@pavish
Copy link
Member

pavish commented May 1, 2023

@silentninja What is the status of this issue?

@silentninja
Copy link
Contributor

I am marking this issue as fixed by #2824. #2824 adds documentation for all the configuration variables which explains how the HTTPS_PORT is meant to be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback: captured Feedback in issue has been captured ready Ready for implementation type: bug Something isn't working user reported Reported by a Mathesar user work: other
Projects
No open projects
Development

No branches or pull requests

6 participants