Fixed #20081 -- Minimized risk of SECRET_KEY leak. #2714

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
@aj-may

aj-may commented May 26, 2014

By default set the SECRET_KEY value to be loaded from env vars. If not
set and DEBUG==True generate a random SECRET_KEY. Also adds the
generatesecret management command, which prints a random secret key to
stdout.

@PaulMcMillan

This comment has been minimized.

Show comment
Hide comment
@PaulMcMillan

PaulMcMillan May 27, 2014

Contributor

https://code.djangoproject.com/ticket/20081

As a general principle, I'm opposed to this approach. Moving the secret key into an environment variable instead of a file doesn't really reduce the odds of it leaking (it may increase them - env variables have historically been leaky, and still must be stored somewhere on-disk in a user-accessible file in order to persist across reboots).

The root of the patch (using os.getenv()) is trivial to implement if a user prefers to inject the secret key that way.

Your patch generates a new secret key whenever debug=True. This means that sessions no longer work across dev server restarts, meaning a developer has to re-login to their app every time a file changes - not a good first user experience.

Furthermore, your patch doesn't address backwards compatibility and documentation.

If you reimplement this storing the secret key in a file rather than as an environment variable, and address the issues raised above, I think this will be a good addition.

Contributor

PaulMcMillan commented May 27, 2014

https://code.djangoproject.com/ticket/20081

As a general principle, I'm opposed to this approach. Moving the secret key into an environment variable instead of a file doesn't really reduce the odds of it leaking (it may increase them - env variables have historically been leaky, and still must be stored somewhere on-disk in a user-accessible file in order to persist across reboots).

The root of the patch (using os.getenv()) is trivial to implement if a user prefers to inject the secret key that way.

Your patch generates a new secret key whenever debug=True. This means that sessions no longer work across dev server restarts, meaning a developer has to re-login to their app every time a file changes - not a good first user experience.

Furthermore, your patch doesn't address backwards compatibility and documentation.

If you reimplement this storing the secret key in a file rather than as an environment variable, and address the issues raised above, I think this will be a good addition.

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may May 27, 2014

@PaulMcMillan
First off I really appreciate the feedback. I did not yet address backwards compatibility and documentation because I wanted to get feedback on this approach first.

I agree that env variables can be leaked just as easily as a file if set up poorly. However, I also see that there are many tools and deployment options available now that help the user do this correctly. The biggest issue with the SECRET_KEY leaking is that everyone is committing their settings.py file with the SECRET_KEY to their repos. Obviously this is a very bad practice.

A google search for "where should I keep my Django secret key" turns up many results pushing this same approach. ENV vars for configuration is also heavily pushed by Heroku and is outlined in The 12 Factor App - http://12factor.net/

Also, so I fully understand your ideal implementation, where would this file be located, what would it be called, what format would it be in, how would it be read in, and what would the server do if the file could not be found?

I am in complete agreement that not keeping sessions across dev server restarts would be really annoying, and should be modified to store the secret key at least semi-permanently. I will work on coming up with a new solution for that.

aj-may commented May 27, 2014

@PaulMcMillan
First off I really appreciate the feedback. I did not yet address backwards compatibility and documentation because I wanted to get feedback on this approach first.

I agree that env variables can be leaked just as easily as a file if set up poorly. However, I also see that there are many tools and deployment options available now that help the user do this correctly. The biggest issue with the SECRET_KEY leaking is that everyone is committing their settings.py file with the SECRET_KEY to their repos. Obviously this is a very bad practice.

A google search for "where should I keep my Django secret key" turns up many results pushing this same approach. ENV vars for configuration is also heavily pushed by Heroku and is outlined in The 12 Factor App - http://12factor.net/

Also, so I fully understand your ideal implementation, where would this file be located, what would it be called, what format would it be in, how would it be read in, and what would the server do if the file could not be found?

I am in complete agreement that not keeping sessions across dev server restarts would be really annoying, and should be modified to store the secret key at least semi-permanently. I will work on coming up with a new solution for that.

@aj-may

This comment has been minimized.

Show comment
Hide comment
@PaulMcMillan

This comment has been minimized.

Show comment
Hide comment
@PaulMcMillan

PaulMcMillan May 27, 2014

Contributor

While it's true that you can't commit a password which is stored only in an environment variable, you can commit the file that's storing that variable so that it gets restored when you reboot the server, which boils down to the same problem.

Environment variables work well with Heroku's model because the container your application runs within is explicitly not multi-tenant - you can't see anyone else's processes. We can't make that assertion generally about Django deployments.

I don't have strong feelings about where the file is located, and what it is called. It should be created with 0700 permissions so that other users can't read it. Part of the reason I haven't patched this myself is that it's not immediately obvious where the file should live - in proper deployments, the webserver only has the capability to write to a few selected directories. It may not be possible to generally know that in advance. This is one problem Horizon's code has - it makes it easy for a new user to get started, but is manifestly inappropriate for many deployment scenarious.

I'd suggest getting input from other developers, since I'm not sure.

Contributor

PaulMcMillan commented May 27, 2014

While it's true that you can't commit a password which is stored only in an environment variable, you can commit the file that's storing that variable so that it gets restored when you reboot the server, which boils down to the same problem.

Environment variables work well with Heroku's model because the container your application runs within is explicitly not multi-tenant - you can't see anyone else's processes. We can't make that assertion generally about Django deployments.

I don't have strong feelings about where the file is located, and what it is called. It should be created with 0700 permissions so that other users can't read it. Part of the reason I haven't patched this myself is that it's not immediately obvious where the file should live - in proper deployments, the webserver only has the capability to write to a few selected directories. It may not be possible to generally know that in advance. This is one problem Horizon's code has - it makes it easy for a new user to get started, but is manifestly inappropriate for many deployment scenarious.

I'd suggest getting input from other developers, since I'm not sure.

@PaulMcMillan

This comment has been minimized.

Show comment
Hide comment
@PaulMcMillan

PaulMcMillan May 27, 2014

Contributor

Perhaps the solution is to try to write the auto-generated secret key into a file the same directory as the settings file. If that's not writable, fail with an easy-to-debug error.

It might make sense to discuss including some default VCS ignore files for that directory explicitly preventing the secret key from being committed.

Contributor

PaulMcMillan commented May 27, 2014

Perhaps the solution is to try to write the auto-generated secret key into a file the same directory as the settings file. If that's not writable, fail with an easy-to-debug error.

It might make sense to discuss including some default VCS ignore files for that directory explicitly preventing the secret key from being committed.

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may May 28, 2014

@PaulMcMillan
That was going to be my next question.. If its just a file, It will be just as easy to commit to a repo.

It might make sense to discuss including some default VCS ignore files for that directory explicitly preventing the secret key from being committed.

I think I can work with that.

aj-may commented May 28, 2014

@PaulMcMillan
That was going to be my next question.. If its just a file, It will be just as easy to commit to a repo.

It might make sense to discuss including some default VCS ignore files for that directory explicitly preventing the secret key from being committed.

I think I can work with that.

@PaulMcMillan

This comment has been minimized.

Show comment
Hide comment
@PaulMcMillan

PaulMcMillan May 28, 2014

Contributor

If its just a file, It will be just as easy to commit to a repo.

Yeah, and there's nothing we can do to prevent that. Any deployment scenario using environment variables also must also store them in some file (which can be easily committed to a repo).

Contributor

PaulMcMillan commented May 28, 2014

If its just a file, It will be just as easy to commit to a repo.

Yeah, and there's nothing we can do to prevent that. Any deployment scenario using environment variables also must also store them in some file (which can be easily committed to a repo).

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may May 28, 2014

I deploy a lot of sites to heroku/dokku. I store all config in env vars and none of them are stored in a file. They are only stored on the application servers and on heroku compiled into the application "slug".

I agree that most use cases a secret key file would be a better option. I am working on an update th the pull request.

aj-may commented May 28, 2014

I deploy a lot of sites to heroku/dokku. I store all config in env vars and none of them are stored in a file. They are only stored on the application servers and on heroku compiled into the application "slug".

I agree that most use cases a secret key file would be a better option. I am working on an update th the pull request.

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may May 28, 2014

@PaulMcMillan

How does this look?

aj-may commented May 28, 2014

@PaulMcMillan

How does this look?

@PaulMcMillan

View changes

django/conf/__init__.py
@@ -111,7 +111,7 @@ def __init__(self, settings_module):
setattr(self, setting, setting_value)
self._explicit_settings.add(setting)
- if not self.SECRET_KEY:
+ if not self.SECRET_KEY and not self.DEBUG:

This comment has been minimized.

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

I still don't want to allow an empty secret key in DEBUG mode. For better or worse, people deploy real sites in debug mode, and if they use the pickle cookie deserializer with signed cookies, that would be a very bad thing.

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

I still don't want to allow an empty secret key in DEBUG mode. For better or worse, people deploy real sites in debug mode, and if they use the pickle cookie deserializer with signed cookies, that would be a very bad thing.

This comment has been minimized.

@aj-may

aj-may May 29, 2014

That makes sense. My idea was that a developer running the dev server shouldn't have to do anything to get working, but generating a secret key file if one doesn't exist achieves the same result.

@aj-may

aj-may May 29, 2014

That makes sense. My idea was that a developer running the dev server shouldn't have to do anything to get working, but generating a secret key file if one doesn't exist achieves the same result.

@PaulMcMillan

View changes

django/conf/project_template/project_name/settings.py
TEMPLATE_DEBUG = True
+# SECURITY WARNING: keep the secret key used in production secret!
+from django.utils.crypto import read_secret_Key_file
+SECRET_KEY = read_secret_Key_file()

This comment has been minimized.

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

Why the capital K in this function? That seems odd...

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

Why the capital K in this function? That seems odd...

This comment has been minimized.

@aj-may

aj-may May 29, 2014

typo.. must have copied and pasted

@aj-may

aj-may May 29, 2014

typo.. must have copied and pasted

@PaulMcMillan

View changes

django/core/management/commands/generatesecret.py
+ requires_system_checks = False
+
+ def handle(self, **options):
+ self.stdout.write(generate_secret_key())

This comment has been minimized.

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

Hmmm... This seems like a duplicate with the generatesecretfile comand - do you think it's necessary?

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

Hmmm... This seems like a duplicate with the generatesecretfile comand - do you think it's necessary?

This comment has been minimized.

@aj-may

aj-may May 29, 2014

I think it belongs here. I would like to have this utility available for anyone who may not want to use a secret key file (eg. env vars, third party config management package, etc)

$ ./manage.py generatesecret | pbcopy

- or -

$ export SECRET_KEY="$(./manage.py generatesecret)"

- or -

$ heroku config:set SECRET_KEY="$(./manage.py generatesecret)"
@aj-may

aj-may May 29, 2014

I think it belongs here. I would like to have this utility available for anyone who may not want to use a secret key file (eg. env vars, third party config management package, etc)

$ ./manage.py generatesecret | pbcopy

- or -

$ export SECRET_KEY="$(./manage.py generatesecret)"

- or -

$ heroku config:set SECRET_KEY="$(./manage.py generatesecret)"

This comment has been minimized.

@PaulMcMillan

PaulMcMillan Jun 4, 2014

Contributor

vs. say, being able to pass - to generate secret as the file path to return the value to stdout? That's a pretty common idiom...

@PaulMcMillan

PaulMcMillan Jun 4, 2014

Contributor

vs. say, being able to pass - to generate secret as the file path to return the value to stdout? That's a pretty common idiom...

This comment has been minimized.

@dstufft

dstufft Jun 4, 2014

Member

I agree with Paul, it'd be nicer to juse have generatesecret and use - to send to stdout.

@dstufft

dstufft Jun 4, 2014

Member

I agree with Paul, it'd be nicer to juse have generatesecret and use - to send to stdout.

@PaulMcMillan

View changes

django/utils/crypto.py
+ except IOError:
+ return None
+
+

This comment has been minimized.

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

These functions don't really belong in the "crypto" file. I'm sure there's somewhere more appropriate for them...

generate_secret_key could maybe live here, but the others definitely shouldn't.

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

These functions don't really belong in the "crypto" file. I'm sure there's somewhere more appropriate for them...

generate_secret_key could maybe live here, but the others definitely shouldn't.

This comment has been minimized.

@aj-may

aj-may May 29, 2014

agreed

@PaulMcMillan

View changes

django/utils/crypto.py
+ os.chmod(path, 0600)
+
+
+def read_secret_Key_file(path=None):

This comment has been minimized.

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

Let's not capitalize Key in this function

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

Let's not capitalize Key in this function

@PaulMcMillan

View changes

django/utils/crypto.py
+ with open(path, "r") as file:
+ return file.read()
+ except IOError:
+ return None

This comment has been minimized.

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

I'm not sure I like returning None. Is there a good reason not to rename this function read_or_create_secret_key_file() and have it do just that? If it can't read, or create and read, maybe it should raise an exception?

@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

I'm not sure I like returning None. Is there a good reason not to rename this function read_or_create_secret_key_file() and have it do just that? If it can't read, or create and read, maybe it should raise an exception?

This comment has been minimized.

@aj-may

aj-may May 29, 2014

agreed

@PaulMcMillan

This comment has been minimized.

Show comment
Hide comment
@PaulMcMillan

PaulMcMillan May 29, 2014

Contributor

I left a few comments, please argue with me about them ;)

Contributor

PaulMcMillan commented May 29, 2014

I left a few comments, please argue with me about them ;)

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may May 29, 2014

@PaulMcMillan
updated

aj-may commented May 29, 2014

@PaulMcMillan
updated

@PaulMcMillan

This comment has been minimized.

Show comment
Hide comment
@PaulMcMillan

PaulMcMillan Jun 4, 2014

Contributor

can I get @dstufft or @alex to weigh in as a second voice on the security implications of this?

Other than my latest comments, it looks pretty good to me.

Contributor

PaulMcMillan commented Jun 4, 2014

can I get @dstufft or @alex to weigh in as a second voice on the security implications of this?

Other than my latest comments, it looks pretty good to me.

@alex

View changes

django/utils/secret_key.py
+
+ with open(path, "w") as file:
+ file.write(key)
+ os.chmod(path, 0600)

This comment has been minimized.

@alex

alex Jun 4, 2014

Member

The file should really be created with the correct mode, so it isn't readable in between.

@alex

alex Jun 4, 2014

Member

The file should really be created with the correct mode, so it isn't readable in between.

This comment has been minimized.

@dstufft

dstufft Jun 4, 2014

Member

Even that's not enough. Doing this in Python is annoying.

import os

def _secure_open_write(filename, fmode):
    # We only want to write to this file, so open it in write only mode
    flags = os.O_WRONLY

    # os.O_CREAT | os.O_EXCL will fail if the file already exists, so we only
    # will open *new* files.
    # We specify this because we want to ensure that the mode we pass is the
    # mode of the file.
    flags |= os.O_CREAT | os.O_EXCL

    # Do not follow symlinks to prevent someone from making a symlink that
    # we follow and insecurely open a file.
    if hasattr(os, "O_NOFOLLOW"):
        flags |= os.O_NOFOLLOW

    # On Windows we'll mark this file as binary
    if hasattr(os, "O_BINARY"):
        flags |= os.O_BINARY

    # Before we open our file, we want to delete any existing file that is
    # there
    try:
        os.remove(filename)
    except (IOError, OSError):
        # The file must not exist already, so we can just skip ahead to opening
        pass

    # Open our file, the use of os.O_CREAT | os.O_EXCL will ensure that if a
    # race condition happens between the os.remove and this line, that an
    # error will be raised.
    fd = os.open(filename, flags, fmode)
    try:
        return os.fdopen(fd, "wb")
    except:
        # An error occurred wrapping our FD in a file object
        os.close(fd)
        raise

That snippet is something I wrote in another project, but it gives the basic idea. You can use that the same way you'd use open() (e.g. in a context manager or not). I'm not sure if we'd want the binary mode on Windows for this.. probably not.

@dstufft

dstufft Jun 4, 2014

Member

Even that's not enough. Doing this in Python is annoying.

import os

def _secure_open_write(filename, fmode):
    # We only want to write to this file, so open it in write only mode
    flags = os.O_WRONLY

    # os.O_CREAT | os.O_EXCL will fail if the file already exists, so we only
    # will open *new* files.
    # We specify this because we want to ensure that the mode we pass is the
    # mode of the file.
    flags |= os.O_CREAT | os.O_EXCL

    # Do not follow symlinks to prevent someone from making a symlink that
    # we follow and insecurely open a file.
    if hasattr(os, "O_NOFOLLOW"):
        flags |= os.O_NOFOLLOW

    # On Windows we'll mark this file as binary
    if hasattr(os, "O_BINARY"):
        flags |= os.O_BINARY

    # Before we open our file, we want to delete any existing file that is
    # there
    try:
        os.remove(filename)
    except (IOError, OSError):
        # The file must not exist already, so we can just skip ahead to opening
        pass

    # Open our file, the use of os.O_CREAT | os.O_EXCL will ensure that if a
    # race condition happens between the os.remove and this line, that an
    # error will be raised.
    fd = os.open(filename, flags, fmode)
    try:
        return os.fdopen(fd, "wb")
    except:
        # An error occurred wrapping our FD in a file object
        os.close(fd)
        raise

That snippet is something I wrote in another project, but it gives the basic idea. You can use that the same way you'd use open() (e.g. in a context manager or not). I'm not sure if we'd want the binary mode on Windows for this.. probably not.

@alex

View changes

django/utils/secret_key.py
+ if path is None:
+ path = '.secret_key'
+
+ try:

This comment has been minimized.

@alex

alex Jun 4, 2014

Member

Should this reject files which are world writable?

@alex

alex Jun 4, 2014

Member

Should this reject files which are world writable?

@alex

This comment has been minimized.

Show comment
Hide comment
@alex

alex Jun 4, 2014

Member

This definitely needs docs before this is landable, this is introducing a totally new convention.

Member

alex commented Jun 4, 2014

This definitely needs docs before this is landable, this is introducing a totally new convention.

@dstufft

View changes

django/utils/secret_key.py
+
+ try:
+ with open(path, "r") as file:
+ return file.read()

This comment has been minimized.

@dstufft

dstufft Jun 4, 2014

Member

This should refuse to a secret key file that doesn't have safe permissions IMO. Much like how ssh won't use a public or private key with unsafe permissions.

@dstufft

dstufft Jun 4, 2014

Member

This should refuse to a secret key file that doesn't have safe permissions IMO. Much like how ssh won't use a public or private key with unsafe permissions.

@dstufft

This comment has been minimized.

Show comment
Hide comment
@dstufft

dstufft Jun 4, 2014

Member

I'm going to follow up on the ticket for discussion about the idea in general.

Member

dstufft commented Jun 4, 2014

I'm going to follow up on the ticket for discussion about the idea in general.

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may Jun 13, 2014

@alex @dstufft @PaulMcMillan

Updated. Still no docs. I want to flesh out these ideas first. I appreciate the feedback.

aj-may commented Jun 13, 2014

@alex @dstufft @PaulMcMillan

Updated. Still no docs. I want to flesh out these ideas first. I appreciate the feedback.

@Vanuan

This comment has been minimized.

Show comment
Hide comment
@Vanuan

Vanuan Jun 23, 2014

But if you deploy to Heroku, you'll still need to check in this file, right? This way you'll have a separate codebase in production, which isn't great.

Vanuan commented Jun 23, 2014

But if you deploy to Heroku, you'll still need to check in this file, right? This way you'll have a separate codebase in production, which isn't great.

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may Jun 23, 2014

@Vanuan Heroku users will have to continue to follow Heroku's directions on using environment variables. The SECRET_KEY setting still exists as to not force the use of a secret key file.

Maybe to make it easier to switch between the two I can add a commented out line in the settings file using env vars. What do you think?

aj-may commented Jun 23, 2014

@Vanuan Heroku users will have to continue to follow Heroku's directions on using environment variables. The SECRET_KEY setting still exists as to not force the use of a secret key file.

Maybe to make it easier to switch between the two I can add a commented out line in the settings file using env vars. What do you think?

@Vanuan

This comment has been minimized.

Show comment
Hide comment
@Vanuan

Vanuan Jun 23, 2014

Yeah, it's a way better to have this as a default

SECRET_KEY = os.environ['SECRET_KEY']

What do you think about Rails approach: secret key is defined in a separate file (example: config/initializers/secret_token.rb). This way you can get advantages of both approaches, i.e. you can use a separate file, which you can check in or not. If you prefer not checking it in, you can use a command to generate it. If you want to have it in a repo, you can set ENV['SECRET_TOKEN'] there.

Vanuan commented Jun 23, 2014

Yeah, it's a way better to have this as a default

SECRET_KEY = os.environ['SECRET_KEY']

What do you think about Rails approach: secret key is defined in a separate file (example: config/initializers/secret_token.rb). This way you can get advantages of both approaches, i.e. you can use a separate file, which you can check in or not. If you prefer not checking it in, you can use a command to generate it. If you want to have it in a repo, you can set ENV['SECRET_TOKEN'] there.

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may Jun 23, 2014

@Vanuan My preference is also to use environment variables, but for many Django installations that could be insecure.

This is why I chose to make the default a file, but leave the user the option to load in a secret key however they choose.

aj-may commented Jun 23, 2014

@Vanuan My preference is also to use environment variables, but for many Django installations that could be insecure.

This is why I chose to make the default a file, but leave the user the option to load in a secret key however they choose.

@dbrgn

View changes

django/conf/__init__.py
+
+ try:
+ with open(self.SECRET_KEY_FILE, 'r') as file:
+ self.SECRET_KEY = file.read()

This comment has been minimized.

@dbrgn

dbrgn Nov 15, 2014

Contributor

Would a .strip() make sense here?

@dbrgn

dbrgn Nov 15, 2014

Contributor

Would a .strip() make sense here?

This comment has been minimized.

@aj-may

aj-may Nov 17, 2014

I'm not sure. My initial thought is that the entire file, whitespace and all should be set as the SECRET_KEY. However, I do see how the current implementation could lead to unexpected results. I'd like to see if anyone else has thoughts on this.

@aj-may

aj-may Nov 17, 2014

I'm not sure. My initial thought is that the entire file, whitespace and all should be set as the SECRET_KEY. However, I do see how the current implementation could lead to unexpected results. I'd like to see if anyone else has thoughts on this.

This comment has been minimized.

@carljm

carljm Nov 17, 2014

Member

I think the value should be stripped. This will still respect "internal" whitespace, but will remove leading and trailing whitespace. The most important reason for this, IMO, is that if you have SECRET_KEY = 'foo' and you move it to a file, and your editor automatically adds a final newline (which many editors do), that should not result in a subtly different secret key.

@carljm

carljm Nov 17, 2014

Member

I think the value should be stripped. This will still respect "internal" whitespace, but will remove leading and trailing whitespace. The most important reason for this, IMO, is that if you have SECRET_KEY = 'foo' and you move it to a file, and your editor automatically adds a final newline (which many editors do), that should not result in a subtly different secret key.

This comment has been minimized.

@aj-may

aj-may Nov 17, 2014

@carljm good point. I'm convinced. I'll add a .strip()

@aj-may

aj-may Nov 17, 2014

@carljm good point. I'm convinced. I'll add a .strip()

@dbrgn

View changes

django/utils/secret_key.py
+ os.close(fd)
+ raise
+ except:
+ raise

This comment has been minimized.

@dbrgn

dbrgn Nov 15, 2014

Contributor

The except: raise is redundant, right?

@dbrgn

dbrgn Nov 15, 2014

Contributor

The except: raise is redundant, right?

This comment has been minimized.

@aj-may

aj-may Nov 17, 2014

Yea. I think that was left over from some earlier revision. I'll remove that.

@aj-may

aj-may Nov 17, 2014

Yea. I think that was left over from some earlier revision. I'll remove that.

@dbrgn

This comment has been minimized.

Show comment
Hide comment
@dbrgn

dbrgn Nov 15, 2014

Contributor

Maybe the FilePermissionError could also be integrated into the 1.7 checks framework?

Contributor

dbrgn commented Nov 15, 2014

Maybe the FilePermissionError could also be integrated into the 1.7 checks framework?

@berkerpeksag

View changes

django/core/exceptions.py
@@ -71,6 +71,11 @@ class ImproperlyConfigured(Exception):
pass
+class FilePermissionError(Exception):

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

Python 3.3+ has PermissionError https://docs.python.org/3/library/exceptions.html#PermissionError We can use it directly, or subclass it. On Python 2 and Python 3.2, FilePermissionError can be a subclass of OSError.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

Python 3.3+ has PermissionError https://docs.python.org/3/library/exceptions.html#PermissionError We can use it directly, or subclass it. On Python 2 and Python 3.2, FilePermissionError can be a subclass of OSError.

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

If we need a new exception, it should be documented in https://docs.djangoproject.com/en/dev/ref/exceptions/

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

If we need a new exception, it should be documented in https://docs.djangoproject.com/en/dev/ref/exceptions/

This comment has been minimized.

@aj-may

aj-may Jan 18, 2015

I'm not sure the description of PermissionError or OSError make sense here.

@aj-may

aj-may Jan 18, 2015

I'm not sure the description of PermissionError or OSError make sense here.

@berkerpeksag

View changes

django/core/management/commands/generatesecret.py
+ try:
+ output_file = output_file[0]
+ except IndexError:
+ raise CommandError("You must specify a file path.")

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

I'd move this logic to the add_arguments method: https://docs.djangoproject.com/en/dev/howto/custom-management-commands/

@berkerpeksag

View changes

django/core/management/commands/generatesecret.py
+import os
+
+
+class Command(BaseCommand):

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

This command needs documentation updates.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

This command needs documentation updates.

@@ -169,6 +170,10 @@ def handle(self, app_or_project, name, target=None, **options):
"probably using an uncommon filesystem setup. No "
"problem." % new_path, self.style.NOTICE)
+ # If we are creating a project, generate the .secret_key file.
+ if app_or_project == 'project':
+ create_secret_key_file(os.path.join(top_dir, '.secret_key'))

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

.secret_key should also be documented.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

.secret_key should also be documented.

This comment has been minimized.

@aj-may

aj-may Feb 3, 2015

Documented!

@aj-may

aj-may Feb 3, 2015

Documented!

@@ -17,11 +17,10 @@
# See https://docs.djangoproject.com/en/{{ docs_version }}/howto/deployment/checklist/
# SECURITY WARNING: keep the secret key used in production secret!
-SECRET_KEY = '{{ secret_key }}'

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

Shouldn't this be deprecated first?

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

Shouldn't this be deprecated first?

This comment has been minimized.

@aj-may

aj-may Jan 18, 2015

I'm not replacing SECRET_KEY, im introducing a new option SECRET_KEY_FILE, and making that the default. Those who wish to continue to use SECRET_KEY may do so.

@aj-may

aj-may Jan 18, 2015

I'm not replacing SECRET_KEY, im introducing a new option SECRET_KEY_FILE, and making that the default. Those who wish to continue to use SECRET_KEY may do so.

@@ -105,6 +105,21 @@ def __init__(self, settings_module):
setattr(self, setting, setting_value)
self._explicit_settings.add(setting)
+ if self.SECRET_KEY_FILE and not self.SECRET_KEY:

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

If I already have SECRET_KEY and I want to migrate to use SECRET_KEY_FILE, this won't work, right? Raising a warning or perhaps ImproperlyConfigured would be good.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

If I already have SECRET_KEY and I want to migrate to use SECRET_KEY_FILE, this won't work, right? Raising a warning or perhaps ImproperlyConfigured would be good.

This comment has been minimized.

@aj-may

aj-may Jan 18, 2015

How it is currently implemented, if both SECRET_KEY and SECRET_KEY_FILE are set, SECRET_KEY wins. Should this be handled differently? @berkerpeksag

@aj-may

aj-may Jan 18, 2015

How it is currently implemented, if both SECRET_KEY and SECRET_KEY_FILE are set, SECRET_KEY wins. Should this be handled differently? @berkerpeksag

This comment has been minimized.

@PaulMcMillan

PaulMcMillan Jan 19, 2015

Contributor

I think having SECRET_KEY win is probably a reasonable choice here. The directory shouldn't be writable, but I'd hate to see an attack where an attacker's .secret_key file was written into the dir and then suddenly magically worked.

@PaulMcMillan

PaulMcMillan Jan 19, 2015

Contributor

I think having SECRET_KEY win is probably a reasonable choice here. The directory shouldn't be writable, but I'd hate to see an attack where an attacker's .secret_key file was written into the dir and then suddenly magically worked.

@berkerpeksag

View changes

django/conf/__init__.py
+ # Verify that file permissions are set to be
+ # only user readable and writable.
+ mode = os.stat(self.SECRET_KEY_FILE).st_mode
+ if bool((s.S_IRGRP | s.S_IWGRP | s.S_IXGRP | s.S_IROTH | s.S_IWOTH | s.S_IXOTH) & mode):

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

You can move this snippet into django/utils/secret_key.py.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

You can move this snippet into django/utils/secret_key.py.

This comment has been minimized.

@aj-may

aj-may Jan 19, 2015

@berkerpeksag This code is never used outside of django.conf so I feel that moving it into django/utils/secret_key.py will just add unnecessary complexity

@aj-may

aj-may Jan 19, 2015

@berkerpeksag This code is never used outside of django.conf so I feel that moving it into django/utils/secret_key.py will just add unnecessary complexity

@berkerpeksag

View changes

django/core/management/commands/generatesecret.py
+
+ if output_file == '-':
+ self.stdout.write(generate_secret_key())
+ return

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

I think this command can be used in deployment scripts. It would be good to add proper sys.exit() calls.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

I think this command can be used in deployment scripts. It would be good to add proper sys.exit() calls.

This comment has been minimized.

@aj-may

aj-may Jan 19, 2015

@berkerpeksag Updated. Does that look better, or should there be exits elsewhere?

@aj-may

aj-may Jan 19, 2015

@berkerpeksag Updated. Does that look better, or should there be exits elsewhere?

@berkerpeksag

View changes

django/conf/__init__.py
@@ -105,6 +105,21 @@ def __init__(self, settings_module):
setattr(self, setting, setting_value)
self._explicit_settings.add(setting)
+ if self.SECRET_KEY_FILE and not self.SECRET_KEY:
+ import stat as s

This comment has been minimized.

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

import stat

@berkerpeksag

berkerpeksag Jan 16, 2015

Contributor

import stat

@collinanderson

View changes

django/conf/__init__.py
+ # only user readable and writable.
+ mode = os.stat(self.SECRET_KEY_FILE).st_mode
+ if bool((stat.S_IRGRP | stat.S_IWGRP | stat.S_IXGRP | stat.S_IROTH | stat.S_IWOTH | stat.S_IXOTH) & mode):
+ raise FilePermissionError("The SECRET_KEY_FILE permissions are not secure. Set the file permissions to be only user readable and writable.")

This comment has been minimized.

@collinanderson

collinanderson Jan 19, 2015

Contributor

Personally, this would be too strict for my situation. I would need group read and write. Generating the file with user-only permissions would be fine for me.

@collinanderson

collinanderson Jan 19, 2015

Contributor

Personally, this would be too strict for my situation. I would need group read and write. Generating the file with user-only permissions would be fine for me.

This comment has been minimized.

@aj-may

aj-may Jan 19, 2015

@PaulMcMillan any opinions on this?

@aj-may

aj-may Jan 19, 2015

@PaulMcMillan any opinions on this?

This comment has been minimized.

@aj-may

aj-may Jan 19, 2015

@collinanderson @PaulMcMillan Updated to allow group perms. Still open to discussion.

@aj-may

aj-may Jan 19, 2015

@collinanderson @PaulMcMillan Updated to allow group perms. Still open to discussion.

@collinanderson

View changes

django/core/management/commands/generatesecret.py
+
+ def handle(self, *output_file, **options):
+ try:
+ output_file = output_file[0]

This comment has been minimized.

@collinanderson

collinanderson Jan 19, 2015

Contributor

Could output_file default to settings.SECRET_KEY_FILE?

@collinanderson

collinanderson Jan 19, 2015

Contributor

Could output_file default to settings.SECRET_KEY_FILE?

This comment has been minimized.

@aj-may

aj-may Jan 19, 2015

That would make a lot of sense. Thanks for the idea.

@aj-may

aj-may Jan 19, 2015

That would make a lot of sense. Thanks for the idea.

A.J. May added some commits May 26, 2014

+ mode = os.stat(self.SECRET_KEY_FILE).st_mode
+ if bool((stat.S_IROTH | stat.S_IWOTH | stat.S_IXOTH) & mode):
+ raise InsecureFilePermissionError("The SECRET_KEY_FILE permissions are not secure. Set the file permissions to be only user readable and writable.")
+
if not self.SECRET_KEY:
raise ImproperlyConfigured("The SECRET_KEY setting must not be empty.")

This comment has been minimized.

@aaugustin

aaugustin Feb 4, 2015

Member

This exception message is inaccurate if SECRET_KEY_FILE is set but points to an empty file.

@aaugustin

aaugustin Feb 4, 2015

Member

This exception message is inaccurate if SECRET_KEY_FILE is set but points to an empty file.

@@ -460,6 +461,10 @@ answer newbie questions, and generally made Django that much better:
Matt Riggott
Matt Robenolt <m@robenolt.com>
mattycakes@gmail.com
+ Glenn Maynard <glenn@zewt.org>

This comment has been minimized.

@timgraham

timgraham Apr 16, 2015

Member

chop these

@timgraham

timgraham Apr 16, 2015

Member

chop these

+ raise ImproperlyConfigured("Unable to read SECRET_KEY_FILE: %s" % self.SECRET_KEY_FILE)
+
+ # Verify that file permissions are not other readable/writable
+ import stat

This comment has been minimized.

@timgraham

timgraham Apr 16, 2015

Member

move import to top of file

@timgraham

timgraham Apr 16, 2015

Member

move import to top of file

# SECURITY WARNING: don't run with debug turned on in production!
DEBUG = True
-

This comment has been minimized.

@timgraham

timgraham Apr 16, 2015

Member

please revert whitespace change

@timgraham

timgraham Apr 16, 2015

Member

please revert whitespace change

@@ -73,6 +73,11 @@ class ImproperlyConfigured(Exception):
pass
+class InsecureFilePermissionError(Exception):

This comment has been minimized.

@timgraham

timgraham Apr 16, 2015

Member

Not sure a custom exception adds much value. How about something like RuntimeError with an informative message?

@timgraham

timgraham Apr 16, 2015

Member

Not sure a custom exception adds much value. How about something like RuntimeError with an informative message?

+from django.core.management.base import BaseCommand, CommandError
+from django.utils.secret_key import generate_secret_key, create_secret_key_file
+
+import os

This comment has been minimized.

@timgraham

timgraham Apr 16, 2015

Member

please rebase and check import order with isort

@timgraham

timgraham Apr 16, 2015

Member

please rebase and check import order with isort

+.. setting:: SECRET_KEY_FILE
+
+SECRET_KEY_FILE
+----------

This comment has been minimized.

@timgraham

timgraham Apr 16, 2015

Member

heading is too short, please check docs build without errors
also please add the setting to the topic index on this page

@timgraham

timgraham Apr 16, 2015

Member

heading is too short, please check docs build without errors
also please add the setting to the topic index on this page

+
+Default: ``None``
+
+As an alternative to specifying a :setting:`SECRET_KEY` in your settings file, Django can load your secret key from a file.

This comment has been minimized.

@timgraham

timgraham Apr 16, 2015

Member

Please wrap at 79 characters

@timgraham

timgraham Apr 16, 2015

Member

Please wrap at 79 characters

+SECRET_KEY_FILE
+----------
+
+.. versionadded:: 1.8

This comment has been minimized.

@timgraham

timgraham Apr 16, 2015

Member

1.9 now (and should be added to the release notes)

@timgraham

timgraham Apr 16, 2015

Member

1.9 now (and should be added to the release notes)

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Apr 16, 2015

Member

I think it would be helpful to summarize the discussion about this on the django-developers mailing list to ensure everyone is okay with the approach.

Member

timgraham commented Apr 16, 2015

I think it would be helpful to summarize the discussion about this on the django-developers mailing list to ensure everyone is okay with the approach.

@aj-may

This comment has been minimized.

Show comment
Hide comment
@aj-may

aj-may Apr 27, 2015

Sorry for the delay. I will try and update this PR this week.

aj-may commented Apr 27, 2015

Sorry for the delay. I will try and update this PR this week.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Jul 2, 2015

Member

Closing due to lack of activity. Please send a new PR if you get around to updating it, thanks!

Member

timgraham commented Jul 2, 2015

Closing due to lack of activity. Please send a new PR if you get around to updating it, thanks!

@timgraham timgraham closed this Jul 2, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment