maxcountryman / tower-sessions

🥠 Sessions as a `tower` and `axum` middleware.
MIT License
249 stars 43 forks source link

Exchange UUID for nanoid #141

Closed nul-reference closed 9 months ago

nul-reference commented 9 months ago

UUIDv4 has 122 random bits, with 6 fixed bits for identifying the format. Additionally, it uses hexadecimal encoding, which makes it take more bandwidth to transfer.

This patch replaces UUIDv4 with an array of bytes derived from the nanoid crate, which gives 66 bits of entropy. This exceeds the minimum recommended by OWASP. Additionally, the byte array is encoded as URL-safe Base64 for usage as a string.

It should be noted that nanoid also uses StdRng from the rand crate, utilizing a ChaCha12 block cypher for a rather strong CSPRNG, removing most of the need to be wary of platform support.

Documentation has been updated everywhere I could find, and tests have passed from what I can tell. (Also have a build of this working utilizing axum-login in a project of mine.)

nul-reference commented 9 months ago

Also while OWASP mandates at least 64 bits of entropy, we're effectively cutting the UUID entropy in half with this change;

As nanoid represents values in effectively URL-safe base64, the value represented by 22 characters is 132 bits long, leaving the entropy at 66 bit, from my understanding. UUIDv4, as a 128 bit value minus 6 fixed bits, has 61 bits of entropy.

Granted, the search space for either is almost certainly within “good enough” space for almost all applications, but imho the smaller size both in bandwidth mixed with higher entropy is nice.

It might be worth considering to add a trait for Id with a default implementation, now that I think of it, so any Id can be used depending on backend and application requirements.

Unrelated, but thank you for the great ecosystem of crates!

maxcountryman commented 9 months ago

That makes sense. I'm comfortable moving to nanoid given that. If applications were to want more entropy for whatever reason we could make the length configurable I think or we could have an Id trait like you mentioned. Either way, I don't think that blocks us from moving forward with this change.

codecov[bot] commented 9 months ago

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (9dea07b) 74.23% compared to head (bc831a9) 74.28%.

Files Patch % Lines
tower-sessions-core/src/session.rs 87.50% 1 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #141 +/- ## ========================================== + Coverage 74.23% 74.28% +0.05% ========================================== Files 12 12 Lines 520 525 +5 ========================================== + Hits 386 390 +4 - Misses 134 135 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.