Skip to content
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

Create separate transport action for render search template action #11170

Merged
merged 3 commits into from
Dec 12, 2023

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Nov 13, 2023

Description

Companion PR in security: opensearch-project/security#3689

Separates the transport action that handles POST /_render/template into a separate transport action from GET /_search/template to resolve an issue with permission the render template endpoint.

The transport action that handles both of these requests is overloaded and it makes permissioning these endpoints a challenge in the security plugin.

  1. GET /_search/template or GET /<list_of_indices>/_search_template is a form of a search request and should consider whether the user has permission to search on the list of indices provided in the request when evaluating whether the user has permission to perform the search template request.

  2. In contrast, POST /_render/template is a simulation and never actually queries data. See the linked issue for an example of a search template request. From the security plugin POV, this type of request does not need indices when evaluating whether the user has permission to perform the request and can be treated as a cluster permission.

This PR separates the transport action that handles both so that permissions can handle both of these types of requests. There is no functionality change on this PR, the testing for this is added in the companion PR.

Related Issues

Resolves opensearch-project/security#3672

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Failing checks are inspected and point to the corresponding known issue(s) (See: Troubleshooting Failing Builds)
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)
  • Public documentation issue/PR created

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.

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

❌ Gradle check result for 61b9dfa: 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?

@cwperks
Copy link
Member Author

cwperks commented Dec 11, 2023

Copy link
Contributor

❌ Gradle check result for 61b9dfa: null

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?

@peternied
Copy link
Member

@cwperks Can you please create an issue on the GitHub action failures reported in #11170 (comment)

Jenkins Workflow Url: https://build.ci.opensearch.org/job/gradle-check/31127/
time pass: 2790
parse error: Invalid numeric literal at line 1, column 10
Complete the run, checking results now......
Please check jenkins url for logs: https://build.ci.opensearch.org/job/gradle-check/31127/
Result: null

@cwperks
Copy link
Member Author

cwperks commented Dec 11, 2023

@peternied Opened an issue: #11574

Copy link
Contributor

❕ Gradle check result for 61b9dfa: UNSTABLE

  • TEST FAILURES:
      1 org.opensearch.remotestore.multipart.RemoteStoreMultipartIT.testRequestDurabilityWhenRestrictSettingImplicitFalse

Please review all flaky tests that succeeded after retry and create an issue if one does not already exist to track the flaky failure.

Copy link

codecov bot commented Dec 11, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (00dd577) 71.03% compared to head (61b9dfa) 71.29%.
Report is 4 commits behind head on main.

Files Patch % Lines
...script/mustache/TransportSearchTemplateAction.java 0.00% 5 Missing ⚠️
...ch/script/mustache/RenderSearchTemplateAction.java 0.00% 3 Missing ⚠️
.../mustache/TransportRenderSearchTemplateAction.java 0.00% 2 Missing ⚠️
...cript/mustache/RestRenderSearchTemplateAction.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #11170      +/-   ##
============================================
+ Coverage     71.03%   71.29%   +0.25%     
- Complexity    58886    59141     +255     
============================================
  Files          4904     4906       +2     
  Lines        278154   278164      +10     
  Branches      40419    40419              
============================================
+ Hits         197600   198320     +720     
+ Misses        64091    63354     -737     
- Partials      16463    16490      +27     

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

@peternied peternied merged commit 66d4e9e into opensearch-project:main Dec 12, 2023
29 of 30 checks passed
@peternied peternied added the backport 2.x Backport to 2.x branch label Dec 12, 2023
@opensearch-trigger-bot
Copy link
Contributor

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/OpenSearch/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/OpenSearch/backport-2.x
# Create a new branch
git switch --create backport/backport-11170-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 66d4e9e0d5fca1c50b84c08dccc9f5e9164c64c1
# Push it to GitHub
git push --set-upstream origin backport/backport-11170-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/OpenSearch/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-11170-to-2.x.

@peternied
Copy link
Member

@cwperks Mind doing the backport when you get a chance?

cwperks added a commit to cwperks/OpenSearch that referenced this pull request Dec 12, 2023
…pensearch-project#11170)

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 66d4e9e)
@cwperks
Copy link
Member Author

cwperks commented Dec 12, 2023

@peternied Opened manual backport: #11589

peternied pushed a commit that referenced this pull request Dec 12, 2023
…11170) (#11589)

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit 66d4e9e)
cwperks added a commit to opensearch-project/security that referenced this pull request Dec 19, 2023
### Description

Companion PRs in core: 

- opensearch-project/OpenSearch#11170
- opensearch-project/OpenSearch#11591

This PR adds render search template as a cluster perm so that its
separately permissioned from a SearchTemplateRequest which needs a set
of indices to authorize the request. The companion PR in core separates
the transport actions that handle search template request and render
search template request so that they can be authorized separately.

I am opening this in Draft until the core PR is merged because this PR
depends on the core PR.

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

Bug fix

### Issues Resolved

- #3672

### Check List
- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
cwperks added a commit to cwperks/security that referenced this pull request Dec 19, 2023
…t#3689)

Companion PRs in core:

- opensearch-project/OpenSearch#11170
- opensearch-project/OpenSearch#11591

This PR adds render search template as a cluster perm so that its
separately permissioned from a SearchTemplateRequest which needs a set
of indices to authorize the request. The companion PR in core separates
the transport actions that handle search template request and render
search template request so that they can be authorized separately.

I am opening this in Draft until the core PR is merged because this PR
depends on the core PR.

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

Bug fix

- opensearch-project#3672

- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
(cherry picked from commit cc57710)
prabhask5 pushed a commit to prabhask5/opensearch-security that referenced this pull request Jan 11, 2024
…t#3689)

### Description

Companion PRs in core:

- opensearch-project/OpenSearch#11170
- opensearch-project/OpenSearch#11591

This PR adds render search template as a cluster perm so that its
separately permissioned from a SearchTemplateRequest which needs a set
of indices to authorize the request. The companion PR in core separates
the transport actions that handle search template request and render
search template request so that they can be authorized separately.

I am opening this in Draft until the core PR is merged because this PR
depends on the core PR.

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

Bug fix

### Issues Resolved

- opensearch-project#3672

### Check List
- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Prabhas Kurapati <prabhask@berkeley.edu>
rayshrey pushed a commit to rayshrey/OpenSearch that referenced this pull request Mar 18, 2024
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…pensearch-project#11170)

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Shivansh Arora <hishiv@amazon.com>
dlin2028 pushed a commit to dlin2028/security that referenced this pull request May 1, 2024
…t#3689)

### Description

Companion PRs in core: 

- opensearch-project/OpenSearch#11170
- opensearch-project/OpenSearch#11591

This PR adds render search template as a cluster perm so that its
separately permissioned from a SearchTemplateRequest which needs a set
of indices to authorize the request. The companion PR in core separates
the transport actions that handle search template request and render
search template request so that they can be authorized separately.

I am opening this in Draft until the core PR is merged because this PR
depends on the core PR.

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

Bug fix

### Issues Resolved

- opensearch-project#3672

### Check List
- [ ] New functionality includes testing
- [ ] New functionality has been documented
- [ ] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks cwperks added v2.12.0 Issues and PRs related to version 2.12.0 and removed v2.2.0 labels May 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch backport-failed v2.12.0 Issues and PRs related to version 2.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] POST /_render/template results in a NullPointerException when security plugin is enabled
2 participants