search

data-govt-nz / ckanext-security

A CKAN extension to hold various security improvements for CKAN
GNU Affero General Public License v3.0
25 stars 32 forks source link

Replace csrf plugin with Queensland Government solution #24

Closed anotheredward closed 5 years ago

anotheredward commented 5 years ago

Our current CSRF solution requires server-side state and doesn't actually protect against CSRF (it does make it very difficult, but not impossible).

This PR replaces it with the approach the Queensland Government have taken which both does protect appropriately and doesn't require server-side state.

Ok, thanks for the help @ThrawnCA , I've managed to get it up and working, the following potential tasks remain:

ThrawnCA commented 5 years ago

This change is not backwards-compatible; any installations that are patching the CKAN middleware will get errors, because the middleware module is no longer available.

Also, the README should be updated.

ebuckley commented 5 years ago

Just an update with where this is at right now, I'm picking up closing off this issue and integrating the double cookie CSRF protection.

I have noticed a couple issues with the current implementation which need to be worked out before we can merge this.

  1. The is_logged_in() method checks for the auth_tkt cookie, but this does not exist when using this plugin as we use Beaker for session tracking (See who.ini config that is required for this plugin)
  2. The CSRF check code does not get executed for the auth routes (login/logout)
  3. The origin/referrer header check needs to have webserver config, where we would prefer to have this check in the application.

I think we can cover these issues with an implementation of the CSRF check from our current Middleware class. I have a branch for these changes, but still need a bit of refinement before I'm happy with the implementation.

onkartibe commented 5 years ago

LGTM

camfindlay commented 5 years ago

Let's look to merge this and make a new SEMVER release of this module. Question @ebuckley have we tagged the current version so those looking to upgrade get the hint that this is a more major release?

ThrawnCA commented 5 years ago

@camfindlay To properly follow SEMVER requires a public API. I suppose the closest thing to that would be the configuration settings? And the middleware patching.

Since this does change how the extension interacts with middleware - ie it no longer needs the patch - but it should be compatible with existing configuration, I suggest updating the minor version to 1.1.0.

camfindlay commented 5 years ago

Minor release seems reasonable - it's backwards compatible (even though just upgrading in place might be breaking if you dont tidy up). 1.1.0 it is.

ebuckley commented 5 years ago

Since this does change how the extension interacts with middleware - ie it no longer needs the patch - but it should be compatible with existing configuration, I suggest updating the minor version to 1.1.0.

FWIW the change here will still requires the middle-ware patch. The middleware is required because there are several routes that were not triggering the previous method of hooking into the request/response

All that said, I'm happy to go with a minor release to 1.1.0

I will merge this PR and action that update now.