add trusted URL regex #518

ylwu-amzn · 2022-11-03T07:04:42Z

Signed-off-by: Yaliang Wu ylwu@amazon.com

Description

Address security review comments by adding trusted URL regex, so user can set their own trusted URL regex to avoid uploading model from untrusted source.

Issues Resolved

[List any issues this PR will resolve]

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

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: Yaliang Wu <ylwu@amazon.com>

codecov-commenter · 2022-11-03T08:56:10Z

Codecov Report

Merging #518 (355d97a) into 2.x (6d3660b) will decrease coverage by 1.51%.
The diff coverage is 55.00%.

@@             Coverage Diff              @@
##                2.x     #518      +/-   ##
============================================
- Coverage     80.06%   78.54%   -1.52%     
+ Complexity      926      906      -20     
============================================
  Files            92       92              
  Lines          3506     3515       +9     
  Branches        323      323              
============================================
- Hits           2807     2761      -46     
- Misses          570      631      +61     
+ Partials        129      123       -6

Flag	Coverage Δ
ml-commons	`78.54% <55.00%> (-1.52%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...h/ml/action/unload/TransportUnloadModelAction.java	`17.39% <0.00%> (-0.39%)`	⬇️
...rg/opensearch/ml/plugin/MachineLearningPlugin.java	`100.00% <ø> (ø)`
...org/opensearch/ml/rest/RestMLPredictionAction.java	`32.35% <0.00%> (+0.92%)`	⬆️
...h/ml/action/upload/TransportUploadModelAction.java	`92.06% <66.66%> (-6.16%)`	⬇️
...arch/ml/action/forward/TransportForwardAction.java	`81.81% <100.00%> (ø)`
...arch/ml/action/models/GetModelTransportAction.java	`78.78% <100.00%> (ø)`
.../org/opensearch/ml/settings/MLCommonsSettings.java	`100.00% <100.00%> (ø)`
.../ml/action/syncup/TransportSyncUpOnNodeAction.java	`18.18% <0.00%> (-37.88%)`	⬇️
.../cluster/MLCommonsClusterManagerEventListener.java	`65.62% <0.00%> (-12.50%)`	⬇️
...org/opensearch/ml/cluster/DiscoveryNodeHelper.java	`89.39% <0.00%> (-9.10%)`	⬇️
... and 3 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Zhangxunmt · 2022-11-03T18:29:03Z

plugin/src/main/java/org/opensearch/ml/settings/MLCommonsSettings.java

@@ -34,7 +34,7 @@ private MLCommonsSettings() {}
    public static final Setting<Long> ML_COMMONS_MONITORING_REQUEST_COUNT = Setting
        .longSetting(
            "plugins.ml_commons.monitoring_request_count",
-            10000,
+            100,


Is this reduction from 10000 to 100 due to the tuning after performance tests result?

This part is mainly for testing/monitoring purpose, big value will use more memory. So tune the default value to a smaller number. User can tune it if they want to monitor more requests.

dhrubo-os

should we have tests for other ways: file, ftp?

dhrubo-os · 2022-11-03T18:30:56Z

plugin/src/main/java/org/opensearch/ml/settings/MLCommonsSettings.java

+    public static final Setting<String> ML_COMMONS_TRUSTED_URL_REGEX = Setting
+        .simpleString(
+            "plugins.ml_commons.trusted_url_regex",
+            "^(https?|ftp|file)://[-a-zA-Z0-9+&@#/%?=~_|!:,.;]*[-a-zA-Z0-9+&@#/%=~_|]",


does this regex cover http & https both?

Yes, I tested manually , I will add more test cases in next PR.

Yes, the question mark means 0 - 1.

opensearch-trigger-bot · 2022-11-03T21:56:02Z

The backport to 2.4 failed:

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

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.4 2.4
# Navigate to the new working tree
cd .worktrees/backport-2.4
# Create a new branch
git switch --create backport/backport-518-to-2.4
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 a36d11abe2b5ce0892fc89992846d58b939b67d0
# Push it to GitHub
git push --set-upstream origin backport/backport-518-to-2.4
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.4

Then, create a pull request where the base branch is 2.4 and the compare/head branch is backport/backport-518-to-2.4.

Zhangxunmt · 2022-11-03T22:27:32Z

plugin/src/main/java/org/opensearch/ml/rest/RestMLPredictionAction.java

+            MLPredictionTaskRequest mlPredictionTaskRequest = getRequest(modelId, algorithm, request);
            return channel -> client
-                .execute(MLPredictionTaskAction.INSTANCE, getRequest(modelId, algorithm, request), new RestToXContentListener<>(channel));
+                .execute(MLPredictionTaskAction.INSTANCE, mlPredictionTaskRequest, new RestToXContentListener<>(channel));


Is this because the JVM complier is not able to dynamic resolve getRequest() before executing client.execute()? If that's the case, we need to use another local variable for line 83. https://github.com/opensearch-project/ml-commons/blob/2.x/plugin/src/main/java/org/opensearch/ml/rest/RestMLPredictionAction.java#L83

just for easy testing

Signed-off-by: Yaliang Wu <ylwu@amazon.com> Signed-off-by: Yaliang Wu <ylwu@amazon.com> (cherry picked from commit a36d11a)

Signed-off-by: Yaliang Wu <ylwu@amazon.com> Signed-off-by: Yaliang Wu <ylwu@amazon.com> (cherry picked from commit a36d11a) Co-authored-by: Yaliang Wu <ylwu@amazon.com>

Signed-off-by: Yaliang Wu <ylwu@amazon.com> Signed-off-by: Yaliang Wu <ylwu@amazon.com> Signed-off-by: Sicheng Song <sicheng.song@outlook.com>

ylwu-amzn marked this pull request as ready for review November 3, 2022 07:04

ylwu-amzn requested a review from a team November 3, 2022 07:04

ylwu-amzn force-pushed the 2.x_custom_m_test_plugin branch from b6b7a5f to aaa3a4b Compare November 3, 2022 07:28

add trusted URL regex

355d97a

Signed-off-by: Yaliang Wu <ylwu@amazon.com>

ylwu-amzn force-pushed the 2.x_custom_m_test_plugin branch from aaa3a4b to 355d97a Compare November 3, 2022 08:47

Zhangxunmt reviewed Nov 3, 2022

View reviewed changes

Zhangxunmt approved these changes Nov 3, 2022

View reviewed changes

dhrubo-os approved these changes Nov 3, 2022

View reviewed changes

ylwu-amzn merged commit a36d11a into opensearch-project:2.x Nov 3, 2022

rbhavna added the backport 2.4 label Nov 3, 2022

Zhangxunmt reviewed Nov 3, 2022

View reviewed changes

ylwu-amzn added backport 2.4 and removed backport 2.4 labels Nov 3, 2022

opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 3, 2022

add trusted URL regex (#518)

a04d4aa

Signed-off-by: Yaliang Wu <ylwu@amazon.com> Signed-off-by: Yaliang Wu <ylwu@amazon.com> (cherry picked from commit a36d11a)

opensearch-trigger-bot bot mentioned this pull request Nov 3, 2022

[Backport 2.4] add trusted URL regex #526

Merged

rbhavna pushed a commit that referenced this pull request Nov 3, 2022

add trusted URL regex (#518) (#526)

3c541a9

Signed-off-by: Yaliang Wu <ylwu@amazon.com> Signed-off-by: Yaliang Wu <ylwu@amazon.com> (cherry picked from commit a36d11a) Co-authored-by: Yaliang Wu <ylwu@amazon.com>

ylwu-amzn added the feature label Nov 4, 2022

b4sjoo pushed a commit to b4sjoo/ml-commons that referenced this pull request Dec 1, 2022

add trusted URL regex (opensearch-project#518)

ed94afd

Signed-off-by: Yaliang Wu <ylwu@amazon.com> Signed-off-by: Yaliang Wu <ylwu@amazon.com> Signed-off-by: Sicheng Song <sicheng.song@outlook.com>

b4sjoo added a commit that referenced this pull request Dec 1, 2022

add trusted URL regex (#518) (#602)

77b1459

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add trusted URL regex #518

add trusted URL regex #518

ylwu-amzn commented Nov 3, 2022

codecov-commenter commented Nov 3, 2022 •

edited

Loading

Zhangxunmt Nov 3, 2022

ylwu-amzn Nov 3, 2022

dhrubo-os left a comment

dhrubo-os Nov 3, 2022

ylwu-amzn Nov 3, 2022

jngz-es Nov 3, 2022

opensearch-trigger-bot bot commented Nov 3, 2022

Zhangxunmt Nov 3, 2022

ylwu-amzn Nov 9, 2022

add trusted URL regex #518

add trusted URL regex #518

Conversation

ylwu-amzn commented Nov 3, 2022

Description

Issues Resolved

Check List

codecov-commenter commented Nov 3, 2022 • edited Loading

Codecov Report

Zhangxunmt Nov 3, 2022

Choose a reason for hiding this comment

ylwu-amzn Nov 3, 2022

Choose a reason for hiding this comment

dhrubo-os left a comment

Choose a reason for hiding this comment

dhrubo-os Nov 3, 2022

Choose a reason for hiding this comment

ylwu-amzn Nov 3, 2022

Choose a reason for hiding this comment

jngz-es Nov 3, 2022

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Nov 3, 2022

Zhangxunmt Nov 3, 2022

Choose a reason for hiding this comment

ylwu-amzn Nov 9, 2022

Choose a reason for hiding this comment

codecov-commenter commented Nov 3, 2022 •

edited

Loading