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

Add CSP headers #5014

Closed
wants to merge 3 commits into from
Closed

Add CSP headers #5014

wants to merge 3 commits into from

Conversation

TAINCER
Copy link

@TAINCER TAINCER commented Mar 21, 2023

No description provided.

@TAINCER TAINCER self-assigned this Mar 21, 2023
@cla-bot cla-bot bot added the cla/signed label Mar 21, 2023
@TAINCER TAINCER changed the base branch from master to fix-csp-errors March 21, 2023 09:58
@TAINCER TAINCER force-pushed the AddCspHeaders branch 2 times, most recently from e2c4535 to 868d513 Compare March 22, 2023 13:04
@TAINCER TAINCER marked this pull request as ready for review March 23, 2023 09:35
@TAINCER TAINCER requested a review from lippserd March 23, 2023 09:35
@TAINCER TAINCER changed the base branch from fix-csp-errors to master March 23, 2023 09:38
@TAINCER TAINCER force-pushed the AddCspHeaders branch 5 times, most recently from 613b971 to bfcb294 Compare March 23, 2023 10:05
Copy link
Member

@lippserd lippserd left a comment

Choose a reason for hiding this comment

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

In addition to the requested changes please:

  • Remove all whitespace changes that are not related to the code you introduce.
  • Regenerate the nonce if it's not an AJAX request.

application/forms/Config/General/ApplicationConfigForm.php Outdated Show resolved Hide resolved
application/forms/Config/General/ApplicationConfigForm.php Outdated Show resolved Hide resolved
application/forms/Config/General/ApplicationConfigForm.php Outdated Show resolved Hide resolved
library/Icinga/Util/CspHeader.php Outdated Show resolved Hide resolved
library/Icinga/Util/CspHeader.php Outdated Show resolved Hide resolved
$useCsp = (bool) Config::app()->get('security', 'use_csp', false);

if ($useCsp) {
$this->setHeader('Strict-Transport-Security', 'max-age=31536000; includeSubdomains; preload', true);
Copy link
Member

Choose a reason for hiding this comment

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

Why all the other headers?

Copy link
Author

Choose a reason for hiding this comment

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

I've used these headers. Since most of these are for blocking XSS, I assumed that we would want all of them. These of course are more restrictive, I can also only include Content-Security-Policy, if we don't want the rest.

Copy link
Member

Choose a reason for hiding this comment

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

Since those headers are easily configureable via the web server, I would not add them here.

Copy link
Author

Choose a reason for hiding this comment

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

Could this be clarified in the docs? Possibly linking to it in the description when setting up CSP? Since this seems to be best practice.

library/Icinga/Util/CspHeader.php Outdated Show resolved Hide resolved
@TAINCER
Copy link
Author

TAINCER commented Mar 28, 2023

  • Regenerate the nonce if it's not an AJAX request.

I did this at first, but when column page contents are being loaded, they are not considered a XML Request $this->getRequest()->isXmlHttpRequest(); (called from Response.php). That's the main reason why I ended up persisting the nonce throughout the session.

@lippserd
Copy link
Member

  • Regenerate the nonce if it's not an AJAX request.

I did this at first, but when column page contents are being loaded, they are not considered a XML Request $this->getRequest()->isXmlHttpRequest(); (called from Response.php). That's the main reason why I ended up persisting the nonce throughout the session.

How is that possible? Do you have proof? Everything would fall apart, if they weren't AJAX requests.

@TAINCER
Copy link
Author

TAINCER commented Mar 28, 2023

I just rechecked. Now it always returns true with XHR Requests, my bad. I think I first tested this when some Javascript was still blocked by CSP.

@TAINCER TAINCER mentioned this pull request Mar 28, 2023
@TAINCER TAINCER force-pushed the AddCspHeaders branch 2 times, most recently from b8a21cc to 154d14b Compare March 29, 2023 08:59
@TAINCER TAINCER requested a review from lippserd March 29, 2023 09:01
library/Icinga/Web/Response.php Outdated Show resolved Hide resolved
library/Icinga/Util/CspHeader.php Outdated Show resolved Hide resolved
library/Icinga/Web/Response.php Outdated Show resolved Hide resolved
application/forms/Config/General/ApplicationConfigForm.php Outdated Show resolved Hide resolved
library/Icinga/Util/Csp.php Show resolved Hide resolved
library/Icinga/Util/Csp.php Outdated Show resolved Hide resolved
library/Icinga/Util/Csp.php Outdated Show resolved Hide resolved
library/Icinga/Util/Csp.php Show resolved Hide resolved
library/Icinga/Util/Csp.php Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants