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

remove cors/csrf java code configuration, in favor of regular spring-cloud-gateway configuration #59

Merged
merged 3 commits into from
Nov 2, 2023

Conversation

pmauduit
Copy link
Member

@pmauduit pmauduit commented Sep 5, 2023

Using the 2 introduced flags georchestra.gateway.corsEnabled and georchestra.gateway.csrfEnabled we can leverage default spring security behaviour related to both security measures.

Removing the cors() / csrf() .disable() calls, leaving the framework do all its own magic for us.

Note: we keep them disabled by default, as this was the default intended behaviour.
Note2: this might require a revisit to make the configuration more flexible.

Note3: With the former security-proxy, the configuration was left to the proxified webapp. If we go with this PR, we will invert the configuration and leave it to the gateway, it will then be necessary to deactivate the filters on proxified webapps, to avoid having the http response headers being set twice, which can mess up the browsers.

@groldan
Copy link
Member

groldan commented Oct 18, 2023

How is the CORS configuration different than the default spring-cloud-gateway way of doing so? e.g.

spring:
  cloud:
    gateway:
      globalcors:
        cors-configurations:
          '[/**]':
            allowedOrigins: "https://docs.spring.io"
            allowedMethods:
            - GET

See https://docs.spring.io/spring-cloud-gateway/reference/spring-cloud-gateway/cors-configuration.html

@pmauduit
Copy link
Member Author

pmauduit commented Oct 18, 2023

How is the CORS configuration different than the default spring-cloud-gateway way of doing so? e.g.

I missed that, sorry. But if we go through official yaml configuration, shall we remove the java calls to http.cors().enable()/disable() ? I am not sure which one will take precedence.

It makes totally sense to me to rely on what spring already provides anyway.

@pmauduit
Copy link
Member Author

okay, stripping the http.cors().csrf() java configuration runtime-tested with the following config locally:

spring:
  cloud:
    gateway:
      globalcors:
        cors-configurations:
          '[/**]':
            allowedOrigins: "*"
            allowedHeaders: "*"
            allowedMethods: "*"

The following curl payload returns indeed a 200 - ok:

 curl 'http://localhost:8080/geonetwork/srv/api/search/records/_search?bucket=bucket' \                                                    2023-10-18 17:55:48 pmauduit pts/7
  -X 'OPTIONS' \
  -H 'authority: localhost:8080' \
  -H 'accept: */*' \
  -H 'accept-language: en-US,en;q=0.9,fr;q=0.8,de;q=0.7' \
  -H 'access-control-request-headers: content-type' \
  -H 'access-control-request-method: POST' \
  -H 'origin: https://dev.datagrandest.fr' \
  -H 'referer: https://dev.datagrandest.fr/' \
  -H 'sec-fetch-dest: empty' \
  -H 'sec-fetch-mode: cors' \
  -H 'sec-fetch-site: cross-site' \
  -H 'user-agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/116.0.0.0 Safari/537.36' \
  --compressed -i
HTTP/1.1 200 OK
Vary: Origin
Vary: Access-Control-Request-Method
Vary: Access-Control-Request-Headers
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: POST
Access-Control-Allow-Headers: content-type
Cache-Control: no-cache, no-store, max-age=0, must-revalidate
Pragma: no-cache
Expires: 0
X-Content-Type-Options: nosniff
X-Frame-Options: DENY
X-XSS-Protection: 1 ; mode=block
Referrer-Policy: no-referrer
content-length: 0

Funny thing is that if I revert my java modification, the configuration stays valid and I am able to get "200 - OK" status, without touching the Java code (which does not really make sense, because with http.csrf().disabled().cors.disabled() I'd have expected it to be disabled.

@groldan does removing the related Java code make sense to you ? As we can use the yaml configuration anyway.

As mentioned by @groldan on #59 (comment)
the spring-cloud-gateway project already provides a convenient way to
configure CORS. About CSRF, it is IMHO the role of each proxified
webapps to mitigate the issue, and outside of the gateway's concern.

As a result, removing cors/csrf management from our code's side.
@pmauduit pmauduit changed the title Being able to activate / deactivate CORS & CSRF spring security management remove cors/csrf java code configuration, in favor of regular spring-cloud-gateway configuration Oct 19, 2023
Copy link
Member

@groldan groldan left a comment

Choose a reason for hiding this comment

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

LGTM

@pmauduit pmauduit merged commit 9949d8b into main Nov 2, 2023
3 checks passed
@pmauduit pmauduit deleted the cors-csrf-activation-by-configuration branch November 2, 2023 12:44
pmauduit added a commit that referenced this pull request Nov 6, 2023
Partially reverts #59
CORS is configurable via yaml config files, CSRF is not.

From my understanding, CSRF protection has to be managed separately by
the underlying webapps being proxified by the gateway.
@pmauduit pmauduit mentioned this pull request Nov 6, 2023
pmauduit added a commit that referenced this pull request Dec 6, 2023
* Testsuite broken because sending a request with no Accept header (or
  "Accept: */*") will make spring sending a 401 instead of a 302
  redirect to /login
* restoring previous work on csrf/cors not present in DT's fork (#73, #59)
* restoring some other works related to login/logout controllers/templates
pmauduit added a commit that referenced this pull request Jan 31, 2024
* Testsuite broken because sending a request with no Accept header (or
  "Accept: */*") will make spring sending a 401 instead of a 302
  redirect to /login
* restoring previous work on csrf/cors not present in DT's fork (#73, #59)
* restoring some other works related to login/logout controllers/templates
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.

3 participants