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

Having a static default for cookieSecret is bad #2880

Closed
molomby opened this issue May 4, 2020 · 1 comment · Fixed by #2882
Closed

Having a static default for cookieSecret is bad #2880

molomby opened this issue May 4, 2020 · 1 comment · Fixed by #2882

Comments

@molomby
Copy link
Member

molomby commented May 4, 2020

Problem Description

Currently, if no cookieSecret config is supplied to the Keystone constructor, it defaults to qwerty. This happens in regardless of the NODE_ENV and no warning is printed communicating to the developer that an unsafe default is being used. About the only protection around this currently is that several mentions in the docs that the default should be overridden before deployment.

Needless to say, this is not at all secure by default. I'd argue that, even in dev, using a publicly known cookie secret is unacceptable for anything other than toy apps.

There is a tradeoff between security and the initial developer experience here -- It's be nice if people could get a Keystone running without messing around too long with env vars. Regardless, the current solution does not shepherd good behaviour.

Proposed Solution

We may want to implement different behaviour in production and dev environments.

When not in production (NODE_ENV !== 'production'), I suggest that if no cookieSecret is supplied, Keystone should log a warning to the console with instructions on how to remedy the problem. In the meantime, Keystone's should generate a secure, random but temporary value to use. This will have the effect of resetting all sessions when the Keystone thread is restarted (since a new secret will be used) but the console warning can call this out specifically. Eg..

WARNING: The Keystone constructor is being called without a cookieSecret value. Please generate a secure value and add it to your app. Until this is done, a random cookieSecret will be generated each time Keystone is started. This will cause sessions to be reset between restarts. See [https://www.keystonejs.com/keystonejs/keystone/#config] for details.

When running in production (NODE_ENV === 'production') I'd suggest we either use this same behaviour ^^ or maybe even go further and not start at all. That is, it probably shouldn't be possible to start a Keystone instance in production mode without this value being supplied.

Using a secure-but-random value in production is secure, but runs the risk of causing not-so-obvious issues that some will confuse some devs. Eg. sessions being reset between restarts or, if multiple servers/threads are used, sessions that aren't recognised by different app instances.

By simply not starting it makes the issue (and the solution) impossible to ignore -- an error calling it out whats happening and how to fix it will be the last thing logged before the app exits. Eg...

ERROR: The cookieSecret config is required when Keystone is run in a production environment. Update your app or environment config so this value is supplied to the Keystone constructor. See [https://www.keystonejs.com/keystonejs/keystone/#config] for details.

(This is all contingent on they Keystone instance actually using sessions, etc.)

@molomby
Copy link
Member Author

molomby commented Jul 13, 2020

There is a tradeoff between security and the initial developer experience [...]

Case in point: #3247.

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 a pull request may close this issue.

1 participant