ronivay / XenOrchestraInstallerUpdater

Xen Orchestra install/update script
GNU General Public License v3.0
1.18k stars 189 forks source link

redirectToHttps does not work - Fix provided #93

Closed pnedkov closed 3 years ago

pnedkov commented 3 years ago

OS Version: Debian 11.0 Node.js version: v14.17.6 Yarn version: 1.22.5

Server specs VM - 2 CPU cores, 4GB RAM

Issue I just did a fresh install as root on a freshly deployed Debian 11.0 and the HTTP->HTTPS redirection did not work. Nothing was listening on port 80. I started troubleshooting and noticed that the HTTP/HTTPS configuration in /root/.config/xo-server/config.toml looked like this:

[http]
redirectToHttps = true

[[http.listen]]
port = 443
cert = '/path/to/crt' # edited
key = '/path/to/key' # edited

After I changed the configuration as shown bellow and restarted xo-server, node started listening on both ports (80 and 443) and the HTTP->HTTPS redirection works:

[http]
redirectToHttps = true

[[http.listen]]
port = 80 # or you could leave it commented out - it will still work, since 80 is the default HTTP port

[[http.listen]]
port = 443 # you must specify it
cert = '/path/to/crt' # edited
key = '/path/to/key' # edited

Basically after xo-install.sh script made all the changes I had to manually:

  1. Comment out port = 443 in the first [[http.listen]] section. Or set it to 80 - either way it will work. This is the one the xo-install.sh script changed from 80 to 443.
  2. Uncomment # [[http.listen]] under # # Basic HTTPS. which introduces a second [[http.listen]] section dedicated for HTTPS.
  3. Uncomment # port = 443 under the second [[http.listen]] section. The cert and key variables are already set under the second [[http.listen]] section, so no changes there.
  4. Restart xo-server

Apparently two separate [[http.listen]] sections are needed for this to work - one for each listening port. I believe these two if-statements have to be reworked slightly: https://github.com/ronivay/XenOrchestraInstallerUpdater/blob/954cc7051df42d49a85567c19f517dea860779dc/xo-install.sh#L663-L679

ronivay commented 3 years ago

Hi,

Yep this is intentional. It's not mentioned anywhere that there would be a automatic redirect from HTTP to HTTPS if you decide to use HTTPS. You define a single port in xo-install.cfg wheter it's going to be listening for HTTP or HTTPS, no other listeners or redirects are configured automatically (which is required for the redirect to work). You can toggle CONFIGUPDATE to false in xo-install.cfg if you want to preserve manual changes to config.toml file in future updates.

I agree that this would make sense if one uses the default 80 and 443 ports, but with anything else, not really. Xen-Orchestra builtin webserver also sends back HSTS header in responses which means (assuming you use the default 80/443 ports) that once you've visited the site using HTTPS, your browser will always force https:// even if you try to access http://.

I think this would be a nice to have feature, but the fact that it would require yet another configuration variable for HTTPS port and for toggling the redirect (which would obviously have to be optional). Also this is the first time it's requested after all these years so i don't think there's very urgent need for it so at least for now i'm going to skip adding it. For now, just use the CONFIGUPDATE setting and edit configuration manually to match your needs.

E: Hmm now i read the actual code portion you pasted and noticed that there indeed is a line that adds the redirect which doesn't do anything as a separate HTTP listener is required, so i understand the possible confusion. I'll look into it, but as was explained above, i'll most likely just remove the obsolete line.

pnedkov commented 3 years ago

E: Hmm now i read the actual code portion you pasted and noticed that there indeed is a line that adds the redirect which doesn't do anything as a separate HTTP listener is required, so i understand the possible confusion. I'll look into it, but as was explained above, i'll most likely just remove the obsolete line.

Precisely. Line 677 suggested that if the user sets PATH_TO_HTTPS_CERT and PATH_TO_HTTPS_KEY, then the HTTP(80)->HTTPS($PORT) redirection will be configured automatically and will be working out of the box. And that was not the case. This is why I considered it as a bug.

The fix would be to either implement a couple of more sed commands in order to deploy the full configuration required for the HTTP->HTTPS redirection or delete line 677 which essentially removes a partially implemented feature that does not work. You chose the latter which is fine by me. I am OK with CONFIGUPDATE=false and manually configuring the HTTP->HTTPS redirection after the first install.

Thank you for your hard work on this project!

lucidnx commented 2 years ago

@ronivay I have just installed xo-server from your repo, it's not working, I had to do changes as @pnedkov mentioned. Can you review this issue?

ronivay commented 2 years ago

@ronivay I have just installed xo-server from your repo, it's not working, I had to do changes as @pnedkov mentioned. Can you review this issue?

Functionality hasn't changed, redirect isn't there on purpose. This script configures one listener, either http or https if certificate files are given so there's nothing to redirect from/to. If you want multiple listeners, you need to edit config manually as is described above.

lucidnx commented 2 years ago

Oh, maybe you can add installation parameter to configure redirect? It will be easier for people. I was literaly looking 30 minutes at configs to find out this. btw thanks for all your work :)

ronivay commented 2 years ago

Oh, maybe you can add installation parameter to configure redirect? It will be easier for people. I was literaly looking 30 minutes at configs to find out this. btw thanks for all your work :)

It is unfortunately quite tricky to implement without breaking any existing configs and expected behaviour for existing installations, as there would need to be new configuration variable for either HTTP or HTTPS port which kinda makes current PORT variable work differently.

I'm still having some difficulties to understand why this is such a big problem. You have one port to connect to which is either http or https port depending on configuration. I kinda understand this with default http/https ports but let's say you've configured ports 8080 for http and 8443 for https, redirect is pretty much obsolete in that case as it doesn't make it any easier for user, would only safeguard from accessing XO with http, which only one listener effectively does as well.

Only sane way i can think this could be implemented is if https is set and redirect chosen, that redirect would be put to port 80 and it's not changeable. I'll look into it at some point.