Allow enforced CSP and Report-Only at the same time

[qll]

The CSP 1.0 specification allows this explicitly in the sixth paragraph of Section 3.3: http://www.w3.org/TR/CSP/#processing-model
An example is given, too: "For example, if a server operator is using one policy but wishes to experiment with a stricter policy, the server operator can monitor the stricter policy while enforcing the original policy."

This would require bigger changes to the way the middleware works (maybe a flag as a parameter to the decorators? ... something like only_report=True). If this is in scope for the middleware, I am willing to propose an implementation soon.

[jsocol]

Definitely in scope, would be happy to support this, but it is going to require a big change, at least in terms of API, we should figure out how we want it to look first. Tagging in @graingert and @mozfreddyb.

One option would be to add a second, parallel set of settings, e.g.:

CSP_DEFAULT_SRC = '...'
CSP_RO_DEFAULT_SRC = '...'
CSP_IMG_SRC = '...'
CSP_RO_IMG_SRC = '...'

Then utils.from_settings would either need a companion (and probably a re-name) or a lot more complexity. The CSP_REPORT_ONLY boolean could still be used to flip the normal settings (i.e. without _RO) to report-only mode, or users could just define the _RO settings instead.

It's verbose, I don't love it. The decorator would still need a keyword to decide which policy is being updated (or both?). But it is a way to do site-wide alternate policies. Doing that via decorator would be painful.

Maybe instead of x number of settings, it makes sense to change it to one setting in a dict, in the style of other complex Django settings? e.g.

CSP_POLICY = {
    'default': {
        'DEFAULT_SRC': "'self'",
        'IMG_SRC': ["'self'", 'img-host.example.com'],
        'REPORT_ONLY': False,
    },
    'test_new_policy': {
        ...,
        'REPORT_ONLY': True,
    }
}

# Optional setting, defaults to 'default'
CSP_POLICIES = ['default', 'test_new_policy']

It's a bigger API change, but maybe lets things be cleaner when building policies? And it condenses, rather than expands, the namespace.

Alternate proposals?

[graingert]

I think you want two policy dicts so as to reflect how the underlying headers operate:

CSP_REPORT_ONLY_POLICY = {
    'DEFAULT_SRC': "'*'",
}
CSP_POLICY = {
     'DEFAULT_SRC': "'self'",
}

[jsocol]

CSP technically does allow multiple policies. Not sure exactly why you'd want to, but allowing n>=1 policy definitions is more in line with that part of the spec.

[graingert]

In that case something like:

CSP_POLICIES = [
        {
            'DEFAULT_SRC': "'*'",
        },
        {
             'DEFAULT_SRC': "'self'",
             'REPORT_ONLY': True
        }
]

Rather than using a dict and defining order in another setting

[graingert]

Also the CSP specification is invalid according to http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html#sec4.2

Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded.

[jsocol]

There is no precedent for an ordered list of dicts in the django settings. A dict allows these to be named, and a separate setting makes it easy to flip them on and off or reorder.

[jsocol]

It's not our job here to resolve issues between RFC2616 and CSP.

[mozfreddyb]

I like @jsocol's original proposal, even though it is a bit verbose, but I prefer explicitly here.
It would need some work for current users of django-csp, but there's little we can do?!

[qll]

Yep, I like the nested dict idea, too. However, why use "DEFAULT_SRC" rather than "default-src"? The latter would copy the original header more closely.

[jsocol]

@mozfreddyb do you mean a nested dict or a parallel set of settings?

@qll copying the style of other settings (e.g. DATABASES and CACHES) but you're right, in this case copying the names directly from the header is a more compelling way to do it.

[mozfreddyb]

Nested dict as in your very first comment, @jsocol.

[DylanYoung]

Love the nested dict idea. You should add support to the decorators as well to replace or append policies based on name.

Knowing that there is support in the spec for multiple policies makes me question whether the CSP decorator (@csp) is adequately clear and explicit? It could be append CSP or replace CSP. I'd suggest clearing the existing CSP should be done through one and only one very clear path (@csp_exempt) to avoid user error in a security-critical component of the app (I would rename this: @csp_clear or @csp_purge for clarity).

Does it also make sense to think about plugging into the sites contrib app to allow per site CSP configurations in the DB? (for now, this could be name-based and hardcoded in settings, but eventually dynamic db-based CSPs could be supported...)

