actix-extras icon indicating copy to clipboard operation
actix-extras copied to clipboard

customizable session key generator

Open wt opened this issue 11 months ago • 6 comments

The OWASP guidelines (the same linked from the comment on generate_session_key) suggest that session ids should be 64 bits long, not 64 characters as implemented in generate_session_key. If you represent a 64 bit long integer as a hexidecimal number, it is only 16 characters long instead of 64.

Is there any chance that you might consider the following replacement for generate_session_key?

fn generate_session_key() -> SessionKey {
    let key: u64 = rand::rng().random();
    let key_str = format!("{:x}", key);
    key_str.try_into().unwrap()
}

This would allow session keys to be far shorter while still complying with the OWASP guidelines. These shorter ids would take less space in storage as well. This could be really beneficial to sites with large numbers of sessions.

wt avatar Jan 21 '25 05:01 wt

I updated the code to be compliant with rust 2024 edition and rand 0.9.

wt avatar Jan 31 '25 00:01 wt

FWIW, I would like the add an option to use the above implementation as a default for the session key generation, selectable via a feature. In the future, I would like to make this the default implementation under the default features, maybe during an appropriate semver bump. It's not a different interface to the existing function, so it seems like a minor version bump would be enough for that change. And maybe remove the old session key generation function during a major version bump.

Also, I am happy to write the PR for this if I can get your support.

wt avatar Feb 01 '25 04:02 wt

To address the original point: As SessionKey is a wrapper over a string, not bytes, makes the generated session key more accessible for the storage backends. The thing we give up is entropy per byte, since we're generating alphanumeric session keys, which are using only 62 out of the 255 possible byte patterns (so about a quarter), we're having to generate longer IDs, by a factor of 4, than we would with fully random data.

While yes, even with that logic, the IDs are still longer than necessary according to OWASP, it's better to have secure defaults when it comes to a library like this.

robjtede avatar Mar 11 '25 03:03 robjtede

Providing customisable session key generation is probably quite easy to work into this library, I'll consider this a feature request issue for that.

robjtede avatar Mar 11 '25 03:03 robjtede

The suggested implementation is also generating a string. It gets 64 bits of entropy (as suggested in the OWASP doc) and serializes that to a utf8 string with hex characters so that it can still be stored in the DB without a lot of magic.

If the OWASP recommendations are secure, I think the recommendation is.

FWIW, a configuration would be nice, but I don't think it addresses the concern here, which is that the current implementation goes far beyond the OWASP recommendations and requires storing much longer session ids (4x what the suggested implementation required, which as you noted, could probably be further optimized, if desired).

wt avatar Mar 11 '25 06:03 wt

OWASP only says:

Session identifiers must have at least 64 bits of entropy

It's a load-bearing "at least", which is satisfied by our current generator. And when they change their recommendation to "at least 128 bits" in the future, we'll be ready 😆

robjtede avatar Mar 11 '25 12:03 robjtede