-
-
Notifications
You must be signed in to change notification settings - Fork 490
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
Adding CSRF support #1967
Adding CSRF support #1967
Conversation
Spring suggest to not use the cookie to store the token as it may depend on the security of the cookie. So it depends on the security of the browser. But that complicates the code a lot because that means we have to manually add it to any page as a javascript variable to be used inside each call. A lot of workaround and even in the Spring doc they recommend to use the cookie approach for AngularJS explicitly. |
Should we also stop GET calls as they may contain sensible data? Maybe on a future enhancement? Maybe as an optional setting? Previous Scenario:
After PR Scenario:
|
Working on adding the csrf token to the doc api: swagger-api/swagger-ui#1380 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks fine.
Not sure where, but would be good once merged to add documentation about new forms development including the csrf token, either in https://github.com/geonetwork/core-geonetwork/tree/develop/software_development or in http://geonetwork-opensource.org/manuals/trunk/eng/users/customizing-application/index.html
@fxprunayre I am trying to add the csrf header to the swagger doc but I cannot make it work. I added it to SwaggerConfig.java as an ApiKey on the array of securitySchemes:
and defined it on index.html as:
It gets applied on the GET requests, but not on the rest (which are the ones that really need it 😅 ) |
Finally I added it as an interceptor before each call. The ApiKey authorization object didn't work for some unknown reason. So, please check and approve if it is good enough. |
Any example when using CURL for example ? You need to first make a request, get the header and then set it in next calls ? |
You can still use basic authentication:
Edit: OK, this is a GET example. Let me get back with a POST example. |
Yes, you need to do first a call to a POST endpoint to get the session and the csrf and then, using the cookie and the token, you can do whatever you want: $ rm -f /tmp/cookie; curl -c /tmp/cookie http://localhost:8080/geonetwork/srv/eng/info?type=me -X POST ; cat /tmp/cookie
$ curl -v -X POST ```
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a CSRF expert, but it looks good to me.
What should we do when having :
? It sounds related to WRO4J - Does it mean that all users moving from 3.2.1 to 3.2.2 will have to manually drop WRO4J db to be able to login ? |
They have to use the new html and javascript. Is it not renewed on startup?
|
I'm not 100% sure WRO is doing something on startup to check if it needs to rebuild the cache or not. Anyone ? It could check if there is file change or at least we could maybe trigger a build if version changed (when db migration is triggered maybe ?). |
I have tested and the wro4j cache file seem updated, at least the timestamp changes. But the problem is there, you need to remove the files manually and restart GeoNetwork, what is not nice. From 3.2.1:
Migrated to 3.2.2, using the same external folder as in 3.2.1:
Related code to fill the wro4j cache in startup: https://github.com/geonetwork/core-geonetwork/blob/3.2.x/web/src/main/java/org/fao/geonet/Geonetwork.java#L430-L444 Not sure what is done in that code to update the cache, but seem not enough. Maybe an option is to search for the |
The database migration is executed before the I have tested the following code in
Seems working fine to solve this issue, but has some issues:
@fxprunayre can you check/test the proposed changes and maybe you have a better idea how to improve this? |
No better idea. |
…ith CSRF token with migrated wro4j cache - geonetwork#1967
@fxprunayre I found a better way, please if you have time, can you try this PR: #2031 in your system with a migration? |
This breaks the GeoNetwork harvester using authentication right ? This should be handled in here too https://github.com/geonetwork/core-geonetwork/blob/master/harvesters/src/main/java/org/fao/geonet/kernel/harvest/harvester/geonet/Harvester.java#L128 ? Anyone did it already or having examples on how to setup this in Java ? |
This fixes a security bug on https://en.wikipedia.org/wiki/Cross-site_request_forgery
So now, if you are logged in on GeoNetwork and an external third party tries to ajax call or redirect you to any endpoint in GeoNetwork, GeoNetwork will reject the call because it won't contain this extra security token. This token is generated on the server by session and stored on a cookie called XSRF-TOKEN on the client.
By default, all Angular calls will add the csrf as header X-XSRF-TOKEN and/or parameter _csrf containing the security token. All current forms that don't use Angular have also been modified so we don't have problems. The multi-part forms for uploading files added this parameter inside the "action" element of the form.
Changes on future developments:
If not, GeoNetwork will return an error asking for this token:
As a good practice, even GET requests should contain this csrf token, but right now GeoNetwork will skip the check on those calls (default Spring configuration, based on the idea that all GET calls don't modify the platform).
Extra: As Spring -Security 3.2 (security library we are using) does not contain the Cookie filter for Csrf, I just ported it from Spring 4. It is a simple code that works. When (if?) we upgrade, we can remove this file and use the native Spring cookie filter.