Skip to content

Conversation

@cwperks
Copy link
Member

@cwperks cwperks commented Dec 2, 2025

Description

This PR adds a new dynamic cluster setting (plugins.security.dls.write_blocked) to block all write operations against indices where restrictions apply for the calling user. When set to true all write operations (IndexRequest, UpdateRequest, DeleteRequest) are blocked. When set to false (the default value) only UpdateRequest is block whereas other write operations are permitted.

Currently, the documentation website has a blurb on dls and write permissions that instructs cluster administrators about this behavior, but this behavior has caused confusion. This PR aims to introduce a cluster setting to prevent cluster admins from shooting themselves in the foot.

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Enhancement

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • 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.

opensearch-trigger-bot bot and others added 15 commits August 11, 2025 19:14
…search-project#5556)

Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Darshit Chanpura <[email protected]>
… from being shaded in opensaml jar (opensearch-project#5559)

Signed-off-by: Darshit Chanpura <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
@codecov
Copy link

codecov bot commented Dec 2, 2025

Codecov Report

❌ Patch coverage is 66.66667% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.68%. Comparing base (cc1d27f) to head (6ffa252).

Files with missing lines Patch % Lines
...search/security/configuration/DlsFlsValveImpl.java 60.00% 2 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5828      +/-   ##
==========================================
+ Coverage   73.66%   73.68%   +0.02%     
==========================================
  Files         438      438              
  Lines       26642    26653      +11     
  Branches     3937     3939       +2     
==========================================
+ Hits        19626    19640      +14     
+ Misses       5146     5138       -8     
- Partials     1870     1875       +5     
Files with missing lines Coverage Δ
.../opensearch/security/OpenSearchSecurityPlugin.java 85.39% <100.00%> (+0.01%) ⬆️
...g/opensearch/security/support/ConfigConstants.java 77.27% <ø> (ø)
.../opensearch/security/support/SecuritySettings.java 83.33% <100.00%> (+3.33%) ⬆️
...search/security/configuration/DlsFlsValveImpl.java 66.36% <60.00%> (+1.58%) ⬆️

... and 9 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes mostly look good to me. Left couple of comments.

I see commits around CHANGELOG entry but don't see the CHANGELOG file modified. We should add an entry for this feature.

new OpenSearchSecurityException("Update is not supported when FLS or DLS or Fieldmasking is activated")
new OpenSearchSecurityException(
inner.request().getClass().getSimpleName()
+ " is not supported when FLS or DLS or Fieldmasking is activated"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this message should change to "...when write block is active". thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the message is bad, but I think it can be reworded differently. Maybe, <WriteRequest> is not support when index has FLS, DLS or FieldMasking Restrictions?

}
List<Response> responses = RestHelper.requestAgainstAllNodes(
testUserRestClient,
adminClient(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why change it to adminClient?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bc the test user has restrictions on the index. Its better to use admin client here which has no restrictions.

This block is for seeding test data for bwc tests and if we made the default for this new setting true then the calls here would fail since the test user has restrictions.

@cwperks
Copy link
Member Author

cwperks commented Dec 2, 2025

I see commits around CHANGELOG entry but don't see the CHANGELOG file modified. We should add an entry for this feature.

I was planning to push it after the CHANGELOG is cleared for 3.5 since I think this will miss the 3.4 release train.

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.

2 participants