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.

Sign up for GitHub

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

Jump to bottom

Proto not set when running behind reverse proxy #27

Closed
jcox10 opened this issue Dec 13, 2022 · 11 comments · Fixed by #29
Closed

Proto not set when running behind reverse proxy #27

jcox10 opened this issue Dec 13, 2022 · 11 comments · Fixed by #29
Assignees
@briantist
Labels
bug Something isn't working

Comments

@jcox10
Copy link
Contributor

jcox10 commented Dec 13, 2022

In our environment, we run most everything behind a NGINX reverse proxy that handles all the SSL termination (we are required to have everything HTTPS). Most everything works properly, except when the flask.url_for generates an external URL, it defaults to standard HTTP. I found this ProxyFix page that has a solution and I tested it with setting app.wsgi_app = ProxyFix(app.wsgi_app, x_proto=1) in the main __init__.py.

However I'm not sure if that is the best option, or if it would be better to add a arg similar to the SERVER_NAME that would be SERVER_PROTO as seen from the client? Then pass that in when generating the url_for?
@briantist
Copy link
Owner

Thanks for bringing this up! I think this would affect a use case that I'm working on myself so it's really useful :)

While even I am currently only running galactory with the werkzeug server (on my workstation or in CI for example), production use should probably use a different WSGI server. In that case, I think it would be up to the implementer to use ProxyFix when creating their WSGI app.

For the built-in server / CLI usage, adding SERVER_PROTO as you suggest may be the better solution.

Does that sound right to you?

@briantist
Copy link
Owner

So I am looking into this some more. See the docs for url_for: https://flask.palletsprojects.com/en/0.12.x/api/?highlight=url_for#flask.url_for

The config value we'd set is PREFERRED_URL_SCHEME in order for it to be "implicit" but that value will only be used if we set _external=True, which we're already doing in all of our calls because the URLs are returned in data structures, so that's just something I'll need to be aware of.

I'm testing this out a bit now.

@briantist
Copy link
Owner

...and I realize now that variable won't work, it's only used when there is no request context (just like it says in the docs), so it can't be implicit. I've implemented it explicitly in testing though; it also surfaced another problem which is setting href in some places to the value of request.url which also would have been incorrect for your described use case, so I have a fix for that as well. I'll have a PR up soon.

@briantist briantist mentioned this issue Dec 14, 2022
Merged
@briantist
Copy link
Owner

@jcox10 would you be able to try the changes in PR #29 ?

You can pull/checkout my branch, or if it's easier to use pip, you can do it this way:

pip install https://github.com/briantist/galactory/archive/urls-under-construction.zip

I've added PREFERRED_URL_SCHEME / --preferred-url-scheme which you can set to https.
Let me know if that works as expected for you!

btw you may also be interested in the change I added in #28 , in case you were wanting to use a WSGI server like gunicorn or waitress or something; that change should make it easier to do so while using the existing configuration system. That might be an opportunity for you to inject ProxyFix.

@briantist
Copy link
Owner

@jcox10 I've merged your changes in #26 🎉 and I've rebased #29 to include those changes

@jcox10
Copy link
Contributor Author

jcox10 commented Dec 16, 2022 via email

@briantist briantist self-assigned this Dec 16, 2022
@briantist briantist added the bug Something isn't working label Dec 16, 2022
@jcox10
Copy link
Contributor Author

jcox10 commented Dec 19, 2022 •
edited
Loading

It looks like the flask auto-redirect to trailing slash doesn't have the correct scheme. The first thing ansible-galaxy does is tries to access /api and then it should get a HTTP308 to /api/. (edit: this was done with ansible-galaxy --server https://my-server-name.com:8888/)

curl https://my-server-name.com:8888/api
<!doctype html>
<html lang=en>
<title>Redirecting...</title>
<h1>Redirecting...</h1>
<p>You should be redirected automatically to the target URL: <a href="http://my-server-name.com:8888/api/">http://my-server-name:8888/api/</a>. If not, click the link.

I'm not sure how to configure flask for what scheme it should use for these auto-redirects.

@jcox10
Copy link
Contributor Author

jcox10 commented Dec 19, 2022 •
edited
Loading

However maybe I specified my --server argument wrong. If I do ansible-galaxy --server https://my-server-name.com:8888/api/ then it works just fine and ansible-galaxy doesn't try to figure out the api path. So maybe its just important to note that the server must be the full path to the api route with trailing slash?

@briantist
Copy link
Owner

Thanks @jcox10 this is good information, I'm going to look into it some more and see what else can be done.

When you use the server option that doesn't result in any redirects, is it otherwise working well to address the original issue?

@briantist
Copy link
Owner

briantist commented Dec 21, 2022 •
edited
Loading

@jcox10 all of the other endpoints in the project define the route twice (once with and one without the trailing /) to avoid the redirects, but I didn't do it on this endpoint. The fix was simply to add the "blank" route. The branch has been updated if you want to take a look and try it again.

Do you have any outstanding concerns or issues about #29?

@jcox10
Copy link
Contributor Author

jcox10 commented Dec 21, 2022

It works good, I've been running it for a few days now! Good find on the missing blank route!

@briantist briantist mentioned this issue Dec 26, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@briantist briantist

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix URL generation for scheme and self-referential URLs briantist/galactory
2 participants
@briantist @jcox10