Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Enable CSP by default #24262

Merged
merged 7 commits into from
Jun 20, 2023
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions UPDATING.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ assists people when migrating to a new version.

### Breaking Changes

- [24262](https://github.com/apache/superset/pull/24262): Enabled `TALISMAN_ENABLED` flag by default and provided stricter default Content Security Policy
- [24415](https://github.com/apache/superset/pull/24415): Removed the obsolete Druid NoSQL REGEX operator.
- [24423](https://github.com/apache/superset/pull/24423): Removed deprecated APIs `/superset/slice_json/...`, `/superset/annotation_json/...`
- [24400](https://github.com/apache/superset/pull/24400): Removed deprecated APIs `/superset/recent_activity/...`, `/superset/fave_dashboards_by_username/...`, `/superset/fave_dashboards/...`, `/superset/created_dashboards/...`, `/superset/user_slices/`, `/superset/created_slices/...`, `/superset/fave_slices/...`, `/superset/favstar/...`,
Expand Down
42 changes: 21 additions & 21 deletions docs/docs/security.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ a certain resource type or policy area. You can check possible directives
It's extremely important to correctly configure a Content Security Policy when deploying Superset to
prevent many types of attacks. Superset provides two variables in `config.py` for deploying a CSP:

- `TALISMAN_ENABLED` defaults to `False`; set this to `True` in order to implement a CSP
- `TALISMAN_ENABLED` defaults to `True`; set this to `False` in order to disable CSP
- `TALISMAN_CONFIG` holds the actual the policy definition (*see example below*) as well as any
other arguments to be passed to Talisman.

Expand All @@ -192,10 +192,20 @@ this warning using the `CONTENT_SECURITY_POLICY_WARNING` key in `config.py`.

#### CSP Requirements

* Superset needs both the `'unsafe-eval'` and `'unsafe-inline'` CSP keywords in order to operate.
* Superset needs the `style-src unsafe-inline` CSP directive in order to operate.

```
default-src 'self' 'unsafe-eval' 'unsafe-inline'
style-src 'self' 'unsafe-inline'
```

* Only scripts marked with a [nonce](https://content-security-policy.com/nonce/) can be loaded and executed.
Nonce is a random string automatically generated by Talisman on each page load.
You can get current nonce value by calling jinja macro `csp_nonce()`.

```
<script nonce="{{ csp_nonce() }}">
/* my script */
</script>
```

* Some dashboards load images using data URIs and require `data:` in their `img-src`
Expand All @@ -211,21 +221,11 @@ this warning using the `CONTENT_SECURITY_POLICY_WARNING` key in `config.py`.
connect-src 'self' https://api.mapbox.com https://events.mapbox.com
```

This is a basic example `TALISMAN_CONFIG` that implements the above requirements, uses `'self'` to
limit content to the same origin as the Superset server, and disallows outdated HTML elements by
setting `object-src` to `'none'`.
* Other CSP directives default to `'self'` to limit content to the same origin as the Superset server.

In order to adjust provided CSP configuration to your needs, follow the instructions and examples provided in
[Content Security Policy Reference](https://content-security-policy.com/)

```python
TALISMAN_CONFIG = {
"content_security_policy": {
"default-src": ["'self'", "'unsafe-inline'", "'unsafe-eval'"],
"img-src": ["'self'", "data:"],
"worker-src": ["'self'", "blob:"],
"connect-src": ["'self'", "https://api.mapbox.com", "https://events.mapbox.com"],
"object-src": "'none'",
}
}
```

#### Other Talisman security considerations

Expand All @@ -234,15 +234,15 @@ of which `content_security_policy` is only one. Those can be found in the
[Talisman documentation](https://pypi.org/project/flask-talisman/) under *Options*.
These generally improve security, but administrators should be aware of their existence.

In particular, the default option of `force_https = True` may break Superset's Alerts & Reports
In particular, the option of `force_https = True` (`False` by default) may break Superset's Alerts & Reports
if workers are configured to access charts via a `WEBDRIVER_BASEURL` beginning
with `http://`. As long as a Superset deployment enforces https upstream, e.g.,
through a loader balancer or application gateway, it should be acceptable to set this
option to `False`, like this:
through a loader balancer or application gateway, it should be acceptable to keep this
option disabled. Otherwise, you may want to enable `force_https` like this:

```python
TALISMAN_CONFIG = {
"force_https": False,
"force_https": True,
"content_security_policy": { ...
```

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,8 @@
*/

export default class ExtensibleFunction extends Function {
// @ts-ignore
constructor(fn: Function) {
super();
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling new Function() breaks the CSP rule that disallows using eval in scripts.
Extending Function is a nice hack that let's us make a class instance (in most cases it's a formatter) callable - for example instead of calling formatter.format(value) we call formatter(value). Removing super() does not break that behaviour while also letting us avoid calling Function constructor.


// eslint-disable-next-line @typescript-eslint/no-unsafe-return, no-constructor-return
return Object.setPrototypeOf(fn, new.target.prototype);
}
Expand Down
36 changes: 33 additions & 3 deletions superset/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -1363,13 +1363,43 @@ def EMAIL_HEADER_MUTATOR( # pylint: disable=invalid-name,unused-argument
CONTENT_SECURITY_POLICY_WARNING = True

# Do you want Talisman enabled?
TALISMAN_ENABLED = False
TALISMAN_ENABLED = True
# If you want Talisman, how do you want it configured??
TALISMAN_CONFIG = {
"content_security_policy": None,
"force_https": True,
"content_security_policy": {
"default-src": ["'self'"],
"img-src": ["'self'", "data:"],
"worker-src": ["'self'", "blob:"],
"connect-src": [
"'self'",
"https://api.mapbox.com",
"https://events.mapbox.com",
],
"object-src": "'none'",
"style-src": ["'self'", "'unsafe-inline'"],
"script-src": ["'self'", "'strict-dynamic'"],
},
"content_security_policy_nonce_in": ["script-src"],
"force_https": False,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kgabryje curious why this force_https change?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to limit the scope of the changes in this PR just to CSP - and force_https was disabled by default so far (since Talisman was disabled by default)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with keeping this disabled by default - I would wager that by far the majority of prod Superset deployments terminate SSL/TLS on the LB.

"force_https_permanent": False,
}
# React requires `eval` to work correctly in dev mode
TALISMAN_DEV_CONFIG = {
"content_security_policy": {
"default-src": ["'self'"],
"img-src": ["'self'", "data:"],
"worker-src": ["'self'", "blob:"],
"connect-src": [
"'self'",
"https://api.mapbox.com",
"https://events.mapbox.com",
],
"object-src": "'none'",
"style-src": ["'self'", "'unsafe-inline'"],
"script-src": ["'self'", "'unsafe-inline'", "'unsafe-eval'"],
},
"content_security_policy_nonce_in": ["script-src"],
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't we set force_https: False here, too? AFAIK it defaults to True, right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup nice catch!


#
# Flask session cookie options
Expand Down
6 changes: 5 additions & 1 deletion superset/initialization/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -613,7 +613,11 @@ def __call__(

# Talisman
talisman_enabled = self.config["TALISMAN_ENABLED"]
talisman_config = self.config["TALISMAN_CONFIG"]
talisman_config = (
self.config["TALISMAN_DEV_CONFIG"]
if self.superset_app.debug
else self.config["TALISMAN_CONFIG"]
)
Comment on lines +616 to +620
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it's a good idea to have separate dev and non-dev configs. Should these maybe be TALISMAN_DEFAULT_DEV_CONFIG and TALISMAN_DEFAULT_PROD_CONFIG, and only if TALISMAN_CONFIG is undefined would we fall back to the default. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We must have "unsafe-eval" in dev mode - React and Webpack use it and there's no way around it

csp_warning = self.config["CONTENT_SECURITY_POLICY_WARNING"]

if talisman_enabled:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
{{ lib.render_pagination(page, page_size, count, modelview_name) }}
{{ lib.render_set_page_size(page, page_size, count, modelview_name) }}
</div>
<script language="javascript">
<script language="javascript" nonce="{{ csp_nonce() }}">
var modelActions = new AdminActions();
</script>

Expand Down
2 changes: 1 addition & 1 deletion superset/templates/appbuilder/general/widgets/search.html
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@
</form>
</div>

<script>
<script nonce="{{ csp_nonce() }}">
(function($) {
function checkSearchButton() {
var hasFilter = $('.filters tr').length;
Expand Down
2 changes: 1 addition & 1 deletion superset/templates/superset/export_dashboards.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
specific language governing permissions and limitations
under the License.
#}
<script>
<script nonce="{{ csp_nonce() }}">
window.onload = function() {

// See issue #7353, window.open fails
Expand Down
2 changes: 1 addition & 1 deletion superset/templates/superset/form_view/csv_scripts.html
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
specific language governing permissions and limitations
under the License.
#}
<script>
<script nonce="{{ csp_nonce() }}">
$('#delimiter').on('change', function () {
var delimiterOptions = $('#delimiter').val();
if (delimiterOptions?.includes("other")) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@
</div>
{% endblock %}
{% block add_tail_js %}
<script src="{{url_for('appbuilder.static',filename='js/ab_keep_tab.js')}}"></script>
<script src="{{url_for('appbuilder.static',filename='js/ab_keep_tab.js')}}" nonce="{{ csp_nonce() }}"></script>
{% endblock %}
{% block tail_js %}
{{ super() }}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
specific language governing permissions and limitations
under the License.
#}
<script>
<script nonce="{{ csp_nonce() }}">
var db = $("#database");
var schema = $("#schema");

Expand Down
8 changes: 4 additions & 4 deletions superset/templates/superset/models/database/macros.html
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
under the License.
#}
{% macro testconn() %}
<script>
<script nonce="{{ csp_nonce() }}">
$("#sqlalchemy_uri").parent()
.append('<button id="testconn" class="btn btn-sm btn-primary">{{ _("Test Connection") }}</button>');
$("#testconn").click(function(e) {
Expand Down Expand Up @@ -72,19 +72,19 @@
{% endmacro %}

{% macro expand_extra_textarea() %}
<script>
<script nonce="{{ csp_nonce() }}">
$('#extra').attr('rows', '5');
</script>
{% endmacro %}

{% macro expand_encrypted_extra_textarea() %}
<script>
<script nonce="{{ csp_nonce() }}">
$('#encrypted_extra').attr('rows', '5');
</script>
{% endmacro %}

{% macro expand_server_cert_textarea() %}
<script>
<script nonce="{{ csp_nonce() }}">
$('#server_cert').attr('rows', '5');
</script>
{% endmacro %}
2 changes: 1 addition & 1 deletion superset/templates/superset/partials/asset_bundle.html
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
with development version #}
<!-- Bundle js {{ filename }} START -->
{% for entry in js_manifest(filename) %}
<script src="{{ entry }}" async></script>
<script src="{{ entry }}" async nonce="{{ csp_nonce() }}"></script>
{% endfor %}
<!-- Bundle js {{ filename }} END -->
{% endmacro %}
Expand Down
14 changes: 11 additions & 3 deletions superset/templates/superset/theme.html
Original file line number Diff line number Diff line change
Expand Up @@ -1340,7 +1340,15 @@ <h4 class="modal-title">Source Code</h4>
{% endblock %}
{% block tail_js %}
{{ super() }}
<script src="https://code.jquery.com/jquery-1.10.2.min.js"></script>
<script src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js"></script>
<script src="{{ assets_prefix }}/static/assets/stylesheets/less/cosmo/cosmoTheme.js"></script>
<script
src="https://code.jquery.com/jquery-1.10.2.min.js"
integrity="sha256-C6CB9UYIS9UJeqinPHWTHVqh/E1uhG5Twh+Y5qFQmYg="
crossorigin="anonymous"
nonce="{{ csp_nonce() }}"></script>
<script
src="https://maxcdn.bootstrapcdn.com/bootstrap/3.3.7/js/bootstrap.min.js"
integrity="sha384-Tc5IQib027qvyjSMfHjOMaLkfuWVxZxUPnCJA7l2mCWNIpG9mGCD8wGNIcPD7Txa"
crossorigin="anonymous"
nonce="{{ csp_nonce() }}"></script>
<script src="{{ assets_prefix }}/static/assets/stylesheets/less/cosmo/cosmoTheme.js" nonce="{{ csp_nonce() }}"></script>
{% endblock %}