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

Add new predefined reserved roles #71710

Merged
merged 5 commits into from
Apr 20, 2021
Merged

Add new predefined reserved roles #71710

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18f07e0
Add new predefined roles
jkakavas Apr 14, 2021
bf212ec
copypaste fix
jkakavas Apr 14, 2021
0c0b02f
adjust number of reserved roles in test
jkakavas Apr 15, 2021
d68ea13
Add couple of checks
jkakavas Apr 15, 2021
f516d5a
simplify role definition
jkakavas Apr 19, 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
2 changes: 1 addition & 1 deletion ...h-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -691,7 +691,7 @@ public void testGetRoles() throws Exception {
List<Role> roles = response.getRoles();
assertNotNull(response);
// 29 system roles plus the three we created
assertThat(roles.size(), equalTo(29 + 3));
assertThat(roles.size(), equalTo(31 + 3));
}

{
Expand Down
84 changes: 84 additions & 0 deletions ...e/src/main/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,9 +336,93 @@ private static Map<String, RoleDescriptor> initializeReservedRoles() {
.indices(".enrich-*")
.privileges("manage", "read", "write")
.build() }, null, MetadataUtils.DEFAULT_RESERVED_METADATA))
.put("viewer", buildViewerRoleDescriptor())
.put("editor", buildEditorRoleDescriptor())
.immutableMap();
}

private static RoleDescriptor buildViewerRoleDescriptor() {
return new RoleDescriptor("viewer",
new String[] {},
new RoleDescriptor.IndicesPrivileges[] {
// Stack
RoleDescriptor.IndicesPrivileges.builder()
.indices("/~(([.]|ilm-history-).*)/")
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought this is supposed to look like /@&~(([.]|ilm-history-).*)/, but tests say it works anyway.

.privileges("read", "view_index_metadata").build(),
// Security
RoleDescriptor.IndicesPrivileges.builder().indices(".siem-signals-*").privileges("read", "view_index_metadata").build() },
new RoleDescriptor.ApplicationResourcePrivileges[] {
RoleDescriptor.ApplicationResourcePrivileges.builder().application("kibana-.kibana").resources("*").privileges(
"feature_discover.read",
"feature_dashboard.read",
"feature_canvas.read",
"feature_maps.read",
"feature_ml.read",
"feature_graph.read",
"feature_visualize.read",
"feature_logs.read",
"feature_infrastructure.read",
"feature_apm.read",
"feature_uptime.read",
"feature_siem.read",
"feature_dev_tools.read",
"feature_advancedSettings.read",
"feature_indexPatterns.read",
"feature_savedObjectsManagement.read",
"feature_savedObjectsTagging.read",
"feature_fleet.read",
"feature_actions.read",
"feature_stackAlerts.read").build() },
null,
null,
MetadataUtils.DEFAULT_RESERVED_METADATA,
null);
}

private static RoleDescriptor buildEditorRoleDescriptor() {
return new RoleDescriptor("editor",
Copy link
Member

Choose a reason for hiding this comment

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

@bytebilly would we expect the editor role to be able to create/update/delete Kibana spaces? Or is this a task reserved for administrators?

If we need the editor to manage spaces, then this role would instead need to grant the all application privilege, and the reserved_ml_admin privilege (all does not automatically grant ML access until 8.0):

RoleDescriptor.ApplicationResourcePrivileges.builder()
                    .application("kibana-.kibana")
                    .resources("*").privileges("all")
                    .build()

RoleDescriptor.ApplicationResourcePrivileges.builder()
                    .application("kibana-*")
                    .resources("*").privileges("reserved_ml_admin")
                    .build()

Copy link
Contributor

Choose a reason for hiding this comment

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

@legrego I'm not sure to follow your comment. We are not using the all shortcut but rather assigning all individual permissions (happy to simplify if we can do differently).

Could you also explain why there is kibana-.kibana in the first row, and kibana-* in the second one? Thanks!

Copy link
Member

@legrego legrego Apr 15, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Sorry for not being clear -- If we want editors to be able to manage spaces, then we have to use the all shortcut in order to achieve this. We don't have a dedicated "manage spaces" privilege today (elastic/kibana#51759), and so it's currently gated behind this all privilege

Copy link
Member

Choose a reason for hiding this comment

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

Could you also explain why there is kibana-.kibana in the first row, and kibana-* in the second one? Thanks!

That is an implementation detail of Kibana's "reserved privileges", and is something we can potentially simplify in 8.x once Kibana drops support for multi-tenant setups. The -* at the end indicates that this privilege will be granted to all Kibana tenants, not just the default .kibana tenant.

Copy link
Contributor

Choose a reason for hiding this comment

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

Larry and I had a sync on that, and he is checking if using his proposed approach will give UX problems or not. Based on that, we can define if we want to use one approach or the other.

Copy link
Member

Choose a reason for hiding this comment

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

I didn't get a chance to verify this today. I struggled to get ES to build locally, and my time was quite limited today. I'm on PTO until Wednesday (after feature freeze), so please don't hold this PR up on me. We can tweak the application privileges definition following FF if we have to without any real interruption

new String[] {},
new RoleDescriptor.IndicesPrivileges[] {
// Stack
RoleDescriptor.IndicesPrivileges.builder()
.indices("/~(([.]|ilm-history-).*)/")
.privileges("read", "view_index_metadata").build(),
// Observability
RoleDescriptor.IndicesPrivileges.builder()
.indices("observability-annotations")
.privileges("read", "view_index_metadata", "write").build(),
// Security
RoleDescriptor.IndicesPrivileges.builder()
.indices(".siem-signals-*", ".lists-*", ".items-*")
.privileges("read", "view_index_metadata", "write", "maintenance").build() },
new RoleDescriptor.ApplicationResourcePrivileges[] {
RoleDescriptor.ApplicationResourcePrivileges.builder().application("kibana-.kibana").resources("*").privileges(
"feature_discover.all",
"feature_dashboard.all",
"feature_canvas.all",
"feature_maps.all",
"feature_ml.all",
"feature_graph.all",
"feature_visualize.all",
"feature_logs.all",
"feature_infrastructure.all",
"feature_apm.all",
"feature_uptime.all",
"feature_siem.all",
"feature_dev_tools.all",
"feature_advancedSettings.all",
"feature_indexPatterns.all",
"feature_savedObjectsManagement.all",
"feature_savedObjectsTagging.all",
"feature_fleet.all",
"feature_actions.all",
"feature_stackAlerts.all").build() },
null,
null,
MetadataUtils.DEFAULT_RESERVED_METADATA,
null);
}

private static RoleDescriptor kibanaAdminUser(String name, Map<String, Object> metadata) {
return new RoleDescriptor(name, null, null,
new RoleDescriptor.ApplicationResourcePrivileges[] {
Expand Down
168 changes: 168 additions & 0 deletions .../test/java/org/elasticsearch/xpack/core/security/authz/store/ReservedRolesStoreTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,8 @@ public void testIsReserved() {
assertThat(ReservedRolesStore.isReserved("snapshot_user"), is(true));
assertThat(ReservedRolesStore.isReserved("code_admin"), is(false));
assertThat(ReservedRolesStore.isReserved("code_user"), is(false));
assertThat(ReservedRolesStore.isReserved("viewer"), is(true));
assertThat(ReservedRolesStore.isReserved("editor"), is(true));
}

public void testSnapshotUserRole() {
Expand Down Expand Up @@ -1655,6 +1657,172 @@ public void testWatcherUserRole() {
assertNoAccessAllowed(role, RestrictedIndicesNames.ASYNC_SEARCH_PREFIX + randomAlphaOfLengthBetween(0, 2));
}

public void testPredefinedViewerRole() {
final TransportRequest request = mock(TransportRequest.class);
final Authentication authentication = mock(Authentication.class);

RoleDescriptor roleDescriptor = new ReservedRolesStore().roleDescriptor("viewer");
assertNotNull(roleDescriptor);
assertThat(roleDescriptor.getMetadata(), hasEntry("_reserved", true));

Role role = Role.builder(roleDescriptor, null).build();
// No cluster privileges
assertThat(role.cluster().check(ClusterHealthAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(ClusterStateAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(ClusterStatsAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(PutIndexTemplateAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(ClusterRerouteAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(ClusterUpdateSettingsAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(MonitoringBulkAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(DelegatePkiAuthenticationAction.NAME, request, authentication), is(false));
// Check index privileges
assertOnlyReadAllowed(role, "observability-annotations");
assertOnlyReadAllowed(role, "logs-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "metrics-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "synthetics-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "apm-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "traces-apm." + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "filebeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "metricbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "heardbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "kibana_sample_data_-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, ".siem-signals-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "apm-" + randomIntBetween(0, 5) + "-transaction-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "logs-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "auditbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "filebeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "packetbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "winlogbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "endgame-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, randomAlphaOfLength(5));
jkakavas marked this conversation as resolved.
Show resolved Hide resolved

assertNoAccessAllowed(role, RestrictedIndicesNames.RESTRICTED_NAMES);
assertNoAccessAllowed(role, "." + randomAlphaOfLengthBetween(6, 10));
assertNoAccessAllowed(role, "ilm-history-" + randomIntBetween(0, 5));
// Check application privileges
assertKibanaFeatureReadButNotAll(role, "feature_discover");
assertKibanaFeatureReadButNotAll(role, "feature_dashboard");
assertKibanaFeatureReadButNotAll(role, "feature_canvas");
assertKibanaFeatureReadButNotAll(role, "feature_maps");
assertKibanaFeatureReadButNotAll(role, "feature_ml");
assertKibanaFeatureReadButNotAll(role, "feature_graph");
assertKibanaFeatureReadButNotAll(role, "feature_visualize");
assertKibanaFeatureReadButNotAll(role, "feature_logs");
assertKibanaFeatureReadButNotAll(role, "feature_infrastructure");
assertKibanaFeatureReadButNotAll(role, "feature_apm");
assertKibanaFeatureReadButNotAll(role, "feature_uptime");
assertKibanaFeatureReadButNotAll(role, "feature_siem");
assertKibanaFeatureReadButNotAll(role, "feature_dev_tools");
assertKibanaFeatureReadButNotAll(role, "feature_advancedSettings");
assertKibanaFeatureReadButNotAll(role, "feature_indexPatterns");
assertKibanaFeatureReadButNotAll(role, "feature_savedObjectsManagement");
assertKibanaFeatureReadButNotAll(role, "feature_savedObjectsTagging");
assertKibanaFeatureReadButNotAll(role, "feature_fleet");
assertKibanaFeatureReadButNotAll(role, "feature_actions");
assertKibanaFeatureReadButNotAll(role, "feature_stackAlerts");

assertThat(role.runAs().check(randomAlphaOfLengthBetween(1, 20)), is(false));
}

public void testPredefinedEditorRole() {
final TransportRequest request = mock(TransportRequest.class);
final Authentication authentication = mock(Authentication.class);

RoleDescriptor roleDescriptor = new ReservedRolesStore().roleDescriptor("editor");
assertNotNull(roleDescriptor);
assertThat(roleDescriptor.getMetadata(), hasEntry("_reserved", true));

Role role = Role.builder(roleDescriptor, null).build();

// No cluster privileges
assertThat(role.cluster().check(ClusterHealthAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(ClusterStateAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(ClusterStatsAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(PutIndexTemplateAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(ClusterRerouteAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(ClusterUpdateSettingsAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(MonitoringBulkAction.NAME, request, authentication), is(false));
assertThat(role.cluster().check(DelegatePkiAuthenticationAction.NAME, request, authentication), is(false));

// Check index privileges
assertOnlyReadAllowed(role, "logs-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "metrics-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "synthetics-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "apm-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "traces-apm." + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "filebeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "metricbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "heardbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "kibana_sample_data_-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "apm-" + randomIntBetween(0, 5) + "-transaction-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "logs-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "auditbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "filebeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "packetbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "winlogbeat-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, "endgame-" + randomIntBetween(0, 5));
assertOnlyReadAllowed(role, randomAlphaOfLength(5));

assertReadWriteDocsAndMaintenanceButNotDeleteIndexAllowed(role, ".siem-signals-" + randomIntBetween(0, 5));
assertReadWriteDocsAndMaintenanceButNotDeleteIndexAllowed(role, ".lists-" + randomIntBetween(0, 5));
assertReadWriteDocsAndMaintenanceButNotDeleteIndexAllowed(role, ".items-" + randomIntBetween(0, 5));
assertReadWriteDocsButNotDeleteIndexAllowed(role, "observability-annotations");

assertNoAccessAllowed(role, RestrictedIndicesNames.RESTRICTED_NAMES);
assertNoAccessAllowed(role, "." + randomAlphaOfLengthBetween(6, 10));
assertNoAccessAllowed(role, "ilm-history-" + randomIntBetween(0, 5));

// Check application privileges
assertKibanaFeatureAll(role, "feature_discover");
assertKibanaFeatureAll(role, "feature_dashboard");
assertKibanaFeatureAll(role, "feature_canvas");
assertKibanaFeatureAll(role, "feature_maps");
assertKibanaFeatureAll(role, "feature_ml");
assertKibanaFeatureAll(role, "feature_graph");
assertKibanaFeatureAll(role, "feature_visualize");
assertKibanaFeatureAll(role, "feature_logs");
assertKibanaFeatureAll(role, "feature_infrastructure");
assertKibanaFeatureAll(role, "feature_apm");
assertKibanaFeatureAll(role, "feature_uptime");
assertKibanaFeatureAll(role, "feature_siem");
assertKibanaFeatureAll(role, "feature_dev_tools");
assertKibanaFeatureAll(role, "feature_advancedSettings");
assertKibanaFeatureAll(role, "feature_indexPatterns");
assertKibanaFeatureAll(role, "feature_savedObjectsManagement");
assertKibanaFeatureAll(role, "feature_savedObjectsTagging");
assertKibanaFeatureAll(role, "feature_fleet");
assertKibanaFeatureAll(role, "feature_actions");
assertKibanaFeatureAll(role, "feature_stackAlerts");

assertThat(role.runAs().check(randomAlphaOfLengthBetween(1, 20)), is(false));
}

private void assertKibanaFeatureReadButNotAll(Role role, String featureName) {
assertThat(role.application()
.grants(new ApplicationPrivilege("kibana-.kibana", "kibana-" + featureName + ".read", featureName + ".read"), "*"), is(true));
assertThat(role.application()
.grants(new ApplicationPrivilege("kibana-.kibana", "kibana-" + featureName + ".all", featureName + ".all"), "*"), is(false));
}

private void assertKibanaFeatureAll(Role role, String featureName) {
assertThat(role.application()
.grants(new ApplicationPrivilege("kibana-.kibana", "kibana-" + featureName + ".all", featureName + ".all"), "*"), is(true));
}

private void assertReadWriteDocsAndMaintenanceButNotDeleteIndexAllowed(Role role, String index) {
assertThat(role.indices().allowedIndicesMatcher(DeleteIndexAction.NAME).test(mockIndexAbstraction(index)), is(false));
assertThat(role.indices().allowedIndicesMatcher(SearchAction.NAME).test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher(GetAction.NAME).test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher(IndexAction.NAME).test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher(UpdateAction.NAME).test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher(DeleteAction.NAME).test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher(BulkAction.NAME).test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher("indices:admin/refresh*").test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher("indices:admin/flush*").test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher("indices:admin/synced_flush").test(mockIndexAbstraction(index)), is(true));
assertThat(role.indices().allowedIndicesMatcher("indices:admin/forcemerge*").test(mockIndexAbstraction(index)), is(true));
}

private void assertReadWriteDocsButNotDeleteIndexAllowed(Role role, String index) {
assertThat(role.indices().allowedIndicesMatcher(DeleteIndexAction.NAME).test(mockIndexAbstraction(index)), is(false));
assertThat(role.indices().allowedIndicesMatcher(SearchAction.NAME).test(mockIndexAbstraction(index)), is(true));
Expand Down