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

Why is secret required? #193

Closed
2 tasks done
youfoundKim opened this issue Apr 1, 2023 · 4 comments
Closed
2 tasks done

Why is secret required? #193

youfoundKim opened this issue Apr 1, 2023 · 4 comments

Comments

@youfoundKim
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the issue has not already been raised

Issue

Why is options.secret required?

I can understand signing a JWT or some other kind of stateless token, but what's the point in signing a completely random string of characters?

The current OWASP recommendation for session IDs is:

  • The session ID length must be at least 128 bits (16 bytes).
  • The session ID value must provide at least 64 bits of entropy.

Source

You only need to sign the cookie when the cookie itself holds state and therefore isn't random. That's why JWTs are signed. They hold state, such as the user id and roles. But a session ID is usually just a random string of characters, such as a uuid or randomBytes -> toString. That kind of data doesn't need a signature.

I think it would be a better solution to remove the options.secret requirement so users who are using randomely generated session IDs are not signing the cookie for no reason. And if some people want to store a stateless token in the cookie with this library, they can set a secret for cookie signing.

Thanks

@mcollina
Copy link
Member

mcollina commented Apr 2, 2023

My understanding is that signing cookies is necessary to prevent some attacks.m, e.g. cookie tossing.

leftieFriele added a commit to leftieFriele/csrf-protection that referenced this issue Aug 30, 2023
As mentioned in fastify/session#193, "secret" is now required by @fastify/session
@rolanday
Copy link

Secret is required, but, similar to @fastify/cookie, the prop optionally accepts mapping for sign and unsigned methods so you can write out whatever you like, including clear. Good thing, because @fastify/session would be useless to me otherwise as I use fastify as backend for frontend, where frontend is remix and I need to be able to read the cookie from remix server-side. Naturally, I encrypt the cookie.

@jshear
Copy link

jshear commented Nov 21, 2023

For anyone looking for more detailed justification, there's a good breakdown of the key benefits of signing session IDs Here.

Namely, the security of the system should not depend on the session ID generator being properly random and secure. Adding a signature removes the risk associated with predictable session IDs from a faulty generator.

Another (non-essential) benefit is that it can significantly reduce lookups during a DOS attack by validating issuance and expiration as part of the signed session ID itself before bothering to look up the session ID in the store.

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 21, 2023

thank you

@Uzlopak Uzlopak closed this as not planned Won't fix, can't repro, duplicate, stale Nov 21, 2023
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

No branches or pull requests

5 participants