And finally a thought on backwards compatibility for users:

You can leave the current settings, just load them into the policy dict under the 'default' key. If the 'default' key is also explicitly defined, throw an error. Set CSP_POLICIES to ['default'] by default ;)

[DylanYoung]

Oh and this issue is super old. Would you like me to take this on?

[EnTeQuAk]

@DylanYoung if you have the time for it, please go for it.

Regarding what you said...

(I would rename this: @csp_clear or @csp_purge for clarity).

Personally, I don't think that's necessary. @...except follows various Django APIs and should be explicit enough imho.

I also don't think we need support for django.contrib.sites - I'd leave that to specific projects to handle dynamically. We shouldn't solve all problems in this library, that'd be out of scope.

Yeah, I like your idea of backwards compatibility. We could first introduce the new policy dictionary but also allow loading of it, if it's specified. That way we can safely deprecate old settings and transition to the new settings format.

Looking forward to reviewing your pull request!

[h1771t]

any update on this? Would be nice to have this in django core, which waits on resolve of this ticket. thanks.

[DylanYoung]

@hiren1771 Thanks for the ping. My time got swallowed by life recently, but I'll see if I can get to this soon.

[DylanYoung]

Got a little start on it. Not quite functional yet.

Couple notes:

NEW settings:

CSP_POLICY_DEFINITIONS (dict)
CSP_UPDATE_TEMPLATE (str)
CSP_POLICIES (list, tuple)

The template is used for appending policies. It will be used as the "base" of the new policy. Can be set to None to use the provided policy "as-is".

I opted to make csp_update also allow appending. Check out the new definition (9fe25d9) and let me know if it makes sense to y'all.

Legacy settings are still supported and there's a toggle in the code to change their precedence after an initial deprecation period.

[h1771t]

thanks, is this ticket still the right approach to use, considering this comment from the django ticket:
https://code.djangoproject.com/ticket/15727#comment:22

[DylanYoung]

@hiren1771 Should be fine I think. Once it's coded, they can fold it into core. I'll keep that in mind while I'm refactoring though.

[h1771t]

Ah I see, this module is also a middleware, and so is SecurityMiddleware. If there is anything I can help with, let me know. I have never worked on django code base but can do general python. Thanks again for picking this ticket up.

[DylanYoung]

@hiren1771 Sweet. I think I'll have a working version tomorrow (maybe you could give it a test and see if it all works and makes sense to you?), but there will be few final decisions to be made.

Pinging @jsocol @graingert @mozfreddyb to try to get some of those discussions rolling. In particular, there are a couple of "per policy" settings that aren't directives, report-only being one. Do we want to differentiate these somehow from the actual directives?

As far as back compatibility goes, I think the easiest thing is to continue supporting mixed case, underscores, and hyphens, then just normalise in the backend. Does that make sense to everyone? (This way we can keep using kwargs too).

[DylanYoung]

Also, I think I need a bit of clarification on the spec. My reading is that multiple comma delineated values are permitted, but only one of each of the two headers. Is that correct?

If not, then I think we have to impose that restriction anyways as I don't think Django's response supports multiple headers with the same key.

[DylanYoung]

All tests are passing now. Haven't written tests for the multi-policy case yet so there might still be some bugs lurking in there. Still have some cleanup to do, but we're close :)

[robhudson]

I've opened a PR that addresses this in #219 and am looking for feedback. This is a backwards incompatible change and we plan to release this with a major version bump to 4.0 to signify this. I feel the settings change should be straight-forward and hopefully not a difficult upgrade path.

To @DylanYoung:

• Apologies for not using your branch. I had already started making a go of it after some discussion with @stevejalim on settings structure and didn't see your branch until I had already made good headway.
• I went with the 2 dict settings approach, one for the enforced policy and one for the report-only policy, rather than a single dict with both differentiated by key. Otherwise I think we both ended up in very similar places. I felt the 2 dict approach more closely matched the 2 headers that will be output which seemed like less of an abstraction.
• I notice some larger changes around the decorators in your PR. I will try to understand more the motivation behind those changes.

I would very much welcome any review and feedback. Thank you.

[some1ataplace]

@robhudson @stevejalim Thank you for all your efforts on this. It has been a long time coming.

I see that you responded to this ticket in hopes of getting content security policy built into django core:

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

Just putting that link here for visibility, since it is not the easiest thing to find.