search

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 V6 configuration #4493

Open nibix opened 4 days ago

nibix commented 4 days ago

Preface

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

Current situation

Configuration file formats

At the moment, the security plugin supports two slightly different configuration file formats:

In the *.yml files, these can be told apart by the _meta.config_version property. Internally, there these can be told apart by the V6 vs V7 suffix of class names: RoleV6 vs RoleV7. V7 in class names corresponds to config_version = 2.

Example for the differences:

New, V7:

_meta:
  type: "roles"
  config_version: 2
example_role:
  cluster_permissions: 
    - CLUSTER_COMPOSITE_OPS
  index_permissions:
    - index_patterns:
        - "foo*"
      dls: 'query'
      allowed_actions:
        - CRUD

Old, V6:

example_role:
  cluster:
    - CLUSTER_COMPOSITE_OPS
  indices:
    'logstash-*':
      '*':
        - CRUD
        - CREATE_INDEX
    '_dls_': 'query'

Actually, the number 6 in the class names still stems from the Elasticsearch version 6. Thus, the roles.yml configuration type allows the specification of document types, even though these are not supported any more in OpenSearch 2. Consequently, the implementation actually ignores this information.

Thus, these configuration files stem from the ODFE times, when it was still available for Elasticsearch 6.

Further changes include some cleanup and restructuring of the configuration file format.

Privilege evaluation code

Even though the central privilege evaluation code is located in the class PrivilegeEvaluator, the actual logic which interprets the the roles is located in the classes ConfigModelV6 and ConfigModelV7:

Depending on the used configuration version, either ConfigModelV6 and ConfigModelV7 is used. Even though both classes should apply the same logic, both contain separate implementations. Both implementations are very complex: ConfigModelV6 has 1316 lines of code and ConfigModelV7 has 1267 lines of code.

Especially for security critical code, it seems to be an anti-pattern to have such fundamental and critical code doubled up. It increases maintenance effort and cognitive load.

An additional drawbacks of the current approach is while the central SecurityDynamicConfiguration includes a generic parameter, it is virtually impossible to use this parameter in a safe way. Most APIs just use SecurityDynamicConfiguration<?> or do unsafe casts.

Current work

In the context of #3870, we are going to replace major parts of the ConfigModel classes by new code. As the goal of the code is performance improvements, its complexity will actually grow a bit compared to the old implementation. See here for the current state:

https://github.com/opensearch-project/security/blob/60af051d28dcd239167a0ce7bcb059ad192f4de3/src/main/java/org/opensearch/security/privileges/ActionPrivileges.java

This implementation presently only supports the RoleV7 classes.

I have doubts whether implementing the same code also for RoleV6 would make sense. Creating also support for RoleV6 would cause significant effort and significant increase of the code complexity. Meanwhile, I would actually have doubts whether anyone is still using this configuration file format on OpenSearch 2.

Proposal: Drop support V6 configuration file formats

Thus, I would propose to drop support for the V6 configuration file formats. I really believe that this is largely just dead code, which just causes maintenance effort.

Alternative proposal: Behind the scenes conversion from V6 to V7.

Alternatively, I would propose to refactor the configuration infrastructure so that the V7 file formats become "first class citizens", while the V6 file formats get "second class citizens", which are never directly processed. The configuration infrastructure code would take care to automatically convert any V6 configuration to V7 configuration. Regular application code would then never need to touch V6 classes.

The V7 classes could then actually drop the V7 suffix to show that these are the main classes.

nibix commented 4 days ago

@scrawfor99 @cwperks @peternied @DarshitChanpura Would be curious on your opinion on this.

scrawfor99 commented 4 days ago

Dropping the V6 configuration format and logic should be fine as long as it is only for version 3.0. I think removing it before then would be considered a breaking change so we would not be able to move it in before 3.0.

There is also some translation between the v6 and v7 versions which will need to be replaced with the new version as well. For instance the user object has some fields in v6 which are not present in the v7 equivalent.

cwperks commented 4 days ago

@nibix I have not personally tested it out, but is it possible to get into situations where you can upgrade from ES <= 6 directly into OpenSearch? In those cases are the V6 objects required to read in the old index, but then the Migrate API can help to migrate to V7?

Its not recommended to skip major versions, but what happens if someone tries?

The recommended upgrade path is to always go to the last release of the next major version. If you want to upgrade multiple versions then you would need to upgrade to each major version as an intermediate step.

I think its fine to remove from OS 2 given that it is 2 major versions removed from ES6. It would certainly simplify the code base greatly :)

nibix commented 4 days ago

@cwperks

I have not personally tested it out, but is it possible to get into situations where you can upgrade from ES <= 6 directly into OpenSearch?

According to the docs at https://opensearch.org/docs/latest/upgrade-to/upgrade-to/ this upgrade path is possible:

Regarding the question whether it is possible to upgrade from ES 7.10.2 directly to OpenSearch 2, or whether an intermediate step via OpenSearch 1 is necessary, I only found recommendations, no hard facts. The recommendation is however to upgrade via OpenSearch 1 to OpenSearch 2.

In those cases are the V6 objects required to read in the old index, but then the Migrate API can help to migrate to V7?

Yes, and with securityadmin migrate:

https://opensearch.org/docs/2.15/security/configuration/security-admin/#backup-restore-and-migrate

I believe that it is theoretically possible to upgrade from ES 6 with ODFE to OpenSearch 1.

nibix commented 4 days ago

One interesting data point is also that all versions for ODFE for ES 6 were 0.x versions, i.e., it was ODFE 0.7.0 to ODFE 0.9.0:

https://opendistro.github.io/for-elasticsearch-docs/version-history/

The first public version of ODFE for ES 6 was released in March 2019. The first version of ODFE for ES 7 was released in June 2019.

nibix commented 4 days ago

@scrawfor99

Dropping the V6 configuration format and logic should be fine as long as it is only for version 3.0. I think removing it before then would be considered a breaking change so we would not be able to move it in before 3.0.

I agree that the clean path is a breaking change in a major release, with a deprecation notice/warning being issued in some preceding version.

Is there somewhere information how far OpenSearch 3 is away?

nibix commented 4 days ago

See also #4495

scrawfor99 commented 3 days ago

Hi @nibix,

Is there somewhere information how far OpenSearch 3 is away?

I am not sure but I suspect it will be sometime in 2025.

nibix commented 2 days ago

So far, I gather from the opinions that the main proposal "Drop support V6 configuration file formats" is not a short-term thing, but should be done for OpenSearch 3.

Regarding #3870, we would have two options: