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

Generate a secret key file, and load SECRET_KEY from said file. #4494

Merged
merged 1 commit into from
Sep 25, 2015

Conversation

aronasorman
Copy link
Collaborator

Fixes #4213. We already have said vulnerability wherein the SECRETT_KEY is both public and the same for each installation. With the introduction of sessions-in-cookies it's a double whammy.

So now the solution is to create a secret key file for each installation if it's not detected, and load that as the value of SECRET_KEY.

Assigning @jamalex, but @benjaoming please take a look too.

@@ -314,8 +314,22 @@
USE_L10N = getattr(local_settings, "USE_L10N", False)

# Make this unique, and don't share it with anybody.
SECRET_KEY = getattr(local_settings, "SECRET_KEY",
"8qq-!fa$92i=s1gjjitd&%s@4%ka9lj+=@n7a&fzjpwu%3kd#u")
SECRET_KEY_FILE = os.path.join(USER_DATA_ROOT, "secretkey.txt")

This comment was marked as spam.

This comment was marked as spam.

@benjaoming
Copy link
Contributor

Why the backport for 0.14?

@benjaoming
Copy link
Contributor

Sorry, why is this not a backport but straight for 0.14?

@aronasorman
Copy link
Collaborator Author

re: backport -- I just plan to cherry pick this single commit to 0.15. Unless you have other preferred method of propagating this change?

@aronasorman
Copy link
Collaborator Author

from django.conf import settings
>>> settings.SECRET_KEY
'MjFiOTJmYzYtNGYyYS00MWViLTk5ZmMtZTUxZGU3NGU1NTZl'

f.flush()

if not os.path.exists(SECRET_KEY_FILE):
generate_secret_key()

This comment was marked as spam.

This comment was marked as spam.

@jamalex
Copy link
Member

jamalex commented Sep 24, 2015

Looks good on my side, but made one suggestion for robustness/pythonicness.

f.write(key)
f.flush()

if not os.path.exists(SECRET_KEY_FILE):

This comment was marked as spam.

This comment was marked as spam.

@aronasorman aronasorman force-pushed the 4213 branch 2 times, most recently from 084f69e to b608baa Compare September 25, 2015 01:11
@aronasorman
Copy link
Collaborator Author

@jamalex @benjaoming comments addressed. Main changes:

  • moved secret key functions to its own file. it's under kalite/project/settings/_utils.py.
  • changed the way we detect SECRET_KEY_FILE to catching exceptions and then generating secret keys from that.
  • saving the secret key is optional. If we can't save it, then we just keep generating a new one every server restart. This will mean that users are logged out every restart, which @EdDixon wants. Might wanna add a flag for that moving forward.

@aronasorman
Copy link
Collaborator Author

@benjaoming added sys.stderr.write calls.

@jamalex
Copy link
Member

jamalex commented Sep 25, 2015

Looks good to me -- might be good to test 1 or 2 weird edge cases before merging.

@aronasorman
Copy link
Collaborator Author

Tried with secretkey.txt as a directory. It prints out this in stderr, then continues:

In the future, it is recommended not to keep your own settings module in the kalite code base but to put the file somewhere else in your python path, for instance in the current directory when running 'kalite --settings=my_module'.
  RemovedInKALite_v015_Warning
Error reading secret key file. Generating one now. Error was: [Errno 21] Is a directory: '/home/aron/src/github.com/aronasorman/ka-lite/secretkey.txt'
Error writing secret key file. Error was: [Errno 21] Is a directory: '/home/aron/src/github.com/aronasorman/ka-lite/secretkey.txt'

It's expected behaviour.

aronasorman added a commit that referenced this pull request Sep 25, 2015
Generate a secret key file, and load SECRET_KEY from said file.
@aronasorman aronasorman merged commit 0818d65 into learningequality:0.14.x Sep 25, 2015
@aronasorman aronasorman deleted the 4213 branch September 25, 2015 21:57
@aronasorman aronasorman mentioned this pull request Sep 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants