ome / omero-web

Django-based OMERO.web client
https://www.openmicroscopy.org/omero
16 stars 29 forks source link

Add additional CSRF setting #477

Closed knabar closed 12 months ago

knabar commented 1 year ago

Following #471, this PR adds another required setting to make CSRF requests work:

omero config set omero.web.csrf_trusted_origins '[".example.com"]'

Without this setting, it is not possible to perform cross-domain unsafe HTTP requests (POST, PUT, and DELETE), even if cookies etc. are configured correctly.

Reference:

will-moore commented 1 year ago

With this PR included on merge-ci I was able to get Django 4.0 working (avoided the CSRF errors I was seeing previously) by setting:

omero config append omero.web.csrf_trusted_origins '"https://merge-ci.openmicroscopy.org"'

This appears to be a requirement for Django 4.0+ to avoid CSRF errors. Also, there is a breaking change in upgrade from 3.2 - previously the origins needed to have no schema (as in the your docstring examples) but in Django 4.0 you need to have the schema. So the update of that setting would be a required step in upgrade from 3.2 to 4.0+.

Just a thought: would it make sense to hold off on this change until we bump Django to 4.0 (or 4.2) to avoid those upgrade issues?

https://docs.djangoproject.com/en/4.0/ref/settings/#csrf-trusted-origins

will-moore commented 1 year ago

I don't really understand why Django 4.0 requires the setting in order for login to work (on merge-ci) but that wasn't needed for Django 3.2. The docs above don't say anything about Django 4.0 being more strict. The only change mentioned for 3.2 -> 4.0 is the addition of the schema to origins.

Although we're not the first to see this https://stackoverflow.com/questions/73338124/csrf-token-issue-when-upgrading-django-to-version-4

sbesson commented 1 year ago

@will-moore do you have a link for the error you were receiving? Scanning quickly the docs, this might be related to the changes described in the CSRF section and bear some relationship with the configuration of the OME CI architecture and the way requests are proxied from merge-ci.openmicroscopy.org to the underlying OMERO.web application. If correct, I would expect other deployment would be affected and we might need to work out best recommendations in terms of the headers that should be set on requests.

will-moore commented 1 year ago

I'm just seeing the csrf_failure response https://github.com/ome/omero-web/blob/ba072f151247df5b62768a0fbf37891675e2802d/omeroweb/feedback/views.py#L175 as configured at https://github.com/ome/omero-web/blob/ba072f151247df5b62768a0fbf37891675e2802d/omeroweb/settings.py#L1668 on login.

But yes, that CSRF section of the docs seems to match what we're seeing.

It might be nice if omero-web could automatically add it's own origin to the CSRF_TRUSTED_ORIGINS setting, so that users didn't have to do it?

chris-allan commented 1 year ago

At least for our systems we have no requirement to set CSRF_TRUSTED_ORIGINS with Django 4.0 in a semi-production setting. This includes scenarios there are SSL certificates.

If you are having CSRF errors you'll definitely want to look at the OMEROweb.log for the full reason. As @sbesson has implied, problems with merge-ci are likely due to Host, Origin or Referrer header mismatch which is more strict in 4.0. This suggests that actually other things in this OMERO.web deployment, including fully qualified URL generation, may not be working correctly if that's the case.

will-moore commented 1 year ago

Ah - that reminds me that on merge-ci Django doesn't know it is running under https. E.g. build_absolute_uri() returns http://... - e.g. https://merge-ci.openmicroscopy.org/web/api/ has "url:base": "http://merge-ci.openmicroscopy.org/web/api/v0/". So it's probably failing because of that mismatch. It would be good to fix that on merge-ci, as it's causing issues elsewhere (e.g. https://github.com/ome/omero-web-zarr/pull/7)

chris-allan commented 1 year ago

If that's true then OMERO.web is indeed misconfigured. See also:

will-moore commented 1 year ago

Tried looking into this some more with no luck...

I found a couple of examples of others seeing similar issues, but can't tell how to apply their fixes to merge-ci:

https://stackoverflow.com/questions/11441832/request-is-secure-always-returns-false-with-uwsgi-server https://stackoverflow.com/questions/66176881/django-request-is-secure-always-returns-false

