CONSOLE-4327: Add new API field to ConsolePlugin CRD for allowing additional CSP sources #1706
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@jhadvig: This pull request references CONSOLE-4265 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. In 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: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…itional CSP sources
|
@jhadvig: This pull request references CONSOLE-4265 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. In 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. |
| #### Validation Rules | ||
| - Each directive can have up to 16 unique values. | ||
| - The total size of all values across directives must not exceed 8192 bytes (8KB). | ||
| - Each value must be unique, and there are additional validation rules to ensure no quotes, spaces, commas, or wildcard symbols are used. |
There was a problem hiding this comment.
I assume the controller will deduplicate if multiple ConsolePlugins do end up duplicating values, since we don't validate for duplications across multiple resources
There was a problem hiding this comment.
yes, this will be responsibility of the console-operator's controller 👍
There was a problem hiding this comment.
Overall LGTM. @JoelSpeed let us know what you think.
@opayne1 We'll want to cover this in the console plugin docs and release notes.
| `ConsolePlugin` introduces the ability for dynamic plugins to specify their own Content Security Policy (CSP) directives in the OpenShift web console, using the `ConsolePluginCSP` field in the `ConsolePluginSpec`. This field is crucial for mitigating potential security risks, such as cross-site scripting (XSS) and data injection attacks, by controlling which external resources the browser can load. | ||
|
|
||
| #### Content Security Policy (CSP) Overview | ||
| CSP is a security feature that helps detect and mitigate attacks by specifying which sources are allowed for fetching content like scripts, styles, images, and fonts. For dynamic plugins that require loading resources from external sources, defining custom CSP rules ensures secure integration into the OpenShift console. |
There was a problem hiding this comment.
I'd link out to the MDN documentation for Content Security Policy either here or above.
There was a problem hiding this comment.
Changes are good, but, we don't have a plan for moving from TechPreview to GA. What requirements would you expect of this new feature/what testing would you want to see, before you're ready to ship, can that be added to the graduation criteria section?
(This is something we are trying to promote engineers to think more about at the moment, in an effort to deliver complete and quality features first time)
|
@JoelSpeed @spadgett I've added additional commit with the Graduation Criteria as suggested. |
| Note: Currently Console uses `Content-Security-Policy-Report-Only` instead of | ||
| `Content-Security-Policy` header. Due to that the browser will only warn about | ||
| Console CSP violations. |
There was a problem hiding this comment.
Should we discuss when we decide to turn on enforcement? It's an important date for plugins because they'll need to adopt these changes before it's enforced.
There was a problem hiding this comment.
Let's talk about this @jhadvig and come up with a plan.
There was a problem hiding this comment.
@spadgett so when discussed with @vojtechszocs there should be at least 2-3 releases before we flip the switch.
First we need to:
- finish the epic with the initial implementation
- make sure all the current plugins are complying with the CSP (4.19-4.20)
- add CI check
- flip the switch
Probably there will be more steps that we need to meet, this is just how I see right now.
| * Update the OpenShift official documentation to include detailed guidelines | ||
| on configuring `ConsolePluginCSP` in the `ConsoleDynamicPlugin` CRD, along with | ||
| recommendations. | ||
| * Add CSP feature to release notes. |
There was a problem hiding this comment.
We should document clearly when this will be enforced in the release notes.
| on configuring `ConsolePluginCSP` in the `ConsoleDynamicPlugin` CRD, along with | ||
| recommendations. | ||
| * Add CSP feature to release notes. | ||
| * Extend e2e test suit for console-operator repository: |
There was a problem hiding this comment.
Are there interactions with other components at all? Do you think adding something to the origin test suite that runs across all PRs in openshift, to prevent other components causing regressions in this feature?
There was a problem hiding this comment.
Hey, @JoelSpeed. The two out-of-the-box console plugins enabled by default are the monitoring plugin and networking plugin. I think we should work with those teams to add Cypress tests for CSP violations in their plugins.
I'm hesitant to add it to the origin suite because we'd be exposing everyone to UI flakes when only a handful of components impact us, and it would mean introducing an entirely new library for UI testing (Cypress). In the past, we've targeted adding UI tests to specific repos we know can break console like the authentication and OLM.
There was a problem hiding this comment.
I agree with @spadgett 👍
There was a problem hiding this comment.
Sounds reasonable, please clarify that within the doc and when it comes to feature promotion, I'd expect to see links to tests that cover this outside of origin, and their results being green over a couple of weeks ideally
jhadvig
changed the title
|
@jhadvig: This pull request references CONSOLE-4327 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set. In 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. |
jhadvig
force-pushed
the
CONSOLE-4265
branch
2 times, most recently
from
bc1b1b8
to
7c164e4
Compare
| * All the dynamic plugins which are enabled on the cluster by default will | ||
| update their CI to check for CSP violations. |
There was a problem hiding this comment.
@jgbernalp @tnisan Any concerns about adding this into CI for the monitoring and networking plugins?
| Once all the above listed requests are met, Console will switch will be switched | ||
| to use `Content-Security-Policy` header and start enforcing the policies. |
There was a problem hiding this comment.
We also need to give teams outside of Red Hat authoring plugins a chance to update their plugins before we begin enforcing, so minimally that at least one release where we report only. I think it would be better to have a 2-3 release grace period, though.
(But of course we need the API available before then so plugins can adopt it and test.)
There was a problem hiding this comment.
@spadgett definitely it will need to be at least 2-3 releases. I've rather avoid mentioning that in the doc, since its a bit unclear, instead I've only listed necessary tasks which need to be done before we actually start think about flipping the switch
There was a problem hiding this comment.
Maybe we say we will stay in report-only mode for at least one release (which doesn't stop us from waiting longer).
| name: cron-tab | ||
| spec: | ||
| displayName: 'Cron Tab' | ||
| contentSecurityPolicy: |
There was a problem hiding this comment.
would this new field be required? I'd assume is optional with default directives that come from the console.
There might be other directives that we need to consider like web workers, fonts, media source and images loaded using base64 data, for example:
base-uri 'self'; object-src 'self'; style-src 'self' 'unsafe-inline'; worker-src 'self'; font-src 'self' blob:; img-src 'self' data: blob:; media-src 'self'
There was a problem hiding this comment.
@jgbernalp yes this will be an optional field. Defaults indeed come from console server.
There might be other directives that we need to consider like web workers, fonts, media source and images loaded using base64 data
Does the monitoring plugin use those ATM?
We wanted to start by exposing a minimal set of directives and making the API easy to extend.
There was a problem hiding this comment.
@jgbernalp We set default-src: 'self' in console, so 'self' should be supported for all of the fetch directives. Here is what we set:
We also have 'unsafe-inline' for stylesheets.
You could add other fetch directives we don't expose by setting DefaultSrc if needed. Maybe we should expose all the fetch directives as config options, though. That lets plugins do the right thing and only set the specific directives they need instead of the default fallback.
It looks like the only things you would need on top of console defaults are font-src blob:; img-src: blob:.
There was a problem hiding this comment.
Maybe we should expose all the fetch directives as config options, though.
@spadgett you mean as we discussed per CM which will operator use to assemble the CSP for the frontend ?
There was a problem hiding this comment.
@jhadvig I'm a little unclear on the question. I only mean we should consider whether to add all fetch directives (like WorkerSrc and ObjectSrc) to the config, even if console doesn't have defaults for these.
There was a problem hiding this comment.
@spadgett I think that its not necessary for the initial implementation. If there will a need for them we can add them down the road, but until then plugins will be able to use the DefaultSrc for these.
There was a problem hiding this comment.
Looking at the CSP spec, I see a lot of fetch directives. I'm OK with what we have to start.
|
@spadgett I've addressed the last pending comment and squashed the commits. |
|
@jhadvig: all tests passed! Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
Add
ConsolePluginCSPField Documentation toConsoleDynamicPluginCRD EnhancementThis pull request updates the enhancement documentation for the
ConsoleDynamicPluginCustom Resource Definition (CRD) by introducing a new section that explains theConsolePluginCSPfield.Link to the API change: openshift/api#2042
Summary of Changes:
New
ConsolePluginCSPField:Supported CSP Directives:
DefaultSrc,ScriptSrc,StyleSrc,ImgSrc, andFontSrc.CSP Directive Validation:
*) and specific formatting (values enclosed in single quotes).Example Usage:
Why This Change?
The addition of this field to the
ConsoleDynamicPluginCRD enables plugin developers to configure secure policies for loading external resources while maintaining compliance with OpenShift’s security best practices. This change strengthens the ability to protect against potential vulnerabilities by enforcing clear rules for CSP configuration in dynamic plugins./assign @spadgett