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

Add support for encrypted sessions with CryptX #2212

Merged
merged 1 commit into from
Nov 22, 2024
Merged

Conversation

kraih
Copy link
Member

@kraih kraih commented Nov 20, 2024

SUSE Hack Week has started, so this is my first proposal for sessions with encrypted cookies. I've tried to stay as close to the mojo.js implementation as possible. To address #2200 i've also added a Mojo::Util::generate_secret function.

The feature is only available if CryptX is installed, and needs to be enabled with $app->sessions->encrypted(1).

@kraih
Copy link
Member Author

kraih commented Nov 20, 2024

As an alternative i will now look into using JWT for the rest of Hack Week.

lib/Mojolicious/Sessions.pm Outdated Show resolved Hide resolved
@kraih kraih force-pushed the encrypted_sessions branch 6 times, most recently from 96c2835 to 02b6199 Compare November 20, 2024 19:24
Copy link

@latk latk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The encryption part looks reasonably sound. 👍

AES-GCM is a slightly dated but reasonable choice and seems to be used correctly here. Alternatively, the more modern ChaCha20-Poly1305 could be used.

However, the signed cookie mechanism remains insecure, so the fixes in #2200 would remain valuable.

Also, the unnecessary repeated invocations of PBKDF2 are a major performance problem. This value should likely be cached at application startup.

(PBKDF2 itself is no longer a state of the art key derivation function, and the default cost parameter of 5000 is a 2005-era security choice. But its use here seems acceptable as we only need key-stretching, not cracking-resistant password hashes.)

lib/Mojo/Util.pm Outdated Show resolved Hide resolved
@kraih
Copy link
Member Author

kraih commented Nov 20, 2024

I've switched to ChaCha20-Poly1305.

@kraih
Copy link
Member Author

kraih commented Nov 21, 2024

My experiments with JWT today have been rather disappointing. Primary focus for most modules seems to be JWS and not JWE, and i've run into more than a few inconsistencies and errors. Also the cookies end up much larger for the simple user login case, which i consider the most common use.

So i expect this PR to end up as my main proposal for a new sessions implementation.

@kraih kraih merged commit 1bcbacf into main Nov 22, 2024
21 checks passed
Copy link
Member

@jberger jberger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit late but 👍

@guest20
Copy link

guest20 commented Nov 22, 2024

Will this be able to automatically upgrade existing, signed but non-encrypted sessions or does everybody have to log in again?

@kraih
Copy link
Member Author

kraih commented Nov 23, 2024

Everybody has to log in again (once it has been manually enabled for the app).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants