Skip to content

Enable bootstrap.serial_filter in integration tests - #6229

Merged
DarshitChanpura merged 3 commits into
opensearch-project:mainfrom
cwperks:reject-all-filter
Jun 23, 2026
Merged

Enable bootstrap.serial_filter in integration tests#6229
DarshitChanpura merged 3 commits into
opensearch-project:mainfrom
cwperks:reject-all-filter

Conversation

@cwperks

@cwperks cwperks commented Jun 19, 2026

Copy link
Copy Markdown
Member

Description

Enables the bootstrap.serial_filter setting (introduced in opensearch-project/OpenSearch#22073) in the security plugin's integration test clusters.

This setting installs a process-wide ObjectInputFilter that rejects all Java deserialization by default. Running the security plugin's integration tests with this filter enabled ensures the plugin works correctly with this runtime enforcement active.

Testing

  • ./gradlew integrationTest
  • ./gradlew test

@github-actions

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

PR Code Analyzer ❗

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

PathLineSeverityDescription
src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java614mediumAddition of undocumented bootstrap setting 'bootstrap.serial_filter' (also added in ClusterHelper.java:484). In Java, 'serial filter' maps to JEP-290/JEP-415 deserialization controls. If this setting disables or bypasses OpenSearch's serialization filter mechanism, it could expose test clusters to deserialization-based attacks. The setting is not a well-known standard OpenSearch bootstrap option, and its behavior should be verified against OpenSearch source code to confirm it enables (rather than disables) deserialization filtering.

The table above displays the top 10 most important findings.

Total: 1 | Critical: 0 | High: 0 | Medium: 1 | 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

github-actions Bot commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

(Review updated until commit 005ef04)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible Issue

The path.home setting is moved from the end of the builder chain to before bootstrap.serial_filter. If the builder chain continues after line 485 (not visible in the diff), this could inadvertently change the order of settings or cause the path.home setting to be overridden by subsequent calls. The original code had path.home as the last setting, which may have been intentional to ensure it takes precedence.

.put("path.home", "./target")
.put("bootstrap.serial_filter", true);

@cwperks
cwperks marked this pull request as draft June 19, 2026 19:42
@cwperks
cwperks force-pushed the reject-all-filter branch 2 times, most recently from ad0b7db to 6698ea9 Compare June 20, 2026 00:36
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 6698ea9

@github-actions

github-actions Bot commented Jun 20, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Latest suggestions up to 72c811c
Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Check filter existence before setting

The serial filter should check if a filter is already set before attempting to set a
new one. The current implementation catches IllegalStateException after the fact,
but it's better to check getSerialFilter() first to avoid the exception entirely and
ensure idempotent behavior.

src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java [104-107]

-java.io.ObjectInputFilter rejectAllFilter = filterInfo -> filterInfo.serialClass() == null
-    ? java.io.ObjectInputFilter.Status.UNDECIDED
-    : java.io.ObjectInputFilter.Status.REJECTED;
-java.io.ObjectInputFilter.Config.setSerialFilter(rejectAllFilter);
+if (java.io.ObjectInputFilter.Config.getSerialFilter() == null) {
+    java.io.ObjectInputFilter rejectAllFilter = filterInfo -> filterInfo.serialClass() == null
+        ? java.io.ObjectInputFilter.Status.UNDECIDED
+        : java.io.ObjectInputFilter.Status.REJECTED;
+    java.io.ObjectInputFilter.Config.setSerialFilter(rejectAllFilter);
+}
Suggestion importance[1-10]: 5

__

Why: While checking getSerialFilter() before setting is a valid approach, the current try-catch implementation is also acceptable and commonly used. The suggestion improves code style by avoiding exception-based control flow, but the impact is minor since both approaches handle the case correctly.

Low

Previous suggestions

Suggestions up to commit 6698ea9
CategorySuggestion                                                                                                                                    Impact
Possible issue
Fix filter status for null classes

The filter logic may incorrectly reject legitimate serialization operations. When
serialClass() is null, it typically indicates the stream is checking depth or array
length, not actual class deserialization. Consider allowing these checks to proceed
by returning ALLOWED instead of UNDECIDED for null classes.

src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java [104-106]

 java.io.ObjectInputFilter rejectAllFilter = filterInfo -> filterInfo.serialClass() == null
-    ? java.io.ObjectInputFilter.Status.UNDECIDED
+    ? java.io.ObjectInputFilter.Status.ALLOWED
     : java.io.ObjectInputFilter.Status.REJECTED;
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that serialClass() == null typically indicates stream metadata checks (depth/array length) rather than class deserialization. Returning ALLOWED instead of UNDECIDED for null classes is more appropriate for these legitimate operations, though the impact depends on whether the current implementation causes issues in practice.

Medium
General
Add logging for filter initialization

Silently catching IllegalStateException when the filter is already set may hide
configuration issues or race conditions during test initialization. Consider logging
a warning or debug message to aid troubleshooting when multiple test classes attempt
to set the filter.

src/integrationTest/java/org/opensearch/test/framework/cluster/LocalOpenSearchCluster.java [108-110]

 } catch (IllegalStateException e) {
-    // Filter already set
+    log.debug("Serial filter already configured, skipping initialization", e);
 }
Suggestion importance[1-10]: 5

__

Why: Adding debug logging when the filter is already set would improve troubleshooting capabilities during test initialization. However, this is a minor enhancement since silently catching IllegalStateException is acceptable when the filter is already configured, and the impact on functionality is minimal.

Low

Installs a process-wide ObjectInputFilter in the test framework that rejects
all Java deserialization by default, mirroring what OpenSearch bootstrap does
when bootstrap.serial_filter is enabled.

This ensures the security plugin's setObjectInputFilter opt-in in
SafeObjectInputStream is exercised during integration tests.

Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks
cwperks force-pushed the reject-all-filter branch from 6698ea9 to 72c811c Compare June 20, 2026 01:12
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 72c811c

cwperks added 2 commits June 19, 2026 23:59
…failure

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
@cwperks
cwperks force-pushed the reject-all-filter branch from 7b5e21d to 005ef04 Compare June 20, 2026 13:18
@github-actions

Copy link
Copy Markdown
Contributor

Persistent review updated to latest commit 005ef04

@cwperks
cwperks marked this pull request as ready for review June 20, 2026 15:02
@DarshitChanpura
DarshitChanpura merged commit 78b0c7e into opensearch-project:main Jun 23, 2026
62 of 63 checks passed
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.

2 participants