-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
fix: Change default SECRET_KEY, improve docs and banner warning on de… #17984
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is awesome! Bonus points for telling the user how to generate the key in a safe way. :)
Codecov Report
@@ Coverage Diff @@
## master #17984 +/- ##
==========================================
- Coverage 67.10% 66.87% -0.24%
==========================================
Files 1612 1612
Lines 64992 65061 +69
Branches 6871 6871
==========================================
- Hits 43615 43511 -104
- Misses 19510 19683 +173
Partials 1867 1867
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
" you generate \n" | ||
"a sufficiently random sequence, ex: openssl rand -base64 42" | ||
) | ||
logger.warning(bottom_banner) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this happens should we do a sys.exit
call as well so the server doesn't even start up?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I evaluated something similar, but I'm avoiding break pip installations and docker. It's really nice to have really fast and frictionless first installations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change LGTM with minor nit. However, since this will break every single installation that is currently using the default key, I would prefer to either make CHANGE_ME_SECRET_KEY
equal to the old default SECRET_KEY
, or add a note to UPDATING.md
. Otherwise I fear we will be swampled by questions on both GitHub and Slack from users wondering why their devenvs on master (hopefully not prod envs!) have stopped working.
Co-authored-by: Ville Brofeldt <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the rotation script, looks awesome!
Loved the instructions on how to generate a new key 👏🏼 |
I believe this change broke this tutorial for running Superset through Docker, running in dev mode. I was working on this issue when this MR's breaking change hit me. What do you recommend doing? Update the tutorial, suggesting running the |
* fix: Change default SECRET_KEY, improve docs and banner warning on default * lint * Update superset/initialization/__init__.py Co-authored-by: Ville Brofeldt <[email protected]> * add a secret migration procedure, update UPDATING * fix lint Co-authored-by: Ville Brofeldt <[email protected]>
…he#17984) * fix: Change default SECRET_KEY, improve docs and banner warning on default * lint * Update superset/initialization/__init__.py Co-authored-by: Ville Brofeldt <[email protected]> * add a secret migration procedure, update UPDATING * fix lint Co-authored-by: Ville Brofeldt <[email protected]>
…he#17984) * fix: Change default SECRET_KEY, improve docs and banner warning on default * lint * Update superset/initialization/__init__.py Co-authored-by: Ville Brofeldt <[email protected]> * add a secret migration procedure, update UPDATING * fix lint Co-authored-by: Ville Brofeldt <[email protected]>
SUMMARY
Simple change to better alert our users when the default
SECRET_KEY
has not been changed.When the default SECRET_KEY is detected, superset will log:
The default
SECRET_KEY
changes to a more standard default, making it more obvious that a change is needed also.This can break current installations that were relying on the previous default
SECRET_KEY
I believe this is reasonable, since this is a dangerous setting and a proper setting is imperative.This PR also adds a cli command to help user's migrate their current secrets. They have to set
PREVIOUS_SECRET_KEY
with their old SECRET and set their new secret withSECRET_KEY
then runsuperset re-encrypt-secrets
. Props to @craig-rueda since this script is highly based (adapted) to some work he has done.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION