Skip to content

Add integration tests for analytics engine index-level authorization#5462

Merged
mengweieric merged 2 commits into
opensearch-project:mainfrom
finnegancarroll:analytics-engine-security-it
May 22, 2026
Merged

Add integration tests for analytics engine index-level authorization#5462
mengweieric merged 2 commits into
opensearch-project:mainfrom
finnegancarroll:analytics-engine-security-it

Conversation

@finnegancarroll
Copy link
Copy Markdown
Contributor

@finnegancarroll finnegancarroll commented May 22, 2026

Description

The test cluster installs the full analytics plugin stack (analytics-engine, arrow-base, arrow-flight-rpc, analytics-backend-lucene, analytics-backend-datafusion, parquet-data-format, composite-engine) plus the security and SQL plugins.

Adds AnalyticsEngineSecurityIT which validates that the analytics engine's FGAC check (indices:data/read/analytics/query) is enforced end-to-end through the SQL plugin PPL/SQL endpoints when querying composite (analytics-engine-backed) indices.

Tests:

  • Authorized user with indices:data/read* can query a composite index
  • Unauthorized user (no index permissions) gets 403
  • Authorized user cannot access an index outside their permissions (403)
  • User with indices:data/read/search* but NOT indices:data/read/analytics/query gets 403, proving the specific analytics action permission is evaluated

Note: This change is still pending while i try to figure out how to correctly provide this test environment in CI.

Run locally with local in the meantime with plugin zips:

  ./gradlew :integ-test:analyticsEngineSecurityIT \
    -PanalyticsEngineZip=/path/to/analytics-engine.zip \
    -ParrowBaseZip=/path/to/arrow-base.zip \
    -ParrowFlightRpcZip=/path/to/arrow-flight-rpc.zip \
    -PanalyticsBackendLuceneZip=/path/to/analytics-backend-lucene.zip \
    -PanalyticsBackendDatafusionZip=/path/to/analytics-backend-datafusion.zip \
    -PparquetDataFormatZip=/path/to/parquet-data-format.zip \
    -PcompositeEngineZip=/path/to/composite-engine.zip \
    -PnativeLibPath=/path/to/rust/target/release

Related Issues

Core PR - opensearch-project/OpenSearch#21789

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • New functionality has javadoc added.
  • New functionality has a user manual doc added.
  • New PPL command checklist all confirmed.
  • API changes companion pull request created.
  • Commits are signed per the DCO using --signoff or -s.
  • Public documentation issue/PR created.

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 22, 2026

PR Code Analyzer ❗

AI-powered 'Code-Diff-Analyzer' found issues on commit caa0189.

PathLineSeverityDescription
integ-test/build.gradle292highMultiple new plugin artifact download tasks are added, pulling ZIP files from a remote CI server (https://ci.opensearch.org/ci/dbc/feature-build-opensearch/feature-datafusion/latest/...). These are build-time dependency/plugin changes that introduce externally-fetched artifacts (arrow-base, test-ppl-frontend, analytics-backend-lucene, parquet-data-format, composite-engine, analytics-backend-datafusion). Per mandatory flagging rules, any dependency or build plugin change must be flagged. Maintainers should verify the artifact source URL, file integrity (checksums), and that none of these plugins contain malicious code before trusting them in test cluster deployments.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0


Pull Requests Author(s): Please update your Pull Request according to the report above.

Repository Maintainer(s): You can bypass diff analyzer by adding label skip-diff-analyzer after reviewing the changes carefully, then re-run failed actions. To re-enable the analyzer, remove the label, then re-run all actions.


⚠️ Note: The Code-Diff-Analyzer helps protect against potentially harmful code patterns. Please ensure you have thoroughly reviewed the changes beforehand.

Thanks.

@github-actions
Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Add thread-safe initialization

The static initialized flag is not thread-safe and can cause race conditions if
multiple test instances run concurrently. Use proper synchronization or a
thread-safe initialization pattern to ensure setup runs exactly once across all
threads.

integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [34-49]

-private static boolean initialized = false;
+private static volatile boolean initialized = false;
+private static final Object INIT_LOCK = new Object();
 
 @Override
 protected void init() throws Exception {
   if (!initialized) {
-    waitForSecurityPlugin();
-    createTestIndices();
-    createSecurityRolesAndUsers();
-    initialized = true;
+    synchronized (INIT_LOCK) {
+      if (!initialized) {
+        waitForSecurityPlugin();
+        createTestIndices();
+        createSecurityRolesAndUsers();
+        initialized = true;
+      }
+    }
   }
 }
Suggestion importance[1-10]: 7

__

Why: The static initialized flag without synchronization could cause race conditions if tests run concurrently. The double-checked locking pattern with volatile and synchronized block properly addresses this thread-safety issue.

Medium
General
Handle index creation errors properly

Silently swallowing 400 errors can hide legitimate index creation failures (e.g.,
invalid settings, malformed JSON). Consider checking if the index already exists
before creation, or log the 400 error to aid debugging when setup fails
unexpectedly.

integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [94-113]

 private void createCompositeIndex(String index) throws IOException {
   try {
     Request req = new Request("PUT", "/" + index);
     req.setJsonEntity("""
         {
           "settings": {
             "number_of_shards": 1,
             "number_of_replicas": 0,
             "index.pluggable.dataformat.enabled": true,
             "index.pluggable.dataformat": "composite"
           }
         }
         """);
     client().performRequest(req);
   } catch (ResponseException e) {
-    if (e.getResponse().getStatusLine().getStatusCode() != 400) {
+    if (e.getResponse().getStatusLine().getStatusCode() == 400) {
+      String body = org.opensearch.sql.legacy.TestUtils.getResponseBody(e.getResponse(), true);
+      if (!body.contains("resource_already_exists_exception")) {
+        throw e;
+      }
+    } else {
       throw e;
     }
   }
 }
Suggestion importance[1-10]: 5

__

Why: The current code silently ignores all 400 errors, which could hide legitimate failures. Checking specifically for resource_already_exists_exception improves error handling by only ignoring expected duplicate index creation attempts while surfacing other issues.

Low
Security
Avoid hardcoded credentials

The hardcoded credentials "admin:admin" pose a security risk in production
environments. Consider extracting these credentials to environment variables or a
secure configuration file to prevent credential exposure in source code.

integ-test/src/test/java/org/opensearch/sql/security/AnalyticsEngineSecurityIT.java [51-67]

 private void waitForSecurityPlugin() throws Exception {
+  String adminCreds = System.getenv("ADMIN_CREDENTIALS");
+  if (adminCreds == null) {
+    adminCreds = "admin:admin"; // fallback for test environments only
+  }
   for (int i = 0; i < 60; i++) {
     try {
       Request req = new Request("GET", "/_plugins/_security/api/roles");
       RequestOptions.Builder opts = RequestOptions.DEFAULT.toBuilder();
       opts.addHeader("Authorization", "Basic " +
-          java.util.Base64.getEncoder().encodeToString("admin:admin".getBytes()));
+          java.util.Base64.getEncoder().encodeToString(adminCreds.getBytes()));
       req.setOptions(opts);
       Response resp = client().performRequest(req);
       if (resp.getStatusLine().getStatusCode() == 200) return;
     } catch (Exception e) {
       // Security not ready yet
     }
     Thread.sleep(1000);
   }
   throw new IllegalStateException("Security plugin did not initialize in time");
 }
Suggestion importance[1-10]: 2

__

Why: While hardcoded credentials are generally a concern, this is an integration test file where admin:admin is a standard test credential. The suggestion to use environment variables would complicate test setup without significant security benefit in a test context.

Low

@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it branch from 1deac38 to 87eda6a Compare May 22, 2026 01:21
@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it branch 2 times, most recently from 12dc610 to dca4329 Compare May 22, 2026 18:39
Adds AnalyticsEngineSecurityIT which validates that the analytics engine's
FGAC check (indices:data/read/analytics/query) is enforced end-to-end
through the production SQL plugin PPL endpoint (/_plugins/_ppl) when
querying composite (analytics-engine-backed) indices.

Tests:
- Authorized user with indices:data/read* can query a composite index
- Unauthorized user (no index permissions) gets 403
- Authorized user cannot access an index outside their permissions (403)
- User with indices:data/read/search* but NOT indices:data/read/analytics/query
  gets 403, proving the specific analytics action permission is evaluated

The test cluster installs the full analytics plugin stack (analytics-engine,
arrow-base, arrow-flight-rpc, analytics-backend-lucene,
analytics-backend-datafusion, parquet-data-format, composite-engine) plus
the security and SQL plugins.

Run locally with local plugin zips:
  ./gradlew :integ-test:analyticsEngineSecurityIT \
    -PanalyticsEngineZip=/path/to/analytics-engine.zip \
    -ParrowBaseZip=/path/to/arrow-base.zip \
    -ParrowFlightRpcZip=/path/to/arrow-flight-rpc.zip \
    -PanalyticsBackendLuceneZip=/path/to/analytics-backend-lucene.zip \
    -PanalyticsBackendDatafusionZip=/path/to/analytics-backend-datafusion.zip \
    -PparquetDataFormatZip=/path/to/parquet-data-format.zip \
    -PcompositeEngineZip=/path/to/composite-engine.zip \
    -PnativeLibPath=/path/to/rust/target/release

Signed-off-by: carrofin <carrofin@amazon.com>
Signed-off-by: Finn Carroll <carrofin@amazon.com>
@finnegancarroll finnegancarroll force-pushed the analytics-engine-security-it branch from dca4329 to dc53c24 Compare May 22, 2026 18:52
Signed-off-by: Finn Carroll <carrofin@amazon.com>
@finnegancarroll finnegancarroll added the infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc. label May 22, 2026
@finnegancarroll
Copy link
Copy Markdown
Contributor Author

    diff_report: <table><tr><th>Path</th><th>Line</th><th>Severity</th><th>Description</th></tr><tr><td>integ-test/build.gradle</td><td>292</td><td>high</td><td>Multiple new plugin artifact download tasks are added, pulling ZIP files from a remote CI server (https://ci.opensearch.org/ci/dbc/feature-build-opensearch/feature-datafusion/latest/...). These are build-time dependency/plugin changes that introduce externally-fetched artifacts (arrow-base, test-ppl-frontend, analytics-backend-lucene, parquet-data-format, composite-engine, analytics-backend-datafusion). Per mandatory flagging rules, any dependency or build plugin change must be flagged. Maintainers should verify the artifact source URL, file integrity (checksums), and that none of these plugins contain malicious code before trusting them in test cluster deployments.</td></tr></table><p><i>The table above displays the top 10 most important findings.</i> <br/><br/><strong>Total: 1 | Critical: 0 | High: 1 | Medium: 0 | Low: 0</strong></p>

Diff analyzer is upset we are downloading he analytics plugin CI artifacts. This pattern was already in use.

@finnegancarroll
Copy link
Copy Markdown
Contributor Author

Still failing:
SQL CLI Integration Test / test-sql-cli-integration (21)

The conflict is caused by:
    The user requested pathspec==0.12.1
    black 26.3.1 depends on pathspec>=1.0.0

...

`Execution failed for task ':installPythonDeps'.`

@finnegancarroll
Copy link
Copy Markdown
Contributor Author

PR for the remaining CLI failure due to dependency conflict in sql-cli:
opensearch-project/sql-cli#71

@mengweieric mengweieric merged commit d5ee441 into opensearch-project:main May 22, 2026
42 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci infrastructure Changes to infrastructure, testing, CI/CD, pipelines, etc.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants