Moved auth interceptors to common module #1703

vmmusings · 2023-06-05T08:48:16Z

Description

Moved auth interceptors to common module, so that they can be used for different connector modules.
Removed DataSourceSettings and merged in OpensearchSettings. This class handles updates as well.

Fixed bug with OpensearchSettings where initially value set in opensearch.yml is not being picked up.

For example in the below code block. PPL_ENABLED is always set to default value(true) even though it is set false in opensearch.yml.

public RestPPLQueryAction( Settings pluginSettings, org.opensearch.common.settings.Settings clusterSettings) {
super();
this.pplEnabled =
    () ->
        MULTI_ALLOW_EXPLICIT_INDEX.get(clusterSettings)
            && (Boolean) pluginSettings.getSettingValue(Settings.Key.PPL_ENABLED);
 }

Introduced new setting plugins.query.datasources.uri.allowhosts for blocking URIs with unwanted hosts. This can protect from ssrf attacks.
Made plugins.query.datasources.encryption.masterkey as final and can only be set in yml and can't be changed via
API.
Updated documentation can be found here: https://github.com/vamsi-amazon/sql/blob/7c3b5069fb71dea71cb1d2e4c28beb531a56b93e/docs/user/ppl/admin/datasources.rst#datasource-allow-hosts-config

Issues Resolved

[List any issues this PR will resolve]

Check List

New functionality includes testing.
- All tests pass, including unit test, integration test and doctest
New functionality has been documented.
- New functionality has javadoc added
- New functionality has user manual doc 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.

codecov · 2023-06-05T09:13:15Z

Codecov Report

Merging #1703 (f838975) into main (691012d) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1703      +/-   ##
============================================
+ Coverage     97.27%   97.30%   +0.02%     
+ Complexity     4330     4326       -4     
============================================
  Files           388      385       -3     
  Lines         10807    10807              
  Branches        761      762       +1     
============================================
+ Hits          10513    10516       +3     
+ Misses          287      284       -3     
  Partials          7        7

Flag	Coverage Δ
sql-engine	`97.30% <100.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...rch/sql/opensearch/setting/OpenSearchSettings.java	`100.00% <100.00%> (ø)`
...l/prometheus/storage/PrometheusStorageFactory.java	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

penghuo · 2023-06-06T21:34:59Z

docs/user/ppl/admin/datasources.rst

@@ -136,6 +136,17 @@ Master Key config for encrypting credential information
    # Print the master key
    print("Generated master key:", master_key)

+Datasource Allow Hosts Config


additional layer of security?

This is to give customer a config to disallow any kind of ssrf attacks.

penghuo · 2023-06-06T21:45:07Z

opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java

@@ -123,6 +146,10 @@ public OpenSearchSettings(ClusterSettings clusterSettings) {
        METRICS_ROLLING_WINDOW_SETTING, new Updater(Key.METRICS_ROLLING_WINDOW));
    register(settingBuilder, clusterSettings, Key.METRICS_ROLLING_INTERVAL,
        METRICS_ROLLING_INTERVAL_SETTING, new Updater(Key.METRICS_ROLLING_INTERVAL));
+    register(settingBuilder, clusterSettings, Key.DATASOURCES_URI_ALLOWHOSTS,
+        DATASOURCE_URI_ALLOW_HOSTS, new Updater(Key.DATASOURCES_URI_ALLOWHOSTS));
+    settingBuilder.put(Key.CLUSTER_NAME, ClusterName.CLUSTER_NAME_SETTING);


why SQL plugin handle cluster_name setting?

SQL Plugin requires this in StorageEngineFactory class in datasource connector modules.

penghuo · 2023-06-06T21:49:25Z

opensearch/src/main/java/org/opensearch/sql/opensearch/setting/OpenSearchSettings.java

@@ -98,6 +102,25 @@ public class OpenSearchSettings extends Settings {
      Setting.Property.NodeScope,
      Setting.Property.Dynamic);

+  // we are keeping this to not break upgrades if the config is already present.
+  // This will be completely removed in 3.0.
+  public static final Setting<InputStream> DATASOURCE_CONFIG = SecureSetting.secureFile(


why move to opensearch module? would it be better in datasource module?

Wanted to make OpenSearchSettings class a single interface for all settings.

This class is an extension for Setting class in commons module and available for all modules.

This class has also got the implementation of consuming latest updates.

Signed-off-by: Vamsi Manohar <[email protected]>

opensearch-trigger-bot · 2023-06-09T01:37:28Z

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

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

(cherry picked from commit bd9b180)

Signed-off-by: Mitchell Gale <[email protected]>

vmmusings force-pushed the refactor-ds branch 7 times, most recently from 365d54d to a9a6589 Compare June 6, 2023 00:20

vmmusings marked this pull request as ready for review June 6, 2023 00:29

vmmusings requested review from pjfitzgibbons, ps48, kavithacm, derek-ho, joshuali925, dai-chen, YANG-DB, rupal-bq, mengweieric, Swiddis, penghuo, seankao-az, MaxKsyunz, Yury-Fridlyand, anirudha, forestmvey, acarbonetto and GumpacG as code owners June 6, 2023 00:29

vmmusings added the backport 2.x label Jun 6, 2023

vmmusings self-assigned this Jun 6, 2023

vmmusings linked an issue Jun 6, 2023 that may be closed by this pull request

[Enhancement] Move interceptors to common module. #1709

Closed

penghuo reviewed Jun 6, 2023

View reviewed changes

penghuo previously approved these changes Jun 6, 2023

View reviewed changes

vmmusings dismissed stale reviews from penghuo, ps48, and anirudha via 5d26422 June 7, 2023 19:22

vmmusings force-pushed the refactor-ds branch from a9a6589 to 5d26422 Compare June 7, 2023 19:22

ps48 previously approved these changes Jun 7, 2023

View reviewed changes

penghuo previously approved these changes Jun 7, 2023

View reviewed changes

vmmusings dismissed stale reviews from penghuo and ps48 via 81472ac June 7, 2023 21:30

vmmusings force-pushed the refactor-ds branch 10 times, most recently from 7a862e1 to 13fa97b Compare June 8, 2023 01:10

Moved auth interceptors to common module

f838975

Signed-off-by: Vamsi Manohar <[email protected]>

vmmusings force-pushed the refactor-ds branch from 13fa97b to f838975 Compare June 8, 2023 01:12

joshuali925 approved these changes Jun 9, 2023

View reviewed changes

rupal-bq approved these changes Jun 9, 2023

View reviewed changes

vmmusings merged commit bd9b180 into opensearch-project:main Jun 9, 2023

vmmusings added a commit that referenced this pull request Jun 9, 2023

Moved auth interceptors to common module (#1703)

14a024c

(cherry picked from commit bd9b180)

MitchellGale pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 12, 2023

Moved auth interceptors to common module (opensearch-project#1703)

a807c3b

vmmusings added a commit that referenced this pull request Jun 12, 2023

Moved auth interceptors to common module (#1703) (#1729)

d7940a8

(cherry picked from commit bd9b180)

MitchellGale pushed a commit to Bit-Quill/opensearch-project-sql that referenced this pull request Jun 12, 2023

Moved auth interceptors to common module (opensearch-project#1703)

7613adb

Signed-off-by: Mitchell Gale <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Moved auth interceptors to common module #1703

Moved auth interceptors to common module #1703

vmmusings commented Jun 5, 2023 •

edited

Loading

codecov bot commented Jun 5, 2023 •

edited

Loading

penghuo Jun 6, 2023

vmmusings Jun 7, 2023

penghuo Jun 6, 2023

vmmusings Jun 7, 2023

penghuo Jun 6, 2023

vmmusings Jun 7, 2023

opensearch-trigger-bot bot commented Jun 9, 2023

Moved auth interceptors to common module #1703

Moved auth interceptors to common module #1703

Conversation

vmmusings commented Jun 5, 2023 • edited Loading

Description

Issues Resolved

Check List

codecov bot commented Jun 5, 2023 • edited Loading

Codecov Report

penghuo Jun 6, 2023

Choose a reason for hiding this comment

vmmusings Jun 7, 2023

Choose a reason for hiding this comment

penghuo Jun 6, 2023

Choose a reason for hiding this comment

vmmusings Jun 7, 2023

Choose a reason for hiding this comment

penghuo Jun 6, 2023

Choose a reason for hiding this comment

vmmusings Jun 7, 2023

Choose a reason for hiding this comment

opensearch-trigger-bot bot commented Jun 9, 2023

vmmusings commented Jun 5, 2023 •

edited

Loading

codecov bot commented Jun 5, 2023 •

edited

Loading