Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 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 @@ -152,8 +152,8 @@ private static PolicyManager createPolicyManager() {
new CreateClassLoaderEntitlement(),
new FilesEntitlement(
List.of(
FileData.ofPath(bootstrapArgs.repoDirResolver().apply(""), READ_WRITE),
FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE)
FileData.ofPath(bootstrapArgs.repoDirResolver().apply(""), READ_WRITE, false),
FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE, false)
)
)
)
Expand All @@ -172,29 +172,29 @@ private static PolicyManager createPolicyManager() {
new FilesEntitlement(
List.of(
// Base ES directories
FileData.ofPath(bootstrapArgs.tempDir(), READ_WRITE),
FileData.ofPath(bootstrapArgs.configDir(), READ),
FileData.ofPath(bootstrapArgs.logsDir(), READ_WRITE),
FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE),
FileData.ofPath(bootstrapArgs.repoDirResolver().apply(""), READ_WRITE),
FileData.ofPath(bootstrapArgs.tempDir(), READ_WRITE, false),
Copy link
Contributor

Choose a reason for hiding this comment

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

This might benefit from an overload of ofPath that assumes false.

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 considered that, and I think you're right. I'll go ahead and add additional ofPaths.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of having a ton of overloads (which would fall to combinatorial overload if we add the platform parameter discussed separately), I wonder if FileData could have withExclusive(boolean). Then each of the 4 impls we have can recreate themselves with exclusive changed. But the ofXXX methods can still be simple as they are now. IMO this is preferable since setting exclusive to true is rare.

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 did end up changing this to @rjernst's suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that's even better.

FileData.ofPath(bootstrapArgs.configDir(), READ, false),
FileData.ofPath(bootstrapArgs.logsDir(), READ_WRITE, false),
FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE, false),
FileData.ofPath(bootstrapArgs.repoDirResolver().apply(""), READ_WRITE, false),

// OS release on Linux
FileData.ofPath(Path.of("/etc/os-release"), READ),
FileData.ofPath(Path.of("/etc/system-release"), READ),
FileData.ofPath(Path.of("/usr/lib/os-release"), READ),
FileData.ofPath(Path.of("/etc/os-release"), READ, false),
FileData.ofPath(Path.of("/etc/system-release"), READ, false),
FileData.ofPath(Path.of("/usr/lib/os-release"), READ, false),
// read max virtual memory areas
FileData.ofPath(Path.of("/proc/sys/vm/max_map_count"), READ),
FileData.ofPath(Path.of("/proc/meminfo"), READ),
FileData.ofPath(Path.of("/proc/sys/vm/max_map_count"), READ, false),
FileData.ofPath(Path.of("/proc/meminfo"), READ, false),
// load averages on Linux
FileData.ofPath(Path.of("/proc/loadavg"), READ),
FileData.ofPath(Path.of("/proc/loadavg"), READ, false),
// control group stats on Linux. cgroup v2 stats are in an unpredicable
// location under `/sys/fs/cgroup`, so unfortunately we have to allow
// read access to the entire directory hierarchy.
FileData.ofPath(Path.of("/proc/self/cgroup"), READ),
FileData.ofPath(Path.of("/sys/fs/cgroup/"), READ),
FileData.ofPath(Path.of("/proc/self/cgroup"), READ, false),
FileData.ofPath(Path.of("/sys/fs/cgroup/"), READ, false),
// // io stats on Linux
FileData.ofPath(Path.of("/proc/self/mountinfo"), READ),
FileData.ofPath(Path.of("/proc/diskstats"), READ)
FileData.ofPath(Path.of("/proc/self/mountinfo"), READ, false),
FileData.ofPath(Path.of("/proc/diskstats"), READ, false)
)
)
)
Expand All @@ -208,23 +208,25 @@ private static PolicyManager createPolicyManager() {
new ManageThreadsEntitlement(),
new FilesEntitlement(
List.of(
FileData.ofPath(bootstrapArgs.configDir(), READ),
FileData.ofPath(bootstrapArgs.tempDir(), READ),
FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE)
FileData.ofPath(bootstrapArgs.configDir(), READ, false),
FileData.ofPath(bootstrapArgs.tempDir(), READ, false),
FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE, false)
)
)
)
),
new Scope(
"org.apache.lucene.misc",
List.of(new FilesEntitlement(List.of(FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE))))
List.of(
new FilesEntitlement(List.of(FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE, false)))
)
),
new Scope("org.apache.logging.log4j.core", List.of(new ManageThreadsEntitlement())),
new Scope(
"org.elasticsearch.nativeaccess",
List.of(
new LoadNativeLibrariesEntitlement(),
new FilesEntitlement(List.of(FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE)))
new FilesEntitlement(List.of(FileData.ofRelativePath(Path.of(""), FilesEntitlement.BaseDir.DATA, READ_WRITE, false)))
)
)
);
Expand All @@ -233,11 +235,17 @@ private static PolicyManager createPolicyManager() {
if (trustStorePath != null) {
Collections.addAll(
serverScopes,
new Scope("org.bouncycastle.fips.tls", List.of(new FilesEntitlement(List.of(FileData.ofPath(trustStorePath, READ))))),
new Scope(
"org.bouncycastle.fips.tls",
List.of(new FilesEntitlement(List.of(FileData.ofPath(trustStorePath, READ, false))))
),
new Scope(
"org.bouncycastle.fips.core",
// read to lib dir is required for checksum validation
List.of(new FilesEntitlement(List.of(FileData.ofPath(bootstrapArgs.libDir(), READ))), new ManageThreadsEntitlement())
List.of(
new FilesEntitlement(List.of(FileData.ofPath(bootstrapArgs.libDir(), READ, false))),
new ManageThreadsEntitlement()
)
)
);
}
Expand All @@ -251,8 +259,8 @@ private static PolicyManager createPolicyManager() {
new ManageThreadsEntitlement(),
new FilesEntitlement(
List.of(
FileData.ofPath(Path.of("/co/elastic/apm/agent/"), READ),
FileData.ofPath(Path.of("/agent/co/elastic/apm/agent/"), READ)
FileData.ofPath(Path.of("/co/elastic/apm/agent/"), READ, false),
FileData.ofPath(Path.of("/agent/co/elastic/apm/agent/"), READ, false)
)
)
);
Expand Down
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.entitlement.runtime.api.NotEntitledException;
import org.elasticsearch.entitlement.runtime.policy.entitlements.FilesEntitlement;

import java.nio.file.Path;
Expand All @@ -23,17 +24,27 @@ public final class FileAccessTree {

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(FilesEntitlement filesEntitlement, PathLookup pathLookup, List<String> exclusivePaths) {
List<String> updatedExclusivePaths = new ArrayList<>();
List<String> readPaths = new ArrayList<>();
List<String> writePaths = new ArrayList<>();
for (FilesEntitlement.FileData fileData : filesEntitlement.filesData()) {
var mode = fileData.mode();
var paths = fileData.resolvePaths(pathLookup);
paths.forEach(path -> {
var normalized = normalizePath(path);
for (String exclusivePath : exclusivePaths) {
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we normalize the exclusive path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This gets normalized as part of the work done in PolicyManager. However, we could pass in a List<Path> here as you suggested in a later comment and just do the same work again to be on the safe side. What do you 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.

Talked with @rjernst and I will change this to use Paths for validation here and normalize in FileAccessTree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to use Path via a record indicating component and module as well.

if (normalized.equals(exclusivePath) == false) {
if (normalized.startsWith(exclusivePath)) {
// TODO: throw
}
updatedExclusivePaths.add(exclusivePath);
}
}
if (mode == FilesEntitlement.Mode.READ_WRITE) {
writePaths.add(normalized);
}
Expand All @@ -49,12 +60,29 @@ private FileAccessTree(FilesEntitlement filesEntitlement, PathLookup pathLookup)
readPaths.sort(String::compareTo);
writePaths.sort(String::compareTo);

this.readPaths = readPaths.toArray(new String[0]);
this.writePaths = writePaths.toArray(new String[0]);
this.exclusivePaths = updatedExclusivePaths.toArray(new String[0]);
this.readPaths = pruneSortedPaths(readPaths).toArray(new String[0]);
this.writePaths = pruneSortedPaths(writePaths).toArray(new String[0]);
}

public static List<String> pruneSortedPaths(List<String> paths) {
List<String> prunedReadPaths = new ArrayList<>();
if (paths.isEmpty() == false) {
String currentPath = paths.get(0);
prunedReadPaths.add(currentPath);
for (int i = 1; i < paths.size(); ++i) {
String nextPath = paths.get(i);
if (nextPath.equals(currentPath) == false && nextPath.startsWith(currentPath) == false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need to check equals because startsWith includes that 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.

:) I will fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was merged as part of #123177 without the .equals

prunedReadPaths.add(nextPath);
currentPath = nextPath;
}
}
}
return prunedReadPaths;
}

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

boolean canRead(Path path) {
Expand All @@ -75,8 +103,8 @@ static String normalizePath(Path path) {
return path.toAbsolutePath().normalize().toString();
}

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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,8 @@
import java.lang.module.ModuleFinder;
import java.lang.module.ModuleReference;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Comparator;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
Expand Down Expand Up @@ -103,7 +105,7 @@ ModuleEntitlements policyEntitlements(String componentName, List<Entitlement> en
return new ModuleEntitlements(
componentName,
entitlements.stream().collect(groupingBy(Entitlement::getClass)),
FileAccessTree.of(filesEntitlement, pathLookup)
FileAccessTree.of(filesEntitlement, pathLookup, List.of())
);
}

Expand Down Expand Up @@ -143,6 +145,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<String> exclusivePaths;
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this need to retain which module has access to the exclusive path, so that when that module's entitlements are initialized we can omit that exclusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This list should already be validated, so that we can safely assume any read or read_write path when we constructor the module's entitlements found as an exclusive path is for that specific module already.

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 ended up adding component name and module name to check which exclusive paths should be added for each FileAccessTree to ensure paths are safely added instead of making tricky assumptions.


public PolicyManager(
Policy serverPolicy,
List<Entitlement> apmAgentEntitlements,
Expand All @@ -161,17 +170,24 @@ 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());

List<ExclusivePath> exclusivePaths = new ArrayList<>();
for (var e : serverEntitlements.entrySet()) {
validateEntitlementsPerModule(SERVER_COMPONENT_NAME, e.getKey(), e.getValue());
buildExclusivePathList(exclusivePaths, pathLookup, SERVER_COMPONENT_NAME, e.getKey(), e.getValue());
}
validateEntitlementsPerModule(APM_AGENT_COMPONENT_NAME, "unnamed", apmAgentEntitlements);
buildExclusivePathList(exclusivePaths, pathLookup, APM_AGENT_COMPONENT_NAME, "unnamed", apmAgentEntitlements);
for (var p : pluginsEntitlements.entrySet()) {
for (var m : p.getValue().entrySet()) {
validateEntitlementsPerModule(p.getKey(), m.getKey(), m.getValue());
buildExclusivePathList(exclusivePaths, pathLookup, p.getKey(), m.getKey(), m.getValue());
}
}
exclusivePaths.sort(Comparator.comparing(ExclusivePath::path));
validateExclusivePaths(exclusivePaths);
this.exclusivePaths = exclusivePaths.stream().map(ExclusivePath::path).toList();
}

private static Map<String, List<Entitlement>> buildScopeEntitlementsMap(Policy policy) {
Expand All @@ -190,6 +206,45 @@ private static void validateEntitlementsPerModule(String componentName, String m
}
}

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

private static void buildExclusivePathList(
List<ExclusivePath> exclusivePaths,
PathLookup pathLookup,
String componentName,
String moduleName,
List<Entitlement> entitlements
) {
for (var e : entitlements) {
if (e instanceof FilesEntitlement fe) {
for (FilesEntitlement.FileData fd : fe.filesData()) {
if (fd.exclusive()) {
List<Path> paths = fd.resolvePaths(pathLookup).toList();
for (Path path : paths) {
String pathStr = FileAccessTree.normalizePath(path);
Copy link
Member

Choose a reason for hiding this comment

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

Could we retain this as Path to pass into FileAccessTree, so that it can do the normalization internally?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have to normalize here to be able to validate. I'm certainly open to alternatives if you have a good one :)

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 will change this based on previous comments.

exclusivePaths.add(new ExclusivePath(componentName, moduleName, pathStr));
}
}
}
}
}
}

private 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 (nextPath.path().equals(currentExclusivePath.path())) {
// TODO: throw
} else if (nextPath.path().startsWith(currentExclusivePath.path())) {
// TODO: throw
}
currentExclusivePath = nextPath;
}
}
}

public void checkStartProcess(Class<?> callerClass) {
neverEntitled(callerClass, () -> "start process");
}
Expand Down
Loading