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

Make SearchTemplateRequest implement IndicesRequest.Replaceable #9122

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

cwperks
Copy link
Member

@cwperks cwperks commented Aug 4, 2023

Description

Companion PR in Security plugin that includes tests of the behavior: opensearch-project/security#2921

This PR makes SearchTemplateRequest (inside modules/lang-mustache) implement IndicesRequest.Replaceable and for the implementation of each method it calls the underlying SearchRequest's implementation.

This change resolves an issue in authorization of SearchTemplateRequests where the security plugin is unable to extract the indices associated with the request properly. See Details: opensearch-project/security#1678

As an effect, even if a user does a SearchTemplateRequest on a specific index:

i.e.

GET /movies/_search/template
{
  "source": {
    "query": {
      "match": {
        "title": "{{title}}"
      }
    }
  },
  "params": {
    "title": "Titanic"
  }
}

within the security plugin, it is not able to determine the request is on movies index and instead tries to evaluate whether the user has permission to do search template requests on * index.

With this change, the Security plugin will be able to extract the indices from the request as is already done for Replaceable requests here: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/resolver/IndexResolverReplacer.java#L752-L758

Related Issues

Resolves opensearch-project/security#1678

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff
  • Commit changes are listed out in CHANGELOG.md file (See: Changelog)

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 <[email protected]>
@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:


> Task :checkCompatibility
Checking compatibility for: https://github.com/opensearch-project/reporting.git with ref: main
Incompatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git]
Compatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/reporting.git]

BUILD SUCCESSFUL in 28m 37s

@cwperks
Copy link
Member Author

cwperks commented Aug 4, 2023

Script that fails before the change, but succeeds after the change:

curl -XPUT https://admin:admin@localhost:9200/_plugins/_security/api/internalusers/craig --insecure -H "Content-Type: application/json" --data '
{  
  "password": "TestPassword123_"
}
'

curl -XPUT https://admin:admin@localhost:9200/_plugins/_security/api/roles/log_search_role --insecure -H "Content-Type: application/json" --data '
{
    "cluster_permissions":
    [
        "cluster_composite_ops",
        "indices_monitor"
    ],
    "index_permissions":
    [
        {
            "index_patterns":
            [
                "logs-*"
            ],
            "allowed_actions":
            [
                "read"
            ]
        }
    ]
}
'

curl -XPUT https://admin:admin@localhost:9200/_plugins/_security/api/rolesmapping/log_search_role --insecure -H "Content-Type: application/json" --data '
{  
  "users" : [ "craig" ]
}
'

curl -XPUT https://admin:admin@localhost:9200/logs-123 --insecure

curl -XPOST https://admin:admin@localhost:9200/logs-123/_bulk --insecure -H "Content-Type: application/json" --data '
{ "create": { "_index": "logs-123", "_id": "tt1392214" } }
{ "title": "Craig", "service": "Oracle" }
{ "create": { "_index": "logs-123", "_id": "tt1392215" } }
{ "title": "Ryan", "service": "Amazon" }
{ "create": { "_index": "logs-123", "_id": "tt1392216" } }
{ "title": "Derek", "service": "Salesforce" }
{ "create": { "_index": "logs-123", "_id": "tt1392217" } }
{ "title": "Dave", "service": "SAP" }
'

curl -XGET https://admin:admin@localhost:9200/logs-123/_search --insecure

curl -XGET https://craig:TestPassword123_@localhost:9200/logs-123/_search --insecure

curl -XGET https://admin:admin@localhost:9200/logs-123/_search/template --insecure -H "Content-Type: application/json" --data '
{
  "source": {
    "query": {
      "match": {
        "service": "{{service_name}}"
      }
    }
  },
  "params": {
    "service_name": "Oracle"
  }
}
'

curl -XGET https://craig:TestPassword123_@localhost:9200/logs-123/_search/template --insecure -H "Content-Type: application/json" --data '
{
  "source": {
    "query": {
      "match": {
        "service": "{{service_name}}"
      }
    }
  },
  "params": {
    "service_name": "Oracle"
  }
}
'

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

Gradle Check (Jenkins) Run Completed with:

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:



> Task :checkCompatibility
Incompatible components: [https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git]
Compatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/reporting.git]

BUILD SUCCESSFUL in 37m 52s

@github-actions
Copy link
Contributor

github-actions bot commented Aug 4, 2023

Gradle Check (Jenkins) Run Completed with:

@cwperks
Copy link
Member Author

cwperks commented Aug 8, 2023

Are any maintainers available for a PR review?

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:



> Task :checkCompatibility
Incompatible components: [https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/security-analytics.git]
Compatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

BUILD SUCCESSFUL in 34m 4s

@github-actions
Copy link
Contributor

github-actions bot commented Aug 8, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Gradle Check (Jenkins) Run Completed with:

@github-actions
Copy link
Contributor

github-actions bot commented Aug 9, 2023

Gradle Check (Jenkins) Run Completed with:

@cwperks
Copy link
Member Author

cwperks commented Aug 9, 2023

Tests for this are being added in the security plugin here: https://github.com/opensearch-project/security/pull/2921/files

@cwperks
Copy link
Member Author

cwperks commented Aug 9, 2023

@reta Could I trouble you for a review of this PR? It's a small one for a bug fix related to privilege evaluation of SearchTemplateRequests

@cwperks
Copy link
Member Author

cwperks commented Aug 10, 2023

@reta or @dblock Would either of you be able to review this PR? This calls on the underlying implementations in the SearchRequest that is part of the SearchTemplateRequest. This fixes an issue with how SearchTemplateRequests are authorized where the security plugin cannot currently get the list of indices the SearchTemplateRequest is executed on and instead assumes *, meaning that a user needs a read permission on * index pattern to do any SearchTemplateRequests.

@reta
Copy link
Collaborator

reta commented Aug 10, 2023

@reta or @dblock Would either of you be able to review this PR?

Sorry about the delay @cwperks , looking right now

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.

Search request templates do not not fit in the security plugins model, they are cluster wide construction. I don't think we should fix opensearch-project/security#1678 which is driving this change.

Seeing aside the merits of that bug - the notion of a replaceable indices on request should be the standard for plugin capabilities. If we are taking this change from an platform perspective should IndicesRequest.Replaceable be more prevalent (including this case)? @macohen can you help us find someone to advise on next steps?

@cwperks
Copy link
Member Author

cwperks commented Aug 11, 2023

@peternied it is not a cluster wide action, it is very much another type of search request which is done on a set of indices. It is a cluster wide action to save a template, but this PR is not about that. This PR is about fixing an issue in authorizing requests that use a search template.

See my comment above about why Replaceable is needed: #9122 (comment)

Its needed to support dnfof

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.

Dug in on opensearch-project/security#1678 the intent is around fixing a place where GET /*/_search works as expected and GET /*/_search/template is broken, so lets unblock that scenario with this change.

Thanks @cwperks

@opensearch-trigger-bot
Copy link
Contributor

Compatibility status:



> Task :checkCompatibility
Incompatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/performance-analyzer.git]
Compatible components: [https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git]

BUILD SUCCESSFUL in 28m 30s

@reta reta merged commit c73f727 into opensearch-project:main Aug 11, 2023
13 of 14 checks passed
@reta reta added the backport 2.x Backport to 2.x branch label Aug 11, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 11, 2023
* Make SearchTemplateRequest implement IndicesRequest.Replaceable

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
(cherry picked from commit c73f727)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
reta pushed a commit that referenced this pull request Aug 14, 2023
… (#9275)

* Make SearchTemplateRequest implement IndicesRequest.Replaceable



* Add to CHANGELOG



---------


(cherry picked from commit c73f727)

Signed-off-by: Craig Perkins <[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>
linuxpi pushed a commit to linuxpi/OpenSearch that referenced this pull request Aug 14, 2023
…search-project#9122)

* Make SearchTemplateRequest implement IndicesRequest.Replaceable

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
linuxpi pushed a commit to linuxpi/OpenSearch that referenced this pull request Aug 16, 2023
…search-project#9122)

* Make SearchTemplateRequest implement IndicesRequest.Replaceable

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
kaushalmahi12 pushed a commit to kaushalmahi12/OpenSearch that referenced this pull request Sep 12, 2023
…search-project#9122)

* Make SearchTemplateRequest implement IndicesRequest.Replaceable

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Kaushal Kumar <[email protected]>
brusic pushed a commit to brusic/OpenSearch that referenced this pull request Sep 25, 2023
…search-project#9122)

* Make SearchTemplateRequest implement IndicesRequest.Replaceable

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Ivan Brusic <[email protected]>
shiv0408 pushed a commit to Gaurav614/OpenSearch that referenced this pull request Apr 25, 2024
…search-project#9122)

* Make SearchTemplateRequest implement IndicesRequest.Replaceable

Signed-off-by: Craig Perkins <[email protected]>

* Add to CHANGELOG

Signed-off-by: Craig Perkins <[email protected]>

---------

Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Shivansh Arora <[email protected]>
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for SearchTemplateRequest while resolving request
4 participants