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.

Sign up for GitHub

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

Jump to bottom

Middleware: Add CSP Report Only support #58074

Merged
merged 9 commits into from Nov 16, 2022
Merged

Middleware: Add CSP Report Only support #58074

merged 9 commits into from Nov 16, 2022

Conversation

jcalisto
Copy link
Contributor

@jcalisto jcalisto commented Nov 2, 2022 •
edited

What is this feature?

Adds a new content_security_policy_report_only config to the security section, to allow setting a Content-Security-Policy-Report-Only in the middleware.

A Content-Security-Policy-Report-Only allows to experiment a policy and monitor the effects without enforcing it.

Which issue(s) does this PR fix?:
https://github.com/grafana/hosted-grafana/issues/2681

It will help us investigating the impact of enabling the CSP on cloud instances, and can also be useful for other users trying to experiment with their own CSP.

Special notes for your reviewer:
There was a need to refactor the CSP middleware function as there is common behaviour between the CSP and CSP-Report-Only headers (i.e., policy nonce and policy variables).

@CLAassistant
Copy link

CLAassistant commented Nov 2, 2022 •
edited

CLA assistant check
All committers have signed the CLA.

@jcalisto jcalisto force-pushed the feature/add-csp-report-only-support branch from f2a616c to 160c0d9 Compare November 2, 2022 15:47
@jcalisto jcalisto requested a review from a team November 2, 2022 16:32
@jcalisto jcalisto marked this pull request as ready for review November 2, 2022 16:33
@jcalisto jcalisto requested review from a team and chri2547 as code owners November 2, 2022 16:33
@jcalisto jcalisto requested review from papagian, sh0rez and idafurjes and removed request for a team November 2, 2022 16:33
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/40469

@github-actions
Copy link
Contributor

github-actions bot commented Nov 2, 2022 •
edited

⚠️   Possible breaking changes

(Open the links below in a new tab to go to the correct steps)

grafana-data has possible breaking changes (more info)
grafana-runtime has possible breaking changes (more info)

Console output
Read our guideline

@grafanabot grafanabot added the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Nov 2, 2022
@jcalisto jcalisto force-pushed the feature/add-csp-report-only-support branch from 160c0d9 to 9440cfa Compare November 3, 2022 15:49
@grafanabot grafanabot removed the levitate breaking change A label indicating a breaking change and assigned by Levitate. label Nov 3, 2022
chri2547
chri2547 reviewed Nov 7, 2022
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

One suggestion and one question. Thank you!

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/41579

@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/41596

chri2547
chri2547 reviewed Nov 9, 2022
View reviewed changes
Copy link
Collaborator

@chri2547 chri2547 left a comment

Choose a reason for hiding this comment

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

The docs look fine to me. I'm looking for a tech review approval from @papagian , @sh0rez , or @idafurjes .

hairyhenderson
hairyhenderson reviewed Nov 12, 2022
View reviewed changes
conf/sample.ini Outdated Show resolved Hide resolved
@hairyhenderson
Copy link
Member

hairyhenderson commented Nov 12, 2022 •
edited

Overall this LGTM @jcalisto 👍 (with the exception of a few minor nitpicks...)

I'd like to see a review from @grafana/backend-platform too though! 😉

@leandro-deveikis leandro-deveikis modified the milestones: 9.3.0, 9.3.0-beta1 Nov 14, 2022
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42103

jcalisto and others added 9 commits November 15, 2022 12:12
@jcalisto
Middleware: Add CSP Report Only support
e1541f9
@jcalisto @chri2547
Update docs/sources/setup-grafana/configure-grafana/_index.md
39a79e3 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@jcalisto @chri2547
Update docs/sources/setup-grafana/configure-grafana/_index.md
5bf6c09 
Co-authored-by: Christopher Moyer <35463610+chri2547@users.noreply.github.com>
@jcalisto
Update csp documentation wording
cb7bad8
@jcalisto @hairyhenderson
Update conf/sample.ini
8faa1d5 
Co-authored-by: Dave Henderson <dave.henderson@grafana.com>
@jcalisto @hairyhenderson
Update docs/sources/setup-grafana/configure-grafana/_index.md
9e3b0af 
Co-authored-by: Dave Henderson <dave.henderson@grafana.com>
@jcalisto @hairyhenderson
Update docs/sources/setup-grafana/configure-grafana/_index.md
522f690 
Co-authored-by: Dave Henderson <dave.henderson@grafana.com>
@jcalisto @hairyhenderson
Update docs/sources/setup-grafana/configure-grafana/_index.md
20fd959 
Co-authored-by: Dave Henderson <dave.henderson@grafana.com>
@jcalisto @hairyhenderson
Update pkg/middleware/csp.go
7aec06a 
Co-authored-by: Dave Henderson <dave.henderson@grafana.com>
@jcalisto jcalisto force-pushed the feature/add-csp-report-only-support branch from dbdc512 to 7aec06a Compare November 15, 2022 12:12
@grafanabot
Copy link
Contributor

Drone build failed: https://drone.grafana.net/grafana/grafana-enterprise/42115

@grafanabot grafanabot removed this from the 9.3.0-beta1 milestone Nov 15, 2022
@grafanabot
Copy link
Contributor

This pull request was removed from the 9.3.0-beta1 milestone because 9.3.0-beta1 is currently being released.

@hairyhenderson hairyhenderson added this to the 9.3.0 milestone Nov 15, 2022
sakjur
sakjur approved these changes Nov 16, 2022
View reviewed changes
Copy link
Contributor

@sakjur sakjur left a comment

Choose a reason for hiding this comment

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

Code changes look great, glad to see the consideration put into these changes and improving the general state around the code you changed :)

I haven’t tested or compared the implementation to the spec, fyi

@jcalisto
Copy link
Contributor Author

Credits to @hairyhenderson for his great suggestions!

@jcalisto jcalisto merged commit f254a37 into main Nov 16, 2022
@jcalisto jcalisto deleted the feature/add-csp-report-only-support branch November 16, 2022 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hairyhenderson hairyhenderson hairyhenderson left review comments

@chri2547 chri2547 chri2547 left review comments

@sakjur sakjur sakjur approved these changes

@papagian papagian Awaiting requested review from papagian papagian was automatically assigned from grafana/backend-platform

@sh0rez sh0rez Awaiting requested review from sh0rez sh0rez was automatically assigned from grafana/backend-platform

@idafurjes idafurjes Awaiting requested review from idafurjes idafurjes was automatically assigned from grafana/backend-platform

Assignees
No one assigned
Labels
add to changelog area/backend enterprise-failed no-backport Skip backport of PR type/docs
Projects
None yet
Milestone
9.3.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jcalisto @CLAassistant @grafanabot @hairyhenderson @sakjur @chri2547 @leandro-deveikis