Skip to content

Mirror upstream elastic/elasticsearch#134454 for AI review (snapshot of HEAD tree) #118

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Mirror upstream elastic/elasticsearch#134454 for AI review (snapshot of HEAD tree) #118

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion ...itlement/src/main/java/org/elasticsearch/entitlement/bootstrap/HardcodedEntitlements.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,13 @@ private static List<Scope> createServerEntitlements(Path pidFile) {
new FilesEntitlement(serverModuleFileDatas)
)
),
new Scope("java.desktop", List.of(new LoadNativeLibrariesEntitlement())),
new Scope(
"java.desktop",
List.of(
new LoadNativeLibrariesEntitlement(),
new ManageThreadsEntitlement() // For sun.java2d.Disposer. TODO: https://elasticco.atlassian.net/browse/ES-12888
)
),
new Scope(
"java.xml",
List.of(
Expand Down
4 changes: 2 additions & 2 deletions ...entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ public class PolicyManager {
*/
static final Logger generalLogger = LogManager.getLogger(PolicyManager.class);

static final Set<String> MODULES_EXCLUDED_FROM_SYSTEM_MODULES = Set.of("java.desktop", "java.xml");
public static final Set<String> MODULES_EXCLUDED_FROM_SYSTEM_MODULES = Set.of("java.desktop", "java.xml");

/**
* Identifies a particular entitlement {@link Scope} within a {@link Policy}.
Expand Down Expand Up @@ -94,7 +94,7 @@ public enum ComponentKind {
* If this kind corresponds to a single component, this is that component's name;
* otherwise null.
*/
final String componentName;
public final String componentName;

ComponentKind(String componentName) {
this.componentName = componentName;
Expand Down
33 changes: 33 additions & 0 deletions ...ent/src/test/java/org/elasticsearch/entitlement/bootstrap/HardcodedEntitlementsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

package org.elasticsearch.entitlement.bootstrap;

import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.test.ESTestCase.WithEntitlementsOnTestCode;

import java.io.ByteArrayInputStream;

import javax.imageio.stream.MemoryCacheImageInputStream;

import static java.nio.charset.StandardCharsets.UTF_8;

@WithEntitlementsOnTestCode
public class HardcodedEntitlementsTests extends ESTestCase {

/**
* The Tika library can do some things we don't ordinarily want to allow.
* <p>
* Note that {@link MemoryCacheImageInputStream} doesn't even use {@code Disposer} in JDK 26,
* so it's an open question how much effort this deserves.
*/
public void testTikaPDF() {
new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8)));
}
Comment on lines +30 to +32
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Close the stream (try-with-resources) to avoid resource leaks

Also consider a minimal assertion to exercise the instance.

Apply:

-    public void testTikaPDF() {
-        new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8)));
-    }
+    public void testTikaPDF() throws Exception {
+        try (var in = new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8)))) {
+            assertNotNull(in);
+        }
+    }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
public void testTikaPDF() {
new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8)));
}
public void testTikaPDF() throws Exception {
try (var in = new MemoryCacheImageInputStream(new ByteArrayInputStream("test test".getBytes(UTF_8)))) {
assertNotNull(in);
}
}
🤖 Prompt for AI Agents 
In
libs/entitlement/src/test/java/org/elasticsearch/entitlement/bootstrap/HardcodedEntitlementsTests.java
around lines 30-32, the MemoryCacheImageInputStream is created without being
closed and there is no assertion; change the test to open the
ByteArrayInputStream and MemoryCacheImageInputStream in a try-with-resources
block to ensure both are closed, read or call a simple method on the
MemoryCacheImageInputStream (e.g., read() or available()) to exercise the
instance, and add a minimal assertion (e.g., assertThat(value,
is(expectedNonNegativeOrNotMinusOne))) to validate the read/available result.

}
62 changes: 52 additions & 10 deletions test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,14 @@

package org.elasticsearch.bootstrap;

import org.elasticsearch.core.Nullable;
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.entitlement.runtime.policy.PolicyManager;
import org.elasticsearch.entitlement.runtime.policy.PolicyManager.PolicyScope;
import org.elasticsearch.logging.LogManager;
import org.elasticsearch.logging.Logger;

import java.lang.module.ModuleDescriptor;
import java.lang.module.ModuleFinder;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.List;
Expand All @@ -22,39 +25,78 @@
import java.util.TreeMap;
import java.util.function.Function;

import static java.util.Objects.requireNonNull;
import static java.util.stream.Collectors.toSet;
import static org.elasticsearch.entitlement.runtime.policy.PolicyManager.ALL_UNNAMED;
import static org.elasticsearch.entitlement.runtime.policy.PolicyManager.ComponentKind.PLUGIN;
import static org.elasticsearch.entitlement.runtime.policy.PolicyManager.ComponentKind.SERVER;
import static org.elasticsearch.entitlement.runtime.policy.PolicyManager.MODULES_EXCLUDED_FROM_SYSTEM_MODULES;

public record TestScopeResolver(Map<String, PolicyManager.PolicyScope> scopeMap) {
public final class TestScopeResolver {

private static final Logger logger = LogManager.getLogger(TestScopeResolver.class);
private final Map<String, PolicyScope> scopeMap;
private static final Map<String, PolicyScope> excludedSystemPackageScopes = computeExcludedSystemPackageScopes();

PolicyManager.PolicyScope getScope(Class<?> callerClass) {
public TestScopeResolver(Map<String, PolicyScope> scopeMap) {
this.scopeMap = scopeMap;
}

private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() {
// Within any one module layer, module names are unique, so we just need the names
Set<String> systemModuleNames = ModuleFinder.ofSystem()
.findAll()
.stream()
.map(ref -> ref.descriptor().name())
.filter(MODULES_EXCLUDED_FROM_SYSTEM_MODULES::contains)
.collect(toSet());

Map<String, PolicyScope> result = new TreeMap<>();
ModuleLayer.boot().modules().stream().filter(m -> systemModuleNames.contains(m.getName())).forEach(m -> {
ModuleDescriptor desc = m.getDescriptor();
if (desc != null) {
desc.packages().forEach(pkg ->
// Our component identification logic returns SERVER for these
result.put(pkg, new PolicyScope(SERVER, SERVER.componentName, m.getName())));
}
});
return result;
}
Comment on lines +45 to +64
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Return an unmodifiable map to prevent accidental mutation

Compute once, then freeze the result to avoid accidental writes.

Apply:

-    private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() {
+    private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() {
         ...
-        return result;
+        return Map.copyOf(result);
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() {
// Within any one module layer, module names are unique, so we just need the names
Set<String> systemModuleNames = ModuleFinder.ofSystem()
.findAll()
.stream()
.map(ref -> ref.descriptor().name())
.filter(MODULES_EXCLUDED_FROM_SYSTEM_MODULES::contains)
.collect(toSet());
Map<String, PolicyScope> result = new TreeMap<>();
ModuleLayer.boot().modules().stream().filter(m -> systemModuleNames.contains(m.getName())).forEach(m -> {
ModuleDescriptor desc = m.getDescriptor();
if (desc != null) {
desc.packages().forEach(pkg ->
// Our component identification logic returns SERVER for these
result.put(pkg, new PolicyScope(SERVER, SERVER.componentName, m.getName())));
}
});
return result;
}
private static Map<String, PolicyScope> computeExcludedSystemPackageScopes() {
// Within any one module layer, module names are unique, so we just need the names
Set<String> systemModuleNames = ModuleFinder.ofSystem()
.findAll()
.stream()
.map(ref -> ref.descriptor().name())
.filter(MODULES_EXCLUDED_FROM_SYSTEM_MODULES::contains)
.collect(toSet());
Map<String, PolicyScope> result = new TreeMap<>();
ModuleLayer.boot().modules().stream()
.filter(m -> systemModuleNames.contains(m.getName()))
.forEach(m -> {
ModuleDescriptor desc = m.getDescriptor();
if (desc != null) {
desc.packages().forEach(pkg ->
// Our component identification logic returns SERVER for these
result.put(pkg, new PolicyScope(SERVER, SERVER.componentName, m.getName()))
);
}
});
return Map.copyOf(result);
}
🤖 Prompt for AI Agents 
In
test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java
around lines 45-64, the method currently returns a mutable TreeMap; change it to
return an unmodifiable map to prevent accidental mutation by wrapping the result
before returning (e.g., Collections.unmodifiableMap(result) or
Map.copyOf(result)) so the computed map is frozen when returned.


public static @Nullable PolicyScope getExcludedSystemPackageScope(Class<?> callerClass) {
return excludedSystemPackageScopes.get(callerClass.getPackageName());
}

PolicyScope getScope(Class<?> callerClass) {
var callerCodeSource = callerClass.getProtectionDomain().getCodeSource();
assert callerCodeSource != null;
if (callerCodeSource == null) {
// This only happens for JDK classes. Furthermore, for trivially allowed modules, we shouldn't even get here.
// Hence, this must be an excluded system module, so check for that.
return requireNonNull(getExcludedSystemPackageScope(callerClass));
}
Comment on lines +70 to +76
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Avoid potential NPE on non-excluded JDK classes

Edge-case: if this is ever called for a JDK class not in the excluded set, requireNonNull will throw. Prefer a safe fallback.

Apply:

-            return requireNonNull(getExcludedSystemPackageScope(callerClass));
+            var scope = getExcludedSystemPackageScope(callerClass);
+            return scope != null ? scope : PolicyScope.unknown(callerClass.getModule().getName());

Alternatively, throw with a clear message:

-            return requireNonNull(getExcludedSystemPackageScope(callerClass));
+            return java.util.Objects.requireNonNull(
+                getExcludedSystemPackageScope(callerClass),
+                "Missing excluded-system package scope for " + callerClass.getName()
+            );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
PolicyScope getScope(Class<?> callerClass) {
var callerCodeSource = callerClass.getProtectionDomain().getCodeSource();
assert callerCodeSource != null;
if (callerCodeSource == null) {
// This only happens for JDK classes. Furthermore, for trivially allowed modules, we shouldn't even get here.
// Hence, this must be an excluded system module, so check for that.
return requireNonNull(getExcludedSystemPackageScope(callerClass));
}
PolicyScope getScope(Class<?> callerClass) {
var callerCodeSource = callerClass.getProtectionDomain().getCodeSource();
if (callerCodeSource == null) {
// This only happens for JDK classes. Furthermore, for trivially allowed modules, we shouldn't even get here.
// Hence, this must be an excluded system module, so check for that.
var scope = getExcludedSystemPackageScope(callerClass);
return scope != null
? scope
: PolicyScope.unknown(callerClass.getModule().getName());
}
…
}
🤖 Prompt for AI Agents 
In
test/framework/src/main/java/org/elasticsearch/bootstrap/TestScopeResolver.java
around lines 70 to 76, avoid using
requireNonNull(getExcludedSystemPackageScope(callerClass)) because if a JDK
class that is not in the excluded set reaches this point it will cause an NPE;
instead capture the result into a local variable, check it for null, and if null
throw an IllegalStateException with a clear message that includes the
callerClass name (or alternatively return an explicit safe default PolicyScope
if that is more appropriate for your design).


var location = callerCodeSource.getLocation().toString();
var scope = scopeMap.get(location);
if (scope == null) {
// Special cases for libraries not handled by our automatically-generated scopeMap
if (callerClass.getPackageName().startsWith("org.bouncycastle")) {
scope = new PolicyManager.PolicyScope(PLUGIN, "security", ALL_UNNAMED);
scope = new PolicyScope(PLUGIN, "security", ALL_UNNAMED);
logger.debug("Assuming bouncycastle is part of the security plugin");
}
}
if (scope == null) {
logger.warn("Cannot identify a scope for class [{}], location [{}]", callerClass.getName(), location);
return PolicyManager.PolicyScope.unknown(location);
return PolicyScope.unknown(location);
}
return scope;
}

public static Function<Class<?>, PolicyManager.PolicyScope> createScopeResolver(
public static Function<Class<?>, PolicyScope> createScopeResolver(
TestBuildInfo serverBuildInfo,
List<TestBuildInfo> pluginsBuildInfo,
Set<String> modularPlugins
) {
Map<String, PolicyManager.PolicyScope> scopeMap = new TreeMap<>(); // Sorted to make it easier to read during debugging
Map<String, PolicyScope> scopeMap = new TreeMap<>(); // Sorted to make it easier to read during debugging
for (var pluginBuildInfo : pluginsBuildInfo) {
boolean isModular = modularPlugins.contains(pluginBuildInfo.component());
for (var location : pluginBuildInfo.locations()) {
Expand All @@ -66,7 +108,7 @@ public static Function<Class<?>, PolicyManager.PolicyScope> createScopeResolver(
String module = isModular ? location.module() : ALL_UNNAMED;
scopeMap.put(
getCodeSource(codeSource, location.representativeClass()),
PolicyManager.PolicyScope.plugin(pluginBuildInfo.component(), module)
PolicyScope.plugin(pluginBuildInfo.component(), module)
);
} catch (MalformedURLException e) {
throw new IllegalArgumentException("Cannot locate class [" + location.representativeClass() + "]", e);
Expand All @@ -81,7 +123,7 @@ public static Function<Class<?>, PolicyManager.PolicyScope> createScopeResolver(
continue;
}
try {
scopeMap.put(getCodeSource(classUrl, location.representativeClass()), PolicyManager.PolicyScope.server(location.module()));
scopeMap.put(getCodeSource(classUrl, location.representativeClass()), PolicyScope.server(location.module()));
} catch (MalformedURLException e) {
throw new IllegalArgumentException("Cannot locate class [" + location.representativeClass() + "]", e);
}
Expand Down
5 changes: 5 additions & 0 deletions ...amework/src/main/java/org/elasticsearch/entitlement/runtime/policy/TestPolicyManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

package org.elasticsearch.entitlement.runtime.policy;

import org.elasticsearch.bootstrap.TestScopeResolver;
import org.elasticsearch.common.util.ArrayUtils;
import org.elasticsearch.entitlement.runtime.policy.entitlements.Entitlement;
import org.elasticsearch.test.ESTestCase;
Expand Down Expand Up @@ -97,6 +98,10 @@ public final void clearModuleEntitlementsCache() {

@Override
protected boolean isTrustedSystemClass(Class<?> requestingClass) {
if (TestScopeResolver.getExcludedSystemPackageScope(requestingClass) != null) {
// We don't trust the excluded packages even though they are in system modules
return false;
}
ClassLoader loader = requestingClass.getClassLoader();
return loader == null || loader == ClassLoader.getPlatformClassLoader();
}
Expand Down