OCPBUGS-77931: Loosen default img-src CSP#16388
OCPBUGS-77931: Loosen default img-src CSP#16388logonoff wants to merge 1 commit intoopenshift:mainfrom
img-src CSP#16388Conversation
openshift-ci-robot
added
jira/valid-reference
|
@logonoff: This pull request references Jira Issue OCPBUGS-77931, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: logonoff The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/backend
openshift-ci-robot
added
jira/valid-bug
|
@logonoff: This pull request references Jira Issue OCPBUGS-77931, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: yapei. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
logonoff
force-pushed
the
OCPBUGS-77931-csp
branch
2 times, most recently
from
88e9fc9 to
e545871
Compare
|
@logonoff: This pull request references Jira Issue OCPBUGS-77931, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@openshift-ci-robot: GitHub didn't allow me to request PR reviews from the following users: yapei. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
📝 WalkthroughWalkthroughThis pull request expands the Content Security Policy (CSP) 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pkg/utils/utils.go (1)
65-70:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the nearby CSP comment.
The
img-srcdefault now includeshttps:, but the comment still says image sources only use'self'andhttp://localhost:8080. That’s now inaccurate.Suggested fix
-// Image source, font source, and style source only use 'self' and -// 'http://localhost:8080'. +// Image source uses 'self', https:, and off-cluster localhost. +// Font source and style source only use 'self' and 'http://localhost:8080'.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/utils/utils.go` around lines 65 - 70, Update the nearby block comment to reflect the current CSP directives: mention that image sources (imgSrcDirective) now include 'https:' in addition to 'self' and the localhost origins, and that baseUriDirective, defaultSrcDirective, and imgSrcDirective include the appended http://localhost:8080 and ws://localhost:8080 when running off-cluster; adjust the wording so it accurately describes that img-src uses 'self', 'https:', and the localhost origin rather than only 'self' and http://localhost:8080.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@pkg/utils/utils.go`:
- Around line 65-70: Update the nearby block comment to reflect the current CSP
directives: mention that image sources (imgSrcDirective) now include 'https:' in
addition to 'self' and the localhost origins, and that baseUriDirective,
defaultSrcDirective, and imgSrcDirective include the appended
http://localhost:8080 and ws://localhost:8080 when running off-cluster; adjust
the wording so it accurately describes that img-src uses 'self', 'https:', and
the localhost origin rather than only 'self' and http://localhost:8080.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: f0341edf-3969-4bed-9518-e7d3a5ce0746
📒 Files selected for processing (4)
frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsxfrontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.mdpkg/utils/utils.gopkg/utils/utils_test.go
💤 Files with no reviewable changes (1)
- frontend/packages/console-app/src/hooks/useCSPViolationDetector.tsx
📜 Review details
🧰 Additional context used
🔀 Multi-repo context openshift/console-operator
[::openshift/console-operator::pkg/console/subresource/consoleserver/config_builder.go]
- This file holds the console server CSP assembly: it stores and returns contentSecurityPolicyList and sets ContentSecurityPolicy in the built CLI config (see matches at lines shown in search output: config_builder.go:83, 270, 329, 610-611). Changes to the default img-src will affect the policy string produced here and what gets sent to browsers.
[::openshift/console-operator::pkg/console/subresource/consoleserver/types.go]
- The server config type includes ContentSecurityPolicy as a map[v1.DirectiveType][]string (types.go:34). The default img-src change changes values placed into this structure before serialization.
[::openshift/console-operator::vendor/github.com/openshift/api/console/v1/types_console_plugin.go]
- ConsolePlugin CRD docs and types reference contentSecurityPolicy and img-src examples (multiple matches in generated docs and types). The repo description explicitly states: "'self' is automatically included ... The OpenShift web console server aggregates the CSP directives and values across its own default values and all enabled ConsolePlugin CRs, merging them into a single policy string" (see generated swagger/doc and CRD manifest matches). Plugin authors may need to update spec.contentSecurityPolicy if their plugins load images over http; the new default img-src (adding https:) changes the effective merged policy.
[::openshift/console-operator::vendor/github.com/openshift/api/console/v1/zz_generated.crd-manifests/90_consoleplugins.crd.yaml]
- The CRD manifest includes example img-src entries and documentation that will represent user-facing docs/examples for plugin authors; these docs reference img-src usage and will be consistent with changes to server defaults.
[::openshift/console-operator::pkg/console/subresource/consoleserver/config_builder_test.go]
- There are many tests around console server config building (numerous test files referencing host/APIServerURL etc. and specifically config_builder_test.go). Tests that assert exact CSP strings or the assembled server config may need updates if expected img-src strings change.
Potential reviewer notes (observed evidence only)
- The server merges defaults with ConsolePlugin CRs (vendor/generated docs). Therefore changing the server default img-src to include https: will change the effective CSP delivered to browsers and may reduce the need for plugin authors to declare https image sources—but could also widen allowed image origins.
- Update surface includes config_builder.go (where default CSP values are assembled) and any tests/assertions that expect the older img-src value.
🔇 Additional comments (2)
frontend/packages/console-dynamic-plugin-sdk/release-notes/4.23.md (1)
5-8: Looks good.The release note matches the CSP behavior change and gives plugin authors the right guidance.
pkg/utils/utils_test.go (1)
10-184: Looks good.The updated expectations track the new default
img-srcoutput cleanly, including the off-cluster localhost case.
logonoff
force-pushed
the
OCPBUGS-77931-csp
branch
2 times, most recently
from
32f8ef5 to
dfcb2e8
Compare
|
After a little more research, maybe this represents a bigger risk than we want. One, this is a blanket approach. We should probably try to use a more nuanced solution with less blast radius. Second, as you said, this does represent a risk for IP tracking and zero-day vulnerabilities, which seem inevitable. A couple of ideas that I had:
|
That could work since a lot of major services already do something like this (e.g., googleusercontent.com, githubusercontent.com, etc...). We could potentially look into using some package like https://pkg.go.dev/willnorris.com/go/imageproxy, https://github.com/discord/lilliput, or https://github.com/cactus/go-camo to proxy images in a sandboxed pod. But from a security standpoint it's a lot more work to ensure we're configuring everything correctly, such that the pod is 100% isolated from the rest of the cluster. IMO a server-side SSRF vuln is a lot more severe than client-side IP tracking risks so we need to be very careful with this approach Also note (according to Claude), that githubusercontent (go-camo) has seen security issues with regards to network isolation, so we really need to spend time on this to ensure we're getting it right: GHSA-xrmp-4542-q746
This would potentially mitigate the "potential zero-days in browser image processing" issue but we have a lot of icons in the software catalog, and having one iframe per card isn't exactly the most performant thing in the world. If we don't introduce a image proxy as well, we would still have the IP tracking issue
I think this would be a good middle ground (e.g., gmail has this option), but at the moment our CSP is in report-only mode and so this would only be applicable when we start actually enforcing the policy. If we were to start actually blocking images that would constitute a major change in our CSP which I think is out of scope for this release |
Changes the default `img-src` CSP from `self data:` to `self https: data:`. There are multiple places where we explicitly allow icons any origin (e.g., helm charts, devfiles, templates, topology). Enforcing icons be `data-uri` only would be very difficult to achieve upstream, and proxying the icons would introduce SSRF risks. I can imagine a few risks to this (e.g., IP fingerprinting, exposure to potential zero-days in browsers rendering images) but we already have these troubles today (as we're in report-only mode) and there have been no reported CVEs yet.. I've changed the policy to `https` and not `*` as all of our pre-existing icons are already linking to `https` endpoints and so we can at least prevent mixed-content warnings here
logonoff
force-pushed
the
OCPBUGS-77931-csp
branch
from
dfcb2e8 to
5afddf4
Compare
|
@logonoff: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Analysis / Root cause:
Changes the default
img-srcCSP fromself data:toself https: data:.There are multiple places where we explicitly allow icons any origin (e.g., helm charts, devfiles, templates, topology). Enforcing icons be
data-urionly would be very difficult to achieve upstream, and proxying the icons would introduce SSRF risks.Solution description:
I can imagine a few risks to this (e.g., IP fingerprinting, exposure to potential zero-days in browsers rendering images) but we already have these troubles today (as we're in report-only mode) and there have been no reported CVEs yet..
I've changed the policy to
httpsand not*as all of our pre-existing icons are already linking tohttpsendpoints and so we can at least prevent mixed-content warnings hereTest cases:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation