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

Granting kibana_system reserved role access to "all" privileges to .alerts* and .siem-signals* index #72181

Merged
merged 3 commits into from
May 12, 2021

Conversation

dhurley14
Copy link
Contributor

@dhurley14 dhurley14 commented Apr 23, 2021

The RAC team is storing alerts as data in the .alerts* and .siem-signals* (legacy) indices, which the Kibana user needs access to in order to CRUD alerts.

Ref: elastic/kibana#95721

drake: elastic/kibana#94502

@elasticsearchmachine elasticsearchmachine added the external-contributor Pull request authored by a developer outside the Elasticsearch team label Apr 23, 2021
@dhurley14 dhurley14 changed the title adds .alerts and .siem-signals (legacy) alerts as data indices for ki… Granting kibana_system reserved role access to "all" privileges for .alerts* and .siem-signals* indices Apr 23, 2021
@dhurley14 dhurley14 self-assigned this Apr 23, 2021
@dhurley14 dhurley14 added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v7.14.0 v8.0.0 labels Apr 23, 2021
@dhurley14 dhurley14 marked this pull request as ready for review April 23, 2021 19:27
@elasticmachine
Copy link
Collaborator

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

@jkakavas jkakavas requested a review from a team April 23, 2021 19:33
@@ -37,6 +37,8 @@
import java.util.stream.Collectors;

public class ReservedRolesStore implements BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>> {
public static final String LEGACY_ALERTS_INDEX = ".siem-signals*";
public static final String ALERTS_INDEX = ".alerts*";
Copy link
Member

@dgieselaar dgieselaar Apr 24, 2021

Choose a reason for hiding this comment

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

The write target for the rule registry currently is prefixed with .kibana, or rather .${kibana.index}. This is needed to support legacy multi-tenancy. If we don't do this, we'll run into weird things where rules or other saved objects are stored in different Kibana instances but end up writing to the same Elasticsearch index, which could lead to conflicts.

Copy link
Member

Choose a reason for hiding this comment

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

Or not being able to find the rule from an alert because it is in a different space. However, that is probably also an issue with cross-cluster search, or with deleted rules even, so maybe making the index configurable and dealing with missing rules in code/UX would be acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay so we really only need to have the change for .siem-signals since .kibana.{alerts} will be covered by the kibana system user's role privileges for the index pattern .kibana* correct?

Copy link
Member

Choose a reason for hiding this comment

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

If we decide to use the internal user, yes, but I think that we are not sure of that yet (unfortunately). If we don't, it would be up to the user to bootstrap the needed templates/indices/alias though we can think of ways to do that for them, eg by using Fleet or something clever with require_alias when bulk indexing.

@dhurley14 dhurley14 force-pushed the add-rac-kibana-user branch from 1a9c353 to d676a8a Compare April 29, 2021 13:48
@dhurley14 dhurley14 changed the title Granting kibana_system reserved role access to "all" privileges for .alerts* and .siem-signals* indices Granting kibana_system reserved role access to "all" privileges for .siem-signals* index Apr 29, 2021
@dhurley14 dhurley14 force-pushed the add-rac-kibana-user branch from d676a8a to c44e163 Compare April 29, 2021 15:20
@dhurley14 dhurley14 changed the title Granting kibana_system reserved role access to "all" privileges for .siem-signals* index Granting kibana_system reserved role access to "all" privileges to .alerts* and .siem-signals* index Apr 29, 2021
@dhurley14 dhurley14 force-pushed the add-rac-kibana-user branch from c44e163 to bad1737 Compare April 29, 2021 22:53
@@ -37,6 +37,10 @@
import java.util.stream.Collectors;

public class ReservedRolesStore implements BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>> {
public static final String LEGACY_ALERTS_INDEX = ".siem-signals*";
public static final String LEGACY_ALERTS_INDEX_CCS = "*:.siem-signals*";
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 had trouble determining if the additional CCS index pattern is necessary considering all privileges for .siem-signals and .alerts grants CCS privileges. Looking for feedback here from people in-the-know.

public static final String LEGACY_ALERTS_INDEX = ".siem-signals*";
public static final String LEGACY_ALERTS_INDEX_CCS = "*:.siem-signals*";
public static final String ALERTS_INDEX = ".alerts*";
public static final String ALERTS_INDEX_CCS = "*:.alerts*";
Copy link
Contributor

@albertzaharovits albertzaharovits May 11, 2021

Choose a reason for hiding this comment

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

The CCS patterns here and above don't work with our authz model for CCS.

TLDR: You shouldn't specify CCS patterns.

When the kibana_system reaches cross cluster, it is the remote cluster that performs the authorization there.
So, if on the local cluster node, the kibana user issues a search request for remote_cluster:.alerts_tenant1, that's automatically authorized locally, but the receiving remote node, part of remote_cluster, performs authorization using the local role definition.

Comment on lines 404 to 405
ReservedRolesStore.LEGACY_ALERTS_INDEX,
ReservedRolesStore.ALERTS_INDEX,
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
ReservedRolesStore.LEGACY_ALERTS_INDEX,
ReservedRolesStore.ALERTS_INDEX,
ReservedRolesStore.LEGACY_ALERTS_INDEX + randomAlphaOfLength(randomIntBetween(0, 13)),
ReservedRolesStore.ALERTS_INDEX + randomAlphaOfLength(randomIntBetween(0, 13)),

Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

Please remove the CCS pattern, and it should be OK.

@kobelb
Copy link
Contributor

kobelb commented May 11, 2021

LGTM after removing the CCS pattern.

@dhurley14 dhurley14 force-pushed the add-rac-kibana-user branch from da076ef to 11e31f4 Compare May 11, 2021 23:26
@dhurley14 dhurley14 force-pushed the add-rac-kibana-user branch from 11e31f4 to bfe416a Compare May 11, 2021 23:32
Copy link
Contributor

@albertzaharovits albertzaharovits left a comment

Choose a reason for hiding this comment

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

LGTM

@albertzaharovits albertzaharovits merged commit 823abb5 into elastic:master May 12, 2021
spong pushed a commit that referenced this pull request Aug 18, 2021
…ernal.alerts* (#76624)

**Parent ticket:** elastic/kibana#101016
**Related to:** #72181

## Summary

Similar to the previous PR (#72181), we'd like to add privileges to a new set of indices to the `kibana_system` role.

The reason for that is we need to have different naming schemes for alerts-as-data index aliases and backing indices pointing to these aliases, which in turn is needed to support backwards compatibility, migrations and reindexing in the future.

We didn't want to prefix the backing indices with `.kibana-`, so we're adding a new `.internal.alerts` prefix. Prefixing with `.kibana-` would make them system indices, which means they would not be supposed to be read by end users, which is not what we want.

`.internal` could become a universal prefix for hidden Kibana indices, but at this point I don't feel confident enough to generalise prematurely.
spong pushed a commit to spong/elasticsearch that referenced this pull request Aug 18, 2021
…ernal.alerts* (elastic#76624)

**Parent ticket:** elastic/kibana#101016
**Related to:** elastic#72181

## Summary

Similar to the previous PR (elastic#72181), we'd like to add privileges to a new set of indices to the `kibana_system` role.

The reason for that is we need to have different naming schemes for alerts-as-data index aliases and backing indices pointing to these aliases, which in turn is needed to support backwards compatibility, migrations and reindexing in the future.

We didn't want to prefix the backing indices with `.kibana-`, so we're adding a new `.internal.alerts` prefix. Prefixing with `.kibana-` would make them system indices, which means they would not be supposed to be read by end users, which is not what we want.

`.internal` could become a universal prefix for hidden Kibana indices, but at this point I don't feel confident enough to generalise prematurely.
elasticsearchmachine pushed a commit that referenced this pull request Aug 18, 2021
…ernal.alerts* (#76624) (#76635)

**Parent ticket:** elastic/kibana#101016
**Related to:** #72181

## Summary

Similar to the previous PR (#72181), we'd like to add privileges to a new set of indices to the `kibana_system` role.

The reason for that is we need to have different naming schemes for alerts-as-data index aliases and backing indices pointing to these aliases, which in turn is needed to support backwards compatibility, migrations and reindexing in the future.

We didn't want to prefix the backing indices with `.kibana-`, so we're adding a new `.internal.alerts` prefix. Prefixing with `.kibana-` would make them system indices, which means they would not be supposed to be read by end users, which is not what we want.

`.internal` could become a universal prefix for hidden Kibana indices, but at this point I don't feel confident enough to generalise prematurely.

Co-authored-by: Georgii Gorbachev <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement external-contributor Pull request authored by a developer outside the Elasticsearch team :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v7.14.0 v8.0.0-alpha1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants