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

Porting features to CKAN core #2

Open amercader opened 7 years ago

amercader commented 7 years ago

@camfindlay This is great!

As @wardi said on the mailing list it would be great to get as many of these as possible in CKAN core. See my comments below:

Stronger password reset tokens

This could be ported straight to core (here)

Brute force protection

Can you describe how this works?

Cookie-based CSRF protection for requests

This has been on our list forever and would be awesome to get integrated in core. Do you think that your implementation is generic enough for this? Do you mind give an overview of the implementation?

Removed ability to change usernames after signup

I just merged this to master and will be part of 2.7.x (https://github.com/ckan/ckan/pull/3531)

Server-side session storage

IIRC this was already available, it's just a matter of configuring the relevant beaker.* config options right? (and perhaps document it better, with a dedicated section in the docs)

Session invalidation on logout

That sounds really sensible and something that could be ported upstream. I could not find where or how you do it though.

Stronger password validators (NZISM compatible)

See https://github.com/ckan/ckan/pull/3601. We definitely want to increase the default requirement but allow extensions to override to fit custom requirements. Obviously the main problem is that there is not an easy way to override the user schema as there is no suitable interface, so you either have to monkey-patch like in your case or override the whole user_update to pass a custom schema. We need to provide an IUserForm that allows the same customization as eg IDatasetForm.

When users try to reset a password for an email address, CKAN will no longer disclose whether or not that email address exists in the DB.

I think this is something that different maintainers may have different views on, so I'm not too worried about getting it into core but also not against it.

It would be great to know your views on this and better yet if some PR come out of it!

cc @ckan/Core

TkTech commented 7 years ago

IIRC this was already available, it's just a matter of configuring the relevant beaker.* config options right? (and perhaps document it better, with a dedicated section in the docs)

Incomplete until your recent flask-related commit: https://github.com/ckan/ckan/commit/6d51cd27d584662970597de6414bddd8c5f965ee

Up until now you couldn't override all the options (which is why I mentioned it when you were working on it :P)

When users try to reset a password for an email address, CKAN will no longer disclose whether or not that email address exists in the DB.

This is a specific requirement in most modern policies. Since we don't include proper rate limiting on resets or attempts it's even more important to increase attack time.

amercader commented 7 years ago

IIRC this was already available, it's just a matter of configuring the relevant beaker.* config options right? (and perhaps document it better, with a dedicated section in the docs)

Incomplete until your recent flask-related commit: ckan/ckan@6d51cd2

I stand corrected, this will be available on 2.7 :)

camfindlay commented 7 years ago

just a chime in on this... time limited (just had twins! 🍼 🍼 ).

Will have to look over to help explain the implementation on some of the features (vendor produced this code). Otherwise, raid the code and take what you need.

amercader commented 7 years ago

@camfindlay congratulations! Go back to the twins :)

camfindlay commented 6 years ago

@amercader anything still needed on this issue? I'll close this issue in the next 24 hours unless there is anything pressing needing looked at. Can always reopen or open new issues to address specific parts 👍

amercader commented 6 years ago

@camfindlay these are all still valuable contributions to CKAN core, but the tech team doesn't have the capacity to work on them right now. We would be more than happy to provide guidance if you or someone else wanted to have a go at submitting some pull requests though!

anotheredward commented 6 years ago

Hi @amercader and @TkTech. I'll be breaking these features up and opening them against ckan, look forward to working with you :).

amercader commented 6 years ago

@anotheredward fantastic! ping us if you are unsure about anything. It might be worth to create an issue in the main CKAN repo if you need to discuss a particular approach.

TkTech commented 6 years ago

Likewise. Also reachable on our IRC channel #ckan @ freenode, although I can be slow to respond.

camfindlay commented 6 years ago

@anotheredward any other core PRs we can reference here?

anotheredward commented 6 years ago

https://github.com/ckan/ckan/pull/4192 Don't confirm user exists on reset form https://github.com/ckan/ckan/issues/4193 Remove update username functionality https://github.com/ckan/ckan/issues/3441 Remove or lockdown /users /users/:username routes https://github.com/ckan/ckan/issues/4002 Confirmed that the maintainers will accept a default strategy of using beaker for session management

Beaker Session/CSRF Protection requires a small amount of additional configuration/documentation, as we're looking at spinning up another 2 redis instances (one for session management and one for CSRF tokens) as we want the instances to have a memory limit and eviction strategy of deleting the least recently used Session/CSRF token (We used to use memcached, which works like this out of the box). Running multiple Redis instances when the configuration differs between databases is the offical recommendation.