I added an output of the nginx conf on merge-ci, see https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-web/213/console

15:47:41 upstream omeroweb_web {
15:47:41     server web:4080 fail_timeout=0;
15:47:41 }
15:47:41 
15:47:41 server {
15:47:41     listen 80;
15:47:41     server_name $hostname;
15:47:41 
15:47:41     sendfile on;
15:47:41     client_max_body_size 0;
15:47:41 
15:47:41 
15:47:41 
15:47:41     # maintenance page serve from here
15:47:41     location @maintenance_web {
15:47:41         root /home/omero-web/workspace/OMERO-web/OMERO.web/etc/templates/error;
15:47:41         try_files $uri /maintainance.html =502;
15:47:41     }
15:47:41 
15:47:41     # weblitz django apps serve media from here
15:47:41     location /web/static {
15:47:41         alias /home/omero-web/static/web;
15:47:41     }
15:47:41 
15:47:41     location @proxy_to_app_web {
15:47:41         proxy_set_header X-Forwarded-Proto $scheme;
15:47:41         proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
15:47:41         proxy_set_header Host $http_host;
15:47:41         proxy_redirect off;
15:47:41         proxy_buffering off;
15:47:41 
15:47:41         proxy_pass [http://omeroweb_web;](http://omeroweb_web%3B/)
15:47:41     }
15:47:41 
15:47:41     location /web {
15:47:41 
15:47:41         error_page 502 @maintenance_web;
15:47:41         # checks for static file, if not found proxy to app
15:47:41         try_files $uri @proxy_to_app_web;
15:47:41     }
15:47:41 
15:47:41 }

which has the line mentioned in one of those posts: proxy_set_header X-Forwarded-Proto $scheme.

chris-allan commented 1 year ago

That's the HTTP rather than HTTPS configuration, no?

will-moore commented 1 year ago

I don't know. I don't see 2 different nginx confs. This is what's generated at https://merge-ci.openmicroscopy.org/jenkins/job/OMERO-web/configure by

omero web config nginx --http "$OMERO_WEB_PORT" >$OMERO_DIST/nginx.conf.tmp

sudo cp $OMERO_DIST/nginx.conf.tmp $HOME/nginx/$NODE_NAME.conf
chris-allan commented 1 year ago

Then you'll need to find out as it's the HTTPS request pipeline that is in question here. If TLS is being terminated further up the chain there is further work to do ^1.

will-moore commented 1 year ago

@jburel - do you have any idea how https is getting set-up on merge-ci?

It looks like we are already doing:

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

as suggested fix for this mentions at https://stackoverflow.com/questions/66176881/django-request-is-secure-always-returns-false

But reading the warning at https://docs.djangoproject.com/en/3.2/ref/settings/#secure-proxy-ssl-header I don't know if we're satisfying all these conditions on merge-ci. Does NGINX do all that for us?

jburel commented 1 year ago

See also https://github.com/openmicroscopy/management_tools/blob/master/ansible/inventory/host_vars/web-proxy.openmicroscopy.org.yml

will-moore commented 1 year ago

@jburel - Thanks for that. I'm not sure exactly what it's doing! Been reading https://realpython.com/django-nginx-gunicorn/ but management_tools and ansible is outside of the scope of that...

It seems like this line is relevant:

nginx_proxy_forward_scheme_header: X-Forwarded-Proto-Web-Proxy

The nginx config generated by omero-web contains:

proxy_set_header X-Forwarded-Proto $scheme;

and this is not configurable since it's in the template: https://github.com/ome/omero-web/blob/ba072f151247df5b62768a0fbf37891675e2802d/omeroweb/templates/nginx.conf.template#L26

The setting passed to Django in merge-ci is currently:

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

I'm not sure if all 3 need to be the same, or if the ansible setting overrides the one in the nginx config generated by omero-web?

imagesc-bot commented 12 months ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/release-of-omero-server-5-6-8-and-omero-web-5-22-0/84157/1

imagesc-bot commented 5 months ago

This pull request has been mentioned on Image.sc Forum. There might be relevant details there:

https://forum.image.sc/t/docker-image-of-omero-web-5-23-0-still-with-v5-22-1/88699/13