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

Remove synthetic role names of API keys as they confuse users #56005

Merged
merged 2 commits into from
Apr 30, 2020

Conversation

ywangd
Copy link
Member

@ywangd ywangd commented Apr 30, 2020

The synthetic role names of API key add confusion to users. This happens to API responses as well as audit logs. This PR removes them for clarity.

@ywangd ywangd added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v8.0.0 v7.8.0 labels Apr 30, 2020
@ywangd ywangd requested a review from tvernum April 30, 2020 09:01
@ywangd ywangd changed the title Do not use synthetic role names for API keys as they confuse users Remove synthetic role names of API keys as they confuse users Apr 30, 2020
@ywangd ywangd added the v7.7.1 label Apr 30, 2020
Copy link
Contributor

@tvernum tvernum left a comment

Choose a reason for hiding this comment

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

LGTM

@ywangd ywangd merged commit 03cd802 into elastic:master Apr 30, 2020
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Apr 30, 2020
…c#56005)

Synthetic role names of API keys add confusion to users. This happens to API responses as well as audit logs. The PR removes them for clarity.
ywangd added a commit to ywangd/elasticsearch that referenced this pull request Apr 30, 2020
…c#56005)

Synthetic role names of API keys add confusion to users. This happens to API responses as well as audit logs. The PR removes them for clarity.
ywangd added a commit that referenced this pull request Apr 30, 2020
#56012)

Synthetic role names of API keys add confusion to users. This happens to API responses as well as audit logs. The PR removes them for clarity.
ywangd added a commit that referenced this pull request Apr 30, 2020
#56011)

Synthetic role names of API keys add confusion to users. This happens to API responses as well as audit logs. The PR removes them for clarity.
@ahmbolu
Copy link

ahmbolu commented Dec 10, 2020

Removing synthetic roles names from API keys broke the case when an API Key is used to generate a report with Kibana. Kibana requires the user to be a member of one of the roles listed in xpack.reporting.roles.allow. Since the API Key has an empty list of roles, it will always fail that check with no possible work around.

@ywangd
Copy link
Member Author

ywangd commented Dec 13, 2020

@ahmbolu Thanks for reporting the issue. I am sorry that the change breaks your workflow. However, role names of API keys are more misleading than they are worth. Because it is possible for two API keys to have the exact same role name but underlyingly each has a completely different set of privileges. That is, my_role of API key 1 can be totally different from my_role of API key 2. This clash of names are not only confusing but also has security implications. Basically, one cannot trust the role names provided by an API Key.

For your specific use case, I think the issue is with Kibana in that it relies on role names instead of actual privileges to grant access. Even though the reporting_user is an empty marker role, I personally think it is not the right approach. It should instead depend on something like application privileges. I will propose a discussion about this topic to the team and get back to you once we have a more concrete answer.

@tvernum
Copy link
Contributor

tvernum commented Dec 14, 2020

This is a Kibana reporting issue that is tracked here: elastic/kibana#76210

@ywangd
Copy link
Member Author

ywangd commented Dec 14, 2020

Thanks Tim. I wasn't aware it is a known issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants