Adding manage_dlm privilege #95512

masseyke · 2023-04-24T19:22:18Z

This adds a new index privilege called manage_dlm. The manage_dlm has permission to perform all DLM actions on an index, including put and delete. It also adds the ability to call DLM get and explain to the view_index_metadata existing index privilege.

elasticsearchmachine · 2023-04-24T19:22:43Z

Hi @masseyke, I've created a changelog YAML for you.

…search into feature/manage_dlm-role

elasticsearchmachine · 2023-04-24T22:00:18Z

Pinging @elastic/es-data-management (Team:Data Management)

andreidan

Thanks for working on this Keith, it's looking great so far.

Just a few questions:

Do we need to update ClusterPrivilegeResolver too ?

Do we need documentation update? i.e. get-builtin-privileges.asciidoc

I think the docs for all DLM related APIs need to have this security instruction:

* If the {es} {security-features} are enabled, you must have the `manage_dlm` or `read_ilm` or both cluster privileges to use this API. For more information, see <<security-privileges>>.

I'd say we should give managed_dlm to the relevant builtin roles defined in ReservedRolesStore ? (at least kibanaSystemRoleDescriptor ?)

andreidan · 2023-04-25T16:14:47Z

...rolling-upgrade/src/test/java/org/elasticsearch/upgrades/ApiKeyBackwardsCompatibilityIT.java

 containsString("failed to parse role [my_role]. unexpected field [remote_indices]"),
- containsString("remote indices not supported for API keys")
+ containsString("remote indices not supported for API keys"),
+ containsString("unknown index privilege")


Do we ... expect an unknown index privilege message error after introducing a new privilege?

This one surprised me, too. I'll spend a little more time trying to understand why this is. I couldn't find any way to tie a privilege to a version though (like we do for example with xcontent). So I don't think there's anything I can do about this if this is our behavior.

The test here pertains to backwards compatibility, specifically for API keys. For API keys, we serialize the role descriptors (as XContent) associated with it (which includes index privileges), and store those as a doc in the security index. The reason the test is failing here is because the test runs on an old version that doesn't have manage_dlm defined as a privilege yet. Further up in the test, we are creating a random role descriptor with a set of random index privileges, that sometimes include manage_dlm leading to the unknown index privilege failure here.

Just wanted to leave this as context, to save a bit of digging.

In terms of addressing this, for the context of this PR, I think it's all good to keep this as is. I might follow this up next week to make things more robust in a mixed-cluster scenario but it's too much of a side quest for the current PR.

This class (and the thinking around the failure) has evolved. In short, the current behavior is correct.

However, to correctly address it within this test, we should add manage_dlm to the excluded privileges here: https://github.com/elastic/elasticsearch/blob/main/x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/ApiKeyBackwardsCompatibilityIT.java#L351

Instead of tolerating containsString("unknown index privilege").

andreidan · 2023-04-25T16:15:49Z

...core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java

 );
 private static final Automaton MANAGE_LEADER_INDEX_AUTOMATON = patterns(ForgetFollowerAction.NAME + "*");
 private static final Automaton MANAGE_ILM_AUTOMATON = patterns("indices:admin/ilm/*");
+ private static final Automaton MANAGE_DLM_AUTOMATON = patterns("indices:admin/dlm/*", "indices:admin/data_lifecycle/*");


This shows we have a bit of an inconsistency in our action names. I'll bring the explain API into the data_lifecycle namespace (this comment is just a remark in general :) nothing about this PR)

Opened #95555 and actually brought all actions under the dlm namespace.
If data_lifecycle is preferable we can go that way ?

Sounds good. I had intended to do the same as a follow-up. I'll fix this once that's in.

Thanks Keith, I merged #95555 so let's update this to just use the dlm namespace

andreidan · 2023-04-25T16:40:53Z

Relates to #93596

n1v0lg · 2023-04-27T14:39:08Z

I believe that this is derived from the action names. Since all of the DLM ones begin with indices:admin (and are actions on indices or data streams) we use index-level privileges.

This part is a little trickier: for all actions we first check if they are cluster actions here which under the hood checks if the action name is part of the ALL_CLUSTER_PATTERN. This means that we can have an action of the form indices:... that is still treated and authorized as a cluster action. We will likely want to do this for the data lifecycle actions so they get authorized against cluster privileges (pending more work on the security model design to say this for sure).

andreidan · 2023-04-27T14:41:25Z

@n1v0lg thanks for the update. We're happy to let you folks drive the design and decisions on the security privileges/model. Please let us know what's needed on our side.
If I understand correctly we don't need to change anything to the existing actions (they remain indices:...) and you will come back to this PR with an update (or take over the PR?) ?

n1v0lg · 2023-04-27T14:55:32Z

@andreidan appreciated!

Please let us know what's needed on our side.

Will do! As soon as I have a better grasp on the final approach I will give feedback directly in this PR and we'll figure out together based on the size of the change what works best, me taking over or just requesting changes. No need to change anything for now. I will also put together a small design doc on the overall approach and keep you in the loop.

masseyke · 2023-04-27T18:34:22Z

One note before putting this PR aside -- the latest test failure appears to have turned up a real bug (https://gradle-enterprise.elastic.co/s/6kb67wslyurna/tests/:x-pack:qa:rolling-upgrade:v7.17.10%23oldClusterTest/org.elasticsearch.upgrades.ApiKeyBackwardsCompatibilityIT/testCreatingAndUpdatingApiKeys?expanded-stacktrace=WyIwIl0&top-execution=1). I've been talking with @slobodanadamovic about it. If you are running in a mixed mode cluster, and if your newer nodes have a new index privilege, and if you create an API key with this new privilege and use it to make a request that goes to an old cluster, then your request fails. I think Slobodan is going to write up a ticket and look into this. I just thought I'd mention it here so that no one spends too much time on that test failure -- it's not a bug in this PR.

n1v0lg · 2023-04-27T18:46:33Z

@masseyke thanks for raising this!

I alluded (quite vaguely) to this issue in my comment here:

I might follow this up next week to make things more robust in a mixed-cluster scenario but it's too much of a side quest for the current PR.

I was only thinking about the new privilege we're adding here, but @slobodanadamovic is correct in that it doesn't just apply to the new privilege, but more generally to other privileges we've added in the past, so it makes sense to track it as a bug beyond the scope of this current PR. Good catch 👍

slobodanadamovic · 2023-05-03T09:30:08Z

@n1v0lg @masseyke

I took some time to look into the issue with BWC and it turns out that it's not only affecting API Keys but also Users.

Creating a role with the newly introduced index privilege will cause problems in a mixed cluster. The same problem can happen when adding a new cluster privileges. I think we need to come up with a generic way of handling BWC for index and cluster privileges.

I've reported a bug here: #95771

n1v0lg · 2023-05-25T11:42:00Z

Reviving this, since it's the final approach we settled on after iterating on design.

I'll leave a review with a few small comments and nits. @masseyke I'm more than happy to push those changes to this PR, or let you address them. Let me know which you prefer! Thanks for working on this.

n1v0lg

Thanks for working on this! Just a few smaller comments. I can address these myself too, if you're swamped. Let me know what you prefer.

docs/changelog/95512.yaml

modules/dlm/qa/with-security/build.gradle

modules/dlm/qa/with-security/src/javaRestTest/java/org/elasticsearch/dlm/PermissionsIT.java

docs/reference/dlm/apis/put-lifecycle.asciidoc

n1v0lg · 2023-05-22T10:08:46Z

modules/dlm/qa/with-security/src/javaRestTest/java/org/elasticsearch/dlm/PermissionsIT.java

+ String index = (String) ((List<Map<String, Object>>) nodes.get(0).get("indices")).get(0).get("index_name");
+
+ Request explainLifecycleRequest = new Request("POST", "/" + index + "/_lifecycle/explain");
+ Request getLifecycleRequest = new Request("GET", "_data_stream/" + dataStreamName + "/_lifecycle");


Nit: would randomize here: randomFrom("_all", "*", dataStreamName) for this and the other calls below.

We've only granted permission on dlm-* though right? So we'd expect it to potentially fail for others. Or are you suggesting I change the test to make sure that it does fail for others?

No sorry, I'm suggesting it will work for these patterns as well: the way that index name resolution works is that _all and "*" will resolve into only the names the users has access for (so essentially dataStreamName, i.e., dlm-test)

So if we have a user with access to dlm-*, there are two data streams other-test and dlm-test, and the user runs PUT .../_all/lifecycle it will apply the changes to all data stream the user has access to, i.e., dlm-test, and will skip the other ones, i.e., other-test. Same for DELETE and GET -- this is something we do more generally for index patterns.

Oh I didn't realize that. Thanks.

Sure thing! Only learned this recently also.

...core/src/test/java/org/elasticsearch/xpack/core/security/authz/privilege/PrivilegeTests.java

n1v0lg · 2023-05-22T10:15:44Z

...rolling-upgrade/src/test/java/org/elasticsearch/upgrades/ApiKeyBackwardsCompatibilityIT.java

 containsString("failed to parse role [my_role]. unexpected field [remote_indices]"),
- containsString("remote indices not supported for API keys")
+ containsString("remote indices not supported for API keys"),
+ containsString("unknown index privilege")


This class (and the thinking around the failure) has evolved. In short, the current behavior is correct.

However, to correctly address it within this test, we should add manage_dlm to the excluded privileges here: https://github.com/elastic/elasticsearch/blob/main/x-pack/qa/rolling-upgrade/src/test/java/org/elasticsearch/upgrades/ApiKeyBackwardsCompatibilityIT.java#L351

Instead of tolerating containsString("unknown index privilege").

modules/dlm/qa/with-security/build.gradle

modules/dlm/qa/with-security/src/javaRestTest/java/org/elasticsearch/dlm/PermissionsIT.java

masseyke · 2023-05-25T14:30:25Z

Reviving this, since it's the final approach we settled on after iterating on design.

I'll leave a review with a few small comments and nits. @masseyke I'm more than happy to push those changes to this PR, or let you address them. Let me know which you prefer! Thanks for working on this.

I've got time to address them today. Thanks.

n1v0lg

LGTM! Few smaller things, but no need for another review from my end.

n1v0lg · 2023-05-26T08:13:08Z

docs/changelog/95512.yaml

+pr: 95512
+summary: Adding `manage_dlm` index privilege and expanding `view_index_metadata` for access to data lifecycle APIs
+area: DLM
+type: enhancement


I'm wondering if we really want a changelog here: the privilege is behind a feature flag, and so is the DLM feature itself. I don't know if we've been including changelogs for DLM so far but it might look weird to users to see changelogs for something that isn't available outside a feature flag yet. We could instead mark this non-issue and skip the changelog.

IMO advertising experimental features is ok

No strong opinion on my end!

...rolling-upgrade/src/test/java/org/elasticsearch/upgrades/ApiKeyBackwardsCompatibilityIT.java

Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>

Porting the DLM permissions REST test to the new style of cluster tests. This has the usual nice perks, and also allows us to remove the separate `qa/with-security` package. No functional or test logic changes. I didn't suggest this as part of the PR review for #95512 so as not to block that PR further, and also because I wasn't sure about the overhead of making this change (it did end up taking some battling with gradle).

masseyke added 3 commits April 20, 2023 17:20

Adding manage_dlm role

6beb7e3

Merge branch 'main' into feature/manage_dlm-role

e8092ce

Adding initial failing integration test

b4bd035

masseyke added >enhancement v8.8.0 :Data Management/DLM labels Apr 24, 2023

Update docs/changelog/95512.yaml

305635c

masseyke added 6 commits April 24, 2023 14:37

fixing integration test and license

120c7f7

Merge branch 'feature/manage_dlm-role' of github.com:masseyke/elastic…

1c35eaa

…search into feature/manage_dlm-role

fixing tests

937bba4

fixing tests

78b7d31

making intellij happy

670d130

test cleanup

b8e783c

masseyke changed the title ~~Adding manage_dlm role~~ Adding manage_dlm privilege Apr 24, 2023

removing unused code

67ec0eb

masseyke marked this pull request as ready for review April 24, 2023 21:59

masseyke requested a review from andreidan April 24, 2023 22:00

elasticsearchmachine added the Team:Data Management Meta label for data/management team label Apr 24, 2023

fixing a test

ba6cd03

masseyke removed the request for review from andreidan April 25, 2023 00:08

masseyke marked this pull request as draft April 25, 2023 00:08

fixing a test

b9bf110

masseyke marked this pull request as ready for review April 25, 2023 15:20

masseyke requested a review from andreidan April 25, 2023 15:21

andreidan reviewed Apr 25, 2023

View reviewed changes

andreidan requested a review from jakelandis April 25, 2023 16:26

andreidan mentioned this pull request Apr 25, 2023

[Meta] Data Lifecycle Management #93596

Closed

19 tasks

n1v0lg mentioned this pull request May 3, 2023

Add named privileges for CCR with new RCS model #95651

Merged

masseyke closed this May 5, 2023

n1v0lg reopened this May 25, 2023

n1v0lg reviewed May 25, 2023

View reviewed changes

masseyke added 5 commits May 25, 2023 09:38

merging main

e1abe6c

code review feedback

bfb646a

code review feedback

6440314

Merge branch 'main' into feature/manage_dlm-role

d20bfe5

removing manage_dlm from get-builtin-privileges

344b206

masseyke requested a review from n1v0lg May 25, 2023 20:17

fixing a test

f3a7b6c

n1v0lg approved these changes May 26, 2023

View reviewed changes

masseyke and others added 3 commits May 26, 2023 08:58

Removing unwanted error messages from acceptable list

a2b039d

Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>

fixing compilation errors

d5d524a

hiding manage_dlm privilege in released docs

1827d02

masseyke mentioned this pull request May 26, 2023

Put manage_dlm into released docs #96386

Closed

masseyke added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label May 26, 2023

elasticsearchmachine merged commit 3228058 into elastic:main May 26, 2023

masseyke deleted the feature/manage_dlm-role branch May 26, 2023 15:17

n1v0lg mentioned this pull request May 30, 2023

Port DLM permissions test to internal REST test style #96434

Merged

n1v0lg mentioned this pull request Jun 20, 2023

Rebranding user facing DLM references #96866

Merged

2 tasks

Adding manage_dlm privilege #95512

Adding manage_dlm privilege #95512

Uh oh!

Conversation

masseyke commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

elasticsearchmachine commented Apr 24, 2023

elasticsearchmachine commented Apr 24, 2023

andreidan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg Apr 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreidan commented Apr 25, 2023

n1v0lg commented Apr 27, 2023

andreidan commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

n1v0lg commented Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

masseyke commented Apr 27, 2023

n1v0lg commented Apr 27, 2023

slobodanadamovic commented May 3, 2023

n1v0lg commented May 25, 2023

n1v0lg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

masseyke commented May 25, 2023

n1v0lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Labels

6 participants

masseyke commented Apr 24, 2023 •

edited

Loading

n1v0lg Apr 26, 2023 •

edited

Loading

andreidan commented Apr 27, 2023 •

edited

Loading

n1v0lg commented Apr 27, 2023 •

edited

Loading

n1v0lg May 25, 2023 •

edited

Loading