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

Deprecate and replace old Index privileges related to indexing documents #47333

Open
1 of 5 tasks
bizybot opened this issue Oct 1, 2019 · 22 comments
Open
1 of 5 tasks
Labels
>deprecation >enhancement Meta :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team

Comments

@bizybot
Copy link
Contributor

bizybot commented Oct 1, 2019

Existing Index privilege names fail to convey the meaning properly and a few like create have their own unexpected semantics (it allows indexing document only via Index API). Since we are adding a new create_doc privilege, we want to take this opportunity to clean up the existing privileges and either provide replica privilege if the semantics are correct or provide an alternative that is semantically correct. Also, the naming can be explicit about the operation that is performed.

Following are the existing Index privileges related to documents indexing lifecycle:
create: The privilege allows for indexing documents but only via Index/Bulk API but no update via Update API. It also allows update mapping actions. This one does not prevent document updates when using Index APIs.
index: The privilege allows indexing documents via Index/Bulk API, also allows for update action and mapping actions.
delete: The privilege allows deletion of documents
write: This privilege allows all write operations (index, deletes, updates) along with update mapping actions.
read: Read-only access to actions like count, explain, get, mget, get indexed scripts, etc.

Following are the proposed new Index privileges:

  • create_doc: This one is with changed semantics, allowing only indexing new documents but no updates. Plus this one removes the unexpected semantics of restricting this operation via Index API only. This will allow indexing new documents via Index/Bulk API and allows update mapping action. Add 'create_doc' index privilege #45806
  • index_doc: This will be similar to index privilege.
  • delete_doc: This will be similar to delete privilege.
  • write_doc: This will be similar to write privilege.
  • read_doc: This will be similar to read privilege.
Privilege ▼ Actions ► Index new document Update document Delete document Update mapping Read actions (search, count etc.)
create_doc Allow Deny Deny Allow Deny
index_doc Allow Allow Deny Allow Deny
delete_doc Deny Deny Allow Deny Deny
write_doc Allow Allow Allow Allow Deny
read_doc Deny Deny Deny Deny Allow

Additional considerations:

  • During the discussion of naming of create_doc we had a proposal to split the index privilege into:
    index_doc: Privilege to index documents. It also includes create_doc privilege.
    I think this would be a breaking change but adding it to discuss this.
    update_doc: Privilege to update documents. Not sure how this would work, for example, an upsert case allows update of an existing document or index new document if it does not exist.
    If we want to support this splitting of index privilege into two with above semantics then it would be a breaking change and might not be possible other than to modify the index engine to handle it.

  • read_doc privilege: We could consider some more granular approach. I see an enhancement request: https://github.com/elastic/enhancements/issues/5470 which talks about providing a way to restrict users to certain search types?

Proposed path forward:

  • When do we introduce these new Index privilege changes and deprecate old?
    Introduce these new index privileges in v8.0.0 and start logging deprecated warnings.
  • When do we remove the usage of old Index privileges? v9.0.0
  • Can we provide some migration tool/upgrade assistance?
    We have an issue opened to discuss this possibility to enhance the deprecation info API Allow Deprecation Info API to check for deprecated security configuration #47714
    We could consider providing a tool for migration but it might not be possible to migrate all the role descriptors using the deprecated privilege names.
    The problem exists for the privilege which does not have the exact alternative or would change the behavior if we use the alternative, eg. create with possible alternative create_doc.
    • There are different places where the index privilege is used during role definition:
      • file (roles.yml).(If an alternative exists for the privilege we can update the role definitions in the file. This option is less likely as we would want to avoid file mutations)
      • built-in roles (If an alternative exists for the privilege we can update the role definitions)
      • role definitions indexed in security index via API (If an alternative exists for the privilege we can update the role definitions)
      • dynamic role definitions - custom roles provider extension (This is a custom security extension and will require users to update the extension).
  • Do we want to consider the additional changes like splitting the index privilege or more granular privileges for read?

This issue exists to gather feedback on the path forward and comments around introducing/removing/updating index privileges in relation to indexing documents.

@bizybot bizybot added >enhancement :Data Management/Indices APIs APIs to create and manage indices and templates >deprecation :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Oct 1, 2019
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-security

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-distributed

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-features

@bizybot
Copy link
Contributor Author

bizybot commented Oct 9, 2019

Hi @ywelsch I have added your suggestions to split the index into index_doc and update_doc privilege but wanted to confirm if it is what you were proposing. I feel that it would be breaking change and do you think strongly about it? Thanks for your inputs.

@bizybot
Copy link
Contributor Author

bizybot commented Oct 9, 2019

UI currently uses GET _security/privilege/_builtin to list the built-in privileges. We could enhance the response to include the deprecated index privilege names and alternatives/suggestions as:

{
  "cluster": [
    "..."
  ],
  "index": [
    "all",
    "create",
    "create_doc"
    "...",
    "write"
  ],
  "deprecations": {
    "cluster": [],
    "index": [
      {
        "name": "create",
        "alternative/suggestion": "create_doc",
        "is_exact_replacement": false
      },
      {
        "name": "index",
        "alternative/suggestion": "index_doc",
        "is_exact_replacement": true
      }
    ]
  }
}

@tvernum
Copy link
Contributor

tvernum commented Oct 9, 2019

@bizybot The whole point of creating new names is so that we can make breaking changes. We explicily don't want the new names to be exact replicas of the old names.

  • index_doc should not allow the _update API. It should behave like the old create privilege. The _update API is not a standard indexing API and has different security implications. It should not be part of the standard "indexing" privilege.

  • update_doc should allow the use of the _update API (which is also op_type=UPDATE). I don't think it should allow any other sort of indexing, mostly because the name implies update-only and the principle of least surprise implies that it should therefore be update-only. If admins want index+update (but not delete) then they can grant two privileges.

@bizybot
Copy link
Contributor Author

bizybot commented Oct 10, 2019

@tvernum Thank you for your comment.

@bizybot The whole point of creating new names is so that we can make breaking changes. We explicily don't want the new names to be exact replicas of the old names.

Agreed, we are making breaking changes here as with the new names.
I wanted to highlight the behavior for update_doc (specifically the upsert) but my sentence was poorly constructed. I have explained below on update_doc behavior that I had observed that lead me to that.

  • index_doc should not allow the _update API. It should behave like the old create privilege. The _update API is not a standard indexing API and has different security implications. It should not be part of the standard "indexing" privilege.

Agreed, this will be similar to create privilege.

  • update_doc should allow the use of the _update API (which is also op_type=UPDATE). I don't think it should allow any other sort of indexing, mostly because the name implies update-only and the principle of least surprise implies that it should therefore be update-only. If admins want index+update (but not delete) then they can grant two privileges.

The _update API allows for upserts and this where my confusion is.
The update_doc would imply update-only which means no indexing of new documents.

But with this new privilege the behavior that I observed:

  • Index API which states if the document already exists, updates the document but a user with update_doc would not be allowed to do updates using this API.
  • Update API that allowed upserts. If we use script then it tries to do an index operation that fails to update existing document (which is incorrect, I assume this happens as internally we convert this into an index request)
    This is what I was referring to as the breaking thing but did not clearly explain it.

@bizybot
Copy link
Contributor Author

bizybot commented Oct 11, 2019

I am proposing the following privileges (a little different from an earlier attempt):

  • create_doc: allows indexing of new documents only no updates. allows update mapping action.
  • update_doc: allows indexing of document updates. does not allow indexing new documents.
    allows update mapping action. This should work with Index and Update APIs
    to support this we can think of introducing request based index privilege so authorization engine can look at the request and deny requests with scripts? Index API might require some changes to support this?
  • index_doc: this is similar to existing index privilege and allows indexing new documents, updates, upserts plus update mapping action.
  • delete_doc: similar to delete privilege
  • write_doc: similar to write privilege
  • read_doc: similar to read privilege
  • read_doc_cross_cluster: similar to read_cross_cluster privilege

create privilege could then use update_doc privilege as a replacement as the behavior would be maintained (avoiding any leaks when using scripts)

thoughts?

@tvernum
Copy link
Contributor

tvernum commented Oct 11, 2019

I still maintain that index_doc should not support _update.

People will expect that "index_doc" is the privilege that they want if they're ingesting data (and can't guarantee that it will be new docs only).
However, the Update API implicitly supports reading existing data which is a surprising behaviour and should not be included in standard indexing privilege.

@bizybot
Copy link
Contributor Author

bizybot commented Oct 11, 2019

I see your point, in that case, I propose the following:

  • index_doc: create_doc + update_doc (given that the update_doc would not allow scripts)
  • upsert_doc: which is similar to the old index privilege. (If we want to have something like index privilege). [The naming could be different but to me, this one seemed to set the expectation right.]

@tvernum
Copy link
Contributor

tvernum commented Oct 11, 2019

Do we see much demand for update without script?
It's true that the ability to run scripts against the existing doc, is the primary reason why the update API has non-obvious security implications, so I agree that a non-scriptable update is safer and is probably safe enough to support being in a "normal" privilege.

But, is it valuable enough to build & maintain?

@tvernum
Copy link
Contributor

tvernum commented Oct 11, 2019

If we want to have something like index privilege

I think we need a privilege that allows the full _update API without allowing deletes. Technically an update (or even just index over an existing doc) can blank out all the fields and effectively delete a doc, but I think users will expect to have a way to grant scriptable updates without granting delete.
I don't care strongly about whether that privilege grants all indexing APIs, or only the update API (mostly I think it comes down to having the name and the access be in alignment), but I don't think we can say that write_doc is the only privilege that grants Update API.

Happy to be convinced otherwise though.

@ywelsch
Copy link
Contributor

ywelsch commented Oct 14, 2019

I wonder if we should even have an update privilege or if we can decompose that into read + index (+ delete). If we consider update just as a shortcut for read + index (or possibly delete in case of scripted updates), perhaps all those privileges should be granted for now if someone wants to use updates. We can then later work on more fine-grained update permissions if the need arises for those.

@bizybot
Copy link
Contributor Author

bizybot commented Oct 16, 2019

Thank you @tvernum and @ywelsch for your comments.

if we should even have an update privilege or if we can decompose that into read + index (+ delete)

I think decomposing update privilege might not be that easy to achieve due to the current privilege model. Our privileges work on the action patterns and given that we can do a combination of operations like create, delete, update on the documents via scripts, it would be a trivial task to achieve this as we would need to authorize not just based on action names but also looking at the request object. We would need to think about changes to the authorization engine/model and we would not want to visit this just with this use case in mind.

Do we see much demand for update without script?

It's hard to tell and it might depend on the use case. If the use case has frequent updates
then the users might prefer upserts and the related hints (I am unsure of this though as we do not give any recommendation in our tune for indexing document).

I think we need a privilege that allows the full _update API without allowing deletes

Scripts, if not used properly, could have a performance impact and maybe the reason that we have kept update action separate. If we say that update with scripts as an advanced option available for users then I agree we should consider it as a separate privilege.

Following is what I propose based on the discussions:

  • create_doc: allows indexing of new documents only no updates. allows update mapping action.
  • index_doc: allows indexing of new documents and overwriting documents, allows update mapping action. This does not allow update action via Update/Bulk API.
  • index_update_doc/upsert_doc: this allows indexing, updating documents via Index/Bulk/Update API and allows update mapping action. The naming could be different, any suggestions?
  • delete_doc: similar to delete privilege

We are not considering granular read privileges and so keeping the privilege names the same. So when we revisit the privilege in future we can consider apt naming for it. The read privileges read, read_cross_cluster will remain the same.

As discussed above, write privilege name gives access to index, update, delete documents and that can be achieved by combining the privileges that exist. For example, upsert_doc + delete_doc would give a privilege that is equivalent of write privilege. So we can think of dropping the write privilege.

From the migration point of view, the following is the mapping:

Existing Privilege New Privilege isExactReplacement
create index_doc true
index upsert_doc true
delete delete_doc true
write user will need to do upsert_doc + delete_doc we could consider adding read_write_delete_doc privilege false

@ywelsch ywelsch removed the :Distributed/Engine Anything around managing Lucene and the Translog in an open shard. label Oct 23, 2019
@ywelsch
Copy link
Contributor

ywelsch commented Oct 24, 2019

My main qualm with the API-oriented privileges (create/index/update, with all the variations of what update can do) is that they allow the user to restrict what APIs they might call. This is unrelated however to the effects that those APIs have. I find it more interesting to limit what changes the users can do to the data (with the basic ones being: only allows creations of new documents / allow overriding of documents / allow reading documents / allow deletion of documents).

I think decomposing update privilege might not be that easy to achieve due to the current privilege model. Our privileges work on the action patterns and given that we can do a combination of operations like create, delete, update on the documents via scripts ...

An update is internally already a "get" + "index". Perhaps we could internally use transport actions for those steps so that they can be captured by the action patterns. The indices:data/write/update* would then just be similar to indices:data/write/bulk, i.e. a container for the actual actions. This would allow a more fine-granular model, e.g. a scripted delete (i.e. update script that explicitly sets ctx.op = 'delete') would only read + delete privileges.

As discussed above, write privilege name gives access to index, update, delete documents and that can be achieved by combining the privileges that exist. For example, upsert_doc + delete_doc would give a privilege that is equivalent of write privilege. So we can think of dropping the write privilege.

I might still be convenient to have this "all things allowed privilege". Perhaps we can just call it all_doc (allows all document operations).

@bizybot
Copy link
Contributor Author

bizybot commented Oct 24, 2019

Thank you @ywelsch for the comments.

Perhaps we could internally use transport actions for those steps

This is interesting as it did come up during my discussion with @tvernum and we did think that if internally we used the transport actions, then it becomes easier from a security perspective and we can enforce granular model. But it seemed that we have avoided using transport actions maybe for other reasons (like performance), pardon my knowledge here.

We are trying to target these changes for the upcoming release, do you think we would be okay to modify the current behavior, how big of this change would be to start using transport actions?

convenient to have this "all things allowed privilege"

Yes, we can consider adding such privilege as a convenience.

@bizybot
Copy link
Contributor Author

bizybot commented Oct 30, 2019

Just an update, I talked to @ywelsch offline and seems like we might be able to use transport actions internally for the steps that update action does in a way such that there are no performance degradation or missing functionality. I have started to explore the approach and with help from Yannick will come up with a draft PR where we can discuss.

@kobelb
Copy link
Contributor

kobelb commented Nov 26, 2019

@elastic/es-security is anyone going to be picking this up for 7.6? We're dependent on the ES API to determine which index privileges are deprecated in elastic/kibana#46609

@jakelandis jakelandis removed the :Data Management/Indices APIs APIs to create and manage indices and templates label Dec 5, 2019
@rjernst rjernst added the Team:Security Meta label for security team label May 4, 2020
@legrego
Copy link
Member

legrego commented Jul 25, 2022

I'm checking in to see if this is still something we eventually want. I'm grooming Kibana's backlog, and I'd like to know if we should keep our associated issue open

@albertzaharovits
Copy link
Contributor

@legrego We discussed this in the ES Security team weekly sync on the 4th of Aug . Overall we do not plan to rename or remove any of the doc index-related privileges.
Renaming privileges to append _doc would overall create a more consistent naming scheme, but the two would have to coexist for a long time which would on the contrary, clutter the user experience in the long interim.
Also, there was no consensus as to which privilege should we remove out of create_doc, create, index and write .

If we sometimes decide we want to phase out some privileges, there is the /_security/privilege/_builtin API that we should maintain to return a list of valid and preferred privileges.

@elasticsearchmachine
Copy link
Collaborator

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

@legrego
Copy link
Member

legrego commented Aug 10, 2022

Thanks for the update, @albertzaharovits. I'll close out our associated issue since there are no plans at this time to move forward with index privilege deprecations. I am happy to re-open if the need arises in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>deprecation >enhancement Meta :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team
Projects
None yet
Development

No branches or pull requests

10 participants