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

Add Integration Test to confirm Core Change to Fix Search template request Auth #2921

Merged
merged 10 commits into from
Aug 16, 2023

Conversation

derek-ho
Copy link
Collaborator

@derek-ho derek-ho commented Jun 29, 2023

Adds integration test to verify change in core, allowing proper authorization of search template request

related to: #1678

Description

[Describe what this change achieves]

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
  • Why these changes are required?
  • What is the old behavior before changes and new behavior after changes?

Issues Resolved

[List any issues this PR will resolve]

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

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.

…ices you have access to

Signed-off-by: Derek Ho <dxho@amazon.com>
@codecov
Copy link

codecov bot commented Jun 29, 2023

Codecov Report

Merging #2921 (f3e5d46) into main (5e8f12c) will increase coverage by 0.03%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2921      +/-   ##
============================================
+ Coverage     62.42%   62.45%   +0.03%     
- Complexity     3352     3354       +2     
============================================
  Files           254      254              
  Lines         19749    19749              
  Branches       3334     3334              
============================================
+ Hits          12329    12335       +6     
+ Misses         5788     5785       -3     
+ Partials       1632     1629       -3     

see 5 files with indirect coverage changes

Signed-off-by: Derek Ho <dxho@amazon.com>
@@ -68,4 +73,11 @@ public void testRegexPattern() throws Exception {
}

}

@Test
public void testSearchTemplateRequest() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Before adding search template into isClusterPerm function this would fail since it would come back with a 403 response code

Signed-off-by: Derek Ho <dxho@amazon.com>
@@ -44,10 +45,15 @@ public class PrivilegesEvaluatorTest {
new Role("negated_regex_role").indexPermissions("read").on("/^[a-z].*/").clusterPermissions("cluster_composite_ops")
);

protected final static TestSecurityConfig.User SEARCH_TEMPLATE = new TestSecurityConfig.User("search_template_user").roles(
new Role("search_template_role").indexPermissions("read").on("/^[a-z].*/").clusterPermissions("indices:data/read/search/template")
Copy link
Member

Choose a reason for hiding this comment

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

Thank you @derek-ho ! This change looks good to me. I only have one concern about breaking existing roles that are unknowingly working with the bug in place, but since it is a bug fix I think its alright to provide documentation on how to update a role to make it so that SearchTemplateRequests are working properly. I also think it is worthwhile to modify the cluster_composite_ops_ro static action group to include search template and msearch template.

To be clear, the role below allows SearchTemplateRequests to work even with the bug in place:

search_all:
  index_permissions:
    - index_patterns:
        - "*"
      allowed_actions:
        - "read"

and after this change the role would need to be changed to:

search_all:
  cluster_permissions:
    - "indices:data/read/search/template" or "cluster_composite_ops_ro" <- action group containing the underlying cluster perm
  index_permissions:
    - index_patterns:
        - "*"
      allowed_actions:
        - "read"

With the bug in place the following role would not permit search template requests to work properly

logs_search:
  index_permissions:
    - index_patterns:
        - "logs-*"
      allowed_actions:
        - "read"

but that would be achievable now with the following role definition:

logs_search:
  cluster_permissions:
    - "indices:data/read/search/template"
  index_permissions:
    - index_patterns:
        - "logs-*"
      allowed_actions:
        - "read"

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can make the change for 2.x since it will be a breaking change. While it is an edge case, I don't think it matters if it is for a bug fix or not. Unless I am mistaken, semantic versioning would dictate we wait until 3.0 to make this available.

Copy link
Member

Choose a reason for hiding this comment

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

Can you remove the clusterPermissions section for this role?

@cwperks
Copy link
Member

cwperks commented Jun 30, 2023

@shikharj05 FYI this PR is related to a bug fix you filed last year.

Copy link
Contributor

@stephen-crawford stephen-crawford left a comment

Choose a reason for hiding this comment

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

This looks like a good contribution @derek-ho. I don't believe we will be able to backport this to 2.x however since it will cause a breaking change and despite the niche case we should be strict in adherence to semantic versioning. Looks good to me after you are able to resolve Craig's comments.

@@ -44,10 +45,15 @@ public class PrivilegesEvaluatorTest {
new Role("negated_regex_role").indexPermissions("read").on("/^[a-z].*/").clusterPermissions("cluster_composite_ops")
);

protected final static TestSecurityConfig.User SEARCH_TEMPLATE = new TestSecurityConfig.User("search_template_user").roles(
new Role("search_template_role").indexPermissions("read").on("/^[a-z].*/").clusterPermissions("indices:data/read/search/template")
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we can make the change for 2.x since it will be a breaking change. While it is an edge case, I don't think it matters if it is for a bug fix or not. Unless I am mistaken, semantic versioning would dictate we wait until 3.0 to make this available.

@davidlago
Copy link

Gentle bump on this one, @derek-ho. Are you still intending to merge this one?

@derek-ho
Copy link
Collaborator Author

Gentle bump on this one, @derek-ho. Are you still intending to merge this one?

Got caught up in some release work - will put it in draft, having some conversations with @cwperks about the right path forward on this one.

@derek-ho derek-ho marked this pull request as draft July 19, 2023 15:46
Signed-off-by: Derek Ho <dxho@amazon.com>
@derek-ho derek-ho changed the title fix search template auth to allow for search template requests on ind… Add Unit Test to confirm Core Change to Fix Search template request Auth Aug 9, 2023
@derek-ho derek-ho marked this pull request as ready for review August 9, 2023 14:21
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Going deep on these tests - last change before my approval, but you can still consider this nitpicking - can you make it clearer why the outcome of the test should be what it is?

I find the easiest format to follow is setup, action, and verification, what that could look like for testSearchTemplateRequestUnauthorizedAllIndices might follow:

final String searchForAllTemplates = "_search/template";
final TestRestClient client = cluster.getRestClient(ONLY_SERVICES_INDEX_ACCESS));

final Request searchForAllTemplatesResponse = client.getWithJsonBody(searchForAllTemplates, TEST_QUERY);

assertThat(getAllTemplatesResponse.getStatusCode(), equalTo(HttpStatus.SC_FORBIDDEN))

@peternied
Copy link
Member

@derek-ho This are integration tests, could you change the name of the pull request and update the description?

cwperks
cwperks previously approved these changes Aug 11, 2023
@cwperks cwperks requested review from cwperks and removed request for cwperks August 11, 2023 20:33
@cwperks cwperks dismissed their stale review August 11, 2023 20:36

Re-checking tests. How are the tests working without the index being created first?

@derek-ho derek-ho changed the title Add Unit Test to confirm Core Change to Fix Search template request Auth Add Integration Test to confirm Core Change to Fix Search template request Auth Aug 15, 2023
@derek-ho
Copy link
Collaborator Author

All tests pass on local after @cwperks's upstream change

Screen Shot 2023-08-16 at 10 03 19 AM

Copy link
Member

@cwperks cwperks left a comment

Choose a reason for hiding this comment

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

Looks good to me. Thank you @derek-ho! 🚢

@willyborankin willyborankin merged commit bd084c8 into opensearch-project:main Aug 16, 2023
55 checks passed
@derek-ho
Copy link
Collaborator Author

derek-ho commented Aug 16, 2023

@willyborankin can you add 2.x backport label? The Core change was backported to 2.x: opensearch-project/OpenSearch#9275

@willyborankin willyborankin added the backport 2.x backport to 2.x branch label Aug 16, 2023
@willyborankin
Copy link
Collaborator

@derek-ho done

@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/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-2921-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 bd084c89baff0312a1b11b961e8b850b68e83829
# Push it to GitHub
git push --set-upstream origin backport/backport-2921-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

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

@cwperks
Copy link
Member

cwperks commented Aug 16, 2023

The integration tests are not on 2.x so this does not need to be backported to 2.x.

@peternied peternied removed backport 2.x backport to 2.x branch backport-failed labels Aug 31, 2023
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.

6 participants