integrating django-secure

[Tim Graham]

I've started tackling one of the ideas that's been on our GSoC ideas
page for a couple years now: integrating django-secure. I chatted with
Carl about the idea and he's onboard. There are a couple of design
decisions we'll need to make.

1. How to integrate django-secure with the checks framework
django-secure essentially implements its own checks framework (which
predates the one in Django). The tricky part is that django-secure's
checks are not ones that generally should pass on a
development instance; they're checks that only make sense to run on a
production server (or at least against a production settings file).
I'm thinking to have some way to skip these new checks by default and
run them only when requested (e.g. manage.py check secure
--settings=prod_settings). Other options include an entirely separate
command like django-secure implements (curently called checksecure),
but perhaps could be called checkdeploy and eventually extended with
other checks that are relevant only in production. Idea/insight from
those more familiar with the checks framework (Chris, Russ), would be
welcome.

2. How to add settings for django-secure
As discussed in the thread below, I'm going to explore developing an
API for storing settings on an AppConfig to avoid adding more global
settings.
https://groups.google.com/d/topic/django-developers/qnnCLppwA3o/discussion

I have imported django-secure into django.contrib.secure and started
work on integrating it with the built-in checks framework as well as
removing some bits of it that have since been added to Django
(frame-deny, SSL-proxy support).

Work in progress: https://github.com/django/django/pull/3128

Thanks for your feedback!

[Russell Keith-Magee]

Hi Tim,

On Thu, Aug 28, 2014 at 3:35 AM, Tim Graham <timog...@gmail.com> wrote:

I've started tackling one of the ideas that's been on our GSoC ideas
page for a couple years now: integrating django-secure. I chatted with
Carl about the idea and he's onboard. There are a couple of design
decisions we'll need to make.

+1 to the idea. It was part of Chris' original proposal; we just didn't get around to it with the available time.

 

1. How to integrate django-secure with the checks framework
django-secure essentially implements its own checks framework (which
predates the one in Django). The tricky part is that django-secure's
checks are not ones that generally should pass on a
development instance; they're checks that only make sense to run on a
production server (or at least against a production settings file).
I'm thinking to have some way to skip these new checks by default and
run them only when requested (e.g. manage.py check secure
--settings=prod_settings). Other options include an entirely separate
command like django-secure implements (curently called checksecure),
but perhaps could be called checkdeploy and eventually extended with
other checks that are relevant only in production. Idea/insight from
those more familiar with the checks framework (Chris, Russ), would be
welcome.

Generally, I'd be opposed to the idea of Yet Another Command to run checks - if you make it optional, it won't get run, and this is something we want to be forced in front of everyone.

We use DEBUG as a proxy for "In Production" in other locations in the codebase - e.g., whether ALLOWED_HOSTS checks are run. If we were to supplement that with a --production=true/false flag to the checks command (to override the rare legitimate occasions where DEBUG=True in production, or DEBUG=False in development), wouldn't that meet all the needs here? 

2. How to add settings for django-secure
As discussed in the thread below, I'm going to explore developing an
API for storing settings on an AppConfig to avoid adding more global
settings.
https://groups.google.com/d/topic/django-developers/qnnCLppwA3o/discussion

I have imported django-secure into django.contrib.secure and started
work on integrating it with the built-in checks framework as well as
removing some bits of it that have since been added to Django
(frame-deny, SSL-proxy support).

 

Is it appropriate for this to be a contrib package? That implies it needs to be added to INSTALLED_APPS; I'm not convinced that this is a desirable approach. 

If django-secure is important enough to include as part of Django's codebase (and I fully agree it is), I'd consider security to be part of core, not something you can opt out of. Including it as django.core.checks.security (or similar) would make more sense to me.

 

Russ %-)

[Curtis Maloney]

For what it's worth, I agree with Russ.

Having security as an optional extra [which is how it will look to outsiders] is a bad look for Django, and certainly doesn't fit with the "Secure by default" philosophy.

--

Curtis

--
You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAJxq84_Tq1Fnm_pG4pMSy98DOLsnLtVp9jEgTo6X8%2BH%2BJYi1dQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Tim Graham]

After I wrote the original email, I found #17101 which is where the checkdeploy idea came from. We can just close that ticket (or modify it) if we decide on a different solution. It was created before the checks framework was merged and I agree a separate command may not be ideal, although it may make the implementation slightly easier. I'll look into using DEBUG tomorrow. One issue is that we don't want these checks run during testing (and DEBUG is often False there). Maybe if these checks are registered with something like checks.register(foo, deploy=True), we can skip any checks registered like that during testing. I'll have to see if there's some way to make this work with 'manage.py test' as well as with 3rd party test runners. If we had a separate checkdeploy command, avoiding this problem might be somewhat easier.

I am fine with putting it in core instead of contrib. That just means we need to figure out what to do about settings since we cannot put them on an AppConfig. Assuming we don't want to add them as normal settings, we may be able to use the approach proposed on this mailing list for the CSRF settings -- using attributes on the middleware class (PR). In that case, the check could work by iterating through MIIDDLEWARE_CLASSES until it finds a subclass of SecurityMiddleware and then check the attributes (settings) on that class. I will look into this approach tomorrow.

Thanks for the feedback!

[Aymeric Augustin]

Le 28 août 2014 à 03:25, Tim Graham <timog...@gmail.com> a écrit :

I am fine with putting it in core instead of contrib. That just means we need to figure out what to do about settings since we cannot put them on an AppConfig. Assuming we don't want to add them as normal settings, we may be able to use the approach proposed on this mailing list for the CSRF settings -- using attributes on the middleware class (PR). In that  could work by iterating through MIIDDLEWARE_CLASSES until it finds a subclass of SecurityMiddleware and then check the attributes (settings) on that class. I will look into this approach tomorrow.

As soon as we have something that is a global setting, I don't find it an improvement to hide it in an object instead of keeping it in plain sight in the settings.

Having to subclass a middleware just to change the lifetime of the CSRF cookie doesn't look like an improvement.

-- 

Aymeric.

[Russell Keith-Magee]

How many new settings are we talking about here? I know we've historically avoided adding new settings, but that's mostly as guidance to those proposing features where "and we'll add a setting to control when it takes effect". When we've actually *needed* a setting, we've never shied away from it. 

Personally - I don't see it as a problem to have a settings for the configuration of a specific behaviour (e.g., a timeout interval). It's when settings relate to behaviour switches or major configuration that I start to get twitchy :-)

Russ %-)

[Yo-Yo Ma]

+1 on basically adding the functionality of checksecure to the default validation.

-1 to adding the middleware.

[Tim Graham]

The settings for the SecurityMiddleware as they appear in django-secure are:

SECURE_HSTS_SECONDS=0

SECURE_HSTS_INCLUDE_SUBDOMAINS=False

SECURE_CONTENT_TYPE_NOSNIFF=False

SECURE_BROWSER_XSS_FILTER=False

SECURE_SSL_REDIRECT=False

SECURE_SSL_HOST=None

SECURE_REDIRECT_EXEMPT=[]

Yo-Yo, would be helpful to say *why* you are -1 so we can take that into consideration.

[Tim Graham]

I've implemented the ability to register "deployment checks" by adding deploy=True to register: @register("tag_name", deploy=True). These checks are only run if you pass the --deploy flag to check. So in development you can run `manage.py check --deploy --settings=settings_prod` to check your production settings file. Running these checks automatically if DEBUG is False would likely give them better visibility, but I don't see an easy way of disabling them when testing if we did that.

Regarding settings, would it be preferable to move them into a single dictionary setting called something like SECURITY_MIDDLEWARE_CONFIG?

[Florian Apolloner]

On Thursday, August 28, 2014 2:44:08 PM UTC+2, Tim Graham wrote:

Regarding settings, would it be preferable to move them into a single dictionary setting called something like SECURITY_MIDDLEWARE_CONFIG?

Definitely.

[Michael Manfre]

On Thu, Aug 28, 2014 at 8:44 AM, Tim Graham <timog...@gmail.com> wrote:

I've implemented the ability to register "deployment checks" by adding deploy=True to register: @register("tag_name", deploy=True). These checks are only run if you pass the --deploy flag to check. So in development you can run `manage.py check --deploy --settings=settings_prod` to check your production settings file. Running these checks automatically if DEBUG is False would likely give them better visibility, but I don't see an easy way of disabling them when testing if we did that.

Regarding settings, would it be preferable to move them into a single dictionary setting called something like SECURITY_MIDDLEWARE_CONFIG?

Yes. It is much nicer having a collection of related settings in a dict.

 

On Thursday, August 28, 2014 6:27:40 AM UTC-4, Tim Graham wrote:

The settings for the SecurityMiddleware as they appear in django-secure are:

SECURE_HSTS_SECONDS=0

SECURE_HSTS_INCLUDE_SUBDOMAINS=False

SECURE_CONTENT_TYPE_NOSNIFF=False

SECURE_BROWSER_XSS_FILTER=False

SECURE_SSL_REDIRECT=False

SECURE_SSL_HOST=None

SECURE_REDIRECT_EXEMPT=[]

Yo-Yo, would be helpful to say *why* you are -1 so we can take that into consideration.

-1 on having to subclass a middleware to define its settings. Doing that seems like the wrong approach and prevents having settings consolidated in a single place. This would make it more difficult to have different settings for different environments. This would likely result in a snippet that subclasses the middleware to look in settings.

 

On Thursday, August 28, 2014 2:45:07 AM UTC-4, Yo-Yo Ma wrote:

+1 on basically adding the functionality of checksecure to the default validation.

-1 to adding the middleware.

--

You received this message because you are subscribed to the Google Groups "Django developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.
Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/5069271f-8a21-4fdd-921f-ee2baa11b45b%40googlegroups.com.

[Carl Meyer]

On 08/28/2014 12:45 AM, Yo-Yo Ma wrote:
> +1 on basically adding the functionality of checksecure to the default validation.
>
> -1 to adding the middleware.

This doesn't make sense to me. Half the checks that checksecure performs
are related to whether you've turned on functionality from the
middleware. If you don't have the middleware, then you can't add those
checks either; how can you add a check for "do you have this setting set
which doesn't do anything?"

I do have some questions about how to "group" middleware; i.e. does it
make sense to have a single SecurityMiddleware (like that in
django-secure) that does a bunch of different things depending on
settings toggles? Or separate middleware for each individual feature,
following the lead of XFrameOptionsMiddleware (a django-secure feature
which has already been independently added to Django)? Or going the
other direction, just forget SecurityMiddleware and add all the various
togglable bits of functionality to CommonMiddleware?

I don't have a clear idea what the best approach is here, just raising
it as a question.

Thanks Tim for taking on this project!

Carl

[Carl Meyer]

On 08/28/2014 06:44 AM, Tim Graham wrote:
> I've implemented the ability to register "deployment checks" by adding
> deploy=True to register: @register("tag_name", deploy=True). These
> checks are only run if you pass the --deploy flag to check. So in
> development you can run `manage.py check --deploy
> --settings=settings_prod` to check your production settings file.
> Running these checks automatically if DEBUG is False would likely give
> them better visibility, but I don't see an easy way of disabling them
> when testing if we did that.

This makes sense to me. I don't like the fact that we assume in some
places that "DEBUG off means production", and I would not prefer to add
another such assumption here.

> Regarding settings, would it be preferable to move them into a single
> dictionary setting called something like SECURITY_MIDDLEWARE_CONFIG?

Sure.

FWIW, catching up on the thread, I fully agree with Russ that this
should be in core, not contrib, and I agree with Aymeric and Russ that
global config is better done via settings than via subclassing middleware.

Carl

[Tim Graham]

Thank-you all for the feedback. The PR now uses a dictionary for the settings. A couple drawbacks to this approach:

* It requires some logic in django/conf/__init__.py to allow users to override select keys in the setting while preserving the other defaults.

* The docs and error messages will have to be much more verbose (e.g. the 'HSTS_INCLUDE_SUBDOMAINS' key in SECURITY_MIDDLEWARE_CONFIG rather than simply 'SECURE_HSTS_INCLUDE_SUBDOMAINS').

There have been some other movements to consolidate settings into a dict (e.g. #22734 for email settings) so it seems like we prefer this approach despite the drawbacks.

As far as splitting up the middleware, I had similar thoughts as well. I don't see a need to split up SecurityMiddleware, although it does seem slightly inconsistent to have other middleware like XFrameOptionsMiddleware which could just as easily be combined in SecurityMiddleware (as it was in django-secure). Splitting it up would require adding more checks (one for each miiddleware) which I wouldn't be wild about.

[Aymeric Augustin]

On 29 août 2014, at 01:33, Tim Graham <timog...@gmail.com> wrote:

> There have been some other movements to consolidate settings into a dict (e.g. #22734 for email settings) so it seems like we prefer this approach despite the drawbacks.

Indeed, I’ve noticed that grouping settings that share a common prefix in a dict is often less convenient.

Advantages are limited:

- It improves the structure of the documentation.
- It ensures that related settings remain grouped together.
- It satisfies our craving for DRY.
- It artificially lowers len(dir(django.conf.global_settings)).

The two last items have especially little value.

The main drawback is that t’s much less practical to change a single value in that very common use case:

# myproject/settings/prod.py
from . base import *
FOO['BAR'] = 'baz'

If base.py doesn’t define FOO, you have to copy or import the default value from django.conf.global_settings. If it does, you can override some keys as shown above, but FOO_BAR = ‘baz’ is still easier.

If it weren’t for backwards compatibility, we could recursively merge dicts from user settings into defaults settings. For example https://github.com/django/django/pull/3138 achieves that in override_settings.

To me that PR looks like “let’s fix the problem for ourselves in the test suite and leave it for our users” :-/ If it’s inconvenient to alter settings in tests, it’s just as inconvenient to combine them in projets that have several settings modules.

EDIT: the PR has been merged while I was writing this email. I need to think and type faster :-)

Considering how many settings we’ve turned into dicts, I’m wondering if we should accept the consequences and implement the merging behavior. We’d have to make sure that setting a key to None is equivalent to not providing it at all. We could take this opportunity to review default values for settings, as we’ve already done in a few specific cases.

Do you think we could accept that level of backwards incompatibility ?

--
Aymeric.

[Florian Apolloner]

On Saturday, August 30, 2014 12:58:18 PM UTC+2, Aymeric Augustin wrote:

If it weren’t for backwards compatibility, we could recursively merge dicts from user settings into defaults settings. For example https://github.com/django/django/pull/3138 achieves that in override_settings.

And what would that give us? if I want override FOO['BAR'] where from would I override FOO with that merging behavior in place?

Considering how many settings we’ve turned into dicts, I’m wondering if we should accept the consequences and implement the merging behavior. We’d have to make sure that setting a key to None is equivalent to not providing it at all. We could take this opportunity to review default values for settings, as we’ve already done in a few specific cases.

Wondering if None is a good value or if it rather should be some sentinel object. That said since it only affects dicts, I think one usually doesn't have a value in the dict if usage isn't wanted, so None might be a good sentinel anyways.

Cheers,
Florian

[Aymeric Augustin]

On 30 août 2014, at 13:39, Florian Apolloner <f.apo...@gmail.com> wrote:

> And what would that give us? if I want override FOO['BAR'] where from would I override FOO with that merging behavior in place?

From your settings file.

--
Aymeric.

[Florian Apolloner]

Okay, so let's assume I have base.py and prod.py as settings (the latter star imports the first), prod.py wants to extend DATABASES from base.py or global_settings if it's not in base.py -- I don't see how that would work. Or is the merging just for the final prod.py and global_settings, in which case I have to say that there isn't much of a win.

Cheers,
Florian

[Tim Graham]

The merging is between the final user settings and global_settings. The advantage is not having to redeclare all the keys in a setting like:

SECURITY_MIDDLEWARE = {

    'HSTS_SECONDS': 0,

    'HSTS_INCLUDE_SUBDOMAINS': False,

    'CONTENT_TYPE_NOSNIFF': False,

    'BROWSER_XSS_FILTER': False,

    'SSL_REDIRECT': False,

    'SSL_HOST': None,

    'REDIRECT_EXEMPT': [],

}

if you only want to customize one of them.

If base.py adds CONTENT_TYPE_NOSNIFF and prod.py wants to add HSTS_SECONDS, it would look like this:

base.py

SECURITY_MIDDLEWARE = {'CONTENT_TYPE_NOSNIFF': True}

prod.py

from base import *

SECURITY_MIDDLEWARE['HSTS_SECONDS'] = 31536000

[Florian Apolloner]

On Saturday, August 30, 2014 2:48:10 PM UTC+2, Tim Graham wrote:

If base.py adds CONTENT_TYPE_NOSNIFF and prod.py wants to add HSTS_SECONDS, it would look like this: <snip>

Oh, that makes sense, I was so focused on "extending" that I didn't see that base.py could define a minimal dict itself. Though the example still assume that base.py defines SECURITY_MIDDLEWARE (although that is probably okay).

[Collin Anderson]

I still don't see the benefit to a dictionary. It's helpful for DATABASES and CACHES because there can be multiple of them.

> It ensures that related settings remain grouped together.

It seems to me a common SECURITY_ prefix should be good enough

- It satisfies our craving for DRY

A _tiny_ more DRY, but it's _more_ code in your settings.py. Who wants to type out all those quotes?

- It artificially lowers len(dir(django.conf.global_settings)).

How is this helpful?

[Aymeric Augustin]

On 30 août 2014, at 17:35, Collin Anderson <cmawe...@gmail.com> wrote:

> - It satisfies our craving for DRY
> A _tiny_ more DRY, but it's _more_ code in your settings.py. Who wants to type out all those quotes?
>
> - It artificially lowers len(dir(django.conf.global_settings)).
> How is this helpful?

These two arguments were not intended to be convincing ;-)

--
Aymeric.

[Carl Meyer]

I was initially indifferent about putting the django-secure settings in
a dictionary, but the more I've thought about the more I feel Collin is
correct; there is no real benefit (just an arguable sense of "tidiness")
and there are several negatives (more verbosity in documentation, check
warnings, and settings files, more complex to work with in
settings-override scenarios).

The django-secure settings are only loosely related to each other to
begin with, and there are several existing settings
(SECURE_PROXY_SSL_HEADER, X_FRAME_OPTIONS) which would also belong in
that conceptual grouping, leaving us with the choice of moving them
(needless backwards-compatibility churn) or not moving them (losing the
conceptual tidiness which is the only advantage of the dictionary
approach in the first place).

So after further reflection, I am -1 on putting the django-secure
settings into a dictionary.

Carl

> --
> You received this message because you are subscribed to the Google
> Groups "Django developers" group.
> To unsubscribe from this group and stop receiving emails from it, send
> an email to django-develop...@googlegroups.com

> <mailto:django-develop...@googlegroups.com>.

> To post to this group, send email to django-d...@googlegroups.com

> <mailto:django-d...@googlegroups.com>.

> Visit this group at http://groups.google.com/group/django-developers.
> To view this discussion on the web visit

> https://groups.google.com/d/msgid/django-developers/2b6dfad2-055a-4e56-a408-c0b04224f4cc%40googlegroups.com
> <https://groups.google.com/d/msgid/django-developers/2b6dfad2-055a-4e56-a408-c0b04224f4cc%40googlegroups.com?utm_medium=email&utm_source=footer>.

[Josh Smeaton]

-1 on using dictionaries to group somewhat related settings. Dicts make it much harder to override specific keys - and implementing a dict merge to get around a problem that we're creating ourselves for reasons of perceived attractiveness seems a little backwards. Mergingi

If I want to simply override one key:

SECURITY_MIDDLEWARE = {

    'HSTS_SECONDS': 10,

}

vs

SECURITY_MIDDLEWARE_HSTS_SECONDS = 10

I think the problem becomes more pronounced when you want to override a single sub-setting between your different environments:

# base.py

SECURITY_MIDDLEWARE = {

    'HSTS_SECONDS': 10,

    'HSTS_INCLUDE_SUBDOMAINS': True,

    'CONTENT_TYPE_NOSNIFF': True,

    'BROWSER_XSS_FILTER': True,

    'SSL_REDIRECT': False,

    'SSL_HOST': 'prod.my.host.com',

}

 

#staging.py

from base import * 

 

SECURITY_MIDDLEWARE = {

    'SSL_HOST': 'staging.my.host.com',

}

Does staging now represent the merged dicts of base and staging, or the merged dicts of default and staging? I believe, with the merged dict implementation, it is the merge of staging and default. Now if all of the settings were their own setting rather than an entry in a dict, I'd just set the single setting I'd need to change, and be done with it.

There are very little gains to using a dict, and I would argue it harms readability and the use of settings in general unless it's actually required.

[Michael Manfre]

On Sat, Aug 30, 2014 at 8:33 PM, Josh Smeaton <josh.s...@gmail.com> wrote:

I think the problem becomes more pronounced when you want to override a single sub-setting between your different environments:

# base.py

SECURITY_MIDDLEWARE = {

    'HSTS_SECONDS': 10,

    'HSTS_INCLUDE_SUBDOMAINS': True,

    'CONTENT_TYPE_NOSNIFF': True,

    'BROWSER_XSS_FILTER': True,

    'SSL_REDIRECT': False,

    'SSL_HOST': 'prod.my.host.com',

}

 

#staging.py

from base import * 

 

SECURITY_MIDDLEWARE = {

    'SSL_HOST': 'staging.my.host.com',

}

Does staging now represent the merged dicts of base and staging, or the merged dicts of default and staging? I believe, with the merged dict implementation, it is the merge of staging and default. Now if all of the settings were their own setting rather than an entry in a dict, I'd just set the single setting I'd need to change, and be done with it.

There are very little gains to using a dict, and I would argue it harms readability and the use of settings in general unless it's actually required.

Auto merging dicts is the wrong approach. Staging.py should contain exactly what it appears to contain, SECURITY_MIDDLEWARE with a single key defined. If some one needs to tweak one of the values, they should use dict's update. This is the same behavior that is required for DATABASES and the other existing dict settings.

SECURITY_MIDDLEWARE.update({

    'SSL_HOST': 'staging.my.host.com',

})

Regards,

Michael Manfre

[Tim Graham]

The way the discussion has gone, I think I favor individual settings over settings grouped in a dictionary in this case.

If we drop magical merging of dictionary settings (which seems like it will cause more confusion), a user's options for customizing a single key in settings are:

1. Redefine the entire dictionary in a project's settings

2. Use the following pattern to update a subset of keys:

from django.conf.global_settings import SECURITY_MIDDLEWARE

SECURITY_MIDDLEWARE.update({

    'SSL_HOST': 'staging.my.host.com',

})

We have admonitions in the docs that users should not import from global_settings and I don't care for 1 either.

[Carl Meyer]

On 08/30/2014 07:28 PM, Tim Graham wrote:
> If we drop magical merging of dictionary settings (which seems like it
> will cause more confusion), a user's options for customizing a single
> key in settings are:
> 1. Redefine the entire dictionary in a project's settings
> 2. Use the following pattern to update a subset of keys:
>
> from django.conf.global_settings import SECURITY_MIDDLEWARE
>
> SECURITY_MIDDLEWARE.update({

> 'SSL_HOST': 'staging.my.host.com <http://staging.my.host.com/>',

> })
>
> We have admonitions in the docs that users should not import from
> global_settings and I don't care for 1 either.

Michael is talking about the "multiple levels of inherited user
settings" situation, where I agree with him that dict.update() is
generally a good-enough solution, and the merge feature committed in
https://github.com/django/django/commit/66757fee7e921ad4c35e0b3f80c25e026100b31c
to fix #23384 doesn't help anyway.

When it comes to user settings extending a built-in dict default, I
agree with you that there's currently no good solution. But I'm also not
sure it's a problem that needed a solution. I can't think of a time when
I've wanted to add a second CACHE or DATABASE, while wanting to use the
built-in default one as well. It's not a problem I've heard from anyone
else either. And if I did have that problem, I would prefer explicitly
repeating the configuration for the default, rather than relying on
implicit special treatment of dict settings. So on the whole I'd
probably favor reverting #23384, just because it's magical and implicit
and I don't think it solves a significant problem in practice. But I
don't feel strongly about it.

Of course, if we're going to start aggressively grouping more built-in
settings into dictionaries, that would provide justification for #23384.
But I think the simpler and better answer is "let's not do that." Flat
simple settings are just easier to deal with, and don't require any
implicit merge behaviors.

Carl

[Claude Paroz]

Hi,

I'm not against reverting #23384 (I'm the commit author) because I admit it can be debatable, but still I don't like that wrong arguments are given against it.

The situation about multiple user settings file is absolutely not changed by that patch. If you import from a parent settings file, it's still your choice to either overwrite the dictionary or update it, no magics happen at all at this stage.

The magics merge effect is only between global settings and user settings, so you could define:

EMAIL = {'USE_SSL': True}

without clearing other EMAIL keys from the default global settings.

Carl, you mention several other arguments which I don't understand:

more verbosity in documentation, check warnings, and settings files, more complex to work with in

settings-override scenarios.

You may look at the pull request https://github.com/django/django/pull/2836 to see if any of the above are confirmed or not in that scenario (email settings).

Yes, it's a design choice to make, judging if a bit of "magic" merging justifies or not having better logically-grouped settings. Keep also in mind that the original use-case (apart from django-secure new settings) was the addition of two new email-related settings (#20743).

Once again, I'm not advocating for dictionary settings, only for fare debate :-)

Cheers,

Claude

[Carl Meyer]

Hi Claude,

On 08/31/2014 12:08 PM, Claude Paroz wrote:
> The situation about multiple user settings file is absolutely not
> changed by that patch. If you import from a parent settings file, it's
> still your choice to either overwrite the dictionary or update it, no
> magics happen at all at this stage.
>
> The magics merge effect is only between global settings and user
> settings, so you could define:
>
> EMAIL = {'USE_SSL': True}
>
> without clearing other EMAIL keys from the default global settings.

I agree; I tried to make the same distinction in my last mail in this
thread.

> Carl, you mention several other arguments which I don't understand:
> more verbosity in documentation, check warnings,

For instance, the "check --deploy" warnings would need to say "to enable
HSTS, you should set the 'HSTS_SECONDS' key of the SECURE_CONFIG setting
to something non-zero", rather than simply "you should set the
SECURE_HSTS_SECONDS setting to something non-zero." Or it could be "you
should set the SECURE_CONFIG['HSTS_SECONDS'] setting", which is not as
verbose but now requires a bunch of punctuation. Every reference to
these settings in documentation or in check warnings becomes similarly
more verbose.

> and settings files,
> more complex to work with in settings-override scenarios.

Figuring out how to correctly override dictionary settings (even between
two user-settings levels, with .update()) is more complex than
overriding simple flat settings.

I don't think either of these are big problems or deal-breakers, only
that they are negatives which should be balanced by some compelling benefit.

> You may look at the pull request
> https://github.com/django/django/pull/2836 to see if any of the above
> are confirmed or not in that scenario (email settings).

I think the pull request confirms a slight increase in the verbosity and
punctuation required both in referring to the settings in documentation,
and using the settings in code.

And the fact that it required #23384 to be committed first confirms that
it also is "more complex to work with in settings-override scenarios."

> Yes, it's a design choice to make, judging if a bit of "magic" merging
> justifies or not having better logically-grouped settings. Keep also in
> mind that the original use-case (apart from django-secure new settings)
> was the addition of two new email-related settings (#20743).

I don't find "better logically-grouped settings" a compelling benefit.
Anyone who cares about having their settings logically grouped can
easily group them in their settings file (and probably already does)
without being forced to do so by having them in a dictionary.

In the case of the email settings, I think introducing a deprecation
that requires people to update their settings files, for zero gain in
capability, is a much bigger negative than any of the ones mentioned
above, and should in itself be enough to scuttle the proposal. As I said
on the ticket, if a significant new capability were introduced that
required a change to the settings structure (e.g. multiple email
backends configured at once, which is why CACHES and DATABASES are now
dictionaries), that might provide enough benefit to justify a deprecation.

I don't think #20743 provides a rationale for moving the email settings
into a dictionary structure, either. I'm not a fan of global settings in
general, but when a particular subsystem (e.g. email) is already
configured via global settings, adding a couple more knobs to that
configuration isn't a significant problem. Adding those knobs inside a
dictionary (which is still a process global) rather than as new
top-level settings doesn't gain anything worthwhile, as far as I can
see. As Aymeric said, `len(dir(django.conf.global_settings))` is not a
useful metric. We don't really have "fewer settings" if we hide them
inside dictionaries; we just have settings that are slightly harder to
work with.

> Once again, I'm not advocating for dictionary settings, only for fare
> debate :-)

I hope you found this email fair ;-)

Carl

[Shai Berger]

This thread has had very little to do with django-secure for some time...

On Sunday 31 August 2014 18:07:04 Carl Meyer wrote:
>
> In the case of the email settings, I think introducing a deprecation
> that requires people to update their settings files, for zero gain in
> capability, is a much bigger negative than any of the ones mentioned
> above, and should in itself be enough to scuttle the proposal. As I said
> on the ticket, if a significant new capability were introduced that
> required a change to the settings structure (e.g. multiple email
> backends configured at once, which is why CACHES and DATABASES are now
> dictionaries), that might provide enough benefit to justify a deprecation.
>

A case in point is a change that was introduced in 1.7 -- putting the TEST
settings of databases into an inner dict. When it was brought up, all
responses were positive. The only negatives I've seen since then had to do
with the deprecation -- with the initial implementation, it was not to make a
settings file with test-settings which would work without warnings on both 1.6
and 1.7. This has been since corrected (by allowing new and old to co-exist
when they are equivalent).

There was no gain in functionality, just the logical grouping -- and that,
itself, is somewhat limited (because the settings at issue were already in a
dictionary). There were some simplifications in backend code (esp. the Oracle
backend, which uses more of these settings than other backends).

On the other hand, test settings are less popular for defaults in multiple-
level user settings, so some considerations may be different.

As I said, everybody who commented on it back then liked it. I still like it
in that context (though, as I mostly work on the Oracle backend, I'm biased).
If we now decide that we globally don't like the concept, perhaps it is not
too late to revert it. Or perhaps the decision shouldn't be so global.

Shai.

[Claude Paroz]

Le lundi 1 septembre 2014 02:07:40 UTC+2, Carl Meyer a écrit :

Hi Claude,

On 08/31/2014 12:08 PM, Claude Paroz wrote:

(...)

> Once again, I'm not advocating for dictionary settings, only for fare
> debate :-)

I hope you found this email fair ;-)

Thanks Carl for developing your points, and yes, they are fair!

Claude

[Claude Paroz]

Le lundi 1 septembre 2014 09:15:21 UTC+2, Shai Berger a écrit :

A case in point is a change that was introduced in 1.7 -- putting the TEST
settings of databases into an inner dict. When it was brought up, all

responses were positive. (...)

(...)

As I said, everybody who commented on it back then liked it. I still like it
in that  context (though, as I mostly work on the Oracle backend, I'm biased).
If we now decide that we globally don't like the concept, perhaps it is not
too late to revert it. Or perhaps the decision shouldn't be so global.

When the dictionary setting is not initially populated in global_settings.py, this is less of a problem. So a rule *might* be: no dictionary setting if Django has default values in it. Even if I'm still +0 on the solution I committed.

Claude

[Erik Romijn]

Hi all,

I think finally integrating django-secure is a great step. Making a separate deploy check also makes sense to me. However, I think we should be very cautious with pushing people to enable HSTS.

Some of our security headers can cause things to break. For example redirecting to SSL when your HTTPS is broken, will break your site. Enabling strict X-Frame-Options when you do use external iframes, will break your site. However, if you then fix the setting, everything will work again. Inconvenient, but easy to recover. Also, these effects are limited to the Django site you are working on.

If I were hosting a Django site on example.com, and enable HSTS with includeSubdomains and a lifetime of 6 months, as seems to be common now, I might not only break my own site, but also every other side under example.com. Upon discovering the error it can be corrected, but not before a unknown set of users has memorized that all of example.com and any site under it must use HTTPS.

So, although I encourage anyone to enable HSTS, we should not recommend people to just "switch it on". They should be well aware of the consequences as it can affect an unknown set of users beyond their Django site, long after the change has been reverted. The possible seriousness should be reflected in any encouragement we make for HSTS to be enabled.

Erik

On 28 Aug 2014, at 02:25, Tim Graham <timog...@gmail.com> wrote:

After I wrote the original email, I found #17101 which is where the checkdeploy idea came from. We can just close that ticket (or modify it) if we decide on a different solution. It was created before the checks framework was merged and I agree a separate command may not be ideal, although it may make the implementation slightly easier. I'll look into using DEBUG tomorrow. One issue is that we don't want these checks run during testing (and DEBUG is often False there). Maybe if these checks are registered with something like checks.register(foo, deploy=True), we can skip any checks registered like that during testing. I'll have to see if there's some way to make this work with 'manage.py test' as well as with 3rd party test runners. If we had a separate checkdeploy command, avoiding this problem might be somewhat easier.

I am fine with putting it in core instead of contrib. That just means we need to figure out what to do about settings since we cannot put them on an AppConfig. Assuming we don't want to add them as normal settings, we may be able to use the approach proposed on this mailing list for the CSRF settings -- using attributes on the middleware class (PR). In that case, the check could work by iterating through MIIDDLEWARE_CLASSES until it finds a subclass of SecurityMiddleware and then check the attributes (settings) on that class. I will look into this approach tomorrow.

Thanks for the feedback!

On Wednesday, August 27, 2014 8:47:26 PM UTC-4, Curtis Maloney wrote:

For what it's worth, I agree with Russ.

Having security as an optional extra [which is how it will look to outsiders] is a bad look for Django, and certainly doesn't fit with the "Secure by default" philosophy.

--

Curtis

On 28 August 2014 10:34, Russell Keith-Magee <rus...@keith-magee.com> wrote:

Hi Tim,

On Thu, Aug 28, 2014 at 3:35 AM, Tim Graham <timog...@gmail.com> wrote:

I've started tackling one of the ideas that's been on our GSoC ideas
page for a couple years now: integrating django-secure. I chatted with
Carl about the idea and he's onboard. There are a couple of design
decisions we'll need to make.

+1 to the idea. It was part of Chris' original proposal; we just didn't get around to it with the available time.

 

1. How to integrate django-secure with the checks framework
django-secure essentially implements its own checks framework (which
predates the one in Django). The tricky part is that django-secure's
checks are not ones that generally should pass on a
development instance; they're checks that only make sense to run on a
production server (or at least against a production settings file).
I'm thinking to have some way to skip these new checks by default and
run them only when requested (e.g. manage.py check secure
--settings=prod_settings). Other options include an entirely separate
command like django-secure implements (curently called checksecure),
but perhaps could be called checkdeploy and eventually extended with
other checks that are relevant only in production. Idea/insight from
those more familiar with the checks framework (Chris, Russ), would be
welcome.

Generally, I'd be opposed to the idea of Yet Another Command to run checks - if you make it optional, it won't get run, and this is something we want to be forced in front of everyone.

We use DEBUG as a proxy for "In Production" in other locations in the codebase - e.g., whether ALLOWED_HOSTS checks are run. If we were to supplement that with a --production=true/false flag to the checks command (to override the rare legitimate occasions where DEBUG=True in production, or DEBUG=False in development), wouldn't that meet all the needs here? 

2. How to add settings for django-secure
As discussed in the thread below, I'm going to explore developing an
API for storing settings on an AppConfig to avoid adding more global
settings.
https://groups.google.com/d/topic/django-developers/qnnCLppwA3o/discussion

I have imported django-secure into django.contrib.secure and started
work on integrating it with the built-in checks framework as well as
removing some bits of it that have since been added to Django
(frame-deny, SSL-proxy support).

 

Is it appropriate for this to be a contrib package? That implies it needs to be added to INSTALLED_APPS; I'm not convinced that this is a desirable approach. 

If django-secure is important enough to include as part of Django's codebase (and I fully agree it is), I'd consider security to be part of core, not something you can opt out of. Including it as django.core.checks.security (or similar) would make more sense to me.

 

Russ %-)

--
You received this message because you are subscribed to the Google Groups "Django developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.

Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/CAJxq84_Tq1Fnm_pG4pMSy98DOLsnLtVp9jEgTo6X8%2BH%2BJYi1dQ%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

--
You received this message because you are subscribed to the Google Groups "Django developers" group.

To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.

Visit this group at http://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2d66a232-1f19-4bcc-8178-7e1e060f497b%40googlegroups.com.

[Donald Stufft]

Eh, I think we should advise people to switch on HSTS and with includeSubdomains if at all possible. 

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/2316E343-8EEF-40B3-B043-CA64FA555382%40solidlinks.nl.

For more options, visit https://groups.google.com/d/optout.

---

Donald Stufft

PGP: 7C6B 7C5D 5E2B 6356 A926 F04F 6E3C BCE9 3372 DCFA

[Erik Romijn]

I strongly agree, *if at all possible*. What I'm saying is that HSTS can break so much, even after you revert everything you've changed, that we should make sure users have a rough idea of determining when it is possible. Please deploy HSTS everywhere, but only after you've thought through what you're actually affecting and for how long.

We can be much more brief in recommendations regarding X-Frame-Options or the secure flag on cookies, because even in if it breaks everything, you can just revert back and everything will work again. And you'll only break the site you're working on.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/6E64C365-8959-40F3-95F2-FDB4C86F7158%40stufft.io.

[Aymeric Augustin]

On 1 sept. 2014, at 19:31, Erik Romijn <ero...@solidlinks.nl> wrote:

If I were hosting a Django site on example.com, and enable HSTS with includeSubdomains and a lifetime of 6 months, as seems to be common now, I might not only break my own site, but also every other side under example.com. Upon discovering the error it can be corrected, but not before a unknown set of users has memorized that all of example.com and any site under it must use HTTPS.

This is not a theoretical scenario. I almost experienced it.

We have a website, say, http://www.company.com/. We plan to deploy a Django-based replacement at https://www.company.com/something/. We’re using a sub-URL for clarity. We’re also taking this opportunity to enforce HTTPS, which should have been done long ago but is impractical due to technical debt (sigh).

A sysadmin who’s reasonably skilled but didn’t have previous experience with nginx / gunicorn / Django had included HSTS in the nginx config file. If I hadn’t reviewed that configuration, and it had gone live, whoever tried the replacement could not have gone back the legacy website and we’d lose their business. I don’t know if Googlebot honors HSTS, but if it does, this could have killed the company!

If we recommend HSTS, we need visible warnings and a small duration in examples, for people who copy-paste without reading.

-- 

Aymeric.

[Michael Manfre]

On Mon, Sep 1, 2014 at 2:12 PM, Aymeric Augustin <aymeric....@polytechnique.org> wrote:

If we recommend HSTS, we need visible warnings and a small duration in examples, for people who copy-paste without reading.

The default duration should also be less than a day for the exact reason brought up. The visible warnings could state why it is a good idea to increase the duration, after it is tested in production.

Regards,

Michael Manfre

[Carl Meyer]

Hi Shai,

On 09/01/2014 01:15 AM, Shai Berger wrote:
> This thread has had very little to do with django-secure for some time...

Thanks :-)

I would not say that I "globally don't like the concept"; just that it's
not an obvious win in all cases; there are tradeoffs. So yes, I would
advocate for "the decision shouldn't be so global", in either direction.
I think a dictionary is good design in the case of DATABASES and CACHES,
and there will probably be future cases where it is also well-justified.

My objection in the case of the email settings is primarily that the
benefit is not worth the backwards-incompatibility, and in the case of
django-secure it's that the settings aren't closely related enough to
justify the downsides.

I'm slightly in favor of the database TEST_* change, despite the
backwards incompatibility, for these reasons that don't apply in the
other cases (roughly in order of importance):

a) the TEST_* settings were rapidly expanding to become a full copy of
the normal per-database settings, and it's logical that they would be:
really you're just defining another database to be used in test. So
there's an additional conceptual simplicity (and possibly code
simplification too, though I haven't looked at the commit) achieved by
making it look more like "just another database" configuration.

b) The database TEST_* settings are, I think, less commonly used than
the email settings, so the backwards-compatibility impact is smaller.

c) The TEST_* settings are less likely to be overridden in a
multiple-user-settings scenario (as you mentioned), and have no default
values (so there's no issue with partial override of defaults).

d) The settings were already contained within the DATABASES dictionary,
so if you were trying to override them you were already dealing with the
added complexity of overriding bits of dicts.

Carl

[Carl Meyer]

On 09/01/2014 11:31 AM, Erik Romijn wrote:
[snip]

> So, although I encourage anyone to enable HSTS, we should not recommend
> people to just "switch it on". They should be well aware of the
> consequences as it can affect an unknown set of users beyond their
> Django site, long after the change has been reverted. The possible
> seriousness should be reflected in any encouragement we make for HSTS to
> be enabled.

I very much agree with this concern. I think Tim has already added good
warnings to the documentation for HSTS; in comments on the pull request
I'm also advocating for adding at least a brief note to the warning
message itself, along the lines of "you can break things badly if you
enable this without reading the documentation first."

Carl

[Carl Meyer]

On 09/01/2014 02:34 PM, Michael Manfre wrote:
> On Mon, Sep 1, 2014 at 2:12 PM, Aymeric Augustin
> <aymeric....@polytechnique.org

> <mailto:aymeric....@polytechnique.org>> wrote:
>
> If we recommend HSTS, we need visible warnings and a small duration
> in examples, for people who copy-paste without reading.
>
>
> The default duration should also be less than a day for the exact reason
> brought up. The visible warnings could state why it is a good idea to
> increase the duration, after it is tested in production.

There is no default duration; the default is no HSTS at all. Tim has
updated the documentation to warn about the possible issues, and
recommend testing with a short duration before increasing.

Carl

[Tim Graham]

Over the past couple days, I've made some more updates and polish to the PR. A couple people have looked at it, but some more eyes would be appreciated as it's hopefully now in a mergeable state. Thanks!

https://github.com/django/django/pull/3128

p.s. It uses flat, non-dict settings. We can continue the debate on other dict settings in another thread.