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

Deprecate kibana user in favor of kibana_system user #63186

Merged
merged 12 commits into from
May 5, 2020

Conversation

legrego
Copy link
Member

@legrego legrego commented Apr 9, 2020

This updates the User Grid page and Edit User page to warn about deprecated users. Support for deprecated users was added to Elasticsearch in elastic/elasticsearch#54967, and is used to denote that the kibana user is deprecated in favor of the kibana_system user.

image

image

Companion PRs:
elastic/stack-docs#991
elastic/elasticsearch#54967

Resolves #25879

@legrego legrego force-pushed the security/support-deprecated-users branch from bfb081d to 03662f2 Compare April 28, 2020 15:07
@legrego legrego added Feature:Security/Authentication Platform Security - Authentication release_note:deprecation Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.8.0 v8.0.0 labels Apr 28, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-security (Team:Security)

@legrego legrego marked this pull request as ready for review April 28, 2020 17:05
@legrego legrego requested review from a team as code owners April 28, 2020 17:05
Copy link
Contributor

@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.

EDIT: tested locally and works great! A few nits aside from the changes you directly made... (using permalinks to the current HEAD in master so they render properly in my comment)


  1. In Additional validation for elasticsearch username #48247 we deprecated using the elastic user in config in favor of using the kibana user. I'm thinking we should probably change that to deprecate it in favor of kibana_system too. This is in a few files...

if (es.username === 'elastic') {
log(
`Setting [${fromPath}.username] to "elastic" is deprecated. You should use the "kibana" user instead.`
);
}

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.'
);
}

if (es.username === 'elastic') {
logger(
`Setting [${fromPath}.username] to "elastic" is deprecated. You should use the "kibana" user instead.`
);
}


  1. Should we update this functional test assertion for the kibana_system user instead?

expect(users.kibana.roles).to.eql(['kibana_system']);
expect(users.kibana.reserved).to.be(true);


  1. This is really nitpicky, but do we want to change the wording from "kibana user" to "Kibana user" to differentiate the intent from the username itself? Or split this into two messages for the kibana and kibana_system users? 😛 I don't feel super strongly about it, just wanted to point it out in case you missed it.

defaultMessage="After you change the password for the kibana user, you must update the {kibana}


  1. I found a few more references to the deprecated kibana user:

By default, this will also set the password for native realm accounts to the password provided (`changeme` by default). This includes that of the `kibana` user which `elasticsearch.username` defaults to in development. If you wish to specific a password for a given native realm account, you can do that like so: `--password.kibana=notsecure`

The password for the built-in `kibana` user is typically set as part of the

kibana: {
metadata: {
_reserved: true,
},
},
non_native: {
metadata: {
_reserved: false,
},
},
logstash_system: {
metadata: {
_reserved: true,
},
},
},
}));
expect(await nativeRealm.getReservedUsers()).toEqual(['kibana', 'logstash_system']);

There were a couple more in code comments, but those don't matter as much IMO.

metadata: {
_reserved: true,
_deprecated: true,
_deprecated_reason: 'This user is not cool anymore.',
Copy link
Contributor

Choose a reason for hiding this comment

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

😄

export const getExtendedUserDeprecationNotice = (user: User) => {
const reason = user.metadata?._deprecated_reason ?? '';
return i18n.translate('xpack.security.management.users.extendedUserDeprecationNotice', {
defaultMessage: `The {username} user is deprecated. {reason}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this should change so it will be consistent with the reason?

Suggested change
defaultMessage: `The {username} user is deprecated. {reason}`,
defaultMessage: `The [{username}] user is deprecated. {reason}`,

Copy link
Member Author

Choose a reason for hiding this comment

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

The current text is consistent with the deprecation message we display for roles today. Gail had actually requested that we remove the brackets from the reason when we did the role deprecations, but then that change would have been inconsistent with the rest of the messages in ES. Overall, I'm 🤷‍♂️ about it, I could go either way.

Given my beautiful backstory, which would you prefer? I'll defer to you 😄

Copy link
Contributor

@pgayvallet pgayvallet left a comment

Choose a reason for hiding this comment

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

LGTM for config/kibana.yml changes

@legrego
Copy link
Member Author

legrego commented Apr 29, 2020

@jportner thanks for the review! I'm not sure how my search missed those other references, so good looking out 🥇

@legrego legrego requested a review from a team as a code owner April 29, 2020 11:00
@legrego legrego requested a review from a team April 29, 2020 11:00
Copy link
Contributor

@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.

Sorry, I could have been clearer in my last comment, a couple more suggestions!

Copy link
Contributor

@tylersmalley tylersmalley left a comment

Choose a reason for hiding this comment

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

Operations changes to kbn-es LGTM

@legrego
Copy link
Member Author

legrego commented May 2, 2020

@elasticmachine merge upstream

@legrego
Copy link
Member Author

legrego commented May 4, 2020

@elasticmachine merge upstream

@legrego
Copy link
Member Author

legrego commented May 5, 2020

@jportner ready for another look whenever you get a chance

@legrego legrego requested a review from jportner May 5, 2020 00:11
Copy link
Contributor

@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.

LGTM

x-pack/README.md Outdated Show resolved Hide resolved
Co-authored-by: Joe Portner <[email protected]>
@legrego
Copy link
Member Author

legrego commented May 5, 2020

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

History

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

@legrego legrego merged commit 6a6deef into elastic:master May 5, 2020
@legrego legrego deleted the security/support-deprecated-users branch May 5, 2020 15:36
legrego added a commit to legrego/kibana that referenced this pull request May 5, 2020
This was referenced Aug 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Security/Authentication Platform Security - Authentication release_note:deprecation Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.8.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce kibana_system user, deprecate kibana user
6 participants