Skip to content

Cleanup SafeSerializationUtils to remove unused Guava classes#6152

Merged
finnegancarroll merged 2 commits into
opensearch-project:mainfrom
cwperks:filter-safe
May 20, 2026
Merged

Cleanup SafeSerializationUtils to remove unused Guava classes#6152
finnegancarroll merged 2 commits into
opensearch-project:mainfrom
cwperks:filter-safe

Conversation

@cwperks
Copy link
Copy Markdown
Member

@cwperks cwperks commented May 19, 2026

Description

In SafeSerializationUtils, we previously allowlisted Guava classes when user attribute serialization was enabled but now switched to wrapping in HashMap. These classes are no longer required and can be safely removed.

  • Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Maintenance

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • 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.

@github-actions
Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 Security concerns

Deserialization vulnerability:
The new ObjectInputFilter in Base64Helper only limits depth (maxdepth=10) without restricting which classes can be deserialized. While SafeObjectInputStream.resolveClass() likely validates classes, relying solely on custom resolveClass() without filter-level class restrictions is risky. If resolveClass() has any bypass or the filter is applied to other deserialization paths, arbitrary classes could be instantiated. Best practice is to configure the filter with explicit class allowlists (e.g., "java.base/;org.opensearch.security.;!*") to provide layered defense against deserialization attacks.

✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Deserialization Filter

The filter "maxdepth=10" only limits object graph depth but does not restrict which classes can be deserialized. This leaves the application vulnerable to deserialization attacks if untrusted data is passed to deserializeObject(). The SafeObjectInputStream.resolveClass() method appears to validate classes, but the filter should also include class restrictions (e.g., using a pattern like "java.base/;!") or reference the same allowlist logic to provide defense in depth.

setObjectInputFilter(ObjectInputFilter.Config.createFilter("maxdepth=10"));

@github-actions
Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Security
Strengthen deserialization filter with class restrictions

The maxdepth=10 filter may be insufficient for complex object graphs and doesn't
restrict class types. Consider adding class pattern restrictions to prevent
deserialization of potentially dangerous classes, such as
maxdepth=10;!;java.base/;org.opensearch.
.**

src/main/java/org/opensearch/security/support/Base64Helper.java [107]

-setObjectInputFilter(ObjectInputFilter.Config.createFilter("maxdepth=10"));
+setObjectInputFilter(ObjectInputFilter.Config.createFilter("maxdepth=10;!*;java.base/*;org.opensearch.**"));
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that adding class pattern restrictions would enhance security by limiting which classes can be deserialized. However, the specific pattern suggested may be too restrictive and could break existing functionality without testing. The suggestion asks for verification rather than pointing to a definite issue.

Medium
Possible issue
Verify backward compatibility for serialized data

Removing Guava's ImmutableMap and ImmutableBiMap serialization forms from safe
classes may break existing serialized data. Verify that no persisted data relies on
these classes before removal, or implement migration logic.

src/main/java/org/opensearch/security/support/SafeSerializationUtils.java [63]

-private static final Set<String> SAFE_CLASS_NAMES = Set.of("org.ldaptive.LdapAttribute$LdapAttributeValues");
+private static final Set<String> SAFE_CLASS_NAMES = Set.of(
+    "org.ldaptive.LdapAttribute$LdapAttributeValues",
+    "com.google.common.collect.ImmutableBiMap$SerializedForm",
+    "com.google.common.collect.ImmutableMap$SerializedForm"
+);
Suggestion importance[1-10]: 6

__

Why: This is a valid concern about backward compatibility when removing ImmutableMap and ImmutableBiMap from safe classes. However, it only asks for verification rather than identifying a concrete bug, and the PR authors may have already considered this. The score reflects the importance of backward compatibility checks.

Low

@codecov
Copy link
Copy Markdown

codecov Bot commented May 19, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.98%. Comparing base (e29af48) to head (28d728c).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6152      +/-   ##
==========================================
+ Coverage   74.75%   74.98%   +0.22%     
==========================================
  Files         447      452       +5     
  Lines       28480    29098     +618     
  Branches     4331     4380      +49     
==========================================
+ Hits        21291    21818     +527     
- Misses       5193     5252      +59     
- Partials     1996     2028      +32     
Files with missing lines Coverage Δ
.../org/opensearch/security/support/Base64Helper.java 91.17% <100.00%> (+0.26%) ⬆️
...earch/security/support/SafeSerializationUtils.java 86.66% <100.00%> (ø)

... and 47 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@finnegancarroll finnegancarroll merged commit 3bb8342 into opensearch-project:main May 20, 2026
113 of 114 checks passed
terryquigleysas pushed a commit to terryquigleysas/security that referenced this pull request May 21, 2026
…arch-project#6152)

Signed-off-by: Craig Perkins <cwperx@amazon.com>
Signed-off-by: Terry Quigley <terry.quigley@sas.com>
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.

3 participants