Skip to content

CONSOLE-4263: Add initial Content Security Policy for Console web application #14156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Oct 8, 2024
Merged

CONSOLE-4263: Add initial Content Security Policy for Console web application #14156

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7841d81
Add initial Content Security Policy for Console index handler
vojtechszocs Aug 14, 2024
865b604
Add Content Security Policy section to plugins README
vojtechszocs Sep 24, 2024
f19c620
Address review comments
vojtechszocs Sep 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions frontend/packages/console-dynamic-plugin-sdk/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,29 @@ import { MonitoringIcon } from '@patternfly/react-icons/dist/esm/icons/monitorin
import { MonitoringIcon } from '@patternfly/react-icons';
```

## Content Security Policy

Console application uses [Content Security Policy](https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP)
(CSP) to detect and mitigate certain types of attacks. By default, the list of allowed
[CSP sources](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/Sources)
includes the document origin `'self'` and Console webpack dev server when running off-cluster.

All dynamic plugin assets _should_ be loaded using `/api/plugins/<plugin-name>` Bridge endpoint which
matches the `'self'` CSP source of Console application.

See `cspSources` and `cspDirectives` in
[`pkg/server/server.go`](https://github.com/openshift/console/blob/master/pkg/server/server.go)
for details on the current Console CSP implementation.

### Changes in Console CSP

This section documents notable changes in the Console Content Security Policy.

#### Console 4.18.x

Console CSP is deployed in report-only mode. CSP violations will be logged in the browser console
but the associated CSP directives will not be enforced.

## Plugin metadata

Older versions of webpack `ConsoleRemotePlugin` assumed that the plugin metadata is specified via
Expand Down
58 changes: 58 additions & 0 deletions frontend/public/components/app.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ initI18n();
// Only linkify url strings beginning with a proper protocol scheme.
linkify.set({ fuzzyLink: false });

const pluginAssetBaseURL = `${document.baseURI}api/plugins/`;

const getPluginNameFromResourceURL = (url) =>
url?.startsWith(pluginAssetBaseURL)
? url.substring(pluginAssetBaseURL.length).split('/')[0]
: null;

const EnhancedProvider = ({ provider: ContextProvider, useValueHook, children }) => {
const value = useValueHook();
return <ContextProvider value={value}>{children}</ContextProvider>;
Expand Down Expand Up @@ -132,13 +139,64 @@ const App = (props) => {
}
}, []);

const onCSPViolation = React.useCallback((event) => {
// eslint-disable-next-line no-console
console.warn('Content Security Policy violation detected', event);

// https://developer.mozilla.org/en-US/docs/Web/API/SecurityPolicyViolationEvent
const cspReportObject = _.pick(event, [
// The URI of the resource that was blocked because it violates a policy.
'blockedURI',
// The column number in the document or worker at which the violation occurred.
'columnNumber',
// Whether the user agent is configured to enforce or just report the policy violation.
'disposition',
// The URI of the document or worker in which the violation occurred.
'documentURI',
// The directive that was violated.
'effectiveDirective',
// The line number in the document or worker at which the violation occurred.
'lineNumber',
// The policy whose enforcement caused the violation.
'originalPolicy',
// The URL for the referrer of the resources whose policy was violated, or null.
'referrer',
// A sample of the resource that caused the violation, usually the first 40 characters.
// This will only be populated if the resource is an inline script, event handler or style.
'sample',
// If the violation occurred as a result of a script, this will be the URL of the script.
'sourceFile',
// HTTP status code of the document or worker in which the violation occurred.
'statusCode',
]);

// Attempt to infer Console plugin name from CSP report object
const pluginName =
getPluginNameFromResourceURL(cspReportObject.blockedURI) ||
getPluginNameFromResourceURL(cspReportObject.sourceFile);

if (pluginName) {
// eslint-disable-next-line no-console
console.warn(
`Content Security Policy violation seems to originate from plugin ${pluginName}`,
);
Comment on lines +180 to +182
Copy link
Contributor

@yapei yapei Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when dynamic plugin triggers CSP violation, we can only see warning message Content Security Policy violation detected

Content Security Policy violation seems to originate from plugin ${pluginName} doesn't print

Screenshot 2024-09-27 at 4 46 18 PM

Copy link
Contributor Author

@vojtechszocs vojtechszocs Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yapei @jhadvig

When detecting whether the CSP violation originates from a dynamic plugin or from Console application code itself, we look at blockedURI and sourceFile properties - if their values start with {consoleBaseURL}/api/plugins/ prefix, it means that the associated resource is fetched via Console Bridge server's plugin asset endpoint which allows us to infer the name of the dynamic plugin.

This is why the message is phrased "seems to originate from plugin" instead of just "originates from plugin". Any CSP violations which are due to {consoleBaseURL}/api/plugins/... requests are associated with dynamic plugins, but any dynamic plugin can also make requests to other (non-Console) endpoints.

If a plugin makes requests to non-Console endpoints (regardless of whether the requested service is inside or outside the cluster) it will trigger CSP violations which we have no way of associating with that dynamic plugin. These kind of CSP violations will be logged, but the "seems to originate from plugin" will not apply to them.

Each dynamic plugin team should test their own plugin on a cluster and ensure there are no CSP violations that may be traced back to their plugin.

In case a plugin really needs to make requests to non-Console endpoints then it should explicitly list such endpoints via ConsolePlugin custom resource (CONSOLE-4265).

}
}, []);

React.useEffect(() => {
window.addEventListener('resize', onResize);
return () => {
window.removeEventListener('resize', onResize);
};
}, [onResize]);

React.useEffect(() => {
document.addEventListener('securitypolicyviolation', onCSPViolation);
return () => {
document.removeEventListener('securitypolicyviolation', onCSPViolation);
};
}, [onCSPViolation]);

React.useLayoutEffect(() => {
// Prevent infinite loop in case React Router decides to destroy & recreate the component (changing key)
const oldLocation = _.omit(prevLocation, ['key']);
Expand Down
18 changes: 8 additions & 10 deletions pkg/proxy/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"crypto/tls"
"encoding/base64"
"fmt"
"log"
"net"
"net/http"
"net/http/httputil"
Expand Down Expand Up @@ -112,8 +111,7 @@ func CopyRequestHeaders(originalRequest, newRequest *http.Request) {
}

func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {

if klog.V(3).Enabled() {
if klog.V(4).Enabled() {
klog.Infof("PROXY: %#q\n", SingleJoiningSlash(p.config.Endpoint.String(), r.URL.Path))
}

Expand Down Expand Up @@ -231,13 +229,13 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
errMsg := fmt.Sprintf("Failed to dial backend: '%v'", err)
statusCode := http.StatusBadGateway
if resp == nil || resp.StatusCode == 0 {
log.Println(errMsg)
klog.Error(errMsg)
} else {
statusCode = resp.StatusCode
if resp.Request == nil {
log.Printf("%s Status: '%v' (no request object)", errMsg, resp.Status)
klog.Errorf("%s Status: '%v' (no request object)", errMsg, resp.Status)
} else {
log.Printf("%s Status: '%v' URL: '%v'", errMsg, resp.Status, resp.Request.URL)
klog.Errorf("%s Status: '%v' URL: '%v'", errMsg, resp.Status, resp.Request.URL)
}
}
http.Error(w, errMsg, statusCode)
Expand All @@ -250,23 +248,23 @@ func (p *Proxy) ServeHTTP(w http.ResponseWriter, r *http.Request) {
CheckOrigin: func(r *http.Request) bool {
origin := r.Header["Origin"]
if p.config.Origin == "" {
log.Printf("CheckOrigin: Proxy has no configured Origin. Allowing origin %v to %v", origin, r.URL)
klog.Infof("CheckOrigin: Proxy has no configured Origin. Allowing origin %v to %v", origin, r.URL)
return true
}
if len(origin) == 0 {
log.Printf("CheckOrigin: No origin header. Denying request to %v", r.URL)
klog.Warningf("CheckOrigin: No origin header. Denying request to %v", r.URL)
return false
}
if p.config.Origin == origin[0] {
return true
}
log.Printf("CheckOrigin '%v' != '%v'", p.config.Origin, origin[0])
klog.Warningf("CheckOrigin '%v' != '%v'", p.config.Origin, origin[0])
return false
},
}
frontend, err := upgrader.Upgrade(w, r, nil)
if err != nil {
log.Printf("Failed to upgrade websocket to client: '%v'", err)
klog.Errorf("Failed to upgrade websocket to client: '%v'", err)
return
}

Expand Down
21 changes: 21 additions & 0 deletions pkg/server/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,27 @@ func (s *Server) indexHandler(w http.ResponseWriter, r *http.Request) {
return
}

// This Content Security Policy (CSP) applies to Console web application resources.
// Console CSP is deployed in report-only mode via "Content-Security-Policy-Report-Only" header.
// See https://developer.mozilla.org/en-US/docs/Web/HTTP/CSP for details on CSP specification.
cspSources := "'self'"
if s.K8sMode == "off-cluster" {
// Console local development involves a webpack server running on port 8080
cspSources = cspSources + " http://localhost:8080 ws://localhost:8080"
}
Comment on lines +680 to +683
Copy link
Member

@spadgett spadgett Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should determine if there is a more precise way to determine if we're using a webpack dev server in development mode. There are other use cases I know of where people run an off-cluster console.

Copy link
Contributor Author

@vojtechszocs vojtechszocs Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spadgett @jhadvig Any ideas on how to do this detection?

My initial idea was to add a new flag that would indicate the need to allow localhost CSP sources, i.e. during Console local development.

Copy link
Member

@jhadvig jhadvig Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, I think that should be the path forward

cspDirectives := []string{
fmt.Sprintf("default-src %s", cspSources),
fmt.Sprintf("base-uri %s", cspSources),
fmt.Sprintf("img-src %s data:", cspSources),
fmt.Sprintf("font-src %s data:", cspSources),
Comment on lines +687 to +688
Copy link
Contributor

@yapei yapei Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, what does an empty data: schema mean here?

Copy link
Contributor Author

@vojtechszocs vojtechszocs Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yapei @jhadvig

This allows the web application to use data: URLs to embed certain content into the HTML markup.

In this case, we allow using data: URLs for img-src (images and favicons) and font-src (CSS fonts).

Using data: URLs is commonly used for web asset optimization. When building Console web application, the module bundler (webpack) may generate these data: URLs for any small-sized images and fonts.

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Security-Policy/Sources#scheme-source

Copy link
Member

@jhadvig jhadvig Oct 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this should be a valid value 👍

fmt.Sprintf("script-src %s 'unsafe-eval'", cspSources),
fmt.Sprintf("style-src %s 'unsafe-inline'", cspSources),
"frame-src 'none'",
"frame-ancestors 'none'",
"object-src 'none'",
}
Comment on lines +684 to +694
Copy link
Member

@spadgett spadgett Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great start.

I could see some of these directives breaking plugins. As we've talked about, we'll minimally need a mechanism for plugins to add values to cspSources and a plan to transition to enforcing.

Copy link
Member

@jhadvig jhadvig Sep 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spadgett

As we've talked about, we'll minimally need a mechanism for plugins to add values to cspSources

Sounds like an API change for the ConsolePlugin CRD

plan to transition to enforcing.

I feel like by the end of 4.18 we should have a clear picture on:

  1. What the CSP which we want to enforce should look like
  2. API change for the ConsolePlugin

w.Header().Set("Content-Security-Policy-Report-Only", strings.Join(cspDirectives, "; "))

plugins := make([]string, 0, len(s.EnabledConsolePlugins))
for plugin := range s.EnabledConsolePlugins {
plugins = append(plugins, plugin)
Expand Down