OCPBUGS-49291: Improving the DevExp for passing the CSP directives to console per flag + Make use of connect-src and object-src directives #14701

jhadvig · 2025-01-27T18:05:51Z

Improving the DevExp for passing the CSP directives to console per flag instead of JSON.
New usage will be:

./bin/bridge --content-security-policy ScriptSrc="localhost:1234 localhost:2345" --content-security-policy FontSrc="localhost:3456 localhost:4567"

Also adding connect-src and object-src directives. These will be set to 'none' if no plugin will specify source-expression-list for them. This API was added by https://issues.redhat.com/browse/OCPBUGS-48740

/assign @TheRealJon @spadgett

openshift-ci-robot · 2025-01-27T18:05:59Z

@jhadvig: This pull request references Jira Issue OCPBUGS-49291, which is valid. The bug has been moved to the POST state.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target version (4.19.0) matches configured target version for branch (4.19.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, POST)

Requesting review from QA contact:
/cc @yapei

The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Improving the DevExp for passing the CSP directives to console per flag instead of JSON.
New usage will be:
./bin/bridge --content-security-policy ScriptSrc="localhost:1234 localhost:2345" --content-security-policy FontSrc="localhost:3456 localhost:4567"
Also adding connect-src and object-src directives. These will be set to 'none' if no plugin will specify source-expression-list for them. This API was added by https://issues.redhat.com/browse/OCPBUGS-48740

/assign @TheRealJon @spadgett

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 · 2025-01-27T21:25:24Z

pkg/utils/utils.go

+	// If the objectSrc directive is not set, add 'none' to it.
+	if len(objectSrcDirective) == 1 && objectSrcDirective[0] == "object-src" {
+		objectSrcDirective = append(objectSrcDirective, none)
+	}
+
+	// If the connectSrc directive is not set, add 'none' to it.
+	if len(connectSrcDirective) == 1 && connectSrcDirective[0] == "connect-src" {
+		connectSrcDirective = append(connectSrcDirective, none)
+	}


I think specifying none overrides default-src so things like self would no longer work.

ok, I see, in that case we should only set self, for both object-src and connect-src

spadgett · 2025-01-27T21:25:47Z

pkg/utils/utils.go

+
+	// If the connectSrc directive is not set, add 'none' to it.
+	if len(connectSrcDirective) == 1 && connectSrcDirective[0] == "connect-src" {
+		connectSrcDirective = append(connectSrcDirective, none)


For connect-src, we want self and also a directive for console.redhat.com

spadgett · 2025-01-27T21:26:54Z

cmd/bridge/main.go

-	fContentSecurityPolicy := fs.String("content-security-policy", "", "Content security policy for the console. (JSON as string)")
+
+	consoleCSPFlags := serverconfig.MultiKeyValue{}
+	fs.Var(&consoleCSPFlags, "content-security-policy", "List of CSP directives that are enabled for the console. Each entry consist of csp-directive-name as a key and csp-directive-value as a value.")


nit: An example here would be nice (like we provide for i18n-namespaces above)

spadgett · 2025-01-27T21:27:47Z

README.md

+Example:
+
+```
+./bin/bridge --content-security-policy ScriptSrc="localhost:1234 localhost:2345" --content-security-policy FontSrc="localhost:3456 localhost:4567"


Here I would probably the directive name font-src instead of FontSrc

Suggested change

./bin/bridge --content-security-policy ScriptSrc="localhost:1234 localhost:2345" --content-security-policy FontSrc="localhost:3456 localhost:4567"

./bin/bridge --content-security-policy ScriptSrc="localhost:1234 localhost:2345" --content-security-policy font-src="localhost:3456 localhost:4567"

I was thinking about it, but then we would need to change the console-config.yaml representation, or accept both formats FontSrc & font-src. Here I would rather go with the first option.

I don't know if we need to change console-config.yaml. Can we just translate between the names in config.go instead?

I wouldn't accept both formats. Let's pick one and go with it. If it's difficult, we can stick with PascalCase.

We will need the change to the console-config.yaml representation. Currently the in console-config/yaml CSPs are set in FontSrc="localhost:3456 localhost:4567" format. Thats how they are passed to the --content-security-policy flag by bridge. OR we need to update the addContentSecurityPolicy() in pkg/serverconfig/config.go so the cspDirectiveName is transformed into the font-src format.

OR we need to update the addContentSecurityPolicy() in pkg/serverconfig/config.go so the cspDirectiveName is transformed into the font-src format.

This is what I was thinking. We have only a handful of well-defined directives we support, so it doesn't seem too bad?

I think its reasonable 👍

spadgett · 2025-01-27T21:29:33Z

pkg/utils/utils.go

 			default:
-				klog.Warningf("ignored invalid CSP directive: %v", directive)
+				klog.Infof("ignored invalid CSP directive: %v", directive)


nit: This isn't necessarily an invalid directive since there are many we don't allow to be set

Suggested change

klog.Infof("ignored invalid CSP directive: %v", directive)

klog.Infof("ignored unsupported CSP directive: %v", directive)

jhadvig · 2025-01-30T00:29:32Z

@spadgett I've vendor the object-src removal and updated code and tests accordingly.
PTAL

spadgett

Thanks @jhadvig 👍

cmd/bridge/main.go

pkg/utils/utils.go

pkg/utils/utils_test.go

jhadvig · 2025-02-12T14:16:18Z

@spadgett thanks for review. Addressed your comments. PTAL

jhadvig · 2025-02-12T14:42:38Z

/retest

spadgett · 2025-02-14T13:10:27Z

pkg/utils/utils.go

-		styleSrcDirective = append(styleSrcDirective, httpLocalHost)
+	directives := map[string][]string{
+		baseURI:        {self},
+		defaultSrc:     {self, consoleDot},


Can we get away with only adding consoleDot to scriptSrc and connectSrc? It would be better if we can avoid putting it in defaultSrc.

spadgett

Code looks good. @jhadvig Do you mind cleaning up the git history a little? I don't mind the separate vendor commits, but maybe squash the address comment commits. Thanks!

spadgett · 2025-02-17T21:32:49Z

Hmm, looks like there's a build failure anyway

jhadvig · 2025-02-18T11:40:46Z

@spadgett squashed last two commits.
The tests are failing due to the vendored changes which are bumping the openshift/api which now requires go-1.23, thats why I've opened:

spadgett · 2025-03-10T10:49:38Z

/retest

jhadvig · 2025-04-22T09:07:06Z

/retest

jhadvig · 2025-04-28T13:26:38Z

/retest
/cherry-pick release-4.19

openshift-cherrypick-robot · 2025-04-28T13:26:46Z

@jhadvig: once the present PR merges, I will cherry-pick it on top of release-4.19 in a new PR and assign it to you.

In response to this:

/retest
/cherry-pick release-4.19

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.

spadgett

You have two bump API commits. Not sure if that's intentional.

Thanks @jhadvig

spadgett · 2025-04-28T16:36:35Z

pkg/utils/utils.go

+		case connectSrc:
+			connectSrcDirective = append(connectSrcDirective, sources)
+		default:
+			klog.Infof("ignored invalid CSP directive: %v", directive)


We should fail here as well

… console per flag + Make use of connect-src directive

jhadvig · 2025-04-29T09:34:22Z

Thank you @spadgett I've addressed your comment and removed unnecessary API bumps. Also added additional commit which is using klog.Fatalf() which is internally calling os.Exit().
PTAL

spadgett

/lgtm

👍
Thanks @jhadvig

openshift-ci · 2025-05-05T12:18:48Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [jhadvig,spadgett]

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

openshift-ci-robot · 2025-05-05T15:55:21Z

/retest-required