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

[openshift-ci]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jhadvig
Once this PR has been reviewed and has the lgtm label, please ask for approval from joelspeed. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[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

👍

[jhadvig]

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

[JoelSpeed]

Do you have the complete error here? Looks like it's been truncated?

This will do a substring match but I'm curious what comes after this

[jhadvig]

@JoelSpeed the whole msg is:
"ConsolePlugin.console.openshift.io \"test-vgzng\" is invalid: spec.contentSecurityPolicy[1]: Duplicate value: map[string]interface {}{\"directive\":\"ScriptSrc\"}"

[JoelSpeed]

Ok, we should update this line then to spec.contentSecurityPolicy[1]: Duplicate value: map[string]interface {}{\"directive\":\"ScriptSrc\"} to capture the field name and the end that got truncated

[JoelSpeed]

Outstanding to fix

Suggested change

	expectedError: "Duplicate value: map[string]interface {}{\"directive\":\"ScriptSrc\""
	expectedError: "spec.contentSecurityPolicy[1]: Duplicate value: map[string]interface {}{\"directive\":\"ScriptSrc\"}"

[JoelSpeed]

Please finish all files with an empty new line char

[JoelSpeed]

This covers spaces, do we also need to cover tabs somehow? Does the regex used in CEL support a whitespace grouping? Perhaps using another function like the findAll function would help to find any whitespace using regex and then catch all whitespace in a single rule?

[jhadvig]

added

[JoelSpeed]

This must go into the godoc for the field that is using this type as well please

[JoelSpeed]

Reminder to add the validation suggested in #2042 (comment)

[JoelSpeed]

Outstanding to fix

Suggested change

	expectedError: "Duplicate value: map[string]interface {}{\"directive\":\"ScriptSrc\""
	expectedError: "spec.contentSecurityPolicy[1]: Duplicate value: map[string]interface {}{\"directive\":\"ScriptSrc\"}"

[JoelSpeed]

Please include the field path part of the error

[JoelSpeed]

This needs a message to explain it

Suggested change

	// +kubebuilder:validation:XValidation:rule="self.map(x, x.values.map(y, y.size()).sum()).sum() < 8192"
	// +kubebuilder:validation:XValidation:rule="self.map(x, x.values.map(y, y.size()).sum()).sum() < 8192",message="the total combined size of values of all directives must not exceed 8192 (8kb)"

[JoelSpeed]

Suggested change

	// The array can contain at most 32 values. Each directive value must have a maximum length
	// The array can contain at most 8 values. Each directive value must have a maximum length

[JoelSpeed]

Suggested change

	// of 1024 characters and must not contain whitespace, commas, or semicolons.
	// of 1024 characters and must not contain whitespace, commas, or semicolons.
	// The value '*' is not permitted.

[JoelSpeed]

Also need to mention that each value in the array must be unique

[vojtechszocs]

Suggested change

	// Dynamic plugins should specify this field if they are loading assets from outside
	// Dynamic plugins should always prefer loading their assets via Console Bridge /api/plugins endpoint. If a plugin really needs to load assets from other endpoints, it should declare these endpoints using the appropriate CSP source directives.

[jhadvig]

Updated the wording a bit:

	// Dynamic plugins should specify this field if need to load assets from outside
	// the cluster or if violation reports are observed. Dynamic plugins should always prefer
	// loading their assets from within the cluster, either by vendoring them, or fetching
	// from a cluster service.

WDYT ?

[vojtechszocs]

Suggested change

	// The value 'self' will be prepended to all source type directives.
	// The value 'self' is automatically included in all fetch directives when generating the Console policy.

[jhadvig]

	// The value 'self' is automatically included in all fetch directives by the OpenShift web
	// console's backend.

WDYT?

[vojtechszocs]

Important ❗ I think we should explicitly disallow any CSP directive values which are enclosed in single quotes.

'self', 'unsafe-eval' etc. are all tokens with special meaning when interpreted by the browser and I think that only Console web application itself should be allowed to use these when generating the security policy.

For example, it makes no sense for a plugin to use 'self' value since that's already included by default. Also, it makes no sense for a plugin to e.g. use 'unsafe-hashes' or 'unsafe-inline' to lower the security constraint for specific asset fetch directives.

The above validation list should include a rule that disallows a string that begins and ends with ' character.

[jhadvig]

added following validation

// +kubebuilder:validation:XValidation:rule="!(self.startsWith(\"'\") && self.endsWith(\"'\"))",message="CSP directive value cannot start or and with a quote"

[yapei]

given following validation checks, it looks like we also need add more tests here

- name: Should throw an error for invalid CSP directive values with comma
- name: Should throw an error for invalid CSP directive values with semicolon
- name: Should throw an error for invalid CSP directive values with strings enclosed in single quotes(based on vojtechszocs's comment)

[JoelSpeed]

Why add a gate if you are adding it to all sets at once? I think you want to not have it as Default for now

[jhadvig]

@JoelSpeed hmm but will the field in this case be available by default? Cause thats what we need.

[JoelSpeed]

To complete the loop from Slack, it should be introduced as TP and then promoted to Default only once QE are happy that the feature is complete and stable

[JoelSpeed]

You'll need to add an enhancementPR link for your feature

Suggested change

	mustRegister()
	enhancementPR("").
	mustRegister()

[JoelSpeed]

I believe the CRD generation needs to be re-run please

[JoelSpeed]

What about unbalanced quotes? What if I had

- a'bad
- thing'b

concatenated this becomes a'bad thing'b, is the 'bad thing' in there problematic?

[jhadvig]

If a CSP header like Content-Security-Policy: script-src a'bad thing'b; is used, the browser will consider it invalid due to the unescaped or unrecognized syntax in a'bad thing'b. The result is that the entire CSP directive could be ignored. That said I think we should disable the quote completely. If you agree will also keep the previous check for a value which starts and ends with quote?

[spadgett]

Do we allow hash values?

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/Sources#hash-algorithm-base64-value

We want to support that for plugins that have inline scripts. It requires using ' characters, though. (nonce we shouldn't support, however, since there's no way to generate a unique value each time.)

[jhadvig]

@spadgett then I think we need to define the validation rules as:

• Hash directive format: Check for a valid hash algorithm (like SHA-256) followed by a hyphen and a valid Base64 value.
• Host source format: Check that the input matches the pattern of a valid URL.

For the CEL rule below accomplishes this by checking for two conditions:

• The value starts with a valid hash algorithm (sha256, sha384, or sha512) followed by a - and a valid Base64 string.
• The value matches the pattern of a valid host source (beginning with http:// or https://).

WDYT ?
ccing @vojtechszocs

[spadgett]

Host sources don't need to start with http:// or https://. For example, *.example.com is a valid host source.

[jhadvig]

Right, sorry. I've unintentionally leaved it out, but it should be valid as well 👍

[vojtechszocs]

@spadgett @jhadvig I think the currently proposed validation rules

// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=1024
// +kubebuilder:validation:XValidation:rule="!self.contains(\"'\")",message="CSP directive value cannot contain a quote"
// +kubebuilder:validation:XValidation:rule="!self.matches('\\\\s')",message="CSP directive value cannot contain a whitespace"
// +kubebuilder:validation:XValidation:rule="!self.contains(',')",message="CSP directive value cannot contain a comma"
// +kubebuilder:validation:XValidation:rule="!self.contains(';')",message="CSP directive value cannot contain a semi-colon"
// +kubebuilder:validation:XValidation:rule="self != '*'",message="CSP directive value cannot be a wildcard"
type CSPDirectiveValue string

are reasonably strict, we can always loosen these rules based on CSP violation reports once we start collecting them with telemetry.

Additional validation logic can be placed into the Console operator (since it's responsible for ensuring valid configuration of Console and all of its resources) or directly into Bridge server.go function indexHandler where we generate the actual CSP string.

One example of additional validation would be e.g. DefaultSrc and ScriptSrc containing the same value, in which case we want to remove that value from ScriptSrc since the DefaultSrc directive acts as a fallback for the ScriptSrc directive.

Another example of additional validation would be to ensure the plugin provided CSP source values are valid URLs with potential * wildcards.

The only 'something' exception that makes sense to me is '<hash-algorithm>-<base64-value>' but I think it's better to wait for CSP violation reports first before we start allowing such exceptions. (Other 'something' special values don't seem too relevant to me at the moment, the main focus should be allowing additional plugin-specific remote sources so they can be loaded without issues in the browser.)

[jhadvig]

I would agree that lets start with stricter validation and relax it down the road, since we will be in report mode for couple of releases.

[jhadvig]

/retest

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