Copilot commented on code in PR #4471: URL: https://github.com/apache/solr/pull/4471#discussion_r3316382622
########## solr/agent-sm/src/java/org/apache/solr/security/agent/FileInterceptor.java: ########## @@ -0,0 +1,277 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.security.agent; + +import java.lang.reflect.Method; +import java.nio.file.LinkOption; +import java.nio.file.OpenOption; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.nio.file.spi.FileSystemProvider; +import java.util.Collection; +import java.util.Locale; +import java.util.Set; +import net.bytebuddy.asm.Advice; + +/** + * ByteBuddy {@link Advice} interceptor for file-system operations. + * + * <p>This file was derived from the OpenSearch project and modified. See {@code NOTICE.txt} for + * attribution. + */ +public class FileInterceptor { + + /** FileInterceptor */ + public FileInterceptor() {} + + /** + * Intercepts file operations. + * + * @param args arguments + * @param method method + * @throws Exception exceptions + */ + @Advice.OnMethodEnter + public static void intercept(@Advice.AllArguments Object[] args, @Advice.Origin Method method) + throws Exception { + if (!AgentPolicy.isInitialized()) return; + final AgentPolicy policy = AgentPolicy.getInstance(); + + FileSystemProvider provider = null; + String filePath = null; + if (args.length > 0 && args[0] instanceof String pathStr) { + filePath = Path.of(pathStr).toAbsolutePath().toString(); + } else if (args.length > 0 && args[0] instanceof Path path) { + filePath = path.toAbsolutePath().toString(); + provider = path.getFileSystem().provider(); + } Review Comment: The `intercept` advice resolves the file path with `Path.of(pathStr).toAbsolutePath().toString()` (and similarly `path.toAbsolutePath().toString()` on line 59), but does **not** call `normalize()`. Because `PermittedPath.permits()` checks `resolvedPath.startsWith(base)` purely on the string form, a caller can pass `Path.of("/var/solr/../etc/passwd")` and the path string will still start with `/var/solr/`, granting access — while the JDK I/O that follows actually opens `/etc/passwd`. This is a policy-bypass via `..` traversal. The advice intentionally avoids `toRealPath()` because of re-entrant interception, but `Path.normalize()` is a pure string operation and does not perform any I/O, so it is safe to call here. The same fix should be applied for the `targetFilePath` computed for `copy()` (lines 128/130). ########## solr/agent-sm/src/java/org/apache/solr/security/agent/FileInterceptor.java: ########## @@ -0,0 +1,277 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.security.agent; + +import java.lang.reflect.Method; +import java.nio.file.LinkOption; +import java.nio.file.OpenOption; +import java.nio.file.Path; +import java.nio.file.StandardOpenOption; +import java.nio.file.spi.FileSystemProvider; +import java.util.Collection; +import java.util.Locale; +import java.util.Set; +import net.bytebuddy.asm.Advice; + +/** + * ByteBuddy {@link Advice} interceptor for file-system operations. + * + * <p>This file was derived from the OpenSearch project and modified. See {@code NOTICE.txt} for + * attribution. + */ +public class FileInterceptor { + + /** FileInterceptor */ + public FileInterceptor() {} + + /** + * Intercepts file operations. + * + * @param args arguments + * @param method method + * @throws Exception exceptions + */ + @Advice.OnMethodEnter + public static void intercept(@Advice.AllArguments Object[] args, @Advice.Origin Method method) + throws Exception { + if (!AgentPolicy.isInitialized()) return; + final AgentPolicy policy = AgentPolicy.getInstance(); + + FileSystemProvider provider = null; + String filePath = null; + if (args.length > 0 && args[0] instanceof String pathStr) { + filePath = Path.of(pathStr).toAbsolutePath().toString(); + } else if (args.length > 0 && args[0] instanceof Path path) { + filePath = path.toAbsolutePath().toString(); + provider = path.getFileSystem().provider(); + } + + if (filePath == null) { + return; // No valid file path found + } + + if (provider != null && policy.trustedFileSystems().contains(provider.getScheme())) { + return; + } + + final StackWalker walker = StackWalker.getInstance(StackWalker.Option.RETAIN_CLASS_REFERENCE); + final String caller = walker.getCallerClass().getName(); + + final String name = method.getName(); + boolean isMutating = name.equals("move") || name.equals("write") || name.startsWith("create"); + final boolean isDelete = isMutating == false ? name.startsWith("delete") : false; + + // This is Windows implementation of UNIX Domain Sockets (close) + boolean isUnixSocketCaller = false; + if (isDelete == true) { + final Collection<Class<?>> chain = walker.walk(StackCallerClassChainExtractor.INSTANCE); + for (final Class<?> cls : chain) { + if (cls.getName().equalsIgnoreCase("sun.nio.ch.PipeImpl$Initializer$LoopbackConnector")) { + isUnixSocketCaller = true; + break; + } + } + } + + if (isDelete == true && isUnixSocketCaller == true) { + // Unix domain socket cleanup — local IPC, always allow + return; + } else { + String targetFilePath = null; + if (isMutating == false && isDelete == false) { + if (name.equals("newByteChannel") == true || name.equals("open") == true) { + if (args.length > 1) { + if (args[1] 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<OpenOption> options = (Set<OpenOption>) 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) { + targetFilePath = Path.of(pathStr).toAbsolutePath().toString(); + } else if (args.length > 1 && args[1] instanceof Path path) { + targetFilePath = path.toAbsolutePath().toString(); + } + } + } + + // Handle FileChannel.open() and newByteChannel() — check read/write permissions + if (method.getName().equals("open") || method.getName().equals("newByteChannel")) { + final String action = isMutating ? "write" : "read"; + if (!policy.isPathPermitted(filePath, action)) { + ViolationMetricsReporter.incrementFile(); + SecurityViolationLogger.log( + isMutating + ? SecurityViolationLogger.ViolationType.FILE_WRITE + : SecurityViolationLogger.ViolationType.FILE_READ, + filePath, + caller, + policy.enforcementMode()); + if (policy.enforcementMode() == AgentPolicy.EnforcementMode.ENFORCE) { + throw new SecurityException( + "Denied " + + (isMutating ? "OPEN (read/write)" : "OPEN (read)") + + " access to file: " + + filePath); + } + } + } + + // Handle Files.copy() — check source read and target write permissions + if (method.getName().equals("copy")) { + if (!policy.isPathPermitted(filePath, "read")) { + ViolationMetricsReporter.incrementFile(); + SecurityViolationLogger.log( + SecurityViolationLogger.ViolationType.FILE_READ, + filePath, + caller, + policy.enforcementMode()); + if (policy.enforcementMode() == AgentPolicy.EnforcementMode.ENFORCE) { + throw new SecurityException("Denied COPY (read) access to file: " + filePath); + } + } + if (targetFilePath != null && !policy.isPathPermitted(targetFilePath, "write")) { + ViolationMetricsReporter.incrementFile(); + SecurityViolationLogger.log( + SecurityViolationLogger.ViolationType.FILE_WRITE, + targetFilePath, + caller, + policy.enforcementMode()); + if (policy.enforcementMode() == AgentPolicy.EnforcementMode.ENFORCE) { + throw new SecurityException("Denied COPY (write) access to file: " + targetFilePath); + } + } + } + + // Plain read operations (e.g. Files.read(), Files.readAllBytes()) + if (name.equals("read") && !policy.isPathPermitted(filePath, "read")) { + ViolationMetricsReporter.incrementFile(); + SecurityViolationLogger.log( + SecurityViolationLogger.ViolationType.FILE_READ, + filePath, + caller, + policy.enforcementMode()); + if (policy.enforcementMode() == AgentPolicy.EnforcementMode.ENFORCE) { + throw new SecurityException("Denied READ access to file: " + filePath); + } + } + Review Comment: The `agent-security.policy` description in this PR claims it intercepts `Files.read`, but the advice on line 184 keys off `name.equals("read")`, while there is no `java.nio.file.Files.read` method — the high-level read APIs are `readAllBytes`, `readString`, `readAllLines`, `newInputStream`, etc., none of which start with `read`. The actual read path goes through `FileSystemProvider.newByteChannel`/`newInputStream` (already intercepted via `open`/`newByteChannel`), so the `read` branch here appears to be dead code in practice. If it's only intended for `FileSystemProvider.read*`, the matcher in `SolrAgentEntryPoint.FILE_INTERCEPTED_METHODS` should be reviewed — currently the only `read*` candidate is `FileSystemProvider.readAttributes`/`readSymbolicLink`, which is probably not what's intended. Either remove the dead branch or add a comment explaining which JDK method(s) it targets. ########## solr/agent-sm/src/java/org/apache/solr/security/agent/PermittedPath.java: ########## @@ -0,0 +1,77 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.security.agent; + +import java.nio.file.FileSystems; +import java.util.Locale; + +/** + * A single file-system access rule from the security policy. + * + * <p>Rules are loaded at startup and are immutable thereafter. A rule grants access to a path (and + * optionally all descendants) for the set of operations encoded in the {@code actions} string + * ({@code "read"}, {@code "write"}, {@code "delete"}, or any comma-separated combination). + */ +public final class PermittedPath { + + private final String path; + private final String actions; + private final boolean recursive; + private final PolicySource source; + + PermittedPath(String path, String actions, boolean recursive, PolicySource source) { + this.path = path; + this.actions = actions != null ? actions.toLowerCase(Locale.ROOT) : "read"; + this.recursive = recursive; + this.source = source; + } + + /** The base path after variable substitution. */ + public String path() { + return path; + } + + /** Comma-separated actions string, lower-cased (e.g. {@code "read,write,delete"}). */ + public String actions() { + return actions; + } + + /** Whether the rule covers all descendants ({@code path/-} in policy syntax). */ + public boolean recursive() { + return recursive; + } + + /** Whether this rule came from the default bundled policy or an operator extension. */ + public PolicySource source() { + return source; + } + + /** Returns {@code true} if this rule permits the given action on the given resolved path. */ + public boolean permits(String resolvedPath, String action) { + boolean pathMatch; + if (recursive) { + // Use the platform separator so the check is correct on both POSIX and Windows. + // Strip any trailing separator from 'path' to avoid double-separator issues. + String sep = FileSystems.getDefault().getSeparator(); + String base = path.endsWith(sep) ? path : path + sep; + pathMatch = resolvedPath.startsWith(base) || resolvedPath.equals(path); + } else { + pathMatch = resolvedPath.equals(path); + } + return pathMatch && actions.contains(action.toLowerCase(Locale.ROOT)); Review Comment: `PermittedPath.permits()` uses the platform separator (`FileSystems.getDefault().getSeparator()`) to compose the recursive base. However, paths stored in the policy come from variable expansion of system properties like `${solr.solr.home}`, which on Windows may contain forward slashes (e.g. `C:/solr/home`) — while the resolved path from `Path.toAbsolutePath().toString()` on Windows uses backslashes. The `startsWith(base)` check will then incorrectly fail. Consider normalizing both paths to use the same separator (or use `Path` objects and `Path.startsWith(Path)`) so Windows policy entries work regardless of the slash style they were written with. ########## solr/agent-sm/src/test/org/apache/solr/security/agent/PolicyLoaderOperatorExtensionTest.java: ########## @@ -0,0 +1,214 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.security.agent; + +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.List; +import org.apache.solr.SolrTestCase; +import org.junit.After; +import org.junit.Test; + +/** + * Tests for the operator extension policy file ({@code agent-security-extra.policy}). + * + * <p>Verifies that: + * + * <ul> + * <li>An extra policy file present at the configured path is loaded and merged with the default + * policy. + * <li>Entries from the extra policy are tagged {@link PolicySource#OPERATOR}. + * <li>Paths listed only in the extra policy are permitted; unlisted paths remain blocked. + * <li>When the extra policy file is absent the default policy still loads normally. + * <li>A malformed extra policy causes {@link IllegalStateException} with a descriptive message. + * <li>The {@code source=OPERATOR} tag is emitted in violation log entries for paths matched by + * operator-policy entries. + * </ul> + */ +public class PolicyLoaderOperatorExtensionTest extends SolrTestCase { + + @After + public void clearExtraPolicy() { + System.clearProperty("solr.security.agent.extra.policy"); + } + + // --------------------------------------------------------------------------- + // Helpers + // --------------------------------------------------------------------------- + + private Path writeDefaultPolicy(Path dir) throws Exception { + Path policy = dir.resolve("agent-security.policy"); + Files.writeString( + policy, + "grant {\n" + " permission java.io.FilePermission \"/opt/solr/-\", \"read\";\n" + "};\n", + StandardCharsets.UTF_8); + return policy; + } + + private AgentPolicy loadWithExtra(Path defaultPolicy, Path extraPolicy) { + if (extraPolicy != null) { + System.setProperty("solr.security.agent.extra.policy", extraPolicy.toString()); + } + return new PolicyLoader().load(defaultPolicy); + } + + // --------------------------------------------------------------------------- + // Extra policy present — entries merged and tagged OPERATOR + // --------------------------------------------------------------------------- + + @Test + public void testExtraPolicyPathIsPermitted() throws Exception { + Path tmpDir = createTempDir(); + Path defaultPolicy = writeDefaultPolicy(tmpDir); + + Path extraPolicy = tmpDir.resolve("agent-security-extra.policy"); + Files.writeString( + extraPolicy, + "grant {\n" + + " permission java.io.FilePermission \"" + + tmpDir + + "/-\", \"read\";\n" + + "};\n", + StandardCharsets.UTF_8); + + AgentPolicy policy = loadWithExtra(defaultPolicy, extraPolicy); + assertTrue(policy.isPathPermitted(tmpDir.resolve("data.txt").toString(), "read")); + } + + @Test + public void testUnlistedPathStillBlockedWhenExtraPolicyPresent() throws Exception { + Path tmpDir = createTempDir(); + Path defaultPolicy = writeDefaultPolicy(tmpDir); + + Path extraPolicy = tmpDir.resolve("agent-security-extra.policy"); + Files.writeString( + extraPolicy, + "grant {\n" + + " permission java.io.FilePermission \"" + + tmpDir + + "/-\", \"read\";\n" + + "};\n", + StandardCharsets.UTF_8); + + AgentPolicy policy = loadWithExtra(defaultPolicy, extraPolicy); + // /etc is not in either policy + assertFalse(policy.isPathPermitted("/etc/shadow", "read")); + } + + @Test + public void testExtraPolicyEntriesTaggedOperator() throws Exception { + Path tmpDir = createTempDir(); + Path defaultPolicy = writeDefaultPolicy(tmpDir); + + Path extraPolicy = tmpDir.resolve("agent-security-extra.policy"); + Files.writeString( + extraPolicy, + "grant {\n" + + " permission java.io.FilePermission \"" + + tmpDir + + "/-\", \"read\";\n" + + "};\n", + StandardCharsets.UTF_8); + + AgentPolicy policy = loadWithExtra(defaultPolicy, extraPolicy); + List<PermittedPath> paths = policy.permittedPaths(); + boolean hasOperator = paths.stream().anyMatch(p -> p.source() == PolicySource.OPERATOR); + assertTrue("Expected at least one OPERATOR-sourced path entry", hasOperator); + } + + @Test + public void testDefaultPolicyEntriesTaggedDefault() throws Exception { + Path tmpDir = createTempDir(); + Path defaultPolicy = writeDefaultPolicy(tmpDir); + + AgentPolicy policy = loadWithExtra(defaultPolicy, null); + List<PermittedPath> paths = policy.permittedPaths(); + boolean hasDefault = paths.stream().anyMatch(p -> p.source() == PolicySource.DEFAULT); + assertTrue("Expected at least one DEFAULT-sourced path entry", hasDefault); + } + + // --------------------------------------------------------------------------- + // Extra policy absent — default still loads + // --------------------------------------------------------------------------- + + @Test + public void testExtraPolicyAbsentIsNonFatal() throws Exception { + Path tmpDir = createTempDir(); + Path defaultPolicy = writeDefaultPolicy(tmpDir); + + // Point to a non-existent extra policy + System.setProperty( + "solr.security.agent.extra.policy", tmpDir.resolve("nonexistent.policy").toString()); + + // Should not throw; default policy still loads + AgentPolicy policy = new PolicyLoader().load(defaultPolicy); + assertTrue(policy.isPathPermitted("/opt/solr/conf", "read")); + } + + // --------------------------------------------------------------------------- + // Malformed extra policy — lenient parsing, default still enforced + // --------------------------------------------------------------------------- + + @Test + public void testMalformedExtraPolicyIsSkippedGracefully() throws Exception { + Path tmpDir = createTempDir(); + Path defaultPolicy = writeDefaultPolicy(tmpDir); + + Path extraPolicy = tmpDir.resolve("agent-security-extra.policy"); + // Content with no recognizable grant blocks — parser silently produces empty result + Files.writeString(extraPolicy, "THIS IS NOT A VALID POLICY\n", StandardCharsets.UTF_8); + + System.setProperty("solr.security.agent.extra.policy", extraPolicy.toString()); + + // Should not throw; default policy still loads + AgentPolicy policy = new PolicyLoader().load(defaultPolicy); + // Default policy (/opt/solr) is still active + assertTrue(policy.isPathPermitted("/opt/solr/conf", "read")); + // Malformed extra policy adds no new paths + assertFalse(policy.isPathPermitted(tmpDir.toString(), "read")); + } Review Comment: `testMalformedExtraPolicyIsSkippedGracefully` asserts that a file containing `THIS IS NOT A VALID POLICY` results in "no new paths added", but the class-level Javadoc on lines 38–39 explicitly states that "A malformed extra policy causes `IllegalStateException` with a descriptive message." The test and the documented contract contradict each other. Since the current behavior is to silently accept garbage (no grant blocks parsed → no entries), this is actually a security-relevant divergence: a typo in the operator policy that was supposed to grant a path would silently leave that path denied (or, with the wrong syntax, leave it unintentionally granted). Please reconcile: either update the Javadoc to reflect lenient behavior, or have `PolicyLoader.load` validate that operator-extension content actually parsed into at least one entry / produces an error on syntactically invalid input. ########## solr/agent-sm/src/java/org/apache/solr/security/agent/PolicyFileParser.java: ########## @@ -0,0 +1,169 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.security.agent; + +import java.io.IOException; +import java.io.Reader; +import java.io.StreamTokenizer; +import java.util.ArrayList; +import java.util.List; + +/** + * Tokenizer-based parser for JDK-style {@code .policy} files. + * + * <p>Uses {@link StreamTokenizer} which natively handles {@code //} line comments, {@code /* *\/} + * block comments, and quoted strings — no regex involved. + * + * <p>This file was derived from the OpenSearch project and modified. See {@code NOTICE.txt} for + * attribution. + */ +class PolicyFileParser { + + private PolicyFileParser() {} + + record GrantEntry(String codeBase, List<PermEntry> permissions) {} + + record PermEntry(String permission, String name, String action) {} + + static class ParsingException extends Exception { + ParsingException(String message) { + super(message); + } + + ParsingException(int line, String expected, String found) { + super("line " + line + ": expected [" + expected + "], found [" + found + "]"); + } + } + + static List<GrantEntry> read(Reader policy) throws ParsingException, IOException { + List<GrantEntry> entries = new ArrayList<>(); + PolicyTokenStream ts = new PolicyTokenStream(policy); + while (!ts.isEOF()) { + if (peek(ts, "grant")) { + entries.add(parseGrantEntry(ts)); + } else { + // skip unexpected top-level token + ts.consume(); + } + } + return entries; + } + + private static GrantEntry parseGrantEntry(PolicyTokenStream ts) + throws ParsingException, IOException { + String codeBase = null; + List<PermEntry> perms = new ArrayList<>(); + + poll(ts, "grant"); + + while (!peek(ts, "{")) { + if (pollOnMatch(ts, "codeBase")) { + String raw = poll(ts, ts.peek().text()); + codeBase = expand(ts, raw); + } else { + // Skip unknown grant modifiers + ts.consume(); + } + } + + poll(ts, "{"); + + while (!peek(ts, "}")) { + if (ts.isEOF()) break; + if (peek(ts, "permission")) { + perms.add(parsePermEntry(ts)); + pollOnMatch(ts, ";"); + } else { + ts.consume(); // skip unexpected token inside grant block + } + } + + poll(ts, "}"); + pollOnMatch(ts, ";"); + + return new GrantEntry(codeBase, List.copyOf(perms)); + } + + private static PermEntry parsePermEntry(PolicyTokenStream ts) + throws ParsingException, IOException { + poll(ts, "permission"); + String permClass = poll(ts, ts.peek().text()); + + String name = null; + if (isQuoted(ts.peek())) { + name = expand(ts, poll(ts, ts.peek().text())); + } + + String action = null; + pollOnMatch(ts, ","); + if (isQuoted(ts.peek())) { + action = expand(ts, poll(ts, ts.peek().text())); + } + + return new PermEntry(permClass, name, action); + } + + private static String expand(PolicyTokenStream ts, String raw) throws ParsingException { + try { + return PolicyPropertyExpander.expand(raw); + } catch (PolicyPropertyExpander.ExpandException e) { + try { + throw new ParsingException(ts.line(), e.getMessage(), raw); + } catch (IOException ioe) { + throw new ParsingException(e.getMessage()); + } Review Comment: In the catch for `PolicyPropertyExpander.ExpandException`, when constructing the `ParsingException` you call `ts.line()` which is declared `throws IOException`. The intent of the inner `try/catch (IOException)` block is unclear — if `ts.line()` fails you fall back to a `ParsingException` constructor that omits both the line number and the original `ExpandException`'s context. It would be cleaner (and lose less debugging information) to capture the line number ahead of the expand call, or to chain the original exception (`new ParsingException(...).initCause(e)`) so the unresolved-variable name is preserved in stack traces. ########## solr/agent-sm/src/java/org/apache/solr/security/agent/ViolationMetricsReporter.java: ########## @@ -0,0 +1,248 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.security.agent; + +import java.lang.reflect.Method; +import java.util.concurrent.atomic.LongAdder; +import java.util.function.Consumer; +import java.util.function.Supplier; + +/** + * Maintains per-type violation counters and registers them with Solr's metrics registry once it + * becomes available. + * + * <h2>Deferred registration pattern</h2> + * + * The agent starts (via {@code premain()}) before Solr's {@code SolrMetricManager} is initialized. + * Counters are maintained from the very first violation using {@link LongAdder}s. When {@code + * CoreContainer} initializes {@code SolrMetricManager}, it calls {@link + * #registerWithSolrMetrics(Object, String)} reflectively (to avoid a compile-time dependency on + * {@code solr:agent-sm} from {@code solr:core}). At that point the accumulated counts are already + * in the counters and the registration just wires them to the metrics registry. + * + * <h2>Metric names</h2> + * + * <ul> + * <li>{@code solr.security.agent.violations.file} + * <li>{@code solr.security.agent.violations.network} + * <li>{@code solr.security.agent.violations.exit} + * <li>{@code solr.security.agent.violations.exec} + * </ul> + * + * <p>In Prometheus format these appear as {@code solr_security_agent_violations_file_total}, etc. + */ +public final class ViolationMetricsReporter { + + // Per-type counters — incremented atomically from interceptor hot paths. + private static final LongAdder FILE_COUNTER = new LongAdder(); + private static final LongAdder NETWORK_COUNTER = new LongAdder(); + private static final LongAdder EXIT_COUNTER = new LongAdder(); + private static final LongAdder EXEC_COUNTER = new LongAdder(); + + // Metric names exposed in the Solr metrics registry. + public static final String METRIC_FILE = "solr.security.agent.violations.file"; + public static final String METRIC_NETWORK = "solr.security.agent.violations.network"; + public static final String METRIC_EXIT = "solr.security.agent.violations.exit"; + public static final String METRIC_EXEC = "solr.security.agent.violations.exec"; + + private ViolationMetricsReporter() {} + + // --------------------------------------------------------------------------- + // Counter increment API (called by interceptors) + // --------------------------------------------------------------------------- + + /** Increments the file-access violation counter. */ + public static void incrementFile() { + FILE_COUNTER.increment(); + } + + /** Increments the network-connection violation counter. */ + public static void incrementNetwork() { + NETWORK_COUNTER.increment(); + } + + /** Increments the System.exit() violation counter. */ + public static void incrementExit() { + EXIT_COUNTER.increment(); + } + + /** Increments the process-exec violation counter. */ + public static void incrementExec() { + EXEC_COUNTER.increment(); + } + + // --------------------------------------------------------------------------- + // Counter read API (used by tests) + // --------------------------------------------------------------------------- + + /** Returns the current file-access violation count. */ + public static long fileCount() { + return FILE_COUNTER.sum(); + } + + /** Returns the current network-connection violation count. */ + public static long networkCount() { + return NETWORK_COUNTER.sum(); + } + + /** Returns the current System.exit() violation count. */ + public static long exitCount() { + return EXIT_COUNTER.sum(); + } + + /** Returns the current process-exec violation count. */ + public static long execCount() { + return EXEC_COUNTER.sum(); + } + + // --------------------------------------------------------------------------- + // Deferred metrics registration (called reflectively from CoreContainer) + // --------------------------------------------------------------------------- + + /** + * Registers the four violation counters with the given {@code SolrMetricManager} in the specified + * registry. + * + * <p>This method is called reflectively from {@code CoreContainer} to avoid a compile-time + * dependency between {@code solr:core} and {@code solr:agent-sm}. The signature must match what + * CoreContainer expects: + * + * <pre>{@code + * Class.forName("org.apache.solr.security.agent.ViolationMetricsReporter", false, null) + * .getMethod("registerWithSolrMetrics", Object.class, String.class) + * .invoke(null, metricManager, "solr.node"); + * }</pre> + * + * <p>Because this module has no compile-time dependency on {@code solr:core}, the parameter type + * is declared as {@link Object}; the reflective call site in {@code CoreContainer} passes the + * real {@code SolrMetricManager} instance. + * + * <p>Metrics are registered as OTel observable counters via {@code + * SolrMetricManager.observableLongCounter()}. In Prometheus format they appear as {@code + * solr_security_agent_violations_file_total} etc. + * + * @param metricManager the {@code SolrMetricManager} instance (type-erased to {@link Object} to + * avoid a compile-time dependency on solr:core) + * @param registryName the target metrics registry name (e.g. {@code "solr.node"}) + */ + public static void registerWithSolrMetrics(Object metricManager, String registryName) { + try { + Class<?> mmClass = metricManager.getClass(); + // SolrMetricManager.observableLongCounter(String registry, String name, String description, + // Consumer<ObservableLongMeasurement> callback, OtelUnit unit) + // SolrMetricManager.observableLongCounter(String, String, String, Consumer, OtelUnit) + Method counterMethod = + findMethod( + mmClass, + "observableLongCounter", + "String", + "String", + "String", + "Consumer", + "OtelUnit"); + if (counterMethod == null) { + agentErr( + "[Solr SecurityAgent] SolrMetricManager.observableLongCounter not found" + + " — violation metrics will not be registered in /admin/metrics"); + return; + } + registerCounter( + counterMethod, + metricManager, + registryName, + METRIC_FILE, + "Security agent file-access violation count", + FILE_COUNTER::sum); + registerCounter( + counterMethod, + metricManager, + registryName, + METRIC_NETWORK, + "Security agent network-connection violation count", + NETWORK_COUNTER::sum); + registerCounter( + counterMethod, + metricManager, + registryName, + METRIC_EXIT, + "Security agent JVM-exit violation count", + EXIT_COUNTER::sum); + registerCounter( + counterMethod, + metricManager, + registryName, + METRIC_EXEC, + "Security agent process-exec violation count", + EXEC_COUNTER::sum); + } catch (Exception e) { + // Log to stderr — SLF4J may not be reachable from bootstrap context during premain. + agentErr("[Solr SecurityAgent] Failed to register violation metrics: " + e); + } + } Review Comment: The reference guide says metrics appear under `solr.security.agent.violations.file` (etc.), and the BATS integration test (line 66–67) asserts on the Prometheus form `solr_security_agent_violations_file`. However, `ViolationMetricsReporter.registerWithSolrMetrics` looks up `observableLongCounter` by simple-name matching including the parameter `"OtelUnit"`, and silently logs to stderr and returns if not found (line 157–162). If `SolrMetricManager.observableLongCounter`'s parameter list ever changes — even just a type rename — registration is silently skipped and the documented metric names will not appear at all. Consider failing loudly here (or at minimum logging at WARN), and add a unit test in `solr:core` against the real `SolrMetricManager` to catch any signature drift, since the reflective lookup is otherwise untested. ########## solr/agent-sm/src/java/org/apache/solr/security/agent/SolrAgentEntryPoint.java: ########## @@ -0,0 +1,219 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.solr.security.agent; + +import java.lang.instrument.Instrumentation; +import java.net.Socket; +import java.nio.channels.FileChannel; +import java.nio.channels.SocketChannel; +import java.nio.file.Path; +import java.nio.file.spi.FileSystemProvider; +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.agent.builder.AgentBuilder; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.implementation.Implementation; +import net.bytebuddy.matcher.ElementMatchers; + +/** + * Java agent entry point for the Solr security agent. + * + * <p>The JVM invokes {@link #premain(String, Instrumentation)} before the application main method + * when the agent JAR is listed on the command line as {@code -javaagent:solr-agent-sm-*.jar}. The + * {@link #agentmain(String, Instrumentation)} method supports attach-based loading (not used by + * Solr's startup scripts but retained for tooling compatibility). + * + * <h2>Startup sequence</h2> + * + * <ol> + * <li>Locate and parse {@code agent-security.policy} (and the optional {@code + * agent-security-extra.policy}) via {@link PolicyLoader}. + * <li>Initialize the {@link AgentPolicy} singleton. + * <li>Register all four ByteBuddy interceptors with the JVM instrumentation API. + * <li>If policy loading fails and enforcement mode is {@code ENFORCE}, halt the JVM; in {@code + * WARN} mode, log the error and continue without protection. + * </ol> + * + * <h2>Bootstrap classloader</h2> + * + * The agent JAR is placed on {@code Boot-Class-Path} so all interceptor and policy classes are + * visible to the bootstrap classloader. SLF4J is intentionally absent from the fat JAR to avoid + * poisoning the SLF4J binding before Log4j2 is loaded. + */ +public final class SolrAgentEntryPoint { + + private SolrAgentEntryPoint() {} + + /** + * Called by the JVM before the application main class is loaded, when the agent JAR is specified + * via {@code -javaagent:}. + */ + public static void premain(String agentArgs, Instrumentation inst) { + bootAgent(inst); + } + + /** Called by the JVM when the agent is attached dynamically; delegates to {@link #premain}. */ + public static void agentmain(String agentArgs, Instrumentation inst) { + premain(agentArgs, inst); + } + + // --------------------------------------------------------------------------- + // Internal bootstrap logic + // --------------------------------------------------------------------------- + + @SuppressForbidden( + reason = + "System.err is the only safe output during premain; System.exit(1) is required to halt " + + "the JVM on fatal policy-load failure in enforce mode.") + private static void bootAgent(Instrumentation inst) { + // Locate the default policy file next to the agent JAR. + Path defaultPolicyPath = resolveDefaultPolicyPath(); + + AgentPolicy policy = null; + try { + PolicyLoader loader = new PolicyLoader(); + policy = loader.load(defaultPolicyPath); + } catch (IllegalStateException e) { + String modeStr = PolicyPropertyExpander.getPropertyOrEnv("solr.security.agent.mode"); + if (modeStr == null) modeStr = "warn"; + if ("enforce".equalsIgnoreCase(modeStr.trim())) { + System.err.println( + "[Solr SecurityAgent] FATAL: Cannot load security policy in enforce mode. " + + "Solr will not start. Error: " + + e.getMessage()); + System.exit(1); + } else { + System.err.println( + "[Solr SecurityAgent] WARNING: Cannot load security policy (" + + e.getMessage() + + "). Security controls are inactive."); + return; + } + } + + AgentPolicy.initialize(policy); Review Comment: In `bootAgent`, the `policy` variable is declared `AgentPolicy policy = null;` and assigned inside a `try`. The `catch (IllegalStateException e)` block has two code paths: enforce mode calls `System.exit(1)` (terminal), warn mode logs and `return`s. If any *other* exception type is thrown by `loader.load()` (e.g. `RuntimeException` due to a parse bug), control falls through to `AgentPolicy.initialize(policy)` with `policy == null`, throwing NPE during premain. A NPE during premain is not user-friendly — consider either catching `Throwable` (with the same enforce/warn branching) or initializing `policy` from a safe empty default before the unconditional `initialize` call. ########## solr/core/src/java/org/apache/solr/core/CoreContainer.java: ########## @@ -951,6 +952,27 @@ private void loadInternal() { fieldCacheBean.initializeMetrics( solrMetricsContext, Attributes.of(CATEGORY_ATTR, SolrInfoBean.Category.CACHE.toString())); + // Register security agent violation metrics if the agent is loaded. + // Uses reflection to avoid a compile-time dependency on solr:agent-sm (see research.md Decision + // 8). + try { + Class<?> reporter = + Class.forName( + "org.apache.solr.security.agent.ViolationMetricsReporter", + false, + CoreContainer.class.getClassLoader()); + reporter + .getMethod("registerWithSolrMetrics", Object.class, String.class) + .invoke(null, metricManager, NODE_REGISTRY); + } catch (ClassNotFoundException ignored) { + // Agent not loaded (e.g. SOLR_SECURITY_AGENT_SKIP=true); metrics registration skipped. + } catch (ReflectiveOperationException e) { + log.warn("Failed to register security agent metrics", e); + } + + // Wire security agent violations to SLF4J; no-op if agent JAR is absent. + AgentViolationBridge.wire(); Review Comment: `AgentViolationBridge.wire()` and the reflective metrics-registration call in `CoreContainer.loadInternal()` use `Class.forName("org.apache.solr.security.agent...")` to detect whether the agent is loaded. However, since this class is in `solr:core`, the lookup goes through the application classloader rather than the bootstrap classloader where the agent classes were injected via `Boot-Class-Path`. Class loading delegates upward, so it works in normal JVM setups — but the comment on line 963 explicitly passes `CoreContainer.class.getClassLoader()`, while `AgentViolationBridge` (line 44) uses no classloader argument. The two reflective lookups should be consistent and ideally use `ClassLoader.getSystemClassLoader()` or `null` (bootstrap) explicitly, with a comment explaining why, to avoid behaviour depending on classloader hierarchy nuances in containerized or modular deployments. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
