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.

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

Jump to bottom

Update security deprecation messages #115241

Merged
merged 5 commits into from
Oct 18, 2021
Merged

Update security deprecation messages #115241

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3913a50
Update security deprecations
jportner Oct 15, 2021
ace4b2c
Merge branch 'master' into update-security-deprecation-messages
kibanamachine Oct 15, 2021
e584ec4
PR review feedback
jportner Oct 15, 2021
2c66a36
Merge branch 'master' into update-security-deprecation-messages
kibanamachine Oct 18, 2021
b409027
Add deprecation levels for other config deprecations
jportner Oct 18, 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
8 changes: 4 additions & 4 deletions src/core/server/elasticsearch/elasticsearch_config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ describe('deprecations', () => {
const { messages } = applyElasticsearchDeprecations({ username: 'elastic' });
expect(messages).toMatchInlineSnapshot(`
Array [
"Setting [${CONFIG_PATH}.username] to \\"elastic\\" is deprecated. You should use the \\"kibana_system\\" user instead.",
"Configure Kibana to use a service account token to authenticate to Elasticsearch.",
]
`);
});
Expand All @@ -331,7 +331,7 @@ describe('deprecations', () => {
const { messages } = applyElasticsearchDeprecations({ username: 'kibana' });
expect(messages).toMatchInlineSnapshot(`
Array [
"Setting [${CONFIG_PATH}.username] to \\"kibana\\" is deprecated. You should use the \\"kibana_system\\" user instead.",
"Configure Kibana to use a service account token to authenticate to Elasticsearch.",
]
`);
});
Expand All @@ -350,7 +350,7 @@ describe('deprecations', () => {
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.",
"Use both settings to enable Kibana to use Mutual TLS authentication with Elasticsearch.",
]
`);
});
Expand All @@ -359,7 +359,7 @@ describe('deprecations', () => {
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.",
"Use both settings to enable Kibana to use Mutual TLS authentication with Elasticsearch.",
]
`);
});
Expand Down
85 changes: 59 additions & 26 deletions src/core/server/elasticsearch/elasticsearch_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed Core deprecations here, but these particular ones were added by the Platform Security team, so I thought it would be prudent to update them along with our other deprecations 👍

import { schema, TypeOf } from '@kbn/config-schema';
import { readPkcs12Keystore, readPkcs12Truststore } from '@kbn/crypto';
import { i18n } from '@kbn/i18n';
import { Duration } from 'moment';
import { readFileSync } from 'fs';
import { ConfigDeprecationProvider } from 'src/core/server';
Expand Down Expand Up @@ -171,49 +172,81 @@ export const configSchema = schema.object({
});

const deprecations: ConfigDeprecationProvider = () => [
(settings, fromPath, addDeprecation) => {
(settings, fromPath, addDeprecation, { branch }) => {
const es = settings[fromPath];
if (!es) {
return;
}
if (es.username === 'elastic') {
addDeprecation({
configPath: `${fromPath}.username`,
message: `Setting [${fromPath}.username] to "elastic" is deprecated. You should use the "kibana_system" user instead.`,
correctiveActions: {
manualSteps: [`Replace [${fromPath}.username] from "elastic" to "kibana_system".`],
},
});
} else if (es.username === 'kibana') {

if (es.username === 'elastic' || es.username === 'kibana') {
const username = es.username;
addDeprecation({
configPath: `${fromPath}.username`,
message: `Setting [${fromPath}.username] to "kibana" is deprecated. You should use the "kibana_system" user instead.`,
correctiveActions: {
manualSteps: [`Replace [${fromPath}.username] from "kibana" to "kibana_system".`],
},
});
}
if (es.ssl?.key !== undefined && es.ssl?.certificate === undefined) {
addDeprecation({
configPath: `${fromPath}.ssl.key`,
message: `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.`,
title: i18n.translate('core.deprecations.elasticsearchUsername.title', {
defaultMessage: 'Using "elasticsearch.username: {username}" is deprecated',
jportner marked this conversation as resolved.
Show resolved Hide resolved
values: { username },
}),
message: i18n.translate('core.deprecations.elasticsearchUsername.message', {
defaultMessage:
'Kibana is configured to authenticate to Elasticsearch with the "{username}" user. Use a service account token instead.',
jportner marked this conversation as resolved.
Show resolved Hide resolved
values: { username },
}),
level: 'warning',
documentationUrl: `https://www.elastic.co/guide/en/elasticsearch/reference/${branch}/service-accounts.html`,
correctiveActions: {
manualSteps: [
`Set [${fromPath}.ssl.certificate] in your kibana configs to enable TLS client authentication to Elasticsearch.`,
i18n.translate('core.deprecations.elasticsearchUsername.manualSteps1', {
defaultMessage:
'Use the elasticsearch-service-tokens CLI tool to create a new service account token for the "elastic/kibana" service account.',
}),
i18n.translate('core.deprecations.elasticsearchUsername.manualSteps2', {
defaultMessage: 'Add the "elasticsearch.serviceAccountToken" setting to kibana.yml.',
}),
i18n.translate('core.deprecations.elasticsearchUsername.manualSteps3', {
defaultMessage:
'Remove "elasticsearch.username" and "elasticsearch.password" from kibana.yml.',
TinaHeiligers marked this conversation as resolved.
Show resolved Hide resolved
}),
],
},
});
} else if (es.ssl?.certificate !== undefined && es.ssl?.key === undefined) {
}

const addSslDeprecation = (existingSetting: string, missingSetting: string) => {
addDeprecation({
configPath: `${fromPath}.ssl.certificate`,
message: `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.`,
configPath: existingSetting,
title: i18n.translate('core.deprecations.elasticsearchSSL.title', {
defaultMessage: 'Using "{existingSetting}" without "{missingSetting}" has no effect',
values: { existingSetting, missingSetting },
}),
message: i18n.translate('core.deprecations.elasticsearchSSL.message', {
defaultMessage:
'Use both settings to enable Kibana to use Mutual TLS authentication with Elasticsearch.',
jportner marked this conversation as resolved.
Show resolved Hide resolved
}),
level: 'warning',
documentationUrl: `https://www.elastic.co/guide/en/kibana/${branch}/elasticsearch-mutual-tls.html`,
correctiveActions: {
manualSteps: [
`Set [${fromPath}.ssl.key] in your kibana configs to enable TLS client authentication to Elasticsearch.`,
i18n.translate('core.deprecations.elasticsearchSSL.manualSteps1', {
defaultMessage: 'Add the "{missingSetting}" setting to kibana.yml.',
values: { missingSetting },
}),
i18n.translate('core.deprecations.elasticsearchSSL.manualSteps2', {
defaultMessage:
'Alternatively, if you do not want to use Mutual TLS authentication, remove "{existingSetting}" from kibana.yml.',
jportner marked this conversation as resolved.
Show resolved Hide resolved
values: { existingSetting },
}),
],
},
});
} else if (es.logQueries === true) {
};

if (es.ssl?.key !== undefined && es.ssl?.certificate === undefined) {
addSslDeprecation(`${fromPath}.ssl.key`, `${fromPath}.ssl.certificate`);
} else if (es.ssl?.certificate !== undefined && es.ssl?.key === undefined) {
addSslDeprecation(`${fromPath}.ssl.certificate`, `${fromPath}.ssl.key`);
}

if (es.logQueries === true) {
addDeprecation({
configPath: `${fromPath}.logQueries`,
message: `Setting [${fromPath}.logQueries] is deprecated and no longer used. You should set the log level to "debug" for the "elasticsearch.queries" context in "logging.loggers".`,
Expand Down
58 changes: 0 additions & 58 deletions x-pack/plugins/monitoring/server/deprecations.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,64 +67,6 @@ describe('monitoring plugin deprecations', function () {
});
});

describe('elasticsearch.username', function () {
it('logs a warning if elasticsearch.username is set to "elastic"', () => {
const settings = { elasticsearch: { username: 'elastic' } };

const addDeprecation = jest.fn();
transformDeprecations(settings, fromPath, addDeprecation);
expect(addDeprecation).toHaveBeenCalled();
});

it('logs a warning if elasticsearch.username is set to "kibana"', () => {
const settings = { elasticsearch: { username: 'kibana' } };

const addDeprecation = jest.fn();
transformDeprecations(settings, fromPath, addDeprecation);
expect(addDeprecation).toHaveBeenCalled();
});

it('does not log a warning if elasticsearch.username is set to something besides "elastic" or "kibana"', () => {
const settings = { elasticsearch: { username: 'otheruser' } };

const addDeprecation = jest.fn();
transformDeprecations(settings, fromPath, addDeprecation);
expect(addDeprecation).not.toHaveBeenCalled();
});

it('does not log a warning if elasticsearch.username is unset', () => {
const settings = { elasticsearch: { username: undefined } };

const addDeprecation = jest.fn();
transformDeprecations(settings, fromPath, addDeprecation);
expect(addDeprecation).not.toHaveBeenCalled();
});

it('logs a warning if ssl.key is set and ssl.certificate is not', () => {
const settings = { elasticsearch: { ssl: { key: '' } } };

const addDeprecation = jest.fn();
transformDeprecations(settings, fromPath, addDeprecation);
expect(addDeprecation).toHaveBeenCalled();
});

it('logs a warning if ssl.certificate is set and ssl.key is not', () => {
const settings = { elasticsearch: { ssl: { certificate: '' } } };

const addDeprecation = jest.fn();
transformDeprecations(settings, fromPath, addDeprecation);
expect(addDeprecation).toHaveBeenCalled();
});

it('does not log a warning if both ssl.key and ssl.certificate are set', () => {
const settings = { elasticsearch: { ssl: { key: '', certificate: '' } } };

const addDeprecation = jest.fn();
transformDeprecations(settings, fromPath, addDeprecation);
expect(addDeprecation).not.toHaveBeenCalled();
});
});

describe('xpack_api_polling_frequency_millis', () => {
it('should call rename for this renamed config key', () => {
const settings = { xpack_api_polling_frequency_millis: 30000 };
Expand Down
57 changes: 7 additions & 50 deletions x-pack/plugins/monitoring/server/deprecations.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,56 +61,13 @@ export const deprecations = ({
}
return config;
},
(config, fromPath, addDeprecation) => {
const es: Record<string, any> = get(config, 'elasticsearch');
if (es) {
if (es.username === 'elastic') {
addDeprecation({
configPath: 'elasticsearch.username',
message: `Setting [${fromPath}.username] to "elastic" is deprecated. You should use the "kibana_system" user instead.`,
correctiveActions: {
manualSteps: [`Replace [${fromPath}.username] from "elastic" to "kibana_system".`],
},
});
} else if (es.username === 'kibana') {
addDeprecation({
configPath: 'elasticsearch.username',
message: `Setting [${fromPath}.username] to "kibana" is deprecated. You should use the "kibana_system" user instead.`,
correctiveActions: {
manualSteps: [`Replace [${fromPath}.username] from "kibana" to "kibana_system".`],
},
});
}
}
return config;
},
(config, fromPath, addDeprecation) => {
const ssl: Record<string, any> = get(config, 'elasticsearch.ssl');
if (ssl) {
if (ssl.key !== undefined && ssl.certificate === undefined) {
addDeprecation({
configPath: 'elasticsearch.ssl.key',
message: `Setting [${fromPath}.key] without [${fromPath}.certificate] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.`,
correctiveActions: {
manualSteps: [
`Set [${fromPath}.ssl.certificate] in your kibana configs to enable TLS client authentication to Elasticsearch.`,
],
},
});
} else if (ssl.certificate !== undefined && ssl.key === undefined) {
addDeprecation({
configPath: 'elasticsearch.ssl.certificate',
message: `Setting [${fromPath}.certificate] without [${fromPath}.key] is deprecated. This has no effect, you should use both settings to enable TLS client authentication to Elasticsearch.`,
correctiveActions: {
manualSteps: [
`Set [${fromPath}.ssl.key] in your kibana configs to enable TLS client authentication to Elasticsearch.`,
],
},
});
}
}
return config;
},
rename('xpack_api_polling_frequency_millis', 'licensing.api_polling_frequency'),

// TODO: Add deprecations for "monitoring.ui.elasticsearch.username: elastic" and "monitoring.ui.elasticsearch.username: kibana".
// TODO: Add deprecations for using "monitoring.ui.elasticsearch.ssl.certificate" without "monitoring.ui.elasticsearch.ssl.key", and
// vice versa.
// ^ These deprecations should only be shown if they are explicitly configured for monitoring -- we should not show Monitoring
// deprecations for these settings if they are inherited from the Core elasticsearch settings.
// See the Core implementation: src/core/server/elasticsearch/elasticsearch_config.ts
Comment on lines +64 to +69
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 monitoring code had identical deprecations for (1), (2), (3), and (4) in the PR description. It would basically make those deprecations show up twice, which is confusing.

We currently don't have the ability to differentiate if these options were set in the raw Monitoring config (monitoring.ui.elasticsearch.*) or if they were inherited from the Core config (elasticsearch.*). Monitoring should only register config deprecations for actual Monitoring config settings.

Since these are not breaking in 8.0, I think we should remove these Monitoring deprecations completely until we refactor the code and can differentiate between the two.

Copy link
Contributor

Choose a reason for hiding this comment

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

Since these are not breaking in 8.0, I think we should remove these Monitoring deprecations completely until we refactor the code and can differentiate between the two

Is there a follow up issue for that? Removing deprecations that seem to be duplicates make sense. I just want to make sure we do actually follow up on this a.s.a.p.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll make a note to create a follow-up after this merges. I'll add a permalink to this section in the follow up issue's description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a follow-up issue for this: #115597

];
};
8 changes: 4 additions & 4 deletions x-pack/plugins/security/server/config_deprecations.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ describe('Config Deprecations', () => {
const { messages, configPaths } = applyConfigDeprecations(cloneDeep(config));
expect(messages).toMatchInlineSnapshot(`
Array [
"\\"xpack.security.authc.providers.saml.<provider-name>.maxRedirectURLSize\\" is no longer used.",
"This setting is no longer used.",
]
`);

Expand All @@ -333,7 +333,7 @@ describe('Config Deprecations', () => {
expect(migrated).toEqual(config);
expect(messages).toMatchInlineSnapshot(`
Array [
"\\"xpack.security.authc.providers\\" accepts an extended \\"object\\" format instead of an array of provider types.",
"Use the new object format instead of an array of provider types.",
]
`);
});
Expand All @@ -352,8 +352,8 @@ describe('Config Deprecations', () => {
expect(migrated).toEqual(config);
expect(messages).toMatchInlineSnapshot(`
Array [
"\\"xpack.security.authc.providers\\" accepts an extended \\"object\\" format instead of an array of provider types.",
"Enabling both \\"basic\\" and \\"token\\" authentication providers in \\"xpack.security.authc.providers\\" is deprecated. Login page will only use \\"token\\" provider.",
"Use the new object format instead of an array of provider types.",
"Use only one of these providers. When both providers are set, the login page will only use the \\"token\\" provider.",
]
`);
});
Expand Down
Loading