openshift / api

Canonical location of the OpenShift API definition.
http://www.openshift.org
Apache License 2.0
94 stars 517 forks source link

CONSOLE-4265: Add new API field to ConsolePlugin CRD for allowing additional CSP sources #2042

Closed jhadvig closed 1 week ago

jhadvig commented 1 month ago

When serving Console HTML index page, we generate the policy that includes allowed (trustworthy) sources. It may be necessary for some dynamic plugins to add new sources in order to avoid CSP violations at Console runtime. We need to extend the ConsolePlugin CRD, in order to give the plugin creators a mechanism how to specify these source.

We discussed with @vojtechszocs two possible implementation:

  1. Add a new field to spec for specifying an array of sources which would apply for all the CSP directives - spec.csp.allowedSources. This implementation is pretty straight-forward but automatically applying the same source across different directives could unintentionally block legitimate content or reduce security.

    Example:

    kind: ConsolePlugin
    metadata:
    name: my-console-plugin
    spec:
    displayName: "My Custom Console Plugin"
    backend:
    service:
      name: "plugin-backend-service"
      namespace: "plugin-namespace"
      port: 8080
    csp:
    allowedSources:
      - "https://trusted-images.com"
      - "https://cdn.images.com"
  2. (Proposed solution) Add a new field to spec for specifying an array of sources for each of the directives. Letting users manage different whitelists for different types of content, gives them more flexibility in controlling security settings.

    Example:

    kind: ConsolePlugin
    metadata:
    name: my-console-plugin
    spec:
    displayName: "My Custom Console Plugin"
    backend:
    service:
      name: "plugin-backend-service"
      namespace: "plugin-namespace"
      port: 8080
    csp:
    - directive: script-src
      sources:
        - "https://trusted-scripts.com"
    - directive: img-src
      sources:
        - "https://trusted-images.com"
        - "https://cdn.images.com"
    - directive: style-src
      sources:
        - "https://trusted-styles.com"

Note: The PR is missing some descriptions and validation and unit tests. Will address those once we align on the API structure.

/assign @spadgett @vojtechszocs

Story: https://issues.redhat.com/browse/CONSOLE-4265

openshift-ci-robot commented 1 month ago

@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](https://github.com/openshift/api/pull/2042): >When serving Console HTML index page, we generate the policy that includes allowed (trustworthy) [sources](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/Sources). >It may be necessary for some dynamic plugins to add new sources in order to avoid CSP violations at Console runtime. >We need to extend the ConsolePlugin CRD, in order to give the plugin creators a mechanism how to specify these source. > >We discussed with @vojtechszocs two possible implementation: >1. Add a new field to spec for specifying an array of sources which would apply for all the CSP directives - `spec.csp.allowedSources`. This implementation is pretty straight-forward but automatically applying the same source across different directives could unintentionally block legitimate content or reduce security. > > Example: >```yaml >kind: ConsolePlugin >metadata: > name: my-console-plugin >spec: > displayName: "My Custom Console Plugin" > backend: > service: > name: "plugin-backend-service" > namespace: "plugin-namespace" > port: 8080 > csp: > allowedSources: > - "self" > - "https://trusted-images.com" > - "https://cdn.images.com" >``` > >2. (Proposed solution) Add a new field to spec for specifying an array of sources for each of the directives. Letting users manage different whitelists for different types of content, gives them more flexibility in controlling security settings. > > Example: >```yaml >kind: ConsolePlugin >metadata: > name: my-console-plugin >spec: > displayName: "My Custom Console Plugin" > backend: > service: > name: "plugin-backend-service" > namespace: "plugin-namespace" > port: 8080 > csp: > - directive: script-src > sources: > - "self" > - "https://trusted-scripts.com" > - directive: img-src > sources: > - "self" > - "https://trusted-images.com" > - "https://cdn.images.com" > - directive: style-src > sources: > - "self" > - "https://trusted-styles.com" >``` > >Note: The PR is missing some descriptions and validation and unit tests. Will address those once we align on the API structure. > >/assign @spadgett @vojtechszocs > >Story: https://issues.redhat.com/browse/CONSOLE-4265 > > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
openshift-ci[bot] commented 1 month ago

Hello @jhadvig! Some important instructions when contributing to openshift/api: API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

jhadvig commented 1 month ago

@spadgett thank you for the review. I've address the comments. PTAL

jhadvig commented 1 month ago

@spadgett thank you for the review. Addressed your comments in the additional commit. PTAL

jhadvig commented 1 month ago

/assign @JoelSpeed

openshift-ci-robot commented 1 month ago

@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](https://github.com/openshift/api/pull/2042): >When serving Console HTML index page, we generate the policy that includes allowed (trustworthy) [sources](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/Sources). >It may be necessary for some dynamic plugins to add new sources in order to avoid CSP violations at Console runtime. >We need to extend the ConsolePlugin CRD, in order to give the plugin creators a mechanism how to specify these source. > >We discussed with @vojtechszocs two possible implementation: >1. Add a new field to spec for specifying an array of sources which would apply for all the CSP directives - `spec.csp.allowedSources`. This implementation is pretty straight-forward but automatically applying the same source across different directives could unintentionally block legitimate content or reduce security. > > Example: >```yaml >kind: ConsolePlugin >metadata: > name: my-console-plugin >spec: > displayName: "My Custom Console Plugin" > backend: > service: > name: "plugin-backend-service" > namespace: "plugin-namespace" > port: 8080 > csp: > allowedSources: > - "https://trusted-images.com" > - "https://cdn.images.com" >``` > >2. (Proposed solution) Add a new field to spec for specifying an array of sources for each of the directives. Letting users manage different whitelists for different types of content, gives them more flexibility in controlling security settings. > > Example: >```yaml >kind: ConsolePlugin >metadata: > name: my-console-plugin >spec: > displayName: "My Custom Console Plugin" > backend: > service: > name: "plugin-backend-service" > namespace: "plugin-namespace" > port: 8080 > csp: > - directive: script-src > sources: > - "https://trusted-scripts.com" > - directive: img-src > sources: > - "https://trusted-images.com" > - "https://cdn.images.com" > - directive: style-src > sources: > - "https://trusted-styles.com" >``` > >Note: The PR is missing some descriptions and validation and unit tests. Will address those once we align on the API structure. > >/assign @spadgett @vojtechszocs > >Story: https://issues.redhat.com/browse/CONSOLE-4265 > > Instructions for interacting with me using PR comments are available [here](https://prow.ci.openshift.org/command-help?repo=openshift%2Fapi). If you have questions or suggestions related to my behavior, please file an issue against the [openshift-eng/jira-lifecycle-plugin](https://github.com/openshift-eng/jira-lifecycle-plugin/issues/new) repository.
spadgett commented 1 month ago

Thanks, @jhadvig. I agree with the approach and appreciate the detailed doc. Let's have @JoelSpeed take a look 👍

jhadvig commented 1 month ago

@JoelSpeed thank you for the comments. Addressed them in a separate commit and replied to some of them. PTAL

jhadvig commented 1 month ago

@JoelSpeed comments addressed. PTAL

jhadvig commented 1 month ago

@JoelSpeed addressing most of the comments. The only missing piece is the size of the directives, size this will only handle a since ConsolePlugin CR and there could be multiple running on the server.

Adding generated CRD schema + integration test.

PTAL

note: Ive accidentally forced pushed the change... sorry :-/

jhadvig commented 2 weeks ago

/retest

jhadvig commented 2 weeks ago

@spadgett @JoelSpeed Ive addressed the last comment and squashed the commits. PTAL

jhadvig commented 2 weeks ago

/retest

JoelSpeed commented 2 weeks ago

/lgtm

openshift-ci-robot commented 2 weeks ago

/retest-required

Remaining retests: 0 against base HEAD d37bb9f7e38019b6cfc16f568ee61ffc402c5e9b and 2 for PR HEAD a04311b4b35116b35506bbeee343dfe8cb9284ef in total

jhadvig commented 2 weeks ago

/retest

jhadvig commented 2 weeks ago

/test e2e-aws-serial

JoelSpeed commented 1 week ago

/lgtm /override ci/prow/verify-crd-schema

Only errors refer to unfixable errors or those in the v1alpha1 schema which is meant to have been dropped by now

openshift-ci[bot] commented 1 week ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/api/blob/master/OWNERS)~~ [JoelSpeed] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
openshift-ci[bot] commented 1 week ago

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema

In response to [this](https://github.com/openshift/api/pull/2042#issuecomment-2464805280): >/lgtm >/override ci/prow/verify-crd-schema > >Only errors refer to unfixable errors or those in the v1alpha1 schema which is meant to have been dropped by now Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
openshift-ci-robot commented 1 week ago

/retest-required

Remaining retests: 0 against base HEAD d37bb9f7e38019b6cfc16f568ee61ffc402c5e9b and 2 for PR HEAD 424692fe047ee7bd98a690ce93d73108ce65f5c5 in total

openshift-ci-robot commented 1 week ago

/retest-required

Remaining retests: 0 against base HEAD 051d53359b9582b786a12a38f697b10282d126f9 and 1 for PR HEAD 424692fe047ee7bd98a690ce93d73108ce65f5c5 in total

jhadvig commented 1 week ago

/retest

openshift-ci-robot commented 1 week ago

/retest-required

Remaining retests: 0 against base HEAD e22f17d9b7f5b1785ce6ad2075d0e6ac88f211af and 0 for PR HEAD 424692fe047ee7bd98a690ce93d73108ce65f5c5 in total

openshift-ci-robot commented 1 week ago

/hold

Revision 424692fe047ee7bd98a690ce93d73108ce65f5c5 was retested 3 times: holding

JoelSpeed commented 1 week ago

/test e2e-aws-serial

JoelSpeed commented 1 week ago

/hold cancel

JoelSpeed commented 1 week ago

/override ci/prow/verify-crd-schema

openshift-ci[bot] commented 1 week ago

@JoelSpeed: Overrode contexts on behalf of JoelSpeed: ci/prow/verify-crd-schema

In response to [this](https://github.com/openshift/api/pull/2042#issuecomment-2466220573): >/override ci/prow/verify-crd-schema Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
openshift-ci-robot commented 1 week ago

/retest-required

Remaining retests: 0 against base HEAD e22f17d9b7f5b1785ce6ad2075d0e6ac88f211af and 2 for PR HEAD 424692fe047ee7bd98a690ce93d73108ce65f5c5 in total

openshift-ci[bot] commented 1 week ago

@jhadvig: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
openshift-bot commented 1 week ago

[ART PR BUILD NOTIFIER]

Distgit: ose-cluster-config-api This PR has been included in build ose-cluster-config-api-container-v4.18.0-202411092139.p0.ga2817b8.assembly.stream.el9. All builds following this will include this PR.