Skip to content

Revert optimized privilege evaluation and pick back only the action part and dependend changes #5239

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

Conversation

nibix
Copy link
Collaborator

@nibix nibix commented Apr 2, 2025

Description

In the discussion in #5181, a strong desire was expressed to not have the DLS/FLS improvements from #4380 in the OpenSearch 3.0.0 release.

As the PR is - however - already merged, we have to roll it back. It gets further complicated by the fact that the DLS/FLS improvements are part of the same PR that also brought the optimizations for action privileges, which was squashed into a single commit while merging.

Due to that, we need to rollback the PR commit 3c635c9 and then later on reinstate everything that is supposed to be kept. This is both the action privilege part itself and additionally any other code change on the rolled back files, which were committed to main afterwards.

As this is a significant and complicated change, the purpose of this PR is for now to provide an impression of the impact caused by the requested change.

This PR is divided into several commits corresponding to the revert itself and then the reinstatement of the action privilege optimization and any further PRs which modified the newly introduced changes. In the case this PR should be merged, I strongly recommend not to squash commits.

Below, I will list further notes and to do items caused by this PR:

Resolved conflicts

During the rollback, there were conflicts with the other PRs. The following sections list the PRs which I could (hopefully) reinstate completely again. Still, a certain scrutiny is necessary during review to make sure that these were properly resolved by me.

Implement new extension points in IdentityPlugin and add ContextProvidingPluginSubject #4896

This PR made changes on ActionPrivileges and ActionPrivilegesTests which got lost due to the rollback. I could reinstate these completely.

Additionally, there were conflicts in PrivilegesEvaluator.

Fix compilation issues after upgrade to Lucene 10 #5053

During rollback conflicts were encountered in DlsFlsFilterLeafReader. I ported the necessary changes back to the old version of the class.

Fix Blake2b hash #5089

This PR made changes to classes which were newly introduced by #4380. I tried to port back the changes to the old classes.

Fixed IllegalArgumentException when building stateful index privileges #5217

This is a fix which exclusively relevant for the new action privileges implementation. Thus, it was rolled back. I could however reinstate it afterwards.

Pending tasks

The biggest pending task is, obviously, to file another PR to reinstate the DLS/FLS improvements when the time is right. However, there are additional changes from other PRs that got completely lost during the rollback, as the corresponding files do not exist any more. These changes must be reinstated after the DLS/FLS improvements are merged.

Ensure that plugin can search on system index when utilizing pluginSubject.runAs #5032

This contains a bugfix to the class DlsFlsBaseContext which is now gone. When DLS/FLS is merged again, this needs to be reinstated.

Fix logic for determining if a role has no DLS/FLS/FieldMasking restrictions #5184

This is a fix which exclusively relevant for the new DLS/FLS implementation. Thus, the changes are completely gone. When DLS/FLS is merged again, this needs to be reinstated.

Ensure no active threads in any threadpool for tests in the integrationTest package #4943

This is a fix which exclusively relevant for the new DLS/FLS implementation. Thus, the changes are completely gone. When DLS/FLS is merged again, this needs to be reinstated.

Fix compilation issues after upgrade to Lucene 10 #5053

When DLS/FLS is reinstated, a couple of Lucene 10 adaptions need to be reinstated.

Fix Blake2b hash #5089

Most of the original changes of the PR are lost (albeit ported back). When DLS/FLS is reinstated, these changes need to be applied again to the new DLS/FLS code.

Open PRs which are impacted

I will try to gather a list of open PRs which will also adjust to this change

Check List

  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@kumargu
Copy link

kumargu commented Apr 4, 2025

Still, a certain scrutiny is necessary during review to make sure that these were properly resolved by me.

The biggest pending task is, obviously, to file another PR to reinstate the DLS/FLS improvements when the time is right. However, there are additional changes from other PRs that got completely lost during the rollback, as the corresponding files do not exist any more. These changes must be reinstated after the DLS/FLS improvements are merged.

thanks @nibix for the evaluation. Looks like there's extreme friction and risk to decouple the changes. I think I am aligned to merge DLS/FLS in 3.0. I don't see any other safe path forward.

We need to make sure that the Cross cluster on mixed cluster versions has no regressions.

I would also need some inputs from you which might be relevant only to AWS Managed service.

During an upgrade progress, the cluster nodes would run in mixed mode. For an upgrade from 2.19 to 3.0, do you see any issues if nodes on 2.19 running on legacy PEvaluator mode (i.e feature flag disabled) would have any problems talking with nodes on 3.0 (with "no" feature flag and new DLS/FLS code). Is it possible to test this in open-source?

@nibix
Copy link
Collaborator Author

nibix commented Apr 4, 2025

@kumargu

Thanks for your reply! Quick follow up:

We need to make sure that the Cross cluster on mixed cluster versions has no regressions.

We did manual tests with CCS and mixed clusters today. So far, the results look good. We will post the test protocol here as soon as possible.

I would also need some inputs from you which might be relevant only to AWS Managed service.

Sure. If you want, we can schedule a call for maybe Monday or Tuesday?

During an upgrade progress, the cluster nodes would run in mixed mode. For an upgrade from 2.19 to 3.0, do you see any issues if nodes on 2.19 running on legacy PEvaluator mode (i.e feature flag disabled) would have any problems talking with nodes on 3.0 (with "no" feature flag and new DLS/FLS code). Is it possible to test this in open-source?

For action privileges, mixed cluster mode is not a challenge, as the nodes are acting here completely autonomous. Nothing changes in the communication protocol.

For DLS/FLS, we have a built-in compatibility mode which takes cares that 2.19 nodes still get the proper DLS/FLS headers when they need them:

public class DlsFlsLegacyHeaders {
/**
* Defines the first OpenSearch version which does not need the legacy headers
* TODO this needs to be adapted if backported
*/
static final Version LEGACY_HEADERS_UNNECESSARY_AS_OF = Version.V_3_0_0;

This is automatically verified by the BWC tests

@kumargu
Copy link

kumargu commented Apr 5, 2025

For DLS/FLS, we have a built-in compatibility mode which takes cares that 2.19 nodes still get the proper DLS/FLS headers when they need them:

Got it. thanks.

I think I am covered. let's try to close on the cross-cluster test results and get all these changes into 3.0 (preferable beta-rc-1).

On a side note: can I request to prepare a threat model for DLS/FLS change?

@kumargu
Copy link

kumargu commented Apr 5, 2025

@cwperks WDYT?

@nibix
Copy link
Collaborator Author

nibix commented Apr 7, 2025

@kumargu

On a side note: can I request to prepare a threat model for DLS/FLS change?

Of course, we can prepare one. Do you have specific requirements regarding the format?

@kumargu
Copy link

kumargu commented Apr 7, 2025

Do you have specific requirements regarding the format?

No. anything you prefer is fine. We will likely use the same content for an AWS Security review.

@nibix
Copy link
Collaborator Author

nibix commented Apr 8, 2025

@kumargu

I have added the threat model here as a comment: #5181 (comment)

@cwperks
Copy link
Member

cwperks commented Apr 8, 2025

thanks @nibix for the evaluation. Looks like there's extreme friction and risk to decouple the changes. I think I am aligned to merge DLS/FLS in 3.0. I don't see any other safe path forward.

I agree with this. There's been a lot of diligence around this area in implementation and review of the Optimized Privilege Evaluation feature. With #5237 there will be additional automated assurances as well.

@nibix
Copy link
Collaborator Author

nibix commented Apr 8, 2025

Closing this as we no longer follow the intent of reverting the DLS/FLS changes

@nibix nibix closed this Apr 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants