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

[jhadvig]

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]

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

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

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. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

[openshift-ci]

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.

[spadgett]

We don't want plugins to be able to change the base-uri.

[jhadvig]

@spadgett not sure how they could, since these are enums?

[spadgett]

Right, we should remove base-uri from the enums. It's not something we want to expose to plugins.

[jhadvig]

Oh sorry, I've misunderstood the comment.
Yes I agree that base-uri directive should be only set by console backend.

[spadgett]

We should describe what these are or link to documentation. A link to the MDN topic would be good (unless we have a policy against linking to 3rd party docs).

https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP

[jhadvig]

From what I see we are using links for 3rd party docs in the API - https://github.com/openshift/api/blob/master/config/v1/types_dns.go#L79-L81

[spadgett]

I would make this something generic like values since we might have directives in the future that aren't sources.

[spadgett]

Need to make clear that this adds to the existing console directive values rather than replacing them.

[spadgett]

I would lean toward spelling it out: contentSecurityPolicy

[jhadvig]

Definitely 👍

[spadgett]

For the final doc, we'll want more detail about what this is and why you'd need to change it. Examples would be helpful.

[jhadvig]

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

[spadgett]

These would still include self as well which the console sets.

[spadgett]

Suggested change

	// Available directive types are default-src, script-src, img-src, style-src and font-src.
	// Available directives are default-src, script-src, img-src, style-src and font-src.

[spadgett]

I'd also note that by default console adds the value self for the various src directives.

[spadgett]

Suggested change

	// The OpenShift web console server aggregates the CSP directives and values across
	// all enabled ConsolePlugin CRs, merging them to set a unified CSP header.
	// The OpenShift web console server aggregates the CSP directives and values across
	// its own default values and all enabled ConsolePlugin CRs, merging them to set a unified
	// CSP header.

[spadgett]

Suggested change

	// values defines an array of source values mostly specifying server origins
	// values defines an array of additional values to append to the console defaults for this directive.

We might add directives that take something other than a source in the future.

[spadgett]

Suggested change

	// directive is a type of CSP directive.
	// directive specifies which Content-Security-Policy directive to configure.

[jhadvig]

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

[jhadvig]

/assign @JoelSpeed

[openshift-ci-robot]

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

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

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.

[spadgett]

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

[JoelSpeed]

Suggested change

	// By default the console server adds the value 'self'to all the various 'src' directives.
	// By default the console server adds the value 'self' to all directives.

Reading this I'd wonder by self is added? And you say by default, does this mean I can disable this behaviour?

[spadgett]

Console needs self to function for all of these values. It can't be removed.

All of the plugin customizations append to the default console values that console needs to work.

[spadgett]

Note that in the future we could add directives that don't specify sources (although right now I think everything we expose is a "src" directive). We'd have to come back and update the text here later if we leave that out.

[JoelSpeed]

So that is to say, if this were a directive that were not a src style, then it wouldn't also need the self prepending?

[spadgett]

Right, self only makes sense as a value for the various CSP src directives.

[JoelSpeed]

Can you expand the godoc here briefly to explain what the directives do, in case the user hasn't explained the higher level field

Eg what is a default vs a script src?

[jhadvig]

Those are explained as part of the enum definition, together with the link for official docs. Thought that it might be more suitable to explain them briefly there together with link.

[JoelSpeed]

The enum definition won't be generated into docs, please copy to here and make sure that it ends up in the CRD schema for oc explain use later

[JoelSpeed]

What is the format of this string? Is it always a URL? What is the maximum possible length? What characters are valid, which are not?

[spadgett]

@JoelSpeed I think we have to leave it open since CSP directive values are not always URLs and don't always follow a specific format.

[jhadvig]

I agree to leave it open and add additional validation on the backend

[spadgett]

We probably should disallow ; since that is a separator in the header between directives. Maybe we can do more validation on the values. Here is the syntax from the standard:

https://www.w3.org/TR/CSP2/#policy-syntax

[spadgett]

directive-value = *( WSP / <VCHAR except ";" and ","> )

Looks like ; and , are not allowed in values and whitespace separates values, so can be disallowed as well. VCHAR refers to printing characters, so I think pretty much anything else is allowed?

[spadgett]

I don't see a length limit on specific values, although some HTTP servers like Apache have limits on the total size of response headers. If we pick it a limit, it would be an arbitrary value.

[jhadvig]

@spadgett this would require following pattern

// +kubebuilder:validation:Pattern=`^[\t\x20!#\$%&'\(\)\*\+\-\.\/0-9:<=>\?@A-Za-z\[\\\]\^_`\{\|\}~]*$`

Whitespace Characters:

• \t: Tab
• \x20: Space

Visible (printable) characters (VCHAR) except ; and ,:

• ! to #: !, #
• $ to +: $, %, &, ', (, ), *, +
• .: -, .
• /: /
• 0-9: All digits 0 through 9
• : to <: :, <
• = to >: =, >
• ? to @: ?, @
• A-Z: Uppercase letters A through Z
• a-z: Lowercase letters a through z
• [ to ]: [, , ]
• ^_: ^, _
• `: Backtick
• { to ~: {, |, }, ~

[JoelSpeed]

Does the regex here not support negative matching? ie, match not this character?

If it doesn't, you could use CEL validations quite easily, !self.Contains(';'), which would be readable.

I don't see a length limit on specific values, although some HTTP servers like Apache have limits on the total size of response headers. If we pick it a limit, it would be an arbitrary value.

Even an arbitrary limit is better than none, how about starting at 1024 and seeing if you get any customers complain that it is too small?

[spadgett]

Even an arbitrary limit is better than none, how about starting at 1024 and seeing if you get any customers complain that it is too small?

@JoelSpeed I agree. Works for me.

[jhadvig]

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

[JoelSpeed]

What would be a reasonable/arbitrary length limit for this list, would you expect someone to have more than say, 32 values?

[spadgett]

32 should be more than enough.

[jhadvig]

right, I dont think any plugin would ever contribute more then 16

[jhadvig]

@JoelSpeed comments addressed. PTAL

[JoelSpeed]

Please make sure when making updates to generate the CRD schema, at the moment I don't think the generation would succeed but you haven't generated so we can't actually see whether it would or not

I'd also like you to write some integration tests (you can see the style in the tests folder in each API group) to check the validations that you are adding actually work and return sensible looking errors

[JoelSpeed]

Is there a combined maximum that we need to be aware of?

[jhadvig]

It's recommended to keep CSP headers under 8 KB to ensure compatibility across all browsers, servers, and proxies, since different browsers impose their own limits on HTTP header size. For example:

• Chrome and Edge generally support headers up to 8 KB.
• Firefox and Safari can support headers up to 16 KB or more.

[JoelSpeed]

In that case, should we impose a total limit in size? You could do this with CEL and a map function that maps every entry in the list to its size and then sum them. We are limiting to 32 items at 1kb each so in theory we could reach 32kb for each of the 5 source types.

I think something like the following on CSP field would work.

// +kubebuilder:validation:XValidation:rule="self.map(x, x.values.map(y, y.size()).sum()).sum() < 8192"

[jhadvig]

right but we are talking only about a single ConsolePlugin CR. There could be multiple CRs, which could contribute the same amount of values, which could cause problem, since console will gather all the directives form all the plugins and aggregates them. But that would need to be handled programatically I guess.
Thinking if we should not start with more strict MaxItems for the values field, something like 4 and if that will cause any issues we could bump the number.
Realistically I dont think plugins will contribute more then 4 values per directive.

[JoelSpeed]

We have the option to restrict the limits down further if you think they can be restricted down, you're the SME on what the quantity of expected values might be here.

And yes, you're right, multiple plugins may contribute, and we may not catch everything with this validation, but, this would at least ensure that the single console plugin doesn't, by itself, violate the restriction. It is the strictest we can be

[jhadvig]

And yes, you're right, multiple plugins may contribute, and we may not catch everything with this validation, but, this would at least ensure that the single console plugin doesn't, by itself, violate the restriction. It is the strictest we can be

👍

[JoelSpeed]

/lgtm

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD d37bb9f and 2 for PR HEAD a04311b in total

[jhadvig]

/retest

[jhadvig]

/test e2e-aws-serial

[JoelSpeed]

Since this is in spec, and you don't otherwise have anything validating that the proxy aliases in the list are unique, I think this list will need to be marked atomic

If we were already validating uniqueness of the aliases, then this would be correct, but I don't see anything that prevents anyone having duplicate aliases right now

[JoelSpeed]

/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]

[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 [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]

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

In response to this:

/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. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD d37bb9f and 2 for PR HEAD 424692f in total

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD 051d533 and 1 for PR HEAD 424692f in total

[jhadvig]

/retest

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD e22f17d and 0 for PR HEAD 424692f in total

[openshift-ci-robot]

/hold

Revision 424692f was retested 3 times: holding

[JoelSpeed]

/test e2e-aws-serial

[JoelSpeed]

/hold cancel

[JoelSpeed]

/override ci/prow/verify-crd-schema

[openshift-ci]

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

In response to this:

/override ci/prow/verify-crd-schema

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.

[openshift-ci-robot]

/retest-required

Remaining retests: 0 against base HEAD e22f17d and 2 for PR HEAD 424692f in total

[openshift-ci]

@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.

[openshift-bot]

[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.