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

buildFromRoleDescriptor uses application privilege look up #91152

Merged
merged 13 commits into from
Oct 28, 2022

Conversation

n1v0lg
Copy link
Contributor

@n1v0lg n1v0lg commented Oct 27, 2022

Building a role from a single role descriptor previously only worked
correctly for application privileges with wildcard patterns. This PR
adds support for concrete name look up and ports the relevant tests.

Relates: #91107 (comment)

@n1v0lg n1v0lg added >non-issue :Security/Security Security issues without another label v8.6.0 labels Oct 27, 2022
@n1v0lg n1v0lg self-assigned this Oct 27, 2022
@@ -491,6 +491,19 @@ private void trySuccessfullyLoadSuperuserRole(CompositeRolesStore compositeRoles
final Role role = future.actionGet();
assertThat(role.names(), arrayContaining("superuser"));
assertThat(role.application().getApplicationNames(), containsInAnyOrder("*"));
assertThat(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We test this elsewhere (e.g., in 30_superuser) but seems like an appropriate addition here

@n1v0lg n1v0lg changed the title buildFromRoleDescriptor supports application privilege looup buildFromRoleDescriptor uses application privilege look up Oct 27, 2022
@n1v0lg n1v0lg requested a review from ywangd October 27, 2022 15:54
@n1v0lg n1v0lg marked this pull request as ready for review October 27, 2022 15:54
@elasticsearchmachine
Copy link
Collaborator

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

@elasticsearchmachine elasticsearchmachine added the Team:Security Meta label for security team label Oct 27, 2022
Copy link
Member

@ywangd ywangd 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 working on this.

It would be great if we can exclusively use ApplicationPrivilege.get in place of the constructors. But it can be a future item.

RESTRICTED_INDICES
RESTRICTED_INDICES,
List.of(
new ApplicationPrivilegeDescriptor("kibana-*", "reserved_monitoring", Set.of(allowedApplicationActionPattern), Map.of())
Copy link
Member

Choose a reason for hiding this comment

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

Stored application privilege must have concrete application name. Are there test failures that prevent you from using something like kibana-kibana instead of kibana-*? It will be a bug if that is the case.

Copy link
Contributor Author

@n1v0lg n1v0lg Oct 28, 2022

Choose a reason for hiding this comment

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

Good point. I can make this work for sure. kibana-kibana doesn't do it because we randomize the application name in the assertions below, but using the randomized application name does the trick.

new FieldPermissionsCache(Settings.EMPTY),
RESTRICTED_INDICES,
List.of(
new ApplicationPrivilegeDescriptor("kibana-*", "reserved_ml_apm_user", Set.of(allowedApplicationActionPattern), Map.of())
Copy link
Member

Choose a reason for hiding this comment

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

Same here for kibana-*.

roleDescriptor,
new FieldPermissionsCache(Settings.EMPTY),
RESTRICTED_INDICES,
List.of(new ApplicationPrivilegeDescriptor("kibana-*", "reserved_ml_admin", Set.of(allowedApplicationActionPattern), Map.of()))
Copy link
Member

Choose a reason for hiding this comment

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

Here as well and a few other places below. I won't be commenting for all of them since it is quite repetitive.

@n1v0lg
Copy link
Contributor Author

n1v0lg commented Oct 28, 2022

It would be great if we can exclusively use ApplicationPrivilege.get in place of the constructors. But it can be a future item.

Yup, one more PR coming to do this 👍

@n1v0lg n1v0lg merged commit 67204a9 into elastic:main Oct 28, 2022
@n1v0lg n1v0lg deleted the application-privilege-loopup branch October 28, 2022 11:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>non-issue :Security/Security Security issues without another label Team:Security Meta label for security team v8.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants