diff --git a/CHANGELOG.md b/CHANGELOG.md index 5309dd0d66b69..17f10c5a89cea 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), - Update API of Message in index to add the timestamp for lag calculation in ingestion polling ([#17977](https://github.com/opensearch-project/OpenSearch/pull/17977/)) - Add composite directory factory ([#17988](https://github.com/opensearch-project/OpenSearch/pull/17988)) - Add pull-based ingestion error metrics and make internal queue size configurable ([#18088](https://github.com/opensearch-project/OpenSearch/pull/18088)) +- [Security Manager Replacement] Enhance Java Agent to intercept newByteChannel ([#17989](https://github.com/opensearch-project/OpenSearch/pull/17989)) - Enabled Async Shard Batch Fetch by default ([#18139](https://github.com/opensearch-project/OpenSearch/pull/18139)) - Allow to get the search request from the QueryCoordinatorContext ([#17818](https://github.com/opensearch-project/OpenSearch/pull/17818)) diff --git a/buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java b/buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java index 55f71753a2e37..a82d49e01ced4 100644 --- a/buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java +++ b/buildSrc/src/main/java/org/opensearch/gradle/OpenSearchTestBasePlugin.java @@ -166,7 +166,6 @@ public void execute(Task t) { "java.security.properties", project.getRootProject().getLayout().getProjectDirectory() + "/distribution/src/config/" + securityFile ); - // don't track these as inputs since they contain absolute paths and break cache relocatability File gradleHome = project.getGradle().getGradleUserHomeDir(); String gradleVersion = project.getGradle().getGradleVersion(); diff --git a/distribution/archives/integ-test-zip/src/test/resources/plugin-security.policy b/distribution/archives/integ-test-zip/src/test/resources/plugin-security.policy index bdfb6f32cb865..9c6575ebc563d 100644 --- a/distribution/archives/integ-test-zip/src/test/resources/plugin-security.policy +++ b/distribution/archives/integ-test-zip/src/test/resources/plugin-security.policy @@ -11,5 +11,5 @@ grant { // Needed to read the log file - permission java.io.FilePermission "${tests.logfile}", "read"; + permission java.io.FilePermission "${{tests.logfile}}", "read"; }; diff --git a/libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/policy/PolicyFile.java b/libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/policy/PolicyFile.java index a259aa667356d..d2a41d1df0f71 100644 --- a/libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/policy/PolicyFile.java +++ b/libs/agent-sm/agent-policy/src/main/java/org/opensearch/secure_sm/policy/PolicyFile.java @@ -137,7 +137,13 @@ private static PermissionEntry expandPermissionName(PermissionEntry pe) { while ((b = pe.name().indexOf("${{", startIndex)) != -1 && (e = pe.name().indexOf("}}", b)) != -1) { sb.append(pe.name(), startIndex, b); String value = pe.name().substring(b + 3, e); - sb.append("${{").append(value).append("}}"); + String propertyValue = System.getProperty(value); + if (propertyValue != null) { + sb.append(propertyValue); + } else { + // replacement not found + sb.append("${{").append(value).append("}}"); + } startIndex = e + 2; } diff --git a/libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/Agent.java b/libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/Agent.java index dc8fb61257e72..efca064d13342 100644 --- a/libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/Agent.java +++ b/libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/Agent.java @@ -15,6 +15,7 @@ import java.nio.channels.FileChannel; import java.nio.channels.SocketChannel; import java.nio.file.Files; +import java.nio.file.spi.FileSystemProvider; import java.util.Map; import net.bytebuddy.ByteBuddy; @@ -77,6 +78,7 @@ private static AgentBuilder createAgentBuilder() throws Exception { .or(ElementMatchers.isSubTypeOf(Socket.class)); final Junction pathType = ElementMatchers.isSubTypeOf(Files.class); final Junction fileChannelType = ElementMatchers.isSubTypeOf(FileChannel.class); + final Junction fileSystemProviderType = ElementMatchers.isSubTypeOf(FileSystemProvider.class); final AgentBuilder.Transformer socketTransformer = (b, typeDescription, classLoader, module, pd) -> b.visit( Advice.to(SocketChannelInterceptor.class) @@ -106,7 +108,7 @@ private static AgentBuilder createAgentBuilder() throws Exception { .ignore(ElementMatchers.nameContains("$MockitoMock$")) /* ingore all Mockito mocks */ .type(socketType) .transform(socketTransformer) - .type(pathType.or(fileChannelType)) + .type(pathType.or(fileChannelType).or(fileSystemProviderType)) .transform(fileTransformer) .type(ElementMatchers.is(java.lang.System.class)) .transform( diff --git a/libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/FileInterceptor.java b/libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/FileInterceptor.java index f17cfccd8d86f..f52f1bc91f559 100644 --- a/libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/FileInterceptor.java +++ b/libs/agent-sm/agent/src/main/java/org/opensearch/javaagent/FileInterceptor.java @@ -21,6 +21,7 @@ import java.security.Policy; import java.security.ProtectionDomain; import java.util.Collection; +import java.util.Set; import net.bytebuddy.asm.Advice; @@ -85,14 +86,33 @@ public static void intercept(@Advice.AllArguments Object[] args, @Advice.Origin String targetFilePath = null; if (isMutating == false && isDelete == false) { if (name.equals("newByteChannel") == true || name.equals("open") == true) { - if (args.length > 1 && args[1] instanceof OpenOption[] opts) { - for (final OpenOption opt : opts) { - if (opt != StandardOpenOption.READ) { - isMutating = true; - break; + if (args.length > 1 && args instanceof Object) { + if (args instanceof OpenOption[] opts) { + for (final OpenOption opt : opts) { + if (opt != StandardOpenOption.READ) { + isMutating = true; + break; + } } + } else if (args[1] instanceof Set opts) { + @SuppressWarnings("unchecked") + final Set options = (Set) args[1]; + for (final OpenOption opt : options) { + if (opt != StandardOpenOption.READ) { + isMutating = true; + break; + } + } + } else if (args[1] instanceof Object[] opts) { + for (final Object opt : opts) { + if (opt != StandardOpenOption.READ) { + isMutating = true; + break; + } + } + } else { + throw new SecurityException("Unsupported argument type: " + args[1].getClass().getName()); } - } } else if (name.equals("copy") == true) { if (args.length > 1 && args[1] instanceof String pathStr) { @@ -106,7 +126,7 @@ public static void intercept(@Advice.AllArguments Object[] args, @Advice.Origin // Check each permission separately for (final ProtectionDomain domain : callers) { // Handle FileChannel.open() separately to check read/write permissions properly - if (method.getName().equals("open")) { + if (method.getName().equals("open") || method.getName().equals("newByteChannel")) { if (isMutating == true && !policy.implies(domain, new FilePermission(filePath, "read,write"))) { throw new SecurityException("Denied OPEN (read/write) access to file: " + filePath + ", domain: " + domain); } else if (!policy.implies(domain, new FilePermission(filePath, "read"))) { diff --git a/libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorIntegTests.java b/libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorIntegTests.java index 537f1d1f38159..3f2290c9c2c37 100644 --- a/libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorIntegTests.java +++ b/libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorIntegTests.java @@ -15,6 +15,7 @@ import java.io.File; import java.io.FileInputStream; import java.io.FilePermission; +import java.io.OutputStream; import java.nio.ByteBuffer; import java.nio.channels.FileChannel; import java.nio.charset.StandardCharsets; @@ -247,4 +248,57 @@ public void testDelete() throws Exception { Files.deleteIfExists(tempPath); } } + + @Test + public void testReadAllLines() throws Exception { + Path tmpDir = getTestDir(); + Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt"); + + try { + // Create a file with some content + String content = "test content"; + Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8)); + + // Verify file exists before deletion + assertTrue("File should exist before deletion", Files.exists(tempPath)); + assertEquals("File should have correct content", content, Files.readString(tempPath, StandardCharsets.UTF_8)); + + // Test delete operation - FileInterceptor should intercept this + String line = Files.readAllLines(tempPath).getFirst(); + + // Verify readAllLines + assertEquals("File contents should be returned", "test content", line); + + } finally { + // Cleanup in case test fails + Files.deleteIfExists(tempPath); + } + } + + @Test + public void testNewOutputStream() throws Exception { + Path tmpDir = getTestDir(); + Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt"); + + try { + // Create an empty file + String content = ""; + Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8)); + + // Verify file exists before deletion + assertTrue("File should exist before deletion", Files.exists(tempPath)); + assertEquals("File should have correct content", content, Files.readString(tempPath, StandardCharsets.UTF_8)); + + // Test for new OutputStream + try (OutputStream os = Files.newOutputStream(tempPath)) { + os.write("test content".getBytes(StandardCharsets.UTF_8)); + } + String line = Files.readAllLines(tempPath).getFirst(); + assertEquals("File contents should be returned", "test content", line); + + } finally { + // Cleanup in case test fails + Files.deleteIfExists(tempPath); + } + } } diff --git a/libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorNegativeIntegTests.java b/libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorNegativeIntegTests.java new file mode 100644 index 0000000000000..1827afe260ac8 --- /dev/null +++ b/libs/agent-sm/agent/src/test/java/org/opensearch/javaagent/FileInterceptorNegativeIntegTests.java @@ -0,0 +1,157 @@ +/* + * SPDX-License-Identifier: Apache-2.0 + * + * The OpenSearch Contributors require contributions made to + * this file be licensed under the Apache-2.0 license or a + * compatible open source license. + */ + +package org.opensearch.javaagent; + +import org.opensearch.javaagent.bootstrap.AgentPolicy; +import org.junit.BeforeClass; +import org.junit.Test; + +import java.nio.channels.FileChannel; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.security.PermissionCollection; +import java.security.Permissions; +import java.security.Policy; +import java.security.ProtectionDomain; +import java.util.UUID; + +import static org.junit.Assert.assertThrows; +import static org.junit.Assert.assertTrue; + +@SuppressWarnings("removal") +public class FileInterceptorNegativeIntegTests { + private static Path getTestDir() { + Path baseDir = Path.of(System.getProperty("user.dir")); + Path integTestFiles = baseDir.resolve("integ-test-files").normalize(); + return integTestFiles; + } + + private String randomAlphaOfLength(int length) { + // Using UUID to generate random string and taking first 'length' characters + return UUID.randomUUID().toString().replaceAll("-", "").substring(0, length); + } + + @BeforeClass + public static void setUp() throws Exception { + Policy policy = new Policy() { + @Override + public PermissionCollection getPermissions(ProtectionDomain domain) { + Permissions permissions = new Permissions(); + return permissions; + } + }; + AgentPolicy.setPolicy(policy); + } + + @Test + public void testFileInputStream() { + Path tmpDir = getTestDir(); + + // Create a unique file name + String fileName = "test-" + randomAlphaOfLength(8) + ".txt"; + Path tempPath = tmpDir.resolve(fileName); + + // Verify error when writing Content + String content = "test content"; + SecurityException exception = assertThrows("", SecurityException.class, () -> { + Files.write(tempPath, content.getBytes(StandardCharsets.UTF_8)); + }); + assertTrue(exception.getMessage().contains("Denied WRITE access to file")); + } + + @Test + public void testOpenForReadAndWrite() { + Path tmpDir = getTestDir(); + + // Create a unique file name + String fileName = "test-" + randomAlphaOfLength(8) + ".txt"; + Path tempPath = tmpDir.resolve(fileName); + + // Verify error when opening file + SecurityException exception = assertThrows("", SecurityException.class, () -> { + FileChannel.open(tempPath, StandardOpenOption.CREATE, StandardOpenOption.READ, StandardOpenOption.WRITE); + }); + assertTrue(exception.getMessage().contains("Denied OPEN (read/write) access to file")); + } + + @Test + public void testCopy() { + Path tmpDir = getTestDir(); + Path sourcePath = tmpDir.resolve("test-source-" + randomAlphaOfLength(8) + ".txt"); + Path targetPath = tmpDir.resolve("test-target-" + randomAlphaOfLength(8) + ".txt"); + + // Verify Error when copying file + SecurityException exception = assertThrows(SecurityException.class, () -> Files.copy(sourcePath, targetPath)); + assertTrue(exception.getMessage().contains("Denied COPY (read) access to file")); + } + + @Test + public void testCreateFile() { + Path tmpDir = getTestDir(); + Path tempPath = tmpDir.resolve("test-create-" + randomAlphaOfLength(8) + ".txt"); + + // Verify Error when creating file + SecurityException exception = assertThrows(SecurityException.class, () -> Files.createFile(tempPath)); + assertTrue(exception.getMessage().contains("Denied WRITE access to file")); + } + + @Test + public void testMove() { + Path tmpDir = getTestDir(); + Path sourcePath = tmpDir.resolve("test-source-" + randomAlphaOfLength(8) + ".txt"); + Path targetPath = tmpDir.resolve("test-target-" + randomAlphaOfLength(8) + ".txt"); + + // Verify Error when moving file + SecurityException exception = assertThrows(SecurityException.class, () -> Files.move(sourcePath, targetPath)); + assertTrue(exception.getMessage().contains("Denied WRITE access to file")); + } + + @Test + public void testCreateLink() throws Exception { + Path tmpDir = getTestDir(); + Path originalPath = tmpDir.resolve("test-original-" + randomAlphaOfLength(8) + ".txt"); + Path linkPath = tmpDir.resolve("test-link-" + randomAlphaOfLength(8) + ".txt"); + + // Verify Error when creating link + SecurityException exception = assertThrows(SecurityException.class, () -> Files.createLink(linkPath, originalPath)); + assertTrue(exception.getMessage().contains("Denied WRITE access to file")); + } + + @Test + public void testDelete() throws Exception { + Path tmpDir = getTestDir(); + Path tempPath = tmpDir.resolve("test-delete-" + randomAlphaOfLength(8) + ".txt"); + + // Verify Error when deleting file + SecurityException exception = assertThrows(SecurityException.class, () -> Files.delete(tempPath)); + assertTrue(exception.getMessage().contains("Denied DELETE access to file")); + } + + @Test + public void testReadAllLines() throws Exception { + Path tmpDir = getTestDir(); + Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt"); + + // Verify Error when reading file + SecurityException exception = assertThrows(SecurityException.class, () -> Files.readAllLines(tempPath)); + assertTrue(exception.getMessage().contains("Denied OPEN (read) access to file")); + } + + @Test + public void testNewOutputStream() throws Exception { + Path tmpDir = getTestDir(); + Path tempPath = tmpDir.resolve("test-readAllLines-" + randomAlphaOfLength(8) + ".txt"); + + // Verify error when calling newOutputStream + SecurityException exception = assertThrows(SecurityException.class, () -> Files.newOutputStream(tempPath)); + assertTrue(exception.getMessage().contains("Denied OPEN (read/write) access to file")); + } +} diff --git a/qa/logging-config/src/test/resources/plugin-security.policy b/qa/logging-config/src/test/resources/plugin-security.policy index bdfb6f32cb865..9c6575ebc563d 100644 --- a/qa/logging-config/src/test/resources/plugin-security.policy +++ b/qa/logging-config/src/test/resources/plugin-security.policy @@ -11,5 +11,5 @@ grant { // Needed to read the log file - permission java.io.FilePermission "${tests.logfile}", "read"; + permission java.io.FilePermission "${{tests.logfile}}", "read"; }; diff --git a/qa/unconfigured-node-name/src/test/resources/plugin-security.policy b/qa/unconfigured-node-name/src/test/resources/plugin-security.policy index bdfb6f32cb865..9c6575ebc563d 100644 --- a/qa/unconfigured-node-name/src/test/resources/plugin-security.policy +++ b/qa/unconfigured-node-name/src/test/resources/plugin-security.policy @@ -11,5 +11,5 @@ grant { // Needed to read the log file - permission java.io.FilePermission "${tests.logfile}", "read"; + permission java.io.FilePermission "${{tests.logfile}}", "read"; };