Skip to content

Incorrect redirect to http instead of https#4341

Merged
joshmoore merged 6 commits intoome:developfrom
atarkowska:missing_schema
Nov 24, 2015
Merged

Incorrect redirect to http instead of https#4341
joshmoore merged 6 commits intoome:developfrom
atarkowska:missing_schema

Conversation

@atarkowska
Copy link
Copy Markdown
Member

This PR should fix incorrect redirection to ​http:// instead of ​https://

curl -i -s -c cookies.txt -b cookies.txt -e https://omero.local/omero/webclient/login/ -d 'csrfmiddlewaretoken=kyuLRduNLY3OQNUMUBiFjuQMDYrcEwt4&username=user-1&password=ome&server=2&url=url=%2Fmerge%2Fwebclient%2F' -X POST https://omero.local/omero/webclient/login/
HTTP/1.1 302 FOUND
Server: nginx/1.8.0
Date: Wed, 18 Nov 2015 13:00:58 GMT
Content-Type: text/html; charset=utf-8
Transfer-Encoding: chunked
Connection: keep-alive
Vary: Cookie
Location: https://omero.local/omero/webclient/
Set-Cookie: sessionid=jix75augbw8i6xdwvydru249sq8vaj9n; httponly; Path=/

cc @manics see https://trello.com/c/7eFWt9Bj/8-django-secure-proxy-ssl-header

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a space.

@jburel jburel added the develop label Nov 18, 2015
@manics
Copy link
Copy Markdown
Member

manics commented Nov 19, 2015

Irrespective of whether this fixes the http/https bug or not it should still be useful for https://trello.com/c/7eFWt9Bj/8-django-secure-proxy-ssl-header
I'll test just this particular scenario.

@manics
Copy link
Copy Markdown
Member

manics commented Nov 19, 2015

After a lot of testing...

No config options set:

http and https works as expected:

  • https is redirected to https
  • http is redirected to http
  • The URL shown in Link to this Image uses the correct protocol

omero.web.application_server.host set to non-localhost:

https is always redirected to http

This could be because gunicorn ignores the header for security unless it's specifically enabled as a gunicorn arg http://gunicorn-docs.readthedocs.org/en/19.3/deploy.html#nginx-configuration

omero.web.application_server.host set to non-localhost, --wsgi-args=--forwarded-allow-ips=*:

http and https works as expected:

  • https is redirected to https
  • http is redirected to http
  • The URL shown in Link to this Image uses the correct protocol

Note * also works with just a single IP for security, e.g. omero web restart --wsgi-args='--forwarded-allow-ips=10.2.1.66'

@manics
Copy link
Copy Markdown
Member

manics commented Nov 19, 2015

FYI, If you double-quote the IP it doesn't work: omero web restart --wsgi-args='--forwarded-allow-ips="10.2.1.66"'

@atarkowska
Copy link
Copy Markdown
Member Author

@manics and I were investigating that issue and my comments are in https://github.com/openmicroscopy/management_tools/pull/77#issuecomment-158083311, basically saying the same

@atarkowska
Copy link
Copy Markdown
Member Author

To confirm I added docker IP + bin/omero web start --wsgi-args=--forwarded-allow-ips=...,10.2.1.83, see https://ci.openmicroscopy.org/view/DEV/job/OMERO-DEV-Python27-merge-deploy/51/console

Then added reverse proxy on docker as follow:

upstream omeroweb_merge{
        server cowfish...:4080 fail_timeout=0;
   }

that gives:

$ curl -i -k https://ola-test-1.docker.../merge/
HTTP/1.1 301 MOVED PERMANENTLY
Server: nginx/1.8.0
Date: Thu, 19 Nov 2015 16:06:37 GMT
Content-Type: text/html; charset=utf-8
Transfer-Encoding: chunked
Connection: keep-alive
Last-Modified: Thu, 19 Nov 2015 16:06:37 GMT
Expires: Thu, 19 Nov 2015 16:06:37 GMT
Location: https://ola-test-1.docker.../merge/webclient/
Cache-Control: max-age=0

@atarkowska
Copy link
Copy Markdown
Member Author

I will also open a PR to allow bin/omero config set omero.web.gunicorn.wsgiargs string that should simplify the entire process

@manics
Copy link
Copy Markdown
Member

manics commented Nov 20, 2015

Tested a multiple proxy configuration:

https://nginx-public-proxy/ -> http://omero-nginx-proxy/

Using the default settings this redirects to http since the internal communication with omero-nginx-proxy is over http, which means Django sees X-Forwarded-Proto=http. This can be overriden by setting a custom header in the front-end nginx-public-proxy:

proxy_set_header X-Forwarded-Proto-Omero-Web $scheme;

and setting the new property defined in this PR:

omero config set omero.web.secure_proxy_ssl_header '["HTTP_X_FORWARDED_PROTO_OMERO_WEB", "https"]'

Note the header name gets mangled as described in https://docs.djangoproject.com/en/1.8/ref/settings/#secure-proxy-ssl-header which can be confusing at first. Also json doesn't support (tuples) so you have to use [square brackets].

Once that's in place, everything works with the two proxies:

  • https is redirected to https
  • http is redirected to http
  • The URL shown in Link to this Image uses the correct protocol

So apart from the minor comment about json brackets, this is good to merge.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use () in json, [] seems to work

@atarkowska
Copy link
Copy Markdown
Member Author

@manics thanks for testing, comment updated

@manics
Copy link
Copy Markdown
Member

manics commented Nov 23, 2015

Text changes look fine, though I haven't checked it renders correctly. Merge and fix later if necessary?

@joshmoore
Copy link
Copy Markdown
Member

Filed a card for the follow up: https://trello.com/c/BJnz66v7/89-regenerate-https-docs

joshmoore added a commit that referenced this pull request Nov 24, 2015
Incorrect redirect to http instead of https
@joshmoore joshmoore merged commit c5a38a1 into ome:develop Nov 24, 2015
@atarkowska atarkowska mentioned this pull request Nov 24, 2015
@atarkowska atarkowska deleted the missing_schema branch November 26, 2015 09:44
@jburel jburel added this to the 5.2.1 milestone Dec 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants