pallets-eco / flask-session

Server side session extension for Flask
https://flask-session.readthedocs.io
BSD 3-Clause "New" or "Revised" License
501 stars 239 forks source link

Added support for signer alternatives #16

Closed twolfson closed 6 months ago

twolfson commented 9 years ago

We are interested in using flask-session as our session system but noticed a lack of support for a SHA256 HMAC. In the past, I have been informed that this is stronger than the current default of a SHA1 HMAC (mostly due to length -- SHA1 is 160 bits, SHA256 is 256 bits).

https://en.wikipedia.org/wiki/Hash-based_message_authentication_code#Security

This PR adds backwards compatible support to opt in to using a SHA256 HMAC. In this PR:

twolfson commented 8 years ago

Is there anything blocking this PR outside of merge conflicts?

fengsp commented 8 years ago

Maybe we should add support for customizing a signer, which will make everyone happy. We can provide a config key SESSION_SIGNER, you can pass in any signer instance you want.

twolfson commented 8 years ago

Could you provide a more detailed example of what using that code would look like?

fengsp commented 8 years ago

app.config['SESSION_SIGNER'] = hmac_sha256_signer

twolfson commented 8 years ago

While that works, I would be opposed to everyone rewriting/reimplementing the same SHA256 signer. As with anything related to security, the algorithm can be great but someone can mess up implementation and ruin it (e.g. what if someone forgets the salt).

fengsp commented 8 years ago

We ship with the default one, mostly people wouldn't change it. If anyone does, there are endless possible demands, we cannot ship all of them, someone who changes the default behavior should be aware of what exactly he is doing.

twolfson commented 8 years ago

While I understand the concern for maintenance, it's pretty irresponsible for something as sensitive as handling user sessions. If someone can forge someone else's user session, then they have full access as that user. Can you elaborate on what your main concern is?

fengsp commented 8 years ago

There is no concern at all. Your solution solves your problem, not everyone's problem, it is that simple.

twolfson commented 8 years ago

Right but it sets up a framework for everyone to reuse signers. I imagine that someone in the future might want sha512 and this would allow for them to reuse that code by adding yet another function.

On the other hand, I don't foresee anyone ever wanting to roll their own signer fully as that violates the rule of not implementing custom security solutions.

fengsp commented 8 years ago

We can add one config SESSION_SIGNER_PARAMS, which should be the all kwargs that itsdangerous.Signer takes, then we check it and make sure every param is passed in. On the one hand, you can customize anything you want for any reason, on the other, everyone can use their own salt.

twolfson commented 8 years ago

That seems more restricting than the originally proposed SESSION_SIGNER parameter itself. The concept of having multiple built-in options and a custom SESSION_SIGNER override aren't mutually exclusive. How about supporting both a SESSION_SIGNER_TYPE (this PR) and SESSION_SIGNER (which fully overrides the signer)?

Lxstr commented 7 months ago

Intent is to deprecate signer from 0.7.0 #212. The relevant entropy should be provided by the session identifier length. Flask-session is for only serverside sessions so I can see no point in signing only the session id and expiry, that is effectively just making the sid longer, which can be ajusted in SESSION_SID_LENGTH

If anyone can put forward an alternative case I would like to hear it

Lxstr commented 6 months ago

Closing due to no activity and deprecation of signer.