Skip to content

[OpenSearch Dashboards] Create API for getting and updating advanced settings in favor of direct .kibana index operation#21038

Open
cwperks wants to merge 5 commits intoopensearch-project:mainfrom
cwperks:settings-api
Open

[OpenSearch Dashboards] Create API for getting and updating advanced settings in favor of direct .kibana index operation#21038
cwperks wants to merge 5 commits intoopensearch-project:mainfrom
cwperks:settings-api

Conversation

@cwperks
Copy link
Copy Markdown
Member

@cwperks cwperks commented Mar 30, 2026

Description

Companion OpenSearch-Dashboards PR: cwperks/OpenSearch-Dashboards#3
Companion Security PR: cwperks/security#75

I'm opening up this PR to discuss something I've been thinking about for a long time. Currently, OpenSearch-Dashboards does direct indexing requests into the .kibana* indices to store saved objects and advanced settings. This leads to a bit of oddities in how to define roles and authorize actions to these indices. They are indices that store metadata for the system, but as of today they are not formally "system" indices. As such, you see funky security configs like the kibana_user role.

This PR is intended to be a step in the direction of creating backend APIs dedicated for OpenSearch Dashboards. In fact, core already has a module called opensearch-dashboards to create dedicated Dashboards APIs though that model is unused.

With this PR in particular, I aim to solve a permissions problem with tiering access to dashboards application-wide settings (or tenant-wide settings in the case of multi-tenancy). As of today, there are 2 tiers of access:

  1. read
  2. write

As the names imply, read users cannot create dashboards + visualizations or update setting and write users can create dashboards + visualization and update settings.

Having only 2 tiers is quite simplistic as an admin may want to restrict who can edit application-wide settings or tenant-wide settings. With the API introduced in this PR, it would allow the security plugin to differentiate between requests that update settings vs those that create or update a saved objects. Currently, to the security plugin both of these operations are equivalent because they are an index operation to the .kibana index. By creating a dedicated API for advanced settings, security will have a better signal to differentiate between a request to update dashboards settings vs on to create or update a saved object.

So with this change and its companions there would be 3 tiers of access:

  1. read
  2. write
  3. admin

I know this change may be controversial (hopefully only at first), but I do believe that directionally that this is a direction I'd like to move towards to remove the oddities for how current-state dashboards authorization works.

Related Issues

Related to Resolves opensearch-project/security-dashboards-plugin#2337

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

cwperks and others added 4 commits March 28, 2026 17:03
…ect .kibana index operation

Signed-off-by: Craig Perkins <craig5008@gmail.com>
Signed-off-by: Craig Perkins <craig5008@gmail.com>
…ect .kibana index operation

Signed-off-by: Craig Perkins <craig5008@gmail.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks cwperks requested review from a team and peternied as code owners March 30, 2026 14:15
@github-actions
Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit 666be7b.

PathLineSeverityDescription
modules/opensearch-dashboards/src/main/java/org/opensearch/dashboards/action/TransportGetAdvancedSettingsAction.java48highUses stashContext() to strip security thread context, then executes a GET on a caller-controlled index/documentId. Any authenticated user reaching this endpoint can read from any index with system-level privileges, bypassing the security plugin's index-level access controls.
modules/opensearch-dashboards/src/main/java/org/opensearch/dashboards/action/TransportWriteAdvancedSettingsAction.java39highUses stashContext() combined with fully caller-controlled index, documentId, and document body. This allows writing arbitrary content to any index with elevated (system) privileges, bypassing the security plugin — a clear privilege escalation / backdoor write path.
modules/opensearch-dashboards/src/main/java/org/opensearch/dashboards/rest/RestGetAdvancedSettingsAction.java35mediumNew REST handler is NOT wrapped in OpenSearchDashboardsWrappedRestHandler, unlike every other handler registered in the plugin. This inconsistency may intentionally bypass whatever authentication/restriction checks that wrapper enforces.
modules/opensearch-dashboards/src/main/java/org/opensearch/dashboards/rest/RestWriteAdvancedSettingsAction.java35mediumNew REST write handler is also NOT wrapped in OpenSearchDashboardsWrappedRestHandler, and accepts an arbitrary 'operation' parameter (CREATE vs UPDATE) from the request, enabling upsert-like behavior with no validation on the target index or content.
modules/opensearch-dashboards/src/main/java/org/opensearch/dashboards/action/WriteAdvancedSettingsRequest.java94lowvalidate() unconditionally returns null, performing no validation of index name, documentId, or document content. While not malicious on its own, combined with the stashContext privilege escalation, it removes the last guard against targeting sensitive system indices.

The table above displays the top 10 most important findings.

Total: 5 | Critical: 0 | High: 2 | Medium: 2 | Low: 1


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@cwperks cwperks added the skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis. label Mar 30, 2026
@github-actions
Copy link
Copy Markdown
Contributor

Failed to generate code suggestions for PR

@github-actions
Copy link
Copy Markdown
Contributor

✅ Gradle check result for 666be7b: SUCCESS

@codecov
Copy link
Copy Markdown

codecov bot commented Mar 30, 2026

Codecov Report

❌ Patch coverage is 0% with 138 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.23%. Comparing base (dbe98aa) to head (666be7b).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...ashboards/action/WriteAdvancedSettingsRequest.java 0.00% 40 Missing ⚠️
.../dashboards/action/GetAdvancedSettingsRequest.java 0.00% 20 Missing ⚠️
...rds/action/TransportGetAdvancedSettingsAction.java 0.00% 18 Missing ⚠️
...ch/dashboards/action/AdvancedSettingsResponse.java 0.00% 17 Missing ⚠️
...s/action/TransportWriteAdvancedSettingsAction.java 0.00% 15 Missing ⚠️
...shboards/rest/RestWriteAdvancedSettingsAction.java 0.00% 14 Missing ⚠️
...dashboards/rest/RestGetAdvancedSettingsAction.java 0.00% 7 Missing ⚠️
...h/dashboards/action/GetAdvancedSettingsAction.java 0.00% 3 Missing ⚠️
...dashboards/action/WriteAdvancedSettingsAction.java 0.00% 3 Missing ⚠️
...h/dashboards/OpenSearchDashboardsModulePlugin.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #21038      +/-   ##
============================================
+ Coverage     73.21%   73.23%   +0.01%     
- Complexity    72620    72638      +18     
============================================
  Files          5849     5858       +9     
  Lines        332066   332204     +138     
  Branches      47951    47955       +4     
============================================
+ Hits         243109   243274     +165     
+ Misses        69456    69390      -66     
- Partials      19501    19540      +39     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@github-actions
Copy link
Copy Markdown
Contributor

Failed to generate code suggestions for PR

@github-actions
Copy link
Copy Markdown
Contributor

❌ Gradle check result for a79d1b7: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

@github-actions
Copy link
Copy Markdown
Contributor

❌ Gradle check result for a79d1b7: FAILURE

Please examine the workflow log, locate, and copy-paste the failure(s) below, then iterate to green. Is the failure a flaky test unrelated to your change?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

skip-diff-analyzer Maintainer to skip code-diff-analyzer check, after reviewing issues in AI analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[FEATURE] Add kibana_admin level for tenant_permissions to allow advanced settings modification in tenant

1 participant