Skip to content
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
4d2f850
add dedup to file access tree
jdconrad Feb 20, 2025
6002f1f
add exclusive to FilesEntitlement
jdconrad Feb 20, 2025
c79fb72
add global exclusive variable
jdconrad Feb 20, 2025
415a744
build and validate exclusive paths in PolicyManager
jdconrad Feb 20, 2025
8897f0b
add exclusive paths to FileAccessTree constructor
jdconrad Feb 20, 2025
8da4660
build and throw for exclusive path access
jdconrad Feb 21, 2025
381ed11
add some basic tests
jdconrad Feb 21, 2025
6f14915
[CI] Auto commit changes from spotless
Feb 21, 2025
c5b9f2a
Merge branch 'main' into exclusive2
jdconrad Feb 21, 2025
fef7dc5
remove conflict for main
jdconrad Feb 23, 2025
0978447
Merge branch 'main' into exclusive2
jdconrad Feb 23, 2025
857ba64
update to use path instead of string and component and module for
jdconrad Feb 23, 2025
a39b3a9
checkpoint
jdconrad Feb 24, 2025
b86d434
add withExclusive to FileData
jdconrad Feb 24, 2025
e839df5
Merge branch 'main' into exclusive2
jdconrad Feb 24, 2025
2b9570a
updates
jdconrad Feb 24, 2025
580c4a3
fix test
jdconrad Feb 24, 2025
a283e0d
add more tests
jdconrad Feb 25, 2025
e36364f
update tests
jdconrad Feb 25, 2025
7ecd086
Merge branch 'main' into exclusive2
jdconrad Feb 25, 2025
b255a2c
Merge branch 'main' into exclusive2
jdconrad Feb 25, 2025
c5dbb64
add policy manager tests for exclusive
jdconrad Feb 25, 2025
3b648b8
[CI] Auto commit changes from spotless
Feb 25, 2025
b78ce2c
more tests
jdconrad Feb 25, 2025
39b9ff7
Merge branch 'main' into exclusive2
jdconrad Feb 25, 2025
d9de8c7
response to pr comments
jdconrad Feb 25, 2025
cc6cd44
Merge branch 'main' into exclusive2
jdconrad Feb 26, 2025
f0b4136
Merge branch 'main' into exclusive2
jdconrad Feb 26, 2025
7383ea8
fix error messages for parsing types
jdconrad Feb 27, 2025
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
Original file line number Diff line number Diff line change
Expand Up @@ -30,17 +30,87 @@

public final class FileAccessTree {

record ExclusiveFileEntitlement(String componentName, String moduleName, FilesEntitlement filesEntitlement) {}

record ExclusivePath(String componentName, String moduleName, String path) {}

static List<ExclusivePath> buildExclusivePathList(List<ExclusiveFileEntitlement> exclusiveFileEntitlements, PathLookup pathLookup) {
List<ExclusivePath> exclusivePaths = new ArrayList<>();
for (ExclusiveFileEntitlement efe : exclusiveFileEntitlements) {
for (FilesEntitlement.FileData fd : efe.filesEntitlement().filesData()) {
if (fd.exclusive()) {
List<Path> paths = fd.resolvePaths(pathLookup).toList();
for (Path path : paths) {
exclusivePaths.add(new ExclusivePath(efe.componentName(), efe.moduleName(), normalizePath(path)));
}
}
}
}
exclusivePaths.sort((ep1, ep2) -> PATH_ORDER.compare(ep1.path(), ep2.path()));
return exclusivePaths;
}

static void validateExclusivePaths(List<ExclusivePath> exclusivePaths) {
if (exclusivePaths.isEmpty() == false) {
ExclusivePath currentExclusivePath = exclusivePaths.get(0);
for (int i = 1; i < exclusivePaths.size(); ++i) {
ExclusivePath nextPath = exclusivePaths.get(i);
if (currentExclusivePath.path().equals(nextPath.path) || isParent(currentExclusivePath.path(), nextPath.path())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, this is pretty subtle.

Assuming the incoming list of paths has been sorted by PATH_ORDER, that does not ensure that a child is always immediately after its parent; but it does ensure that any list of paths containing one or more parent-child pairs will have at least one of those pairs adjacent, and that's enough!

For example:

  /a
  /a/b
  /a/c

The child /a/c is not next to its parent /a, but for the sake of validation, that's ok, because to have something else (like /a/b) interleave, that must itself be a child (or descendant) of /a, and so the validation will fail on that.

I'm 95% sure this reasoning works and we're not missing a case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should be path order and since that moves file separators to be the first character this should work. This works similarly to pruning and I think has the same guarantees here. Thanks for checking it :)

throw new IllegalArgumentException(
"duplicate/overlapping exclusive paths found in files entitlements: "
+ "[["
+ currentExclusivePath.componentName()
+ "] ["
+ currentExclusivePath.moduleName()
+ "] ["
+ currentExclusivePath.path()
Copy link
Contributor

Choose a reason for hiding this comment

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

(Minor: I suppose it could be handy if we override ExclusivePath.toString to have this format.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

+ "]] and [["

+ nextPath.componentName()
+ "] ["
+ nextPath.moduleName()
+ "] ["
+ nextPath.path()
+ "]]"
);
}
currentExclusivePath = nextPath;
}
}
}

private static final Logger logger = LogManager.getLogger(FileAccessTree.class);
private static final String FILE_SEPARATOR = getDefaultFileSystem().getSeparator();

private final String[] exclusivePaths;
private final String[] readPaths;
private final String[] writePaths;

private FileAccessTree(FilesEntitlement filesEntitlement, PathLookup pathLookup) {
private FileAccessTree(
String componentName,
String moduleName,
FilesEntitlement filesEntitlement,
PathLookup pathLookup,
List<ExclusivePath> exclusivePaths
) {
List<String> updatedExclusivePaths = new ArrayList<>();
for (ExclusivePath exclusivePath : exclusivePaths) {
if (exclusivePath.componentName().equals(componentName) == false || exclusivePath.moduleName().equals(moduleName) == false) {
updatedExclusivePaths.add(exclusivePath.path());
}
}

List<String> readPaths = new ArrayList<>();
List<String> writePaths = new ArrayList<>();
BiConsumer<Path, Mode> addPath = (path, mode) -> {
var normalized = normalizePath(path);
for (String exclusivePath : updatedExclusivePaths) {
if (exclusivePath.equals(normalized) || isParent(exclusivePath, normalized)) {
throw new IllegalArgumentException(
"[" + componentName + "] [" + moduleName + "] cannot use exclusive path [" + exclusivePath + "]"
);
}
}
if (mode == Mode.READ_WRITE) {
writePaths.add(normalized);
}
Expand Down Expand Up @@ -83,9 +153,11 @@ private FileAccessTree(FilesEntitlement filesEntitlement, PathLookup pathLookup)
Path jdk = Paths.get(System.getProperty("java.home"));
addPathAndMaybeLink.accept(jdk.resolve("conf"), Mode.READ);

updatedExclusivePaths.sort(PATH_ORDER);
readPaths.sort(PATH_ORDER);
writePaths.sort(PATH_ORDER);

this.exclusivePaths = updatedExclusivePaths.toArray(new String[0]);
this.readPaths = pruneSortedPaths(readPaths).toArray(new String[0]);
this.writePaths = pruneSortedPaths(writePaths).toArray(new String[0]);
}
Expand All @@ -106,8 +178,14 @@ private static List<String> pruneSortedPaths(List<String> paths) {
return prunedReadPaths;
}

public static FileAccessTree of(FilesEntitlement filesEntitlement, PathLookup pathLookup) {
return new FileAccessTree(filesEntitlement, pathLookup);
public static FileAccessTree of(
String componentName,
String moduleName,
FilesEntitlement filesEntitlement,
PathLookup pathLookup,
List<ExclusivePath> exclusivePaths
) {
return new FileAccessTree(componentName, moduleName, filesEntitlement, pathLookup, exclusivePaths);
}

boolean canRead(Path path) {
Expand All @@ -132,8 +210,8 @@ static String normalizePath(Path path) {
return result;
}

private static boolean checkPath(String path, String[] paths) {
if (paths.length == 0) {
private boolean checkPath(String path, String[] paths) {
if (paths.length == 0 || Arrays.binarySearch(exclusivePaths, path) >= 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can someone configure exclusive access to a directory? If so, this check is insufficient I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please double check this logic for me -- I intend to throw in the constructor for FileAccessTree if we catch a read or read_write path that would require exclusive access to a directory for a read or read_write path. In which case, that leaves the exact match for an exclusive path in an existing read or read_write directory or it will already have thrown in the constructor.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gosh. Tricky.

Ok, suppose:

  • X has read access to /a
  • Y has exclusive access to /a/b
  • X attempts to read /a/b/c

I believe this combo would not be caught in the constructor (because it's valid) and we would fail to detect the violation because it's not an exact file name match.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I will add this test and move the logic :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed this. Thanks for showing me what was incorrect with a simple example.

return false;
}
int ndx = Arrays.binarySearch(paths, path, PATH_ORDER);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
import org.elasticsearch.core.SuppressForbidden;
import org.elasticsearch.entitlement.instrumentation.InstrumentationService;
import org.elasticsearch.entitlement.runtime.api.NotEntitledException;
import org.elasticsearch.entitlement.runtime.policy.FileAccessTree.ExclusiveFileEntitlement;
import org.elasticsearch.entitlement.runtime.policy.FileAccessTree.ExclusivePath;
import org.elasticsearch.entitlement.runtime.policy.entitlements.CreateClassLoaderEntitlement;
import org.elasticsearch.entitlement.runtime.policy.entitlements.Entitlement;
import org.elasticsearch.entitlement.runtime.policy.entitlements.ExitVMEntitlement;
Expand All @@ -32,6 +34,7 @@
import java.lang.module.ModuleFinder;
import java.lang.module.ModuleReference;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -91,7 +94,7 @@ ModuleEntitlements defaultEntitlements(String componentName) {
}

// pkg private for testing
ModuleEntitlements policyEntitlements(String componentName, List<Entitlement> entitlements) {
ModuleEntitlements policyEntitlements(String componentName, String moduleName, List<Entitlement> entitlements) {
FilesEntitlement filesEntitlement = FilesEntitlement.EMPTY;
for (Entitlement entitlement : entitlements) {
if (entitlement instanceof FilesEntitlement) {
Expand All @@ -101,7 +104,7 @@ ModuleEntitlements policyEntitlements(String componentName, List<Entitlement> en
return new ModuleEntitlements(
componentName,
entitlements.stream().collect(groupingBy(Entitlement::getClass)),
FileAccessTree.of(filesEntitlement, pathLookup)
FileAccessTree.of(componentName, moduleName, filesEntitlement, pathLookup, exclusivePaths)
);
}

Expand Down Expand Up @@ -143,6 +146,13 @@ private static Set<Module> findSystemModules() {
*/
private final Module entitlementsModule;

/**
* Paths that are only allowed for a single module. Used to generate
* structures to indicate other modules aren't allowed to use these
* files in {@link FileAccessTree}s.
*/
private final List<ExclusivePath> exclusivePaths;

public PolicyManager(
Policy serverPolicy,
List<Entitlement> apmAgentEntitlements,
Expand All @@ -162,25 +172,34 @@ public PolicyManager(
this.apmAgentPackageName = apmAgentPackageName;
this.entitlementsModule = entitlementsModule;
this.pathLookup = requireNonNull(pathLookup);
this.defaultFileAccess = FileAccessTree.of(FilesEntitlement.EMPTY, pathLookup);
this.defaultFileAccess = FileAccessTree.of("", "", FilesEntitlement.EMPTY, pathLookup, List.of());
Copy link
Contributor

@prdoyle prdoyle Feb 25, 2025

Choose a reason for hiding this comment

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

These empty strings give me the willies, but I'm not sure what to replace them with. 🤔

I'm fine with them if everyone else is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I switched this to use to the existing (unknown)

this.mutedClasses = suppressFailureLogClasses;

List<ExclusiveFileEntitlement> exclusiveFileEntitlements = new ArrayList<>();
for (var e : serverEntitlements.entrySet()) {
validateEntitlementsPerModule(SERVER_COMPONENT_NAME, e.getKey(), e.getValue());
validateEntitlementsPerModule(SERVER_COMPONENT_NAME, e.getKey(), e.getValue(), exclusiveFileEntitlements);
}
validateEntitlementsPerModule(APM_AGENT_COMPONENT_NAME, "unnamed", apmAgentEntitlements);
validateEntitlementsPerModule(APM_AGENT_COMPONENT_NAME, ALL_UNNAMED, apmAgentEntitlements, exclusiveFileEntitlements);
for (var p : pluginsEntitlements.entrySet()) {
for (var m : p.getValue().entrySet()) {
validateEntitlementsPerModule(p.getKey(), m.getKey(), m.getValue());
validateEntitlementsPerModule(p.getKey(), m.getKey(), m.getValue(), exclusiveFileEntitlements);
}
}
List<ExclusivePath> exclusivePaths = FileAccessTree.buildExclusivePathList(exclusiveFileEntitlements, pathLookup);
FileAccessTree.validateExclusivePaths(exclusivePaths);
this.exclusivePaths = exclusivePaths;
}

private static Map<String, List<Entitlement>> buildScopeEntitlementsMap(Policy policy) {
return policy.scopes().stream().collect(toUnmodifiableMap(Scope::moduleName, Scope::entitlements));
}

private static void validateEntitlementsPerModule(String componentName, String moduleName, List<Entitlement> entitlements) {
private static void validateEntitlementsPerModule(
String componentName,
String moduleName,
List<Entitlement> entitlements,
List<ExclusiveFileEntitlement> exclusiveFileEntitlements
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is an "out parameter"? That probably deserves a javadoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added.

) {
Set<Class<? extends Entitlement>> found = new HashSet<>();
for (var e : entitlements) {
if (found.contains(e.getClass())) {
Expand All @@ -189,6 +208,9 @@ private static void validateEntitlementsPerModule(String componentName, String m
);
}
found.add(e.getClass());
if (e instanceof FilesEntitlement fe) {
exclusiveFileEntitlements.add(new ExclusiveFileEntitlement(componentName, moduleName, fe));
}
}
}

Expand Down Expand Up @@ -498,7 +520,7 @@ private ModuleEntitlements computeEntitlements(Class<?> requestingClass) {

if (requestingModule.isNamed() == false && requestingClass.getPackageName().startsWith(apmAgentPackageName)) {
// The APM agent is the only thing running non-modular in the system classloader
return policyEntitlements(APM_AGENT_COMPONENT_NAME, apmAgentEntitlements);
return policyEntitlements(APM_AGENT_COMPONENT_NAME, ALL_UNNAMED, apmAgentEntitlements);
}

return defaultEntitlements(UNKNOWN_COMPONENT_NAME);
Expand All @@ -513,7 +535,7 @@ private ModuleEntitlements getModuleScopeEntitlements(
if (entitlements == null) {
return defaultEntitlements(componentName);
}
return policyEntitlements(componentName, entitlements);
return policyEntitlements(componentName, moduleName, entitlements);
}

private static boolean isServerModule(Module requestingModule) {
Expand Down
Loading