Generate tokens with HMAC instead of random strings, #25

[ThrawnCA]

This makes it almost impossible for an attacker to forge a token, even if they find a way to set a cookie. It also provides a verified timestamp so we can easily check the token age. In future, it might even be possible to eliminate the cookie altogether, so long as sufficient information is contained in the token, eg username + timestamp + intended action.

Note that this is an attempt to merge the ckanext-security code with the ckanext-qgov code, and they have different middleware approaches, so it is possible that errors have been introduced; it should be carefully tested.

[ThrawnCA]

@ebuckley Any feedback on this?

[ThrawnCA]

This looks great @ThrawnCA, apologies that this has been left for a while. The team member you mentioned on this PR is no longer on the project, and I wasn't aware of the history around this.

Ah, good to know.

I'm keen to get this tested locally and hopefully merged over the next few days :)

Sounds good :)

What do you see is remaining on this? Just the couple of TODO's you've noted about the config options?

Those are pretty minor, really. We've already been using similar code in production; just need to test that I merged it properly. As mentioned in the other comment, though, there would be at least a small change needed if it's to defend against Login CSRF.

[markstuart]

Do you think you'll have some capacity to to make those config variable changes and the one you mentioned re Login CSRF @ThrawnCA? I could probably pick those up pretty quickly as part of testing (assuming that goes well) if you don't.

[ThrawnCA]

It's not really been a priority at work. Should be pretty quick to do, though. I'll let you know by close of business tomorrow.

[ThrawnCA]

@markstuart Sorry about the delay. I've made the ages configurable. Preventing login CSRF would be a separate piece of work; the token format can be updated if and when doing that.

[ThrawnCA]

Merged latest master changes.

[markstuart]

Hi @ThrawnCA, I haven't had a huge amount of experience with this CSRF implementation and I'd be really interested to hear how you would recommend testing whether these changes are working as expected? I'd appreciate your input on that so that we can get this merged.

[ThrawnCA]

Well, for testing that it blocks attacks, you could try creating a standalone HTML file that posts to the site, and verify that it's blocked with a 403 error for lacking a CSRF token. Then add an arbitrary token and verify that it's blocked for not having the signature match up. Get a token for one person's session, add that to the form, try posting while logged in as someone else, it should fail (so the attacker can't use their own token to attack your account).

To verify that it hasn't broken anything, you basically need to try all of the site functions. Javascript-based interactions like removing a member from an organization are particular targets for special handling.

Someone recently suggested that it doesn't work properly with newer Flask-based interactions; I haven't had the chance to properly investigate that. I don't think it was breaking the pages, though, just not protecting them.

[ThrawnCA]

@markstuart We've done some further development on this filter, eg making it compatible with CKAN 2.9, and the results are now in a standalone extension at https://github.com/qld-gov-au/ckanext-csrf-filter

I'd suggest incorporating the latest changes from that one, or else dropping the feature from ckanext-security and just installing the standalone extension alongside it.

[markstuart]

@ThrawnCA we've decided to drop CSRF protection as part of making it CKAN 2.9 compatible and advise users of this extension to use https://github.com/qld-gov-au/ckanext-csrf-filter going forward. Closing this PR, thanks for your contribution!