Skip to content

Enable fgac support for machine learning plugin.#1308

Closed
spbjss wants to merge 5 commits intoopensearch-project:mainfrom
spbjss:main
Closed

Enable fgac support for machine learning plugin.#1308
spbjss wants to merge 5 commits intoopensearch-project:mainfrom
spbjss:main

Conversation

@spbjss
Copy link
Copy Markdown

@spbjss spbjss commented Jul 6, 2021

Signed-off-by: Alex pengsun@amazon.com

opensearch-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)
    New feature

  2. Github Issue # or road-map entry, if available:
    ML Framework Security Support ml-commons#55

  3. Description of changes:
    Enable the security support to machine learning plugin.

  • The models will be saved in system index.
    
  • Permission control (transport actions) by pre-defined security roles.
    
  • Backend roles with users.
    
  1. Why these changes are required?
    The new machine learning plugin needs to support security control.

  2. What is the old behavior before changes and new behavior after changes? (Please add any example/logs/screen-shot if available)
    Machine learning plugin is a new plugin to support machine learning for open search.

  3. Testing done: (Please provide details of testing done: Unit testing, integration testing and manual testing)
    Unit test and manual testing done.

  4. TO-DOs, if any: (Please describe pending items and provide Github issues# for each of them)
    N/A.

  5. Is it backport from main branch? (If yes, please add backport PR # and commits #)
    N/A.

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.

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: Alex <pengsun@amazon.com>
@spbjss spbjss requested a review from a team July 6, 2021 23:38
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jul 7, 2021

Codecov Report

Merging #1308 (f1da506) into main (2b69d9c) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##               main    #1308      +/-   ##
============================================
+ Coverage     64.73%   64.76%   +0.03%     
- Complexity     3193     3195       +2     
============================================
  Files           247      247              
  Lines         17230    17232       +2     
  Branches       3045     3046       +1     
============================================
+ Hits          11153    11160       +7     
+ Misses         4527     4522       -5     
  Partials       1550     1550              
Impacted Files Coverage Δ
...security/configuration/DlsFlsFilterLeafReader.java 59.76% <0.00%> (-0.71%) ⬇️
...a/org/opensearch/security/tools/SecurityAdmin.java 47.52% <0.00%> (+0.26%) ⬆️
...arch/security/dlic/rest/api/AbstractApiAction.java 76.89% <0.00%> (+0.98%) ⬆️
...nsearch/security/dlic/rest/api/AuditApiAction.java 68.08% <0.00%> (+4.25%) ⬆️
...ecurity/configuration/StaticResourceException.java 25.00% <0.00%> (+25.00%) ⬆️

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 2b69d9c...f1da506. Read the comment docs.

echo 'plugins.security.restapi.roles_enabled: ["all_access", "security_rest_api_access"]' | $SUDO_CMD tee -a "$OPENSEARCH_CONF_FILE" > /dev/null
echo 'plugins.security.system_indices.enabled: true' | $SUDO_CMD tee -a "$OPENSEARCH_CONF_FILE" > /dev/null
echo 'plugins.security.system_indices.indices: [".opendistro-alerting-config", ".opendistro-alerting-alert*", ".opendistro-anomaly-results*", ".opendistro-anomaly-detector*", ".opendistro-anomaly-checkpoints", ".opendistro-anomaly-detection-state", ".opendistro-reports-*", ".opendistro-notifications-*", ".opendistro-notebooks", ".opendistro-asynchronous-search-response*"]' | $SUDO_CMD tee -a "$OPENSEARCH_CONF_FILE" > /dev/null
echo 'plugins.security.system_indices.indices: [".os_ml_model_result", ".opendistro-alerting-config", ".opendistro-alerting-alert*", ".opendistro-anomaly-results*", ".opendistro-anomaly-detector*", ".opendistro-anomaly-checkpoints", ".opendistro-anomaly-detection-state", ".opendistro-reports-*", ".opendistro-notifications-*", ".opendistro-notebooks", ".opendistro-asynchronous-search-response*"]' | $SUDO_CMD tee -a "$OPENSEARCH_CONF_FILE" > /dev/null
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Alerting plugin plans to move their plugin index out of system indices opensearch-project/alerting#125. Should we put new plugin index into system indices list? cc @skkosuri-amzn

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@spbjss Did you get chance to close this?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Good suggestion.
Updated as suggested.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

opensearch-project/alerting#125 (comment) based on this discussion, .os_ml_model_result should be part of system index.

Considering .os_ml_model_result --> stores model which will be used by ML plugin. Any change in models stored in this index must be allowed only via APIs exposed by ML plugin. So, we can avoid accidental update to this index via any users.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated, thank you!

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated the ml index name to '.plugins-ml-model' based on the discussion with @dblock @ylwu-amzn and @saratvemulapalli

ylwu-amzn
ylwu-amzn previously approved these changes Jul 14, 2021
Signed-off-by: Alex <pengsun@amazon.com>
ylwu-amzn
ylwu-amzn previously approved these changes Jul 14, 2021
hardik-k-shah
hardik-k-shah previously approved these changes Jul 14, 2021
Signed-off-by: Alex <pengsun@amazon.com>
@spbjss spbjss dismissed stale reviews from hardik-k-shah and ylwu-amzn via d5054cc July 20, 2021 23:27
Signed-off-by: Alex <pengsun@amazon.com>
Copy link
Copy Markdown
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.

Long term security plugin should provide better support for other plugins to register dynamic plugin roles and system indices that needs to be protected by the security plugin. The current model where plugins needs to change security plugin source code is not scalable

@vrozov
Copy link
Copy Markdown
Contributor

vrozov commented Jul 21, 2021

@spbjss Please sign commits

- index_patterns:
- '*'
allowed_actions:
- 'indices_monitor' No newline at end of file
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: add EOL

@davidlago
Copy link
Copy Markdown

@spbjss it looks like this is almost ready to go, after resolving merge conflicts.

@ylwu-amzn
Copy link
Copy Markdown
Contributor

We opened a new PR #1654, close this one.

@cliu123 cliu123 closed this Mar 1, 2022
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.

7 participants