Skip to content

Conversation

@cwperks
Copy link
Member

@cwperks cwperks commented Dec 1, 2025

Description

This PR replaces usages of java.security.AccessController in the modules/ dir

Related Issues

Related to #18339

Check List

  • Functionality includes testing.
  • API changes companion pull request created, if applicable.
  • Public documentation issue/PR created, if applicable.

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.

Summary by CodeRabbit

  • Refactor

    • Simplified security execution patterns and modernized code constructs across multiple modules for improved maintainability and consistency.
  • Chores

    • Removed deprecated language patterns and cleaned up legacy code structures to align with current development standards and best practices.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Dec 1, 2025

Walkthrough

This pull request systematically replaces Java's standard java.security.AccessController with OpenSearch's custom org.opensearch.secure_sm.AccessController across nine modules. The refactoring also simplifies privileged action handling by converting verbose PrivilegedAction anonymous inner classes into concise lambda expressions, while removing associated deprecation suppression annotations and updating imports.

Changes

Cohort / File(s) Summary
Expression & Mustache Script Engines
modules/lang-expression/src/main/java/org/opensearch/script/expression/ExpressionScriptEngine.java, modules/lang-mustache/src/main/java/org/opensearch/script/mustache/MustacheScriptEngine.java
Replaced java.security.AccessController with org.opensearch.secure_sm.AccessController and converted PrivilegedAction blocks to lambdas. Removed @SuppressWarnings("removal") annotations.
Painless Module – Core Engine
modules/lang-painless/src/main/java/org/opensearch/painless/PainlessScriptEngine.java
Removed static COMPILATION_CONTEXT initialization block with restricted AccessControlContext. Converted two PrivilegedAction blocks to lambdas in compile methods. Updated imports to use org.opensearch.secure_sm.AccessController.
Painless Module – Supporting Classes
modules/lang-painless/src/main/java/org/opensearch/painless/LambdaBootstrap.java, modules/lang-painless/src/main/java/org/opensearch/painless/lookup/PainlessLookupBuilder.java, modules/lang-painless/spi/src/main/java/org/opensearch/painless/spi/AllowlistLoader.java
Converted PrivilegedAction invocations to lambdas in createLambdaClass, bridge class loader creation, and resource class loader initialization. Updated imports across all three files.
Painless Test
modules/lang-painless/src/test/java/org/opensearch/painless/DocFieldsPhaseTests.java
Replaced PrivilegedAction anonymous inner class with lambda in compiler loader retrieval. Removed @SuppressWarnings("removal") and updated imports.
Repository & Transport Modules
modules/repository-url/src/main/java/org/opensearch/common/blobstore/url/URLBlobContainer.java, modules/systemd/src/main/java/org/opensearch/systemd/Libsystemd.java, modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java
Replaced java.security.AccessController and PrivilegedAction patterns with org.opensearch.secure_sm.AccessController lambdas. Removed associated deprecation warnings and obsolete imports.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

  • Verification points:
    • Ensure org.opensearch.secure_sm.AccessController API is fully compatible with all call sites (particularly doPrivileged, doPrivilegedChecked signatures)
    • Confirm lambda syntax correctly preserves exception handling semantics (especially in URLBlobContainer with doPrivilegedChecked)
    • Validate that removal of COMPILATION_CONTEXT and ProtectionDomain initialization in PainlessScriptEngine.java does not inadvertently weaken security constraints
    • Spot-check that no instances of java.security.AccessController or PrivilegedAction were missed across the codebase

Poem

🐰 Old security guards have taken their bows,
New OpenSearch sentries now guard the house,
Lambdas dance lighter through privileged vows,
Simpler, more elegant—quiet as a mouse! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 23.08% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically describes the main change: replacing usages of java.security.AccessController in the modules/ directory, which is exactly what the changeset accomplishes across multiple files.
Description check ✅ Passed The description includes all required sections from the template: a description of the change, a related issue reference, and the completed checklist with Apache 2.0 license confirmation.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 859f7c3 and c99cc1b.

📒 Files selected for processing (10)
  • modules/lang-expression/src/main/java/org/opensearch/script/expression/ExpressionScriptEngine.java (2 hunks)
  • modules/lang-mustache/src/main/java/org/opensearch/script/mustache/MustacheScriptEngine.java (2 hunks)
  • modules/lang-painless/spi/src/main/java/org/opensearch/painless/spi/AllowlistLoader.java (2 hunks)
  • modules/lang-painless/src/main/java/org/opensearch/painless/LambdaBootstrap.java (2 hunks)
  • modules/lang-painless/src/main/java/org/opensearch/painless/PainlessScriptEngine.java (3 hunks)
  • modules/lang-painless/src/main/java/org/opensearch/painless/lookup/PainlessLookupBuilder.java (2 hunks)
  • modules/lang-painless/src/test/java/org/opensearch/painless/DocFieldsPhaseTests.java (2 hunks)
  • modules/repository-url/src/main/java/org/opensearch/common/blobstore/url/URLBlobContainer.java (2 hunks)
  • modules/systemd/src/main/java/org/opensearch/systemd/Libsystemd.java (1 hunks)
  • modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java (2 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
modules/systemd/src/main/java/org/opensearch/systemd/Libsystemd.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/lang-mustache/src/main/java/org/opensearch/script/mustache/MustacheScriptEngine.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/lang-painless/spi/src/main/java/org/opensearch/painless/spi/AllowlistLoader.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/repository-url/src/main/java/org/opensearch/common/blobstore/url/URLBlobContainer.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/lang-painless/src/main/java/org/opensearch/painless/PainlessScriptEngine.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/lang-expression/src/main/java/org/opensearch/script/expression/ExpressionScriptEngine.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/lang-painless/src/test/java/org/opensearch/painless/DocFieldsPhaseTests.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/lang-painless/src/main/java/org/opensearch/painless/LambdaBootstrap.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
modules/lang-painless/src/main/java/org/opensearch/painless/lookup/PainlessLookupBuilder.java (1)
libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/AccessController.java (1)
  • AccessController (44-144)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
  • GitHub Check: gradle-check
  • GitHub Check: detect-breaking-change
  • GitHub Check: Analyze (java)
  • GitHub Check: assemble (25, ubuntu-24.04-arm)
  • GitHub Check: assemble (25, ubuntu-latest)
  • GitHub Check: assemble (25, windows-latest)
  • GitHub Check: assemble (21, windows-latest)
  • GitHub Check: assemble (21, ubuntu-latest)
  • GitHub Check: assemble (21, ubuntu-24.04-arm)
  • GitHub Check: precommit (21, windows-2025, true)
  • GitHub Check: precommit (25, macos-15)
  • GitHub Check: precommit (25, ubuntu-24.04-arm)
  • GitHub Check: precommit (25, windows-latest)
  • GitHub Check: precommit (25, macos-15-intel)
  • GitHub Check: precommit (21, ubuntu-24.04-arm)
  • GitHub Check: precommit (21, windows-latest)
  • GitHub Check: precommit (21, macos-15-intel)
  • GitHub Check: precommit (25, ubuntu-latest)
  • GitHub Check: precommit (21, macos-15)
  • GitHub Check: precommit (21, ubuntu-latest)
🔇 Additional comments (21)
modules/lang-painless/src/main/java/org/opensearch/painless/LambdaBootstrap.java (2)

35-35: LGTM: Import updated correctly.

The import has been properly updated to use OpenSearch's internal AccessController implementation.


508-508: LGTM: Simplified privileged action using lambda.

The replacement of the verbose PrivilegedAction with a concise lambda expression improves readability while maintaining equivalent functionality.

modules/transport-netty4/src/main/java/org/opensearch/transport/netty4/ssl/SecureNetty4Transport.java (2)

46-46: LGTM: Import updated correctly.


273-273: LGTM: Clean method reference usage.

The method reference provides a clean and concise alternative to the verbose PrivilegedAction wrapper.

modules/lang-mustache/src/main/java/org/opensearch/script/mustache/MustacheScriptEngine.java (2)

49-49: LGTM: Import updated correctly.


136-136: LGTM: Simplified privileged template execution.

The lambda-based approach correctly replaces the explicit PrivilegedAction wrapper. The template.execute() method is properly invoked within the privileged context.

modules/systemd/src/main/java/org/opensearch/systemd/Libsystemd.java (2)

37-37: LGTM: Import updated correctly.


45-45: LGTM: Clean static initialization with lambda.

The lambda-based privileged call in the static initializer is concise and correct for native library registration.

modules/lang-painless/src/main/java/org/opensearch/painless/lookup/PainlessLookupBuilder.java (2)

48-48: LGTM: Import updated correctly.


2191-2193: LGTM: Bridge loader creation with proper privilege handling.

The lambda-based approach cleanly creates the BridgeLoader with the appropriate parent classloader in a privileged context.

modules/repository-url/src/main/java/org/opensearch/common/blobstore/url/URLBlobContainer.java (2)

41-41: LGTM: Import updated correctly.


163-163: LGTM: Proper use of doPrivilegedChecked for IOException.

Correctly uses doPrivilegedChecked since openStream() throws a checked IOException. The method reference provides a clean and type-safe implementation.

modules/lang-painless/spi/src/main/java/org/opensearch/painless/spi/AllowlistLoader.java (2)

36-36: LGTM: Import updated correctly.


515-515: LGTM: Clean classloader retrieval with method reference.

The method reference provides a concise way to obtain the classloader in a privileged context.

modules/lang-painless/src/main/java/org/opensearch/painless/PainlessScriptEngine.java (3)

45-45: LGTM: Import updated correctly.


129-129: LGTM: Simplified loader creation.

The lambda-based approach cleanly creates the Compiler.Loader in a privileged context.


431-434: LGTM: Clean compilation in privileged context.

The multi-line lambda correctly performs the script compilation within a privileged context and returns the ScriptScope result. This is functionally equivalent to the previous implementation but more readable.

modules/lang-expression/src/main/java/org/opensearch/script/expression/ExpressionScriptEngine.java (2)

35-61: Import switch to secure_sm.AccessController looks correct

The new org.opensearch.secure_sm.AccessController import cleanly replaces the JDK AccessController usage and matches the updated doPrivileged call below; no issues here.


172-183: Privileged compile lambda preserves behavior

Wrapping JavascriptCompiler.compile in AccessController.doPrivileged(() -> …) with the existing SpecialPermission.check() keeps the previous flow: compile succeeds and returns an Expression, and ParseException is still converted into a ScriptException via convertToScriptException. No functional or typing problems stand out.

modules/lang-painless/src/test/java/org/opensearch/painless/DocFieldsPhaseTests.java (2)

35-41: AccessController import update is consistent

Using org.opensearch.secure_sm.AccessController here aligns with the rest of the PR and the updated privileged invocation below; nothing else needed.


49-60: Loader creation via doPrivileged lambda is sound

The new AccessController.doPrivileged(() -> compiler.createLoader(getClass().getClassLoader())) cleanly replaces the previous PrivilegedAction boilerplate without changing behavior. Type inference to Compiler.Loader is correct and no error-handling regressions are introduced.


Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 1, 2025

✅ Gradle check result for c99cc1b: SUCCESS

@codecov
Copy link

codecov bot commented Dec 1, 2025

Codecov Report

❌ Patch coverage is 80.00000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.26%. Comparing base (0197084) to head (c99cc1b).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...arch/script/expression/ExpressionScriptEngine.java 75.00% 1 Missing ⚠️
...c/main/java/org/opensearch/systemd/Libsystemd.java 0.00% 1 Missing ⚠️
...ch/transport/netty4/ssl/SecureNetty4Transport.java 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main   #20137      +/-   ##
============================================
+ Coverage     73.23%   73.26%   +0.03%     
- Complexity    71621    71638      +17     
============================================
  Files          5787     5787              
  Lines        327853   327831      -22     
  Branches      47218    47216       -2     
============================================
+ Hits         240090   240183      +93     
+ Misses        68505    68368     -137     
- Partials      19258    19280      +22     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@prudhvigodithi
Copy link
Member

prudhvigodithi commented Dec 1, 2025

I see this error with intellij java: warnings found and -Werror specified for RestClientBuilder.java java: java.security.AccessController in java.security has been deprecated and marked for removal

@cwperks
Copy link
Member Author

cwperks commented Dec 1, 2025

@prudhvigodithi This replaces the usages under modules/ not the entire repo. There are many usages so I have been targeting some areas at a time to keep the reviews simpler.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants