Skip to content
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.

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

Allow additive csp configuration #102059

Merged
merged 30 commits into from
Jun 25, 2021
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
80ecded
add additive csp configuration
pgayvallet Jun 14, 2021
73890df
add unit tests for new class
pgayvallet Jun 14, 2021
303d60a
fix types
pgayvallet Jun 14, 2021
b3d60b2
adapt test utils
pgayvallet Jun 14, 2021
ee9f805
fix tests
pgayvallet Jun 14, 2021
c8eccd5
Merge remote-tracking branch 'upstream/master' into kbn-94414-additiv…
pgayvallet Jun 14, 2021
d531ac0
Merge remote-tracking branch 'upstream/master' into kbn-94414-additiv…
pgayvallet Jun 15, 2021
dca375f
more unit tests on config
pgayvallet Jun 15, 2021
4b45111
generated doc
pgayvallet Jun 15, 2021
ca5616d
review comments
pgayvallet Jun 15, 2021
2188b0e
Merge remote-tracking branch 'upstream/master' into kbn-94414-additiv…
pgayvallet Jun 15, 2021
0d3bd2c
update ascii doc
pgayvallet Jun 15, 2021
400b2d1
Merge remote-tracking branch 'upstream/master' into kbn-94414-additiv…
pgayvallet Jun 16, 2021
e1eb1b5
update ascii doc links
pgayvallet Jun 16, 2021
42c81ec
automatically add single quotes for keywords
pgayvallet Jun 16, 2021
227c5b7
Merge remote-tracking branch 'upstream/master' into kbn-94414-additiv…
pgayvallet Jun 22, 2021
592a119
add missing csp directives
pgayvallet Jun 22, 2021
383aa95
add more tests
pgayvallet Jun 22, 2021
ec383c3
add additional settings to asciidoc
pgayvallet Jun 23, 2021
8a28a62
add null-check
pgayvallet Jun 23, 2021
c74ada5
revert test config props
pgayvallet Jun 23, 2021
59303f4
fix usage collection usage
pgayvallet Jun 23, 2021
3d7947c
Merge remote-tracking branch 'upstream/master' into kbn-94414-additiv…
pgayvallet Jun 24, 2021
3609981
some review comments
pgayvallet Jun 24, 2021
966fc55
last review comments
pgayvallet Jun 24, 2021
04a4f59
add kibana-docker variables
pgayvallet Jun 24, 2021
e9590d4
try to fix doc reference
pgayvallet Jun 24, 2021
fff64d8
try to fix doc reference again
pgayvallet Jun 24, 2021
46681a3
Merge remote-tracking branch 'upstream/master' into kbn-94414-additiv…
pgayvallet Jun 25, 2021
596db0d
fix tests
pgayvallet Jun 25, 2021
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
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<!-- Do not edit this file. It is automatically generated by API Documenter. -->

[Home](./index.md) &gt; [kibana-plugin-core-server](./kibana-plugin-core-server.md) &gt; [CspConfig](./kibana-plugin-core-server.cspconfig.md) &gt; ["\#private"](./kibana-plugin-core-server.cspconfig.__private_.md)

## CspConfig."\#private" property

<b>Signature:</b>

```typescript
#private;
```
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ The constructor for this class is marked as internal. Third-party code should no

| Property | Modifiers | Type | Description |
| --- | --- | --- | --- |
| ["\#private"](./kibana-plugin-core-server.cspconfig.__private_.md) | | <code></code> | |
| [DEFAULT](./kibana-plugin-core-server.cspconfig.default.md) | <code>static</code> | <code>CspConfig</code> | |
| [disableEmbedding](./kibana-plugin-core-server.cspconfig.disableembedding.md) | | <code>boolean</code> | |
| [header](./kibana-plugin-core-server.cspconfig.header.md) | | <code>string</code> | |
Expand Down
12 changes: 11 additions & 1 deletion docs/setup/settings.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,21 @@ Set to `false` to disable Console. *Default: `true`*
<<ops-cGroupOverrides-cpuAcctPath, `ops.cGroupOverrides.cpuAcctPath`>>.

| `csp.rules:`
| A https://w3c.github.io/webappsec-csp/[content-security-policy] template
| deprecated:[7.14.0,"In 8.0 and later, this setting will no longer be supported."]
jportner marked this conversation as resolved.
Show resolved Hide resolved
A https://w3c.github.io/webappsec-csp/[content-security-policy] template
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
that disables certain unnecessary and potentially insecure capabilities in
the browser. It is strongly recommended that you keep the default CSP rules
that ship with {kib}.

| `csp.script_src:`
| Add values for the `script-src` https://w3c.github.io/webappsec-csp/[content-security-policy] directive.
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved

| `csp.worker_src:`
| Add values for the `worker-src` https://w3c.github.io/webappsec-csp/[content-security-policy] directive.
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved

| `csp.style_src:`
| Add values for the `style-src` https://w3c.github.io/webappsec-csp/[content-security-policy] directive.
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved

|[[csp-strict]] `csp.strict:`
| Blocks {kib} access to any browser that
does not enforce even rudimentary CSP rules. In practice, this disables
Expand Down
47 changes: 46 additions & 1 deletion src/core/server/csp/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,56 @@
import { config } from './config';

describe('config.validate()', () => {
test(`does not allow "disableEmbedding" to be set to true`, () => {
it(`does not allow "disableEmbedding" to be set to true`, () => {
// This is intentionally not editable in the raw CSP config.
// Users should set `server.securityResponseHeaders.disableEmbedding` to control this config property.
expect(() => config.schema.validate({ disableEmbedding: true })).toThrowError(
'[disableEmbedding]: expected value to equal [false]'
);
});

it('throws if both `rules` and `script_src` are specified', () => {
expect(() =>
config.schema.validate({
rules: [
`script-src 'unsafe-eval' 'self'`,
`worker-src 'unsafe-eval' 'self'`,
`style-src 'unsafe-eval' 'self'`,
],
script_src: [`'self'`],
})
).toThrowErrorMatchingInlineSnapshot(
`"\\"csp.rules\\" cannot be used when specifying per-directive additions such as \\"script_src\\", \\"worker_src\\" or \\"style_src\\""`
);
});

it('throws if both `rules` and `worker_src` are specified', () => {
expect(() =>
config.schema.validate({
rules: [
`script-src 'unsafe-eval' 'self'`,
`worker-src 'unsafe-eval' 'self'`,
`style-src 'unsafe-eval' 'self'`,
],
worker_src: [`'self'`],
})
).toThrowErrorMatchingInlineSnapshot(
`"\\"csp.rules\\" cannot be used when specifying per-directive additions such as \\"script_src\\", \\"worker_src\\" or \\"style_src\\""`
);
});

it('throws if both `rules` and `style_src` are specified', () => {
expect(() =>
config.schema.validate({
rules: [
`script-src 'unsafe-eval' 'self'`,
`worker-src 'unsafe-eval' 'self'`,
`style-src 'unsafe-eval' 'self'`,
],
style_src: [`'self'`],
})
).toThrowErrorMatchingInlineSnapshot(
`"\\"csp.rules\\" cannot be used when specifying per-directive additions such as \\"script_src\\", \\"worker_src\\" or \\"style_src\\""`
);
});
});
60 changes: 44 additions & 16 deletions src/core/server/csp/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,28 +7,56 @@
*/

import { TypeOf, schema } from '@kbn/config-schema';
import { ServiceConfigDescriptor } from '../internal_types';

const configSchema = schema.object(
{
rules: schema.maybe(schema.arrayOf(schema.string())),
script_src: schema.arrayOf(schema.string(), { defaultValue: [] }),
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
worker_src: schema.arrayOf(schema.string(), { defaultValue: [] }),
style_src: schema.arrayOf(schema.string(), { defaultValue: [] }),
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
strict: schema.boolean({ defaultValue: true }),
warnLegacyBrowsers: schema.boolean({ defaultValue: true }),
disableEmbedding: schema.oneOf([schema.literal<boolean>(false)], { defaultValue: false }),
},
{
validate: (cspConfig) => {
if (
cspConfig.rules &&
(cspConfig.script_src.length || cspConfig.worker_src.length || cspConfig.style_src.length)
) {
return `"csp.rules" cannot be used when specifying per-directive additions such as "script_src", "worker_src" or "style_src"`;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forbidding both usages simultaneously was what we talked about. Technically we could allow it, as the new CspDirectives system could merge both sources though, but it's probably better to just not mix things

},
}
);

/**
* @internal
*/
export type CspConfigType = TypeOf<typeof config.schema>;
export type CspConfigType = TypeOf<typeof configSchema>;

export const config = {
export const config: ServiceConfigDescriptor<CspConfigType> = {
// TODO: Move this to server.csp using config deprecations
// ? https://github.com/elastic/kibana/pull/52251
path: 'csp',
schema: schema.object({
rules: schema.arrayOf(schema.string(), {
defaultValue: [
`script-src 'unsafe-eval' 'self'`,
`worker-src blob: 'self'`,
`style-src 'unsafe-inline' 'self'`,
],
}),
strict: schema.boolean({ defaultValue: true }),
warnLegacyBrowsers: schema.boolean({ defaultValue: true }),
disableEmbedding: schema.oneOf([schema.literal<boolean>(false)], { defaultValue: false }),
}),
schema: configSchema,
deprecations: () => [
(rawConfig, fromPath, addDeprecation) => {
const cspConfig = rawConfig[fromPath];
if (cspConfig?.rules) {
addDeprecation({
message:
'csp.rules is deprecated in favor of directive specific configuration. ' +
'Please use `csp.script_src`, `csp.worker_src` and `csp.style_src` instead',
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
correctiveActions: {
manualSteps: [
`Remove "csp.rules" from the Kibana config file"`,
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
`Add directive specific configurations to the config file, using "csp.script_src", "csp.worker_src" and "csp.style_src"`,
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
],
},
});
}
},
],
};

export const FRAME_ANCESTORS_RULE = `frame-ancestors 'self'`; // only used by CspConfig when embedding is disabled
96 changes: 84 additions & 12 deletions src/core/server/csp/csp_config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
*/

import { CspConfig } from './csp_config';
import { FRAME_ANCESTORS_RULE } from './config';
import { config as cspConfig, CspConfigType } from './config';

// CSP rules aren't strictly additive, so any change can potentially expand or
// restrict the policy in a way we consider a breaking change. For that reason,
Expand All @@ -23,6 +23,12 @@ import { FRAME_ANCESTORS_RULE } from './config';
// the nature of a change in defaults during a PR review.

describe('CspConfig', () => {
let defaultConfig: CspConfigType;

beforeEach(() => {
defaultConfig = cspConfig.schema.validate({});
});

test('DEFAULT', () => {
expect(CspConfig.DEFAULT).toMatchInlineSnapshot(`
CspConfig {
Expand All @@ -40,50 +46,116 @@ describe('CspConfig', () => {
});

test('defaults from config', () => {
expect(new CspConfig()).toEqual(CspConfig.DEFAULT);
expect(new CspConfig(defaultConfig)).toEqual(CspConfig.DEFAULT);
});

describe('partial config', () => {
test('allows "rules" to be set and changes header', () => {
const rules = ['foo', 'bar'];
const config = new CspConfig({ rules });
const rules = [`foo 'self'`, `bar 'self'`];
const config = new CspConfig({ ...defaultConfig, rules });
expect(config.rules).toEqual(rules);
expect(config.header).toMatchInlineSnapshot(`"foo; bar"`);
expect(config.header).toMatchInlineSnapshot(`"foo 'self'; bar 'self'"`);
});

test('allows "strict" to be set', () => {
const config = new CspConfig({ strict: false });
const config = new CspConfig({ ...defaultConfig, strict: false });
expect(config.strict).toEqual(false);
expect(config.strict).not.toEqual(CspConfig.DEFAULT.strict);
});

test('allows "warnLegacyBrowsers" to be set', () => {
const warnLegacyBrowsers = false;
const config = new CspConfig({ warnLegacyBrowsers });
const config = new CspConfig({ ...defaultConfig, warnLegacyBrowsers });
expect(config.warnLegacyBrowsers).toEqual(warnLegacyBrowsers);
expect(config.warnLegacyBrowsers).not.toEqual(CspConfig.DEFAULT.warnLegacyBrowsers);
});

test('allows "worker_src" to be set and changes header', () => {
const config = new CspConfig({
...defaultConfig,
rules: [],
worker_src: ['foo', 'bar'],
Comment on lines +73 to +77
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These tests are effectively mostly testing the CspDirectives class, but I thought it made sense to still test the config configuration / behavior without mocking the underlying implementation (to preserve the implementation during test isolation)

});
expect(config.rules).toEqual([`worker-src foo bar`]);
expect(config.header).toEqual(`worker-src foo bar`);
});

test('allows "style_src" to be set and changes header', () => {
const config = new CspConfig({
...defaultConfig,
rules: [],
style_src: ['foo', 'bar'],
});
expect(config.rules).toEqual([`style-src foo bar`]);
expect(config.header).toEqual(`style-src foo bar`);
});

test('allows "script_src" to be set and changes header', () => {
const config = new CspConfig({
...defaultConfig,
rules: [],
script_src: ['foo', 'bar'],
});
expect(config.rules).toEqual([`script-src foo bar`]);
expect(config.header).toEqual(`script-src foo bar`);
});

test('allows all directives to be set and changes header', () => {
const config = new CspConfig({
...defaultConfig,
rules: [],
script_src: ['script', 'foo'],
worker_src: ['worker', 'bar'],
style_src: ['style', 'dolly'],
});
expect(config.rules).toEqual([
`script-src script foo`,
`worker-src worker bar`,
`style-src style dolly`,
]);
expect(config.header).toEqual(
`script-src script foo; worker-src worker bar; style-src style dolly`
);
});

test('applies defaults when `rules` is undefined', () => {
const config = new CspConfig({
...defaultConfig,
rules: undefined,
script_src: ['script'],
worker_src: ['worker'],
style_src: ['style'],
});
expect(config.rules).toEqual([
`script-src 'unsafe-eval' 'self' script`,
`worker-src blob: 'self' worker`,
`style-src 'unsafe-inline' 'self' style`,
]);
expect(config.header).toEqual(
`script-src 'unsafe-eval' 'self' script; worker-src blob: 'self' worker; style-src 'unsafe-inline' 'self' style`
);
});

describe('allows "disableEmbedding" to be set', () => {
const disableEmbedding = true;

test('and changes rules/header if custom rules are not defined', () => {
const config = new CspConfig({ disableEmbedding });
const config = new CspConfig({ ...defaultConfig, disableEmbedding });
expect(config.disableEmbedding).toEqual(disableEmbedding);
expect(config.disableEmbedding).not.toEqual(CspConfig.DEFAULT.disableEmbedding);
expect(config.rules).toEqual(expect.arrayContaining([FRAME_ANCESTORS_RULE]));
expect(config.rules).toEqual(expect.arrayContaining([`frame-ancestors 'self'`]));
expect(config.header).toMatchInlineSnapshot(
`"script-src 'unsafe-eval' 'self'; worker-src blob: 'self'; style-src 'unsafe-inline' 'self'; frame-ancestors 'self'"`
);
});

test('and does not change rules/header if custom rules are defined', () => {
const rules = ['foo', 'bar'];
const config = new CspConfig({ disableEmbedding, rules });
const rules = [`foo 'self'`, `bar 'self'`];
const config = new CspConfig({ ...defaultConfig, disableEmbedding, rules });
expect(config.disableEmbedding).toEqual(disableEmbedding);
expect(config.disableEmbedding).not.toEqual(CspConfig.DEFAULT.disableEmbedding);
expect(config.rules).toEqual(rules);
expect(config.header).toMatchInlineSnapshot(`"foo; bar"`);
expect(config.header).toMatchInlineSnapshot(`"foo 'self'; bar 'self'"`);
});
});
});
Expand Down
27 changes: 15 additions & 12 deletions src/core/server/csp/csp_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@
* Side Public License, v 1.
*/

import { config, FRAME_ANCESTORS_RULE } from './config';
import { config, CspConfigType } from './config';
import { CspDirectives } from './csp_directives';

const DEFAULT_CONFIG = Object.freeze(config.schema.validate({}));

Expand Down Expand Up @@ -50,8 +51,9 @@ export interface ICspConfig {
* @public
*/
export class CspConfig implements ICspConfig {
static readonly DEFAULT = new CspConfig();
static readonly DEFAULT = new CspConfig(DEFAULT_CONFIG);

readonly #directives: CspDirectives;
public readonly rules: string[];
public readonly strict: boolean;
public readonly warnLegacyBrowsers: boolean;
Expand All @@ -62,16 +64,17 @@ export class CspConfig implements ICspConfig {
* Returns the default CSP configuration when passed with no config
* @internal
*/
constructor(rawCspConfig: Partial<Omit<ICspConfig, 'header'>> = {}) {
const source = { ...DEFAULT_CONFIG, ...rawCspConfig };

this.rules = [...source.rules];
this.strict = source.strict;
this.warnLegacyBrowsers = source.warnLegacyBrowsers;
this.disableEmbedding = source.disableEmbedding;
if (!rawCspConfig.rules?.length && source.disableEmbedding) {
this.rules.push(FRAME_ANCESTORS_RULE);
constructor(rawCspConfig: CspConfigType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The partial constructor thingy was just a workaround to instantiate the default config. Adapted for better consistency with our other config classes (and also because it didn't make a lot of sense imho)

this.#directives = CspDirectives.fromConfig(rawCspConfig);
if (!rawCspConfig.rules?.length && rawCspConfig.disableEmbedding) {
this.#directives.addDirectiveValue('frame-ancestors', `'self'`);
pgayvallet marked this conversation as resolved.
Show resolved Hide resolved
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that we're supporting additive config, I'm not sure the rawCspConfig.rules?.length part of the if condition makes sense, as we're now capable of adding additional rules even if frame-ancestors is already specified. WDYT?

this.header = this.rules.join('; ');

this.rules = this.#directives.getRules();
this.header = this.#directives.getCspHeader();

this.strict = rawCspConfig.strict;
this.warnLegacyBrowsers = rawCspConfig.warnLegacyBrowsers;
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: we can probably remove this warning feature now that we no longer support IE11

this.disableEmbedding = rawCspConfig.disableEmbedding;
}
}
Loading