From b8fd76d9acbdfba97e9bf264321e05e83be854a6 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Thu, 9 Jan 2020 12:17:37 -0500 Subject: [PATCH 1/6] Migrate ElasticsearchConfig deprecation out of Core deprecations --- .../deprecation/core_deprecations.test.ts | 31 ---- .../config/deprecation/core_deprecations.ts | 11 -- .../elasticsearch_config.test.ts | 43 +++++ .../elasticsearch/elasticsearch_config.ts | 166 ++++++++++-------- src/core/server/server.ts | 4 + 5 files changed, 140 insertions(+), 115 deletions(-) diff --git a/src/core/server/config/deprecation/core_deprecations.test.ts b/src/core/server/config/deprecation/core_deprecations.test.ts index 7851522ec899f..b40dbdc1b6651 100644 --- a/src/core/server/config/deprecation/core_deprecations.test.ts +++ b/src/core/server/config/deprecation/core_deprecations.test.ts @@ -208,35 +208,4 @@ describe('core deprecations', () => { ).toEqual([`worker-src blob:`]); }); }); - - describe('elasticsearchUsernameDeprecation', () => { - it('logs a warning if elasticsearch.username is set to "elastic"', () => { - const { messages } = applyCoreDeprecations({ - elasticsearch: { - username: 'elastic', - }, - }); - expect(messages).toMatchInlineSnapshot(` - Array [ - "Setting elasticsearch.username to \\"elastic\\" is deprecated. You should use the \\"kibana\\" user instead.", - ] - `); - }); - - it('does not log a warning if elasticsearch.username is set to something besides "elastic"', () => { - const { messages } = applyCoreDeprecations({ - elasticsearch: { - username: 'otheruser', - }, - }); - expect(messages).toHaveLength(0); - }); - - it('does not log a warning if elasticsearch.username is unset', () => { - const { messages } = applyCoreDeprecations({ - elasticsearch: {}, - }); - expect(messages).toHaveLength(0); - }); - }); }); diff --git a/src/core/server/config/deprecation/core_deprecations.ts b/src/core/server/config/deprecation/core_deprecations.ts index e3b66414ee163..36fe95e05cb53 100644 --- a/src/core/server/config/deprecation/core_deprecations.ts +++ b/src/core/server/config/deprecation/core_deprecations.ts @@ -91,16 +91,6 @@ const cspRulesDeprecation: ConfigDeprecation = (settings, fromPath, log) => { return settings; }; -const elasticsearchUsernameDeprecation: ConfigDeprecation = (settings, _fromPath, log) => { - const username: string | undefined = get(settings, 'elasticsearch.username'); - if (username === 'elastic') { - log( - `Setting elasticsearch.username to "elastic" is deprecated. You should use the "kibana" user instead.` - ); - } - return settings; -}; - export const coreDeprecationProvider: ConfigDeprecationProvider = ({ unusedFromRoot, renameFromRoot, @@ -120,5 +110,4 @@ export const coreDeprecationProvider: ConfigDeprecationProvider = ({ dataPathDeprecation, rewriteBasePathDeprecation, cspRulesDeprecation, - elasticsearchUsernameDeprecation, ]; diff --git a/src/core/server/elasticsearch/elasticsearch_config.test.ts b/src/core/server/elasticsearch/elasticsearch_config.test.ts index c0db7369b4b99..d6b5cce628513 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.test.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.test.ts @@ -26,6 +26,9 @@ import { import { ElasticsearchConfig, config, ElasticsearchConfigType } from './elasticsearch_config'; import { loggingServiceMock } from '../mocks'; import { Logger } from '../logging'; +import { applyDeprecations, configDeprecationFactory } from '../config/deprecation'; + +const CONFIG_PATH = 'elasticsearch'; const createElasticsearchConfig = (rawConfig: ElasticsearchConfigType, log?: Logger) => { if (!log) { @@ -34,6 +37,25 @@ const createElasticsearchConfig = (rawConfig: ElasticsearchConfigType, log?: Log return new ElasticsearchConfig(rawConfig, log); }; +const applyElasticsearchDeprecations = (settings: Record = {}) => { + const deprecations = config.deprecations!(configDeprecationFactory); + const deprecationMessages: string[] = []; + const _config: any = {}; + _config[CONFIG_PATH] = settings; + const migrated = applyDeprecations( + _config, + deprecations.map(deprecation => ({ + deprecation, + path: CONFIG_PATH, + })), + msg => deprecationMessages.push(msg) + ); + return { + messages: deprecationMessages, + migrated, + }; +}; + test('set correct defaults', () => { const configValue = createElasticsearchConfig(config.schema.validate({})); expect(configValue).toMatchInlineSnapshot(` @@ -319,6 +341,27 @@ describe('logs warnings', () => { }); }); +describe('deprecations', () => { + it('logs a warning if elasticsearch.username is set to "elastic"', () => { + const { messages } = applyElasticsearchDeprecations({ username: 'elastic' }); + expect(messages).toMatchInlineSnapshot(` + Array [ + "Setting [${CONFIG_PATH}.username] to \\"elastic\\" is deprecated. You should use the \\"kibana\\" user instead.", + ] + `); + }); + + it('does not log a warning if elasticsearch.username is set to something besides "elastic"', () => { + const { messages } = applyElasticsearchDeprecations({ username: 'otheruser' }); + expect(messages).toHaveLength(0); + }); + + it('does not log a warning if elasticsearch.username is unset', () => { + const { messages } = applyElasticsearchDeprecations({}); + expect(messages).toHaveLength(0); + }); +}); + test('#username throws if equal to "elastic", only while running from source', () => { const obj = { username: 'elastic', diff --git a/src/core/server/elasticsearch/elasticsearch_config.ts b/src/core/server/elasticsearch/elasticsearch_config.ts index 815005f65c6e7..6f22a28ec3376 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.ts @@ -20,92 +20,112 @@ import { schema, TypeOf } from '@kbn/config-schema'; import { Duration } from 'moment'; import { readFileSync } from 'fs'; +import { ConfigDeprecationProvider } from 'src/core/server'; import { readPkcs12Keystore, readPkcs12Truststore } from '../../utils'; import { Logger } from '../logging'; +import { ServiceConfigDescriptor } from '../internal_types'; const hostURISchema = schema.uri({ scheme: ['http', 'https'] }); export const DEFAULT_API_VERSION = 'master'; -export type ElasticsearchConfigType = TypeOf; +export type ElasticsearchConfigType = TypeOf; type SslConfigSchema = ElasticsearchConfigType['ssl']; -export const config = { - path: 'elasticsearch', - schema: schema.object({ - sniffOnStart: schema.boolean({ defaultValue: false }), - sniffInterval: schema.oneOf([schema.duration(), schema.literal(false)], { - defaultValue: false, - }), - sniffOnConnectionFault: schema.boolean({ defaultValue: false }), - hosts: schema.oneOf([hostURISchema, schema.arrayOf(hostURISchema, { minSize: 1 })], { - defaultValue: 'http://localhost:9200', - }), - preserveHost: schema.boolean({ defaultValue: true }), - username: schema.maybe( - schema.conditional( - schema.contextRef('dist'), - false, - schema.string({ - validate: rawConfig => { - if (rawConfig === 'elastic') { - return ( - 'value of "elastic" is forbidden. This is a superuser account that can obfuscate ' + - 'privilege-related issues. You should use the "kibana" user instead.' - ); - } - }, - }), - schema.string() - ) - ), - password: schema.maybe(schema.string()), - requestHeadersWhitelist: schema.oneOf([schema.string(), schema.arrayOf(schema.string())], { - defaultValue: ['authorization'], - }), - customHeaders: schema.recordOf(schema.string(), schema.string(), { defaultValue: {} }), - shardTimeout: schema.duration({ defaultValue: '30s' }), - requestTimeout: schema.duration({ defaultValue: '30s' }), - pingTimeout: schema.duration({ defaultValue: schema.siblingRef('requestTimeout') }), - startupTimeout: schema.duration({ defaultValue: '5s' }), - logQueries: schema.boolean({ defaultValue: false }), - ssl: schema.object( - { - verificationMode: schema.oneOf( - [schema.literal('none'), schema.literal('certificate'), schema.literal('full')], - { defaultValue: 'full' } - ), - certificateAuthorities: schema.maybe( - schema.oneOf([schema.string(), schema.arrayOf(schema.string(), { minSize: 1 })]) - ), - certificate: schema.maybe(schema.string()), - key: schema.maybe(schema.string()), - keyPassphrase: schema.maybe(schema.string()), - keystore: schema.object({ - path: schema.maybe(schema.string()), - password: schema.maybe(schema.string()), - }), - truststore: schema.object({ - path: schema.maybe(schema.string()), - password: schema.maybe(schema.string()), - }), - alwaysPresentCertificate: schema.boolean({ defaultValue: false }), - }, - { +const configSchema = schema.object({ + sniffOnStart: schema.boolean({ defaultValue: false }), + sniffInterval: schema.oneOf([schema.duration(), schema.literal(false)], { + defaultValue: false, + }), + sniffOnConnectionFault: schema.boolean({ defaultValue: false }), + hosts: schema.oneOf([hostURISchema, schema.arrayOf(hostURISchema, { minSize: 1 })], { + defaultValue: 'http://localhost:9200', + }), + preserveHost: schema.boolean({ defaultValue: true }), + username: schema.maybe( + schema.conditional( + schema.contextRef('dist'), + false, + schema.string({ validate: rawConfig => { - if (rawConfig.key && rawConfig.keystore.path) { - return 'cannot use [key] when [keystore.path] is specified'; - } - if (rawConfig.certificate && rawConfig.keystore.path) { - return 'cannot use [certificate] when [keystore.path] is specified'; + if (rawConfig === 'elastic') { + return ( + 'value of "elastic" is forbidden. This is a superuser account that can obfuscate ' + + 'privilege-related issues. You should use the "kibana" user instead.' + ); } }, - } - ), - apiVersion: schema.string({ defaultValue: DEFAULT_API_VERSION }), - healthCheck: schema.object({ delay: schema.duration({ defaultValue: 2500 }) }), - ignoreVersionMismatch: schema.boolean({ defaultValue: false }), + }), + schema.string() + ) + ), + password: schema.maybe(schema.string()), + requestHeadersWhitelist: schema.oneOf([schema.string(), schema.arrayOf(schema.string())], { + defaultValue: ['authorization'], }), + customHeaders: schema.recordOf(schema.string(), schema.string(), { defaultValue: {} }), + shardTimeout: schema.duration({ defaultValue: '30s' }), + requestTimeout: schema.duration({ defaultValue: '30s' }), + pingTimeout: schema.duration({ defaultValue: schema.siblingRef('requestTimeout') }), + startupTimeout: schema.duration({ defaultValue: '5s' }), + logQueries: schema.boolean({ defaultValue: false }), + ssl: schema.object( + { + verificationMode: schema.oneOf( + [schema.literal('none'), schema.literal('certificate'), schema.literal('full')], + { defaultValue: 'full' } + ), + certificateAuthorities: schema.maybe( + schema.oneOf([schema.string(), schema.arrayOf(schema.string(), { minSize: 1 })]) + ), + certificate: schema.maybe(schema.string()), + key: schema.maybe(schema.string()), + keyPassphrase: schema.maybe(schema.string()), + keystore: schema.object({ + path: schema.maybe(schema.string()), + password: schema.maybe(schema.string()), + }), + truststore: schema.object({ + path: schema.maybe(schema.string()), + password: schema.maybe(schema.string()), + }), + alwaysPresentCertificate: schema.boolean({ defaultValue: false }), + }, + { + validate: rawConfig => { + if (rawConfig.key && rawConfig.keystore.path) { + return 'cannot use [key] when [keystore.path] is specified'; + } + if (rawConfig.certificate && rawConfig.keystore.path) { + return 'cannot use [certificate] when [keystore.path] is specified'; + } + }, + } + ), + apiVersion: schema.string({ defaultValue: DEFAULT_API_VERSION }), + healthCheck: schema.object({ delay: schema.duration({ defaultValue: 2500 }) }), + ignoreVersionMismatch: schema.boolean({ defaultValue: false }), +}); + +const deprecations: ConfigDeprecationProvider = () => [ + (settings, fromPath, log) => { + const es = settings[fromPath]; + if (!es) { + return settings; + } + if (es.username === 'elastic') { + log( + `Setting [${fromPath}.username] to "elastic" is deprecated. You should use the "kibana" user instead.` + ); + } + return settings; + }, +]; + +export const config: ServiceConfigDescriptor = { + path: 'elasticsearch', + schema: configSchema, + deprecations, }; export class ElasticsearchConfig { diff --git a/src/core/server/server.ts b/src/core/server/server.ts index eced24b84908c..611842e8a7de0 100644 --- a/src/core/server/server.ts +++ b/src/core/server/server.ts @@ -256,6 +256,10 @@ export class Server { ]; this.configService.addDeprecationProvider(rootConfigPath, coreDeprecationProvider); + this.configService.addDeprecationProvider( + elasticsearchConfig.path, + elasticsearchConfig.deprecations! + ); this.configService.addDeprecationProvider( uiSettingsConfig.path, uiSettingsConfig.deprecations! From a19698a0c1ca20c29397592d4bdc1d7ca4282a43 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Thu, 9 Jan 2020 12:57:54 -0500 Subject: [PATCH 2/6] Add ElasticsearchConfig SSL deprecations --- .../elasticsearch/elasticsearch_config.test.ts | 18 ++++++++++++++++++ .../elasticsearch/elasticsearch_config.ts | 9 +++++++++ 2 files changed, 27 insertions(+) diff --git a/src/core/server/elasticsearch/elasticsearch_config.test.ts b/src/core/server/elasticsearch/elasticsearch_config.test.ts index d6b5cce628513..a9001568691a9 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.test.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.test.ts @@ -360,6 +360,24 @@ describe('deprecations', () => { const { messages } = applyElasticsearchDeprecations({}); expect(messages).toHaveLength(0); }); + + it('logs a warning if ssl.key is set and ssl.certificate is not', () => { + const { messages } = applyElasticsearchDeprecations({ ssl: { key: 'foo' } }); + expect(messages).toMatchInlineSnapshot(` + Array [ + "Setting [${CONFIG_PATH}.ssl.key] without [${CONFIG_PATH}.ssl.certificate] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.", + ] + `); + }); + + it('logs a warning if ssl.certificate is set and ssl.key is not', () => { + const { messages } = applyElasticsearchDeprecations({ ssl: { certificate: 'foo' } }); + expect(messages).toMatchInlineSnapshot(` + Array [ + "Setting [${CONFIG_PATH}.ssl.certificate] without [${CONFIG_PATH}.ssl.key] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.", + ] + `); + }); }); test('#username throws if equal to "elastic", only while running from source', () => { diff --git a/src/core/server/elasticsearch/elasticsearch_config.ts b/src/core/server/elasticsearch/elasticsearch_config.ts index 6f22a28ec3376..14a8165233c51 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.ts @@ -118,6 +118,15 @@ const deprecations: ConfigDeprecationProvider = () => [ `Setting [${fromPath}.username] to "elastic" is deprecated. You should use the "kibana" user instead.` ); } + if (es.ssl?.key && !es.ssl?.certificate) { + log( + `Setting [${fromPath}.ssl.key] without [${fromPath}.ssl.certificate] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.` + ); + } else if (es.ssl?.certificate && !es.ssl?.key) { + log( + `Setting [${fromPath}.ssl.certificate] without [${fromPath}.ssl.key] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.` + ); + } return settings; }, ]; From 906c1004259c87e84ffaa857a4216ddee846a9cf Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Thu, 9 Jan 2020 13:05:12 -0500 Subject: [PATCH 3/6] Remove logger from ElasticsearchConfig This was used to log warnings messages. We have decided to deprecate the config settings instead of just logging warnings. So ElasticsearchConfig doesn't need to have a logger at all anymore. --- .../elasticsearch_config.test.ts | 95 ++++++------------- .../elasticsearch/elasticsearch_config.ts | 9 +- .../elasticsearch/elasticsearch_service.ts | 2 +- 3 files changed, 31 insertions(+), 75 deletions(-) diff --git a/src/core/server/elasticsearch/elasticsearch_config.test.ts b/src/core/server/elasticsearch/elasticsearch_config.test.ts index a9001568691a9..3196c8e32efb8 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.test.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.test.ts @@ -23,20 +23,11 @@ import { mockReadPkcs12Truststore, } from './elasticsearch_config.test.mocks'; -import { ElasticsearchConfig, config, ElasticsearchConfigType } from './elasticsearch_config'; -import { loggingServiceMock } from '../mocks'; -import { Logger } from '../logging'; +import { ElasticsearchConfig, config } from './elasticsearch_config'; import { applyDeprecations, configDeprecationFactory } from '../config/deprecation'; const CONFIG_PATH = 'elasticsearch'; -const createElasticsearchConfig = (rawConfig: ElasticsearchConfigType, log?: Logger) => { - if (!log) { - log = loggingServiceMock.create().get('config'); - } - return new ElasticsearchConfig(rawConfig, log); -}; - const applyElasticsearchDeprecations = (settings: Record = {}) => { const deprecations = config.deprecations!(configDeprecationFactory); const deprecationMessages: string[] = []; @@ -57,7 +48,7 @@ const applyElasticsearchDeprecations = (settings: Record = {}) => { }; test('set correct defaults', () => { - const configValue = createElasticsearchConfig(config.schema.validate({})); + const configValue = new ElasticsearchConfig(config.schema.validate({})); expect(configValue).toMatchInlineSnapshot(` ElasticsearchConfig { "apiVersion": "master", @@ -92,17 +83,17 @@ test('set correct defaults', () => { }); test('#hosts accepts both string and array of strings', () => { - let configValue = createElasticsearchConfig( + let configValue = new ElasticsearchConfig( config.schema.validate({ hosts: 'http://some.host:1234' }) ); expect(configValue.hosts).toEqual(['http://some.host:1234']); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ hosts: ['http://some.host:1234'] }) ); expect(configValue.hosts).toEqual(['http://some.host:1234']); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ hosts: ['http://some.host:1234', 'https://some.another.host'], }) @@ -111,17 +102,17 @@ test('#hosts accepts both string and array of strings', () => { }); test('#requestHeadersWhitelist accepts both string and array of strings', () => { - let configValue = createElasticsearchConfig( + let configValue = new ElasticsearchConfig( config.schema.validate({ requestHeadersWhitelist: 'token' }) ); expect(configValue.requestHeadersWhitelist).toEqual(['token']); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ requestHeadersWhitelist: ['token'] }) ); expect(configValue.requestHeadersWhitelist).toEqual(['token']); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ requestHeadersWhitelist: ['token', 'X-Forwarded-Proto'], }) @@ -144,7 +135,7 @@ describe('reads files', () => { }); it('reads certificate authorities when ssl.keystore.path is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { keystore: { path: 'some-path' } } }) ); expect(mockReadPkcs12Keystore).toHaveBeenCalledTimes(1); @@ -152,7 +143,7 @@ describe('reads files', () => { }); it('reads certificate authorities when ssl.truststore.path is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { truststore: { path: 'some-path' } } }) ); expect(mockReadPkcs12Truststore).toHaveBeenCalledTimes(1); @@ -160,21 +151,21 @@ describe('reads files', () => { }); it('reads certificate authorities when ssl.certificateAuthorities is specified', () => { - let configValue = createElasticsearchConfig( + let configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { certificateAuthorities: 'some-path' } }) ); expect(mockReadFileSync).toHaveBeenCalledTimes(1); expect(configValue.ssl.certificateAuthorities).toEqual(['content-of-some-path']); mockReadFileSync.mockClear(); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { certificateAuthorities: ['some-path'] } }) ); expect(mockReadFileSync).toHaveBeenCalledTimes(1); expect(configValue.ssl.certificateAuthorities).toEqual(['content-of-some-path']); mockReadFileSync.mockClear(); - configValue = createElasticsearchConfig( + configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { certificateAuthorities: ['some-path', 'another-path'] }, }) @@ -187,7 +178,7 @@ describe('reads files', () => { }); it('reads certificate authorities when ssl.keystore.path, ssl.truststore.path, and ssl.certificateAuthorities are specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { keystore: { path: 'some-path' }, @@ -207,7 +198,7 @@ describe('reads files', () => { }); it('reads a private key and certificate when ssl.keystore.path is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { keystore: { path: 'some-path' } } }) ); expect(mockReadPkcs12Keystore).toHaveBeenCalledTimes(1); @@ -216,7 +207,7 @@ describe('reads files', () => { }); it('reads a private key when ssl.key is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { key: 'some-path' } }) ); expect(mockReadFileSync).toHaveBeenCalledTimes(1); @@ -224,7 +215,7 @@ describe('reads files', () => { }); it('reads a certificate when ssl.certificate is specified', () => { - const configValue = createElasticsearchConfig( + const configValue = new ElasticsearchConfig( config.schema.validate({ ssl: { certificate: 'some-path' } }) ); expect(mockReadFileSync).toHaveBeenCalledTimes(1); @@ -247,8 +238,8 @@ describe('throws when config is invalid', () => { it('throws if key is invalid', () => { const value = { ssl: { key: '/invalid/key' } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"ENOENT: no such file or directory, open '/invalid/key'"` ); @@ -256,8 +247,8 @@ describe('throws when config is invalid', () => { it('throws if certificate is invalid', () => { const value = { ssl: { certificate: '/invalid/cert' } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"ENOENT: no such file or directory, open '/invalid/cert'"` ); @@ -265,15 +256,15 @@ describe('throws when config is invalid', () => { it('throws if certificateAuthorities is invalid', () => { const value = { ssl: { certificateAuthorities: '/invalid/ca' } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot(`"ENOENT: no such file or directory, open '/invalid/ca'"`); }); it('throws if keystore path is invalid', () => { const value = { ssl: { keystore: { path: '/invalid/keystore' } } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"ENOENT: no such file or directory, open '/invalid/keystore'"` ); @@ -282,8 +273,8 @@ describe('throws when config is invalid', () => { it('throws if keystore does not contain a key or certificate', () => { mockReadPkcs12Keystore.mockReturnValueOnce({}); const value = { ssl: { keystore: { path: 'some-path' } } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"Did not find key or certificate in Elasticsearch keystore."` ); @@ -291,8 +282,8 @@ describe('throws when config is invalid', () => { it('throws if truststore path is invalid', () => { const value = { ssl: { keystore: { path: '/invalid/truststore' } } }; - expect(() => - createElasticsearchConfig(config.schema.validate(value)) + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) ).toThrowErrorMatchingInlineSnapshot( `"ENOENT: no such file or directory, open '/invalid/truststore'"` ); @@ -313,34 +304,6 @@ describe('throws when config is invalid', () => { }); }); -describe('logs warnings', () => { - let logger: ReturnType; - let log: Logger; - - beforeAll(() => { - mockReadFileSync.mockResolvedValue('foo'); - }); - - beforeEach(() => { - logger = loggingServiceMock.create(); - log = logger.get('config'); - }); - - it('warns if ssl.key is set and ssl.certificate is not', () => { - createElasticsearchConfig(config.schema.validate({ ssl: { key: 'some-path' } }), log); - expect(loggingServiceMock.collect(logger).warn[0][0]).toMatchInlineSnapshot( - `"Detected a key without a certificate; mutual TLS authentication is disabled."` - ); - }); - - it('warns if ssl.certificate is set and ssl.key is not', () => { - createElasticsearchConfig(config.schema.validate({ ssl: { certificate: 'some-path' } }), log); - expect(loggingServiceMock.collect(logger).warn[0][0]).toMatchInlineSnapshot( - `"Detected a certificate without a key; mutual TLS authentication is disabled."` - ); - }); -}); - describe('deprecations', () => { it('logs a warning if elasticsearch.username is set to "elastic"', () => { const { messages } = applyElasticsearchDeprecations({ username: 'elastic' }); diff --git a/src/core/server/elasticsearch/elasticsearch_config.ts b/src/core/server/elasticsearch/elasticsearch_config.ts index 14a8165233c51..e254f95166b6f 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.ts @@ -22,7 +22,6 @@ import { Duration } from 'moment'; import { readFileSync } from 'fs'; import { ConfigDeprecationProvider } from 'src/core/server'; import { readPkcs12Keystore, readPkcs12Truststore } from '../../utils'; -import { Logger } from '../logging'; import { ServiceConfigDescriptor } from '../internal_types'; const hostURISchema = schema.uri({ scheme: ['http', 'https'] }); @@ -234,7 +233,7 @@ export class ElasticsearchConfig { */ public readonly customHeaders: ElasticsearchConfigType['customHeaders']; - constructor(rawConfig: ElasticsearchConfigType, log: Logger) { + constructor(rawConfig: ElasticsearchConfigType) { this.ignoreVersionMismatch = rawConfig.ignoreVersionMismatch; this.apiVersion = rawConfig.apiVersion; this.logQueries = rawConfig.logQueries; @@ -256,12 +255,6 @@ export class ElasticsearchConfig { const { alwaysPresentCertificate, verificationMode } = rawConfig.ssl; const { key, keyPassphrase, certificate, certificateAuthorities } = readKeyAndCerts(rawConfig); - if (key && !certificate) { - log.warn(`Detected a key without a certificate; mutual TLS authentication is disabled.`); - } else if (certificate && !key) { - log.warn(`Detected a certificate without a key; mutual TLS authentication is disabled.`); - } - this.ssl = { alwaysPresentCertificate, key, diff --git a/src/core/server/elasticsearch/elasticsearch_service.ts b/src/core/server/elasticsearch/elasticsearch_service.ts index de32e7f6cf225..db3fda3a504ab 100644 --- a/src/core/server/elasticsearch/elasticsearch_service.ts +++ b/src/core/server/elasticsearch/elasticsearch_service.ts @@ -52,7 +52,7 @@ export class ElasticsearchService implements CoreService('elasticsearch') - .pipe(map(rawConfig => new ElasticsearchConfig(rawConfig, coreContext.logger.get('config')))); + .pipe(map(rawConfig => new ElasticsearchConfig(rawConfig))); } public async setup(deps: SetupDeps): Promise { From 7c247efa5b289e52aec5ccb36f43fddff44c8856 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Thu, 9 Jan 2020 15:07:32 -0500 Subject: [PATCH 4/6] node scripts/check_core_api_changes.js --accept --- src/core/server/server.api.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/core/server/server.api.md b/src/core/server/server.api.md index 72934175f96e5..36b613aaebdee 100644 --- a/src/core/server/server.api.md +++ b/src/core/server/server.api.md @@ -450,11 +450,11 @@ export interface AuthToolkit { export class BasePath { // @internal constructor(serverBasePath?: string); - get: (request: KibanaRequest | LegacyRequest) => string; + get: (request: LegacyRequest | KibanaRequest) => string; prepend: (path: string) => string; remove: (path: string) => string; readonly serverBasePath: string; - set: (request: KibanaRequest | LegacyRequest, requestSpecificBasePath: string) => void; + set: (request: LegacyRequest | KibanaRequest, requestSpecificBasePath: string) => void; } // Warning: (ae-forgotten-export) The symbol "BootstrapArgs" needs to be exported by the entry point index.d.ts From 75d0f706cc07bd078231d9cd6cf90046d7221cfc Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Sat, 11 Jan 2020 13:46:46 -0500 Subject: [PATCH 5/6] Changed keystore parse error to be more strict The Elasticsearch config should error out if a PKCS12 keystore does not contain a key *or* a certificate. This was intended to be the functionality in PR #53810, but it was overlooked. Changing it now since this PR is changing code in the same file. --- .../elasticsearch/elasticsearch_config.test.ts | 14 ++++++++++---- .../server/elasticsearch/elasticsearch_config.ts | 6 ++++-- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/src/core/server/elasticsearch/elasticsearch_config.test.ts b/src/core/server/elasticsearch/elasticsearch_config.test.ts index 3196c8e32efb8..3833e585ce851 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.test.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.test.ts @@ -270,14 +270,20 @@ describe('throws when config is invalid', () => { ); }); - it('throws if keystore does not contain a key or certificate', () => { + it('throws if keystore does not contain a key', () => { mockReadPkcs12Keystore.mockReturnValueOnce({}); const value = { ssl: { keystore: { path: 'some-path' } } }; expect( () => new ElasticsearchConfig(config.schema.validate(value)) - ).toThrowErrorMatchingInlineSnapshot( - `"Did not find key or certificate in Elasticsearch keystore."` - ); + ).toThrowErrorMatchingInlineSnapshot(`"Did not find key in Elasticsearch keystore."`); + }); + + it('throws if keystore does not contain a certificate', () => { + mockReadPkcs12Keystore.mockReturnValueOnce({ key: 'foo' }); + const value = { ssl: { keystore: { path: 'some-path' } } }; + expect( + () => new ElasticsearchConfig(config.schema.validate(value)) + ).toThrowErrorMatchingInlineSnapshot(`"Did not find certificate in Elasticsearch keystore."`); }); it('throws if truststore path is invalid', () => { diff --git a/src/core/server/elasticsearch/elasticsearch_config.ts b/src/core/server/elasticsearch/elasticsearch_config.ts index e254f95166b6f..9373477158b41 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.ts @@ -283,8 +283,10 @@ const readKeyAndCerts = (rawConfig: ElasticsearchConfigType) => { rawConfig.ssl.keystore.path, rawConfig.ssl.keystore.password ); - if (!keystore.key && !keystore.cert) { - throw new Error(`Did not find key or certificate in Elasticsearch keystore.`); + if (!keystore.key) { + throw new Error(`Did not find key in Elasticsearch keystore.`); + } else if (!keystore.cert) { + throw new Error(`Did not find certificate in Elasticsearch keystore.`); } key = keystore.key; certificate = keystore.cert; From 3120c8f0771882f82c54bd8c7fe81b7aad64f4a1 Mon Sep 17 00:00:00 2001 From: Joe Portner <5295965+jportner@users.noreply.github.com> Date: Sat, 11 Jan 2020 13:47:40 -0500 Subject: [PATCH 6/6] Address PR review feedback Check for key existence, not key truthiness. --- .../server/elasticsearch/elasticsearch_config.test.ts | 9 +++++++-- src/core/server/elasticsearch/elasticsearch_config.ts | 4 ++-- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/core/server/elasticsearch/elasticsearch_config.test.ts b/src/core/server/elasticsearch/elasticsearch_config.test.ts index 3833e585ce851..1b4fc5eafec76 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.test.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.test.ts @@ -331,7 +331,7 @@ describe('deprecations', () => { }); it('logs a warning if ssl.key is set and ssl.certificate is not', () => { - const { messages } = applyElasticsearchDeprecations({ ssl: { key: 'foo' } }); + const { messages } = applyElasticsearchDeprecations({ ssl: { key: '' } }); expect(messages).toMatchInlineSnapshot(` Array [ "Setting [${CONFIG_PATH}.ssl.key] without [${CONFIG_PATH}.ssl.certificate] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.", @@ -340,13 +340,18 @@ describe('deprecations', () => { }); it('logs a warning if ssl.certificate is set and ssl.key is not', () => { - const { messages } = applyElasticsearchDeprecations({ ssl: { certificate: 'foo' } }); + const { messages } = applyElasticsearchDeprecations({ ssl: { certificate: '' } }); expect(messages).toMatchInlineSnapshot(` Array [ "Setting [${CONFIG_PATH}.ssl.certificate] without [${CONFIG_PATH}.ssl.key] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.", ] `); }); + + it('does not log a warning if both ssl.key and ssl.certificate are set', () => { + const { messages } = applyElasticsearchDeprecations({ ssl: { key: '', certificate: '' } }); + expect(messages).toEqual([]); + }); }); test('#username throws if equal to "elastic", only while running from source', () => { diff --git a/src/core/server/elasticsearch/elasticsearch_config.ts b/src/core/server/elasticsearch/elasticsearch_config.ts index 9373477158b41..5f06c51a53d53 100644 --- a/src/core/server/elasticsearch/elasticsearch_config.ts +++ b/src/core/server/elasticsearch/elasticsearch_config.ts @@ -117,11 +117,11 @@ const deprecations: ConfigDeprecationProvider = () => [ `Setting [${fromPath}.username] to "elastic" is deprecated. You should use the "kibana" user instead.` ); } - if (es.ssl?.key && !es.ssl?.certificate) { + if (es.ssl?.key !== undefined && es.ssl?.certificate === undefined) { log( `Setting [${fromPath}.ssl.key] without [${fromPath}.ssl.certificate] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.` ); - } else if (es.ssl?.certificate && !es.ssl?.key) { + } else if (es.ssl?.certificate !== undefined && es.ssl?.key === undefined) { log( `Setting [${fromPath}.ssl.certificate] without [${fromPath}.ssl.key] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.` );