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 options to set additional nginx headers #3817

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

nealormsbee
Copy link
Member

What does this PR change?

Adds options to values.yaml and the nginx template for injecting extra add_header directives at the server and location levels. I haven't gone and stubbed this out for every location directive yet - I'd like directional feedback on the concept.

The primary motivation here is that security tools (correctly) flag that Access-Control-Allow-Origin '*' is, in many cases, overly-permissive, and that the absence of a Content-Security-Policy header opens attack vectors for XSS and clickjacking.

The default values supplied in values.yaml with this commit keep the CORS behavior consistent, by defaulting to the permissive approach. Users will now be able to override this permissive configuration to harden their application. The default values for CSP disallow iframe embedding to prevent clickjacking, and forbid pulling in sources (scripts, images, etc) from domains other than self, and Userway (an accessibility application leveraged by some Kubecost users). Users who embed Kubecost in an iframe in their own context would need to update their Helm charts to allow this behavior again.

Does this PR rely on any other PRs?

No.

How does this PR impact users? (This is the kind of thing that goes in release notes!)

Users who currently embed Kubecost in an iframe will need to update their values.yaml files. Users who want to add extra headers on a per-location basis will now be able to do so via Helm.

Links to Issues or tickets this PR addresses or fixes

What risks are associated with merging this PR? What is required to fully test this PR?

One risk is that the CSP could inadvertently block externally-loaded content that I missed in testing.

How was this PR tested?

It hasn't been (yet), hence draft mode. My test plan will be to deploy to an environment using this chart and explore the application with the network tab open, and look for any content requests blocked by the CSP or CORS errors.

Have you made an update to documentation? If so, please provide the corresponding PR.

Not yet, but I believe this will require an update.

… add_header directives at the server and location levels. I haven't gone and stubbed this out for every location directive yet - I'd like directional feedback on the concept.

The primary motivation here is that security tools (correctly) flag that `Access-Control-Allow-Origin '*'` is, in many cases, overly-permissive, and that the absence of a `Content-Security-Policy` header opens attack vectors for XSS and clickjacking.

The default values supplied in `values.yaml` with this commit keep the CORS behavior consistent, by defaulting to the permissive approach. Users will now be able to override this permissive configuration to harden their application.
The default values for CSP disallow iframe embedding to prevent clickjacking, and forbid pulling in sources (scripts, images, etc) from domains other than self, and Userway (an accessibility application leveraged by some Kubecost users). Users who embed Kubecost in an iframe in their own context would need to update their Helm charts to allow this behavior again.

Signed-off-by: Neal Ormsbee <neal.ormsbee@gmail.com>
nginxHeaders:
# applied to all route locations
server:
- Content-Security-Policy "default-src 'self' cdn.userway.org; frame-ancestors 'none';";
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it'd be a new header. I'm not super familiar with CSPs, do we know this will work?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, I still need to test it 😅

The idea is that sources like scripts, images, styles, etc can only be loaded from the same domain the UI is being accessed through, and cdn.userway.org. And also that the application cannot be embedded in an iframe. MDN has great CSP documentation if you feel like digging in more to check my understanding

Copy link
Member Author

Choose a reason for hiding this comment

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

Just pushed an update that I have tested, and it actually does work now.

I had to include directives for font-src data:; and I added unsafe-inline to default-src. I'm not sure whether these will still be flagged. It's safer than having no CSP, and we have something, somewhere, that is adding inline styles to the document.

Comment on lines 547 to 572
# per-location headers
location:
root: []
healthz: []
model: []
clusterTurndown:
- "'Access-Control-Allow-Origin' '*' always;"
- "'Access-Control-Allow-Methods' 'GET, PUT, POST, DELETE, OPTIONS' always;"
hideOrphanedResources:
- "'Access-Control-Allow-Origin' '*' always;"
- "'Access-Control-Allow-Methods' 'GET, PUT, POST, DELETE, OPTIONS' always;"
hideDiagnostics:
- "'Access-Control-Allow-Origin' '*' always;"
- "'Access-Control-Allow-Methods' 'GET, PUT, POST, DELETE, OPTIONS' always;"
multiClusterDiagnosticsEnabled:
- "'Access-Control-Allow-Origin' '*' always;"
- "'Access-Control-Allow-Methods' 'GET, PUT, POST, DELETE, OPTIONS' always;"
multiClusterDiagnostics:
- "'Access-Control-Allow-Origin' '*' always;"
- "'Access-Control-Allow-Methods' 'GET, PUT, POST, DELETE, OPTIONS' always;"
forecastingEnabled:
- "'Access-Control-Allow-Origin' '*' always;"
- "'Access-Control-Allow-Methods' 'GET, PUT, POST, DELETE, OPTIONS' always;"
productConfigs:
- "'Access-Control-Allow-Origin' '*' always;"
- "'Access-Control-Allow-Methods' 'GET, PUT, POST, DELETE, OPTIONS' always;"
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 worried that one configuration per route could quickly become unwieldy. Would it be possible start with:

nginxHeaders:
  routes:
    - "'Access-Control-Allow-Origin' '*' always;"
    - "'Access-Control-Allow-Methods' 'GET, PUT, POST, DELETE, OPTIONS' always;"

These headers would then be added to all routes. Do you see an issue with this from a frontend or security perspective?

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 don't think that's an issue. I was attempting to keep the same behavior we have today, where some location directives don't support CORS...personally I have no problem with broadly applying it! I can update.

nealormsbee and others added 3 commits January 23, 2025 15:31
Signed-off-by: Neal Ormsbee <neal.ormsbee@gmail.com>
…eader configs for now

Signed-off-by: Neal Ormsbee <neal.ormsbee@gmail.com>
Copy link
Contributor

@Sean-Holcomb Sean-Holcomb left a comment

Choose a reason for hiding this comment

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

Makes sense to me

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants