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 support for ResolveIndexAction handling #1312

Merged

Conversation

hsiang9431-amzn
Copy link
Contributor

@hsiang9431-amzn hsiang9431-amzn commented Jul 13, 2021

opendistro-for-elasticsearch/security pull request intake form

Please provide as much details as possible to get feedback/acceptance on your PR quickly

  1. Category: (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)
    Bug fix

  2. Github Issue # or road-map entry, if available:
    https://github.com/opendistro-for-elasticsearch/security/issues/893
    https://github.com/opendistro-for-elasticsearch/security-kibana-plugin/issues/598

  3. Description of changes:
    Fix requests hitting _resolve endpoint failed with 403 forbidden error due to accessing indices without proper permission. By re-configuring the logic in code, requests that were to return 403 will now success with 200 and not permitted indices will be filtered from its result.

Additional code added to unit tests and configuration ensures such behavior change does not compromise code integrity.

  1. Why these changes are required?
    Customers reported being unable to create index in Kibana due to previously mentioned 403 forbidden error. Such error is effecting customer production thus needs ti be fixed.

  2. What is the old behavior before changes and new behavior after changes? (Please add any example/logs/screen-shot if available)
    Old behavior:
    _resolve endpoint is not properly handled in code, thus causing it to return 403 status code when ever indices that are not allowed access to the user are contained in the response. This error causes Kibana failed to allow user to create indices that they should otherwise be allowed.

New behavior:
_resolve api is properly handled. If previously mentioned invalid access is encountered, security plugin automatically remove invalid indices accesses in the response and return with 200 ok status.

ResolveIndexAction is now supported in the execution path when do_not_fail_on_forbidden flag set to true

  1. Testing done: (Please provide details of testing done: Unit testing, integration testing and manual testing)
    Unit test regarding such feature is updated to accommodate with such change.
    Manual tests on Kibana web UI is conducted to ensure creation of desired index pattern is allowed.

  2. TO-DOs, if any: (Please describe pending items and provide Github issues# for each of them)
    Update multi-cluster integratoin tests as _resolve api introduced cross-cluster support for getting different index patterns

  3. Is it backport from main branch? (If yes, please add backport PR # and commits #)
    opendistro-1.11 opendistro-1.13

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
have the right to submit it under the open source license
indicated in the file; or

(b) The contribution is based upon previous work that, to the best
of my knowledge, is covered under an appropriate open source
license and I have the right under that license to submit that
work with modifications, whether created in whole or in part
by me, under the same open source license (unless I am
permitted to submit under a different license), as indicated
in the file; or

(c) The contribution was provided directly to me by some other
person who certified (a), (b) or (c) and I have not modified
it.

(d) I understand and agree that this project and the contribution
are public and that a record of the contribution (including all
personal information I submit with it, including my sign-off) is
maintained indefinitely and may be redistributed consistent with
this project or the open source license(s) involved.

@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2021

Codecov Report

Merging #1312 (da9f003) into main (960d420) will increase coverage by 0.03%.
The diff coverage is 28.57%.

❗ Current head da9f003 differs from pull request most recent head 7c7d6f3. Consider uploading reports for the commit 7c7d6f3 to get more accurate results
Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1312      +/-   ##
============================================
+ Coverage     64.78%   64.81%   +0.03%     
+ Complexity     3204     3202       -2     
============================================
  Files           247      247              
  Lines         17252    17243       -9     
  Branches       3053     3050       -3     
============================================
  Hits          11176    11176              
+ Misses         4526     4517       -9     
  Partials       1550     1550              
Impacted Files Coverage Δ
...earch/security/privileges/PrivilegesEvaluator.java 67.76% <23.07%> (+1.80%) ⬆️
...earch/security/resolver/IndexResolverReplacer.java 63.63% <100.00%> (ø)
.../dlic/auth/ldap2/LDAPConnectionFactoryFactory.java 57.25% <0.00%> (+0.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 960d420...7c7d6f3. Read the comment docs.

@hsiang9431-amzn hsiang9431-amzn marked this pull request as ready for review August 5, 2021 07:54
@hsiang9431-amzn hsiang9431-amzn requested a review from a team August 5, 2021 07:54
@vrozov
Copy link
Contributor

vrozov commented Aug 6, 2021

squash commits

@vrozov vrozov changed the title Fix resolve requests Add support for ResolveIndexAction handling Aug 6, 2021
@hsiang9431-amzn
Copy link
Contributor Author

squash commits

Will squash when merging

@vrozov
Copy link
Contributor

vrozov commented Aug 6, 2021

squash commits

Will squash when merging

I don't see a need to keep history of unrelated commits on the PR. It makes PR review harder, not easier. Number of commits is larger than number of files modified in the PR. Please squash commits.

add resolve to dnfof logic

unify operation flows where possible

add dnfof pattern

add test cases for resolve
vrozov
vrozov previously approved these changes Aug 6, 2021
@vrozov vrozov dismissed their stale review August 6, 2021 18:55

Need confirmation on Replacable

Copy link
Contributor

@vrozov vrozov left a comment

Choose a reason for hiding this comment

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

Please file issue to revisit Replaceable handling with dnfof flag on.

@hsiang9431-amzn hsiang9431-amzn merged commit b20155a into opensearch-project:main Aug 6, 2021
hsiang9431-amzn added a commit to hsiang9431-amzn/security that referenced this pull request Aug 7, 2021
hsiang9431-amzn added a commit to hsiang9431-amzn/security that referenced this pull request Aug 7, 2021
hsiang9431-amzn added a commit to hsiang9431-amzn/security that referenced this pull request Aug 7, 2021
hsiang9431-amzn added a commit to hsiang9431-amzn/security that referenced this pull request Aug 9, 2021
hsiang9431-amzn added a commit to hsiang9431-amzn/security that referenced this pull request Aug 9, 2021
hsiang9431-amzn added a commit to hsiang9431-amzn/security that referenced this pull request Aug 10, 2021
hsiang9431-amzn added a commit to hsiang9431-amzn/security that referenced this pull request Aug 10, 2021
vrozov pushed a commit that referenced this pull request Aug 10, 2021
vrozov pushed a commit that referenced this pull request Aug 10, 2021
AMoo-Miki pushed a commit to AMoo-Miki/security that referenced this pull request Aug 13, 2021
@cliu123 cliu123 added the bug Something isn't working label Aug 23, 2021
lbreinig pushed a commit to lbreinig/security that referenced this pull request Dec 23, 2021
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
gaobinlong pushed a commit to gaobinlong/security that referenced this pull request Aug 30, 2023
* Add release note for 1.3.8

Signed-off-by: Ryan Liang <jiallian@amazon.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants