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

[Reporting] Internally correct the hostname to "localhost" if "server.host" is "0.0.0.0" #117022

Merged
merged 9 commits into from
Nov 3, 2021
92 changes: 63 additions & 29 deletions x-pack/plugins/reporting/server/config/create_config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,27 +5,29 @@
* 2.0.
*/

import { CoreSetup, PluginInitializerContext } from 'src/core/server';
import * as Rx from 'rxjs';
import { CoreSetup, HttpServerInfo, PluginInitializerContext } from 'src/core/server';
import { coreMock } from 'src/core/server/mocks';
import { LevelLogger } from '../lib';
import { createMockConfigSchema } from '../test_helpers';
import { LevelLogger } from '../lib/level_logger';
import { createMockConfigSchema, createMockLevelLogger } from '../test_helpers';
import { ReportingConfigType } from './';
import { createConfig$ } from './create_config';

const createMockConfig = (
mockInitContext: PluginInitializerContext<unknown>
): Rx.Observable<ReportingConfigType> => mockInitContext.config.create();

describe('Reporting server createConfig$', () => {
let mockCoreSetup: CoreSetup;
let mockInitContext: PluginInitializerContext;
let mockLogger: LevelLogger;
let mockLogger: jest.Mocked<LevelLogger>;

beforeEach(() => {
mockCoreSetup = coreMock.createSetup();
mockInitContext = coreMock.createPluginInitializerContext(
createMockConfigSchema({ kibanaServer: {} })
);
mockLogger = {
warn: jest.fn(),
debug: jest.fn(),
clone: jest.fn().mockImplementation(() => mockLogger),
} as unknown as LevelLogger;
mockLogger = createMockLevelLogger();
});

afterEach(() => {
Expand All @@ -37,19 +39,12 @@ describe('Reporting server createConfig$', () => {
...createMockConfigSchema({ kibanaServer: {} }),
encryptionKey: undefined,
});
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.encryptionKey).toMatch(/\S{32,}/); // random 32 characters
expect(result.kibanaServer).toMatchInlineSnapshot(`
Object {
"hostname": "localhost",
"port": 80,
"protocol": "http",
}
`);
expect((mockLogger.warn as any).mock.calls.length).toBe(1);
expect((mockLogger.warn as any).mock.calls[0]).toMatchObject([
expect(mockLogger.warn.mock.calls.length).toBe(1);
expect(mockLogger.warn.mock.calls[0]).toMatchObject([
'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.',
]);
Comment on lines +46 to 49
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand that the change was about removing the any. Since I have more relevant feedback below, maybe we can commit the following to prettify the code even more.

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(1);
expect(mockLogger.warn.mock.calls[0]).toMatchObject([
'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.',
]);
expect(mockLogger.warn).toHaveBeenCalledTimes(1);
expect(mockLogger.warn).toHaveBeenCalledWith(
'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.'
);

});
Expand All @@ -60,10 +55,10 @@ describe('Reporting server createConfig$', () => {
encryptionKey: 'iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii',
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();
expect(result.encryptionKey).toMatch('iiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiiii');
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

it('uses the user-provided encryption key, reporting kibanaServer settings to override server info', async () => {
Expand All @@ -77,7 +72,7 @@ describe('Reporting server createConfig$', () => {
},
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result).toMatchInlineSnapshot(`
Expand Down Expand Up @@ -108,7 +103,7 @@ describe('Reporting server createConfig$', () => {
},
}
`);
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

it('uses user-provided disableSandbox: false', async () => {
Expand All @@ -118,11 +113,11 @@ describe('Reporting server createConfig$', () => {
capture: { browser: { chromium: { disableSandbox: false } } },
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: false });
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

it('uses user-provided disableSandbox: true', async () => {
Expand All @@ -132,11 +127,11 @@ describe('Reporting server createConfig$', () => {
capture: { browser: { chromium: { disableSandbox: true } } },
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: true });
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

it('provides a default for disableSandbox', async () => {
Expand All @@ -145,10 +140,49 @@ describe('Reporting server createConfig$', () => {
encryptionKey: '888888888888888888888888888888888',
})
);
const mockConfig$: any = mockInitContext.config.create();
const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();

expect(result.capture.browser.chromium).toMatchObject({ disableSandbox: expect.any(Boolean) });
expect((mockLogger.warn as any).mock.calls.length).toBe(0);
expect(mockLogger.warn.mock.calls.length).toBe(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(mockLogger.warn.mock.calls.length).toBe(0);
expect(mockLogger.warn).not.toHaveBeenCalled();

});

for (const hostname of [
'0',
'0.0',
'0.0.0',
'0.0.0.0',
'0000:0000:0000:0000:0000:0000:0000:0000',
'::',
]) {
it(`apply failover logic when hostname is given as ${hostname}`, async () => {
Comment on lines +150 to +158
Copy link
Contributor

Choose a reason for hiding this comment

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

Jest provides tooling for such cases. Please use it.each instead.

Suggested change
for (const hostname of [
'0',
'0.0',
'0.0.0',
'0.0.0.0',
'0000:0000:0000:0000:0000:0000:0000:0000',
'::',
]) {
it(`apply failover logic when hostname is given as ${hostname}`, async () => {
it.each`
hostname
${'0'}
${'0.0'}
${'0.0.0'}
${'0.0.0.0'}
${'0000:0000:0000:0000:0000:0000:0000:0000'}
${'::'}
`('apply failover logic when hostname is given as $hostname', async ({ hostname }) => {

mockInitContext = coreMock.createPluginInitializerContext(
createMockConfigSchema({
encryptionKey: 'aaaaaaaaaaaaabbbbbbbbbbbbaaaaaaaaa',
// overwrite settings added by createMockConfigSchema and apply the default settings
// TODO make createMockConfigSchema _not_ default xpack.reporting.kibanaServer.hostname to 'localhost'
kibanaServer: {
hostname: undefined,
port: undefined,
},
})
);
mockCoreSetup.http.getServerInfo = jest.fn().mockImplementation(
(): HttpServerInfo => ({
name: 'cool server',
hostname,
port: 5601,
protocol: 'http',
})
);
Comment on lines +170 to +177
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be simplified a bit.

Suggested change
mockCoreSetup.http.getServerInfo = jest.fn().mockImplementation(
(): HttpServerInfo => ({
name: 'cool server',
hostname,
port: 5601,
protocol: 'http',
})
);
mockCoreSetup.http.getServerInfo = jest.fn((): HttpServerInfo => ({
name: 'cool server',
hostname,
port: 5601,
protocol: 'http',
}));


const mockConfig$ = createMockConfig(mockInitContext);
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();
expect(result.kibanaServer).toMatchObject({
hostname: 'localhost',
port: 5601,
protocol: 'http',
});
Comment on lines +180 to +185
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to check that the promise is resolved and the result is not empty and has the property. In case when the test fails, jest can provide additional logging of where exactly it went wrong (e.g. promises rejects or empty result).

Suggested change
const result = await createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise();
expect(result.kibanaServer).toMatchObject({
hostname: 'localhost',
port: 5601,
protocol: 'http',
});
await expect(
createConfig$(mockCoreSetup, mockConfig$, mockLogger).toPromise()
).resolves.toHaveProperty(
'kibanaServer',
expect.objectContaining({
hostname: 'localhost',
port: 5601,
protocol: 'http',
})
);

});
}
});
51 changes: 24 additions & 27 deletions x-pack/plugins/reporting/server/config/create_config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
* 2.0.
*/

import { i18n } from '@kbn/i18n';
import crypto from 'crypto';
import { upperFirst } from 'lodash';
import ipaddr from 'ipaddr.js';
import { sum, upperFirst } from 'lodash';
import { Observable } from 'rxjs';
import { map, mergeMap } from 'rxjs/operators';
import { CoreSetup } from 'src/core/server';
Expand All @@ -33,20 +33,29 @@ export function createConfig$(
let encryptionKey = config.encryptionKey;
if (encryptionKey === undefined) {
logger.warn(
i18n.translate('xpack.reporting.serverConfig.randomEncryptionKey', {
defaultMessage:
'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on ' +
'restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.',
})
'Generating a random key for xpack.reporting.encryptionKey. To prevent sessions from being invalidated on ' +
'restart, please set xpack.reporting.encryptionKey in the kibana.yml or use the bin/kibana-encryption-keys command.'
);
encryptionKey = crypto.randomBytes(16).toString('hex');
}
const { kibanaServer: reportingServer } = config;
const serverInfo = core.http.getServerInfo();
// kibanaServer.hostname, default to server.host
const kibanaServerHostname = reportingServer.hostname
// set kibanaServer.hostname, default to server.host, don't allow "0.0.0.0" as it breaks in Windows
let kibanaServerHostname = reportingServer.hostname
? reportingServer.hostname
: serverInfo.hostname;

if (
ipaddr.isValid(kibanaServerHostname) &&
!sum(ipaddr.parse(kibanaServerHostname).toByteArray())
) {
logger.warn(
`Found 'server.host: "0.0.0.0"' in Kibana configuration. Reporting is not able to use this as the Kibana server hostname.` +
` To enable PNG/PDF Reporting to work, 'xpack.reporting.kibanaServer.hostname: localhost' is automatically set in the configuration.` +
` You can prevent this message by adding 'xpack.reporting.kibanaServer.hostname: localhost' in kibana.yml.`
);
kibanaServerHostname = 'localhost';
}
// kibanaServer.port, default to server.port
const kibanaServerPort = reportingServer.port ? reportingServer.port : serverInfo.port;
// kibanaServer.protocol, default to server.protocol
Expand All @@ -66,36 +75,24 @@ export function createConfig$(
mergeMap(async (config) => {
if (config.capture.browser.chromium.disableSandbox != null) {
// disableSandbox was set by user
return config;
return { ...config };
}

// disableSandbox was not set by user, apply default for OS
const { os, disableSandbox } = await getDefaultChromiumSandboxDisabled();
const osName = [os.os, os.dist, os.release].filter(Boolean).map(upperFirst).join(' ');

logger.debug(
i18n.translate('xpack.reporting.serverConfig.osDetected', {
defaultMessage: `Running on OS: '{osName}'`,
values: { osName },
})
);
logger.debug(`Running on OS: '{osName}'`);

if (disableSandbox === true) {
logger.warn(
i18n.translate('xpack.reporting.serverConfig.autoSet.sandboxDisabled', {
defaultMessage: `Chromium sandbox provides an additional layer of protection, but is not supported for {osName} OS. Automatically setting '{configKey}: true'.`,
values: {
configKey: 'xpack.reporting.capture.browser.chromium.disableSandbox',
osName,
},
})
`Chromium sandbox provides an additional layer of protection, but is not supported for ${osName} OS.` +
` Automatically setting 'xpack.reporting.capture.browser.chromium.disableSandbox: true'.`
);
} else {
logger.info(
i18n.translate('xpack.reporting.serverConfig.autoSet.sandboxEnabled', {
defaultMessage: `Chromium sandbox provides an additional layer of protection, and is supported for {osName} OS. Automatically enabling Chromium sandbox.`,
values: { osName },
})
`Chromium sandbox provides an additional layer of protection, and is supported for ${osName} OS.` +
` Automatically enabling Chromium sandbox.`
);
}

Expand Down
28 changes: 21 additions & 7 deletions x-pack/plugins/reporting/server/config/schema.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,25 @@ describe('Reporting Config Schema', () => {
`);
});

it(`logs the proper validation messages`, () => {
// kibanaServer
const throwValidationErr = () => ConfigSchema.validate({ kibanaServer: { hostname: '0' } });
expect(throwValidationErr).toThrowError(
`[kibanaServer.hostname]: value must be a valid hostname (see RFC 1123).`
);
});
for (const address of ['0', '0.0', '0.0.0']) {
it(`fails to validate "kibanaServer.hostname" with an invalid hostname: "${address}"`, () => {
expect(() =>
ConfigSchema.validate({
kibanaServer: { hostname: address },
})
).toThrowError(`[kibanaServer.hostname]: value must be a valid hostname (see RFC 1123).`);
});
}
Comment on lines +291 to +299
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (const address of ['0', '0.0', '0.0.0']) {
it(`fails to validate "kibanaServer.hostname" with an invalid hostname: "${address}"`, () => {
expect(() =>
ConfigSchema.validate({
kibanaServer: { hostname: address },
})
).toThrowError(`[kibanaServer.hostname]: value must be a valid hostname (see RFC 1123).`);
});
}
it.each`
hostname
${'0'}
${'0.0'}
${'0.0.0'}
`('fails to validate "kibanaServer.hostname" with an invalid hostname: "$hostname"', ({ hostname }) => {
expect(() =>
ConfigSchema.validate({
kibanaServer: { hostname },
})
).toThrowError(`[kibanaServer.hostname]: value must be a valid hostname (see RFC 1123).`);
});


for (const address of ['0.0.0.0', '0000:0000:0000:0000:0000:0000:0000:0000', '::']) {
it(`fails to validate "kibanaServer.hostname" hostname as zero: "${address}"`, () => {
expect(() =>
ConfigSchema.validate({
kibanaServer: { hostname: address },
})
).toThrowError(
`[kibanaServer.hostname]: cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`
);
});
}
Comment on lines +301 to +311
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (const address of ['0.0.0.0', '0000:0000:0000:0000:0000:0000:0000:0000', '::']) {
it(`fails to validate "kibanaServer.hostname" hostname as zero: "${address}"`, () => {
expect(() =>
ConfigSchema.validate({
kibanaServer: { hostname: address },
})
).toThrowError(
`[kibanaServer.hostname]: cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`
);
});
}
it.each`
hostname
${'0.0.0.0'}
${'0000:0000:0000:0000:0000:0000:0000:0000'}
${'::'}
`('fails to validate "kibanaServer.hostname" hostname as zero: "$hostname"', ({ hostname }) => {
expect(() =>
ConfigSchema.validate({
kibanaServer: { hostname },
})
).toThrowError(
`[kibanaServer.hostname]: cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`
);
});

});
14 changes: 13 additions & 1 deletion x-pack/plugins/reporting/server/config/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,22 @@
*/

import { ByteSizeValue, schema, TypeOf } from '@kbn/config-schema';
import ipaddr from 'ipaddr.js';
import { sum } from 'lodash';
import moment from 'moment';

const KibanaServerSchema = schema.object({
hostname: schema.maybe(schema.string({ hostname: true })),
hostname: schema.maybe(
schema.string({
hostname: true,
validate(value) {
if (ipaddr.isValid(value) && !sum(ipaddr.parse(value).toByteArray())) {
// prevent setting a hostname that fails in Chromium on Windows
return `cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we substitute the actual value here? It is going to be a valid IP address, so that it should not print anything insecure.

Suggested change
return `cannot use '0.0.0.0' as Kibana host name, consider using the default (localhost) instead`;
return `cannot use '${value}' as Kibana host name, consider using the default (localhost) instead`;

}
},
})
),
port: schema.maybe(schema.number()),
protocol: schema.maybe(
schema.string({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,13 +16,14 @@ export function createMockLevelLogger() {

const logger = new LevelLogger(loggingSystemMock.create()) as jest.Mocked<LevelLogger>;

logger.clone.mockImplementation(createMockLevelLogger);
// logger.debug.mockImplementation(consoleLogger('debug')); // uncomment this to see debug logs in jest tests
logger.info.mockImplementation(consoleLogger('info'));
logger.warn.mockImplementation(consoleLogger('warn'));
logger.warning = jest.fn().mockImplementation(consoleLogger('warn'));
logger.error.mockImplementation(consoleLogger('error'));
logger.trace.mockImplementation(consoleLogger('trace'));

logger.clone.mockImplementation(() => logger);

return logger;
}
4 changes: 0 additions & 4 deletions x-pack/plugins/translations/translations/ja-JP.json
Original file line number Diff line number Diff line change
Expand Up @@ -19243,10 +19243,6 @@
"xpack.reporting.screenCapturePanelContent.canvasLayoutLabel": "全ページレイアウト",
"xpack.reporting.screenCapturePanelContent.optimizeForPrintingHelpText": "複数のページを使用します。ページごとに最大2のビジュアライゼーションが表示されます",
"xpack.reporting.screenCapturePanelContent.optimizeForPrintingLabel": "印刷用に最適化",
"xpack.reporting.serverConfig.autoSet.sandboxDisabled": "Chromiumサンドボックスは保護が強化されていますが、{osName} OSではサポートされていません。自動的に'{configKey}: true'を設定しています。",
"xpack.reporting.serverConfig.autoSet.sandboxEnabled": "Chromiumサンドボックスは保護が強化され、{osName} OSでサポートされています。自動的にChromiumサンドボックスを有効にしています。",
"xpack.reporting.serverConfig.osDetected": "OSは'{osName}'で実行しています",
"xpack.reporting.serverConfig.randomEncryptionKey": "xpack.reporting.encryptionKey のランダムキーを生成しています。再起動時にセッションが無効にならないようにするには、kibana.yml でxpack.reporting.encryptionKey を設定するか、bin/kibana-encryption-keys コマンドを使用してください。",
"xpack.reporting.shareContextMenu.csvReportsButtonLabel": "CSV レポート",
"xpack.reporting.shareContextMenu.pdfReportsButtonLabel": "PDF レポート",
"xpack.reporting.shareContextMenu.pngReportsButtonLabel": "PNG レポート",
Expand Down
4 changes: 0 additions & 4 deletions x-pack/plugins/translations/translations/zh-CN.json
Original file line number Diff line number Diff line change
Expand Up @@ -19520,10 +19520,6 @@
"xpack.reporting.screenCapturePanelContent.canvasLayoutLabel": "全页面布局",
"xpack.reporting.screenCapturePanelContent.optimizeForPrintingHelpText": "使用多页,每页最多显示 2 个可视化",
"xpack.reporting.screenCapturePanelContent.optimizeForPrintingLabel": "打印优化",
"xpack.reporting.serverConfig.autoSet.sandboxDisabled": "Chromium 沙盒提供附加保护层,但不受 {osName} OS 支持。自动设置“{configKey}: true”。",
"xpack.reporting.serverConfig.autoSet.sandboxEnabled": "Chromium 沙盒提供附加保护层,受 {osName} OS 支持。自动启用 Chromium 沙盒。",
"xpack.reporting.serverConfig.osDetected": "正在以下 OS 上运行:“{osName}”",
"xpack.reporting.serverConfig.randomEncryptionKey": "为 xpack.reporting.encryptionKey 生成随机密钥。为防止会话在重启时失效,请在 kibana.yml 中设置 xpack.reporting.encryptionKey 或使用 bin/kibana-encryption-keys 命令。",
"xpack.reporting.shareContextMenu.csvReportsButtonLabel": "CSV 报告",
"xpack.reporting.shareContextMenu.pdfReportsButtonLabel": "PDF 报告",
"xpack.reporting.shareContextMenu.pngReportsButtonLabel": "PNG 报告",
Expand Down
Loading