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: 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