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

[RFC] Retire support for config option config.dynamic.multi_rolespan_enabled in config.yml #4495

Open
nibix opened this issue Jun 27, 2024 · 5 comments
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.

Comments

@nibix
Copy link
Collaborator

nibix commented Jun 27, 2024

Background

I am asking for opinions and comments on the following topic because it influences the work on #3870.

Current situation

The OpenSearch security config file config.yml supports an option called config.dynamic.multi_rolespan_enabled.

This config option is undocumented. Doing a search for it only returns hits which includes dumps of config.yml where the option with default value is just coincidentally contained in the config file:

The only very brief mention of this option can be found in the docs of a related project:

Functionality

If this option is set to true (which is the default if a config.yml file in version 2/7 is in use), privileges for a single OpenSearch operation can be collected from several roles. If this option is set to false, privileges for a single operation must be located in exactly one role.

This can pertain to actions or indices.

Several indices

If I issue a search request on indices a and b, I need the privilege indices:data/read/search for both indices.

With multi_rolespan_enabled = true, it is possible that the user has two different roles which provide the different privileges:

role_1:
  index_permissions:
    index_patterns:
    - a
    allowed_actions:  
    - indices:data/read/search
role_2:
  index_permissions:
    index_patterns:
    - b
    allowed_actions:  
    - indices:data/read/search

With multi_rolespan_enabled = false, however, these roles would be not sufficient for a single search operation on both indices. Instead, a user would need to have a single role with both privileges:

role_1:
  index_permissions:
    index_patterns:
    - a
    - b
    allowed_actions:  
    - indices:data/read/search

Several actions

For example, in order to issue a bulk index insert request, I need two privileges:

  • index privilege indices:data/write/bulk
  • index privilege indices:data/write/index

With multi_rolespan_enabled = true, it is possible that the user has two different roles which provide the different privileges:

role_1:
  index_permissions:
    index_patterns:
    - foo
    allowed_actions:  
    - indices:data/write/bulk
role_2:
  index_permissions:
    index_patterns:
    - foo
    allowed_actions:  
    - indices:data/write/index

With multi_rolespan_enabled = false, however, these roles would be not sufficient for a bulk index operation. Instead, a user would need to have a single role with both privileges:

role_1:
  index_permissions:
    index_patterns:
    - foo
    allowed_actions:  
    - indices:data/write/bulk
    - indices:data/write/index

The set of operations which require two privileges at once is very limited, all can be quickly found by checking the code at:

private Set<String> evaluateAdditionalIndexPermissions(final ActionRequest request, final String originalAction) {

This shows that the following operations need two privileges:

  • _bulk, indices:data/write/bulk: requires indices:data/write/bulk and depending on the bulk item type indices:data/write/index, indices:data/write/delete or indices:data/write/update
  • Cross cluster search backing action indices:admin/shards/search_shards requires additionally indices:data/read/search
  • _aliases, indices:admin/aliases with an item type remove needs additionally indices:admin/delete
  • Create index, indices:admin/create with a specified alias needs additionally indices:admin/aliases
  • Restore snapshot, cluster:admin/snapshot/restore needs additionally indices:admin/create, indices:data/write/index -- but only if the config option plugins.security.check_snapshot_restore_write_privileges is set to true

Default value

On OpenSearch clusters where the config.yml configuration with _meta.config_version=2 is in use, this defaults to true.

On OpenSearch clusters where a config.yml configuration from ODFE 0.x in version 1 is in use, this defaults to false.

Reasons for using the non-default value multi_rolespan_enabled = false

Seriously, I do not know any reasons for using the non-default value multi_rolespan_enabled = false.

Having the option set to false allows a more granular control over privileges.

However, I would argue that this added granularity does not have any value:

  • It does not make sense to allow me searching in index a in one request and additionally in index b in another request, but denying my to search a and b at the same time.
  • It does not make sense to deny me deleting indices using the POST _aliases API while allowing it me using the normal DELETE index API.

Current work

In the context of #3870, we need to re-write significant parts of the privilege evaluation code. Supporting multi_rolespan_enabled would significantly increase the complexity of the code - and thus also increase the necessary work and later maintenance effort.

Proposal: Just drop it

In my opinion, it would be not economical to put any effort into an option that offers hardly any value.

Thus, I am proposing to drop support for that option. We should always behave like its value would be true -- the default value.

As it is an undocumented option, I would argue that such a breaking change would be even possible in a minor version upgrade.

@nibix nibix added enhancement New feature or request untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jun 27, 2024
@nibix
Copy link
Collaborator Author

nibix commented Jun 27, 2024

@scrawfor99 @cwperks @peternied @DarshitChanpura Another proposal for a simplification :) Would be curious on your opinion on this as well.

@stephen-crawford
Copy link
Contributor

Hi @nibix, I agree. Let's drop the false option. It serves no real value.

@stephen-crawford
Copy link
Contributor

[Triage] Hi @nibix, thanks for filing this issue. Going to mark as triaged.

@stephen-crawford stephen-crawford added triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable. and removed untriaged Require the attention of the repository maintainers and may need to be prioritized labels Jul 1, 2024
@cwperks
Copy link
Member

cwperks commented Jul 1, 2024

Breaking changes like removing config options are generally done on major releases. I'd be in favor of deprecating in 2.x and dropping it in 3.0.0, but I don't think it should be dropped in 2.x or else it could break some clusters.

Edit: On further review, this feature is undocumented and does not provide clear benefit. I think the benefits from Optimized Privilege Evaluation outweigh support of this undocumented setting.

@cwperks
Copy link
Member

cwperks commented Sep 6, 2024

Taking a fresh look at this RFC. This configuration option doesn't make any sense to me because there is a clear workaround where a cluster admin could always create a role that has all necessary permissions which would render config.dynamic.multi_rolespan_enabled not very useful.

When thinking about tradeoffs, I think the performance improvements from Optimized Privilege Evaluation outweigh the support for config.dynamic.multi_rolespan_enabled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request triaged Issues labeled as 'Triaged' have been reviewed and are deemed actionable.
Projects
None yet
Development

No branches or pull requests

3 participants