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

Jump to bottom

Disallow mapping updates for doc ingestion privileges #58784

Merged
merged 43 commits into from Jul 14, 2020
Merged

Disallow mapping updates for doc ingestion privileges #58784

merged 43 commits into from Jul 14, 2020

Conversation

albertzaharovits
Copy link
Contributor

@albertzaharovits albertzaharovits commented Jun 30, 2020 •
edited

The create_doc, create, write and index privileges lose the Put Mapping allowed action in the next major release.
Create_doc, create and index also lose the Auto Put Mapping allowed action (mapping updates generated internally from templates).

In order to maintain the bwc in the 7.x releases, the above privileges will still allow the Put and Auto Put Mapping actions, but only when the "index" entity is an alias or a concrete index, but not a data stream or a backing index of a data stream.

@albertzaharovits albertzaharovits self-assigned this Jun 30, 2020
@tvernum
Copy link
Contributor

tvernum commented Jul 1, 2020

Separately to the implementation details, I think we need to discuss exactly which privileges this should apply to.
For example, I wonder whether write should continue to allow mapping changes, but I don't hold a strong view.

tvernum
tvernum reviewed Jul 1, 2020
View reviewed changes
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.

I like it, and it ends up not too complex. I think this is a reasonable option on how to proceed.

It's not quite how I was thinking I would do it, but I think it might be easier for me to explain my idea in code, so I'm going to whip something up.

.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java Outdated Show resolved Hide resolved
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java Outdated Show resolved Hide resolved
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java Outdated Show resolved Hide resolved
...core/src/main/java/org/elasticsearch/xpack/core/security/authz/privilege/IndexPrivilege.java Outdated Show resolved Hide resolved
@albertzaharovits
Copy link
Contributor Author

https://gradle-enterprise.elastic.co/s/mxy44ehb4drbw is legit test failure, and welcomed. It's due to the deprecation logs, but I want another run.

@elasticmachine test this please

ywangd
ywangd approved these changes Jul 13, 2020
View reviewed changes
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. My comments are mostly about code structures and I don't think they are critical enough to block approval.

tvernum
tvernum reviewed Jul 13, 2020
View reviewed changes
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java Outdated Show resolved Hide resolved
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java Outdated Show resolved Hide resolved
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java Show resolved Hide resolved
.../src/main/java/org/elasticsearch/xpack/core/security/authz/permission/IndicesPermission.java Outdated
deprecationLogger.deprecate("[" + indexOrAlias + "] mapping update for ingest privilege [" +
privilegeName + "]", "the mapping update action [" + action + "] on the [" +
indexOrAlias + "] index, is granted by the [" + privilegeName + "] privilege," +
" but the privilege has been tightened to not allow it in the next major release");
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a fan of this wording - I'll propose an alternative during my day.

@tvernum
Copy link
Contributor

tvernum commented Jul 13, 2020

if one grants create_doc on a data stream, automatic mapping updates are not allowed if ingesting via the data stream but could be allowed when ingesting via the backing index.

That would be a bad outcome. The point of this change is so that granting create_doc on a data stream gives a true minimal privilege. If it were possible to by-pass that by simply specifying the backing index name, it would not achieve the desired outcome.

it's simpler if auto mapping updates are not allowed on data streams as well as on the backing indices

That is the better result - the mirroring of privileges from data stream to backing index is a necessary detail to make data streams work, and should therefore follow the definition of how those privileges are defined for a data stream, not how they would have been defined if they were applied to an index.

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-sample-windows

@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-sample-windows

1 similar comment
@albertzaharovits
Copy link
Contributor Author

@elasticmachine run elasticsearch-ci/packaging-sample-windows

@albertzaharovits albertzaharovits merged commit dd99bef into elastic:master Jul 14, 2020
@albertzaharovits albertzaharovits deleted the remove_put_mapping_priv branch July 14, 2020 18:45
@albertzaharovits albertzaharovits mentioned this pull request Jul 14, 2020
Merged
albertzaharovits added a commit that referenced this pull request Jul 14, 2020
@albertzaharovits
Disallow mapping updates for doc ingestion privileges (#58784)
4eb310c 
The `create_doc`, `create`, `write` and `index` privileges do not grant
the PutMapping action anymore. Apart from the `write` privilege, the other
three privileges also do NOT grant (auto) updating the mapping when ingesting
a document with unmapped fields, according to the templates.

In order to maintain the BWC in the 7.x releases, the above privileges will still grant
the Put and AutoPutMapping actions, but only when the "index" entity is an alias
or a concrete index, but not a data stream or a backing index of a data stream.
@albertzaharovits albertzaharovits mentioned this pull request Jul 21, 2020
Merged
albertzaharovits added a commit that referenced this pull request Jul 23, 2020
@albertzaharovits
[DOCS] Mapping updates are deprecated for ingestion privileges (#60024)
2eaf5e1 
This PR contains the deprecation notice that `create`, `create_doc`, `index` and
`write` ingest privileges do not permit mapping updates in version 8. It also
updates the docs description of said privileges. 

This should've been part of #58784
albertzaharovits added a commit that referenced this pull request Jul 23, 2020
@albertzaharovits
[DOCS] Mapping updates are deprecated for ingestion privileges (#60024)
6b8736f 
This PR contains the deprecation notice that `create`, `create_doc`, `index` and
`write` ingest privileges do not permit mapping updates in version 8. It also
updates the docs description of said privileges. 

This should've been part of #58784
@albertzaharovits albertzaharovits mentioned this pull request Mar 31, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ywangd ywangd ywangd approved these changes

@tvernum tvernum tvernum approved these changes

Assignees

@albertzaharovits albertzaharovits

Labels
>deprecation >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team v7.9.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@albertzaharovits @tvernum @elasticmachine @ywangd @jakelandis