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

Update security deprecation messages #115241

Merged

Conversation

jportner
Copy link
Contributor

@jportner jportner commented Oct 15, 2021

See also: #111160

This PR updates various security deprecation messages for display in the Upgrade Assistant. Note that currently the Upgrade Assistant is not enabled for the master branch, only for 7.x.

I updated the following deprecations:

  1. Using "elasticsearch.username: kibana" is deprecated
  2. Using "elasticsearch.username: elastic" is deprecated
  3. Using "elasticsearch.ssl.certificate" without "elasticsearch.ssl.key" has no effect
  4. Using "elasticsearch.ssl.key" without "elasticsearch.ssl.certificate" has no effect
  5. The array format for "xpack.security.authc.providers" is deprecated
  6. Using both "basic" and "token" providers in "xpack.security.authc.providers" has no effect
  7. "xpack.security.authc.providers.saml..maxRedirectURLSize" has no effect

Note, all of these have been deprecated for some time, but will not be actually changing/breaking in the 8.0 release. I am following the deprecation message guidance which says we should not say "this will be removed in the future" or anything to that effect. All of these are a warning level, which indicates nothing will break if the operator chooses not to address these deprecations.

Screenshots

(1) and (2), which use the same string templates and are mutually exclusive

image

(3) and (4), which use the same string templates are mutually exclusive

image

(5)

image

(6)

image

(7)

image

@jportner jportner added v8.0.0 release_note:skip Skip the PR/issue when compiling release notes v7.16.0 labels Oct 15, 2021
Copy link
Contributor Author

@jportner jportner left a comment

Choose a reason for hiding this comment

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

Author's notes for reviewers. I already went over these once with @gchaps but will tag her again for a final review.

Comment on lines +66 to +71
// 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
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,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 👍

@jportner jportner marked this pull request as ready for review October 15, 2021 18:10
@jportner jportner requested review from a team as code owners October 15, 2021 18:10
@jportner jportner requested review from a team and legrego October 15, 2021 18:10
@jportner
Copy link
Contributor Author

@elasticmachine merge upstream

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Changes look great! I'm about to test locally now, but wanted to leave my minor feedback in the meantime

src/core/server/elasticsearch/elasticsearch_config.ts Outdated Show resolved Hide resolved
'Remove the "xpack.security.authc.providers" setting from kibana.yml.',
}),
i18n.translate('xpack.security.deprecations.authcProviders.manualSteps2', {
defaultMessage: 'Add your authentication providers using the new object format.',
Copy link
Member

Choose a reason for hiding this comment

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

I really wish we had markdown support here. I think we have enough information here to build a correct object-based representation of their array-based configuration, which would make the upgrade process so much nicer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree 100%. It doesn't sound like that will land in 7.16.0, but I think we'd be justified to backport it to 7.16.1 to make the upgrade experience smoother. In that case I'd like to go back and tweak some of these messages that would be better served by markdown

Copy link
Contributor

@TinaHeiligers TinaHeiligers Oct 15, 2021

Choose a reason for hiding this comment

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

I hear you! Time permitting, we'll try to slot kibana/issues/111068 in. However, there are some gotcha's with i18n and a few other considerations we need to account for that make the task a little more complex.

}),
message: i18n.translate('xpack.security.deprecations.basicAndTokenProvidersMessage', {
defaultMessage:
'Enabling both "basic" and "token" authentication providers in "xpack.security.authc.providers" is deprecated. Login page will only use "token" provider.',
'Use only one of these providers. When both providers are set, the login page only uses the "{tokenProvider}" provider.',
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason to say "the login page", and not "Kibana"? This also technically applies to API consumers, where the login page is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I wasn't sure, the existing string said "login page" so I left it. @azasypkin do you have any opinion here? Should we just change it to "When both providers are set, Kibana only uses the "token" provider. ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tentatively changed in e584ec4 👍

Copy link
Member

Choose a reason for hiding this comment

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

It seems I wanted to be technically correct here since you can hit login endpoint with the specific provider name directly and use that specific provider, login page cannot do this 😆 But yeah, don't have a strong opinion here, Kibana instead of login page sounds good to me as well.

x-pack/plugins/security/server/config_deprecations.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Thanks for updating the deprecation levels here!
A few comments/suggestions and a question about wording.

}),
message: i18n.translate('xpack.security.deprecations.basicAndTokenProvidersMessage', {
defaultMessage:
'Enabling both "basic" and "token" authentication providers in "xpack.security.authc.providers" is deprecated. Login page will only use "token" provider.',
'Use only one of these providers. When both providers are set, the login page only uses the "{tokenProvider}" provider.',
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
'Use only one of these providers. When both providers are set, the login page only uses the "{tokenProvider}" provider.',
'Only use one provider. When both providers are set, the login page only uses the "{tokenProvider}" provider.',

Copy link
Contributor Author

@jportner jportner Oct 15, 2021

Choose a reason for hiding this comment

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

I'll defer to @gchaps here, but I worded it this way because the user may have other providers configured too (saml, oidc, etc). So I don't want to suggest that they should remove all but one provider.

Copy link
Contributor

@phillipb phillipb left a comment

Choose a reason for hiding this comment

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

LGTM

@TinaHeiligers TinaHeiligers self-requested a review October 15, 2021 19:12
Copy link
Contributor

@TinaHeiligers TinaHeiligers left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!
LGTM

Also fixed a few broken Jest unit tests!
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jportner
Copy link
Contributor Author

@elasticmachine merge upstream

@jportner jportner requested a review from legrego October 18, 2021 12:44
Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for the edits!

Note: I skipped `xpack.security.enabled`, `xpack.spaces.enabled`, and
the legacy audit logger because those are being removed in 8.0 / they
have their own 7.x-specific PRs to make those changes.
@jportner
Copy link
Contributor Author

I added b409027 to address #115344 👍

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jportner jportner merged commit 6792bdf into elastic:master Oct 18, 2021
@jportner jportner deleted the update-security-deprecation-messages branch October 18, 2021 15:34
jportner added a commit to jportner/kibana that referenced this pull request Oct 18, 2021
jportner added a commit that referenced this pull request Oct 18, 2021
* Update security deprecation messages (#115241)

* Fix Jest tests that were changed upstream

* Add missing deprecation levels for plugins' `enabled` config options

For the Security, Spaces, and ESO plugins.
@TinaHeiligers
Copy link
Contributor

TinaHeiligers commented Nov 2, 2021

@jportner we're auditing the kibana.yml file and the deprecations added here mention using a service account token rather than username/password for Kibana <-> ES auth when deprecated values are set (elastic/kibana).

Should we consider removing the kibana username/password config option in the main template or at least mention that it might be deprecated in a future release?
OTOH, with the new make it minor strategy, maybe we shouldn't mention it will be removed in favor of service account tokens.
WDYT?

@jportner
Copy link
Contributor Author

jportner commented Nov 2, 2021

Hey @TinaHeiligers, great question!

I apologize that this isn't super clear, some of this discussion took place over email with the ES Security team and was not captured in GitHub.

To summarize our current state of thinking:

  • We are not planning to deprecate elasticsearch.username and elasticsearch.password
  • However, with the new Security On By Default initiative, we want to start guiding users towards using a Service Account Token instead, which is a better paradigm in general for machine-to-machine authentication
  • Since we deprecated using the "elastic" and "kibana" users to authenticate Kibana to Elasticsearch (e.g., elasticsearch.username: elastic and elasticsearch.username: kibana, respectively), we decided to go ahead and tell affected users to use a Service Account Token in the Upgrade Assistant, because Elasticsearch allows it even without TLS enabled starting in 7.16

The kibana.yml template file already contains elasticsearch.username, elasticsearch.password, and elasticsearch.serviceAccountToken, so I think it's good for now.

I do think we need to make additional changes in the 8.0 Kibana Security docs to make sure they flow well with the 8.0 Elastic Stack Security docs, so I'm glad you brought this up. I created an issue for it here: #117223

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
buildkite-ci release_note:skip Skip the PR/issue when compiling release notes v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants