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

Session improvements: re-keying, signing, laziness, GC, etc. #13

Open
4 of 17 tasks
aantron opened this issue Apr 8, 2021 · 4 comments
Open
4 of 17 tasks

Session improvements: re-keying, signing, laziness, GC, etc. #13

aantron opened this issue Apr 8, 2021 · 4 comments

Comments

@aantron
Copy link
Owner

aantron commented Apr 8, 2021

  • Session re-keying
  • Sign session tokens to reduce database access by random client-generated tokens.
  • Version the token format for transparent upgrades of the session token scheme.
  • Garbage collection.
  • Lazy sessions. There is no need to persist state for a pre-session that hasn't had any data associated with it, except to reserve the key. Is that important? This can be addressed by making the session key getter evaluate to a promise. However, that would infect the CSRF generator, template helper, etc. It may be better for it to always return a promise, since then one could implement reads by always reading from persistent storage. However, at that point, someone may want a custom session manager, anyway. There is no need for duplicate checking, as a ~144-bit session ID collision, assuming a good RNG, is not worth protecting against, even for a service with billions of monthly visits, assuming each one generates a fresh session, and all the sessions are concurrent. A service with stricter requirements still, should be easily able to implement a checking session id generator.
  • Optionally preserve session data across invalidation.
  • Session reload from persistent storage for synchronization? Do we need data generation numbers, etc., to detect concurrent modifications? Or is this best left to custom session managers?
  • Expose the session back end-making interface.
  • Cross-invalidation, e.g. on login from another device.
  • Make sure client IP "locking" is easy to implement. Create an example.
  • Consider key length carefully.
  • Rename session_key to session_id, because ID is the common term for it, and the term key collides with usage in cryptographic key, for example keys in TLS. OTOH the session key is generated in about the same way, using a CSRNG, so maybe this is fine. NIST uses session secret, but session identifier remains in one place.
  • Is it safe to just print the session ID to logs? No. OWASP recommends against this. Dream already implements a separate log-safe identifier, which should probably become a hash so that there is at least a 1-way correlation.
  • Should the session ID be hashed when storing in a database?
  • Log session lookups, etc., at level `Info.
  • Derive the label from the key by hashing; in fact, this can just be what is stored in the database.
  • Remember me as an option, i.e. don't use a 2-week lifetime by default.
@aantron aantron changed the title Session re-keying Session improvements: re-keying, signing, laziness, GC, etc. Apr 12, 2021
aantron added a commit that referenced this issue Apr 13, 2021
aantron added a commit that referenced this issue Apr 13, 2021
aantron added a commit that referenced this issue Apr 15, 2021
aantron added a commit that referenced this issue Apr 15, 2021
@aantron aantron added this to the alpha3 milestone May 11, 2021
@cemerick
Copy link

cemerick commented Dec 7, 2021

I was surprised to see Dream storing empty sessions. I thought it was probably just a low-priority TODO, but there's a "Lazy sessions" item here, with a stricken section that (I think?) implies that there's some value in persisting empty sessions? Can you clarify what's going on there?

@aantron
Copy link
Owner Author

aantron commented Dec 7, 2021

I wasn't able to come to a decision in the summer about whether the existence of the session is itself important, even if the session is empty. So I chose for now that if a request passes through the session middleware, it always gets a session, even if nothing is stored in it. This allows, at least, recognizing subsequent requests as belonging to the same session. I'm very open to discussing whether this is right or not.

@cemerick
Copy link

cemerick commented Dec 7, 2021

Seems there are two (IMO) good reasons not to create sessions until they're necessary:

  1. In practice, it will result in a (maybe heavy) slurry of junk dream_session records in the database / redis / whatever. Not great, not terrible (but also, why?)
  2. More problematic is that always issuing session cookies means one can't (easily) make an app be cookie-free until login (a helpful attribute vis a vis GDPR compliance, etc).

@aantron
Copy link
Owner Author

aantron commented Dec 7, 2021

Seems there are two (IMO) good reasons not to create sessions until they're necessary:

  1. In practice, it will result in a (maybe heavy) slurry of junk dream_session records in the database / redis / whatever. Not great, not terrible (but also, why?)
  2. More problematic is that always issuing session cookies means one can't (easily) make an app be cookie-free until login (a helpful attribute vis a vis GDPR compliance, etc).

Indeed, I completely agree, and now I recall that I was considering these issues as well. And the "Lazy sessions" bullet point is still open, to be addressed. That part of it is stricken is confusing. It is confusing even me at this point, as a reader of this issue months later :P But your comment serves as a good clarification, thank you!

probably just a low-priority TODO

This whole issue is fairly high-priority. However, I'm personally currently working out details and quirks of Dream I/O streams and a Dream client, so I have to delay looking at sessions in detail again until I've taken care of those two things.

@aantron aantron removed this from the alpha3 milestone Feb 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants