opensearch-project / security

🔐 Secure your cluster with TLS, numerous authentication backends, data masking, audit logging as well as role-based access control on indices, documents, and fields
https://opensearch.org/docs/latest/security-plugin/index/
Apache License 2.0
180 stars 263 forks source link

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

Open nibix opened 4 days ago

nibix commented 4 days ago

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:

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:

https://github.com/opensearch-project/security/blob/6dedfb455766601b1fdee8e5334cd0905bfe2ed2/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java#L601

This shows that the following operations need two privileges:

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:

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 commented 4 days ago

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

scrawfor99 commented 3 days ago

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