On the session id type and overriding it's value

[and-reas-se]

Hi,

I've started implementing a store for tokio-postgres.

Internally tower-sessions session id is a i128, which altough it's behind a newtype, the inner value is accessible. We use UUID primary keys in our databases, which are also 128-bit. When create() is called by tower-sessions I insert the record letting the database generate a uuid primary key, which I then in rust convert to a i128 and replace the session id with. That way when load() or save() is called I can convert the session id to a UUID again and query the database using the primary key.

This approach should avoid the id collision issue, and reduces the complexity of the database table. But I noted the todo comment in tower-sessions-core/session.rs:876 mentioning that the store being able to override the id is undesireable.

Should I expect this approach to not be compatible with future releases of tower-session? Why is it undesirable to let the store implementer pick the session id? I thought it was a rather neat way to do it.

[maxcountryman]

Why is it undesirable to let the store implementer pick the session id?

Because there's no guarantee that an ID will be securely generated. Put another way: any i128 may be used.

letting the database generate a uuid primary key

As in this case, it entirely depends on the secure nature of the database's UUID generation method: UUIDv4 does not require its random bits be securely generated and so this will depend on the specific implementation.

Whereas calling Id::default provides a strong cryptographic guarantee that the ID is secure in nature (i.e. via rand).

In the interest of promoting better security, we'll likely make this field private and only available via an accessor in the future.

[arcstur]

It seems most stores in tower-sessions-stores implement the create method by generating a new random Id over and over until it is available. I'm using a SQLx store and using the IDs as it is right now is not good for the index. Since they are random, new session Id's could be all over the place, and, in my situation, I would like for the session id to be a uuid v7 for example. I don't know about the security implications of this. Maybe "Id" could be an associated type of the SessionStore trait?

[maxcountryman]

Do not use UUIDv7 for session IDs: these IDs are sequential, a la ULID, and not at all secure. An attacker will be able to easily guess your session IDs and this is a complete failure of the security of the system.

This is part of why I don't think it's a good idea to make the ID easily configurable because currently the concrete type provides a guarantee about the secure nature of session IDs.

[arcstur]

Ohhh, okay, that makes sense. Yeah, maybe it's best to not make it easily configurable. Maybe we could add a phrase or so in the Secure nature of cookies section of the documentation, explaning why implementors should use the crate implementation of Id::default.

Maybe that loop and Id::default should be the default create? Or explaning why that is a good idea in tower-sessions documentation.