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

Feature: https only -- adds option to enforce https on running instances #48

Merged
merged 5 commits into from
May 15, 2020

Conversation

benbusby
Copy link
Owner

Enforces https by default on Docker instances, adds an option to enforce https if running through pip/pipx.

benbusby and others added 2 commits May 15, 2020 10:29
Enabled by default in docker containers, but not pip/pipx runs. Command
line runs of Whoogle Search through pip/pipx/etc will need the
`--https-only` flag appended to the run command.
Moved https-only note to top of docker run command, updated pip runner help output
@benbusby benbusby changed the title Feature/https only Feature: https only -- adds option to enforce https on running instances May 15, 2020
@benbusby
Copy link
Owner Author

@FoxxMD @chpatton013 (or anyone else who comes across this and is a regular Docker user), do you have any input on this new feature? It basically just forces the Flask server to reroute to HTTPS by default if running via Docker, but instances run through other methods require a flag to enforce this behavior.

I'm a bit torn since I don't really like having different deployment options behave differently by default, but it solves a problem brought up in #17 where the Flask server (believing that it's running as HTTP only) would reroute to the non-HTTPS variety of whatever URL is being used. If the site wasn't configured properly, it either would get stuck on the HTTP connection from this point on, or just hang if port 80 wasn't opened.

Overall I think HTTPS enforcement is a good addition to solve issues like this, but I might be overlooking some cases where this would be a breaking change. Let me know your thoughts (if you have any, feel free to ignore if you're indifferent about it).

@FoxxMD
Copy link
Sponsor

FoxxMD commented May 15, 2020

First off, it's configurable so that's good 👍

WRT deployment options I agree with you that they should be consistent. If the default using pip is not to enforce then I don't think you should enforce in docker either. IMO this is a configuration setting that should be left up to the user to use correctly. If you feel like users need guidance on using it correctly you could always add some info to the readme, however.

As far as breaking changes this (enforcing https by default) would definitely break deployments for anyone who is already running the docker and using it behind a reverse proxy (like me).

@FoxxMD
Copy link
Sponsor

FoxxMD commented May 15, 2020

Issue adjacent -- allowing docker users to specify the port whoogle runs on via ENV variable would be much appreciated. An unraid user asked me about running on port 443 in a configuration (in unraid) that doesn't allow us to map ports between container/host so they have no real recourse ATM. Would be an easy fix I think ;)

Dockerfile no longer enforces an HTTPS connection, but still allows for
setting via a build arg. The Flask server port is now configurable as a
build arg as well, by setting a port number to "whoogle_port"
@chpatton013
Copy link
Contributor

chpatton013 commented May 15, 2020

Agreed with @FoxxMD; Keep it consistent and configurable. I also run all my docker apps behind a reverse-proxy, so I prefer them to do dumb HTTP by default.

And @FoxxMD WRT your request for a configurable port variable, I was actually just about to open a PR to do that.

@benbusby
Copy link
Owner Author

benbusby commented May 15, 2020

Great points, I hadn't thought of the breaking changes to reverse proxies yet, so I'm glad I got a second (and now third) set of eyes on this.

I just updated this to default to HTTP, and snuck in an update to set a configurable port variable as well since they're (sort of) related.

I think what I'll do is just maintain a separate branch for the Heroku deployment that will have a slightly different Dockerfile, which will default to HTTPS. That seems to be the main issue that myself and others have come across, as Heroku's HTTPS enforcement is weirdly inconsistent across deployments. It may be slightly inconvenient, but worthwhile imo.

Dockerfile Outdated Show resolved Hide resolved
@chpatton013
Copy link
Contributor

:shipit:

@benbusby benbusby merged commit 1ed6178 into master May 15, 2020
@benbusby benbusby deleted the feature/https-only branch May 20, 2020 17:15
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 this pull request may close these issues.

None yet

3 participants