Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

customizable session key generator #497

Open
wt opened this issue Jan 21, 2025 · 6 comments
Open

customizable session key generator #497

wt opened this issue Jan 21, 2025 · 6 comments
Labels
A-session Project: actix-session C-improvement Category: an improvement to existing functionality good first issue Good for newcomers

Comments

@wt
Copy link
Contributor

wt commented Jan 21, 2025

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
Copy link
Contributor Author

wt commented Jan 31, 2025

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

@wt
Copy link
Contributor Author

wt commented Feb 1, 2025

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.

@robjtede robjtede added C-improvement Category: an improvement to existing functionality A-session Project: actix-session labels Feb 28, 2025
@robjtede
Copy link
Member

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
Copy link
Member

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 robjtede added the good first issue Good for newcomers label Mar 11, 2025
@robjtede robjtede changed the title actix-session: generate_session_key seems to generate overly long ids actix-session: customizable session key generator Mar 11, 2025
@robjtede robjtede changed the title actix-session: customizable session key generator customizable session key generator Mar 11, 2025
@wt
Copy link
Contributor Author

wt commented Mar 11, 2025

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).

@robjtede
Copy link
Member

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 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-session Project: actix-session C-improvement Category: an improvement to existing functionality good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

2 participants