Add an exclusive parameter for files entitlements #123087

jdconrad · 2025-02-21T00:33:32Z

This adds an exclusive parameter for FilesEntitlement where a path can be made exclusive for a certain module. Should two modules attempt to both specify the same path as exclusive an exception is thrown.

ES-10845

elasticsearchmachine · 2025-02-21T00:33:56Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

rjernst

A couple questions

rjernst · 2025-02-21T00:38:06Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java

            var paths = fileData.resolvePaths(pathLookup);
            paths.forEach(path -> {
                var normalized = normalizePath(path);
+                for (String exclusivePath : exclusivePaths) {


Shouldn't we normalize the exclusive path?

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?

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

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

rjernst · 2025-02-21T00:45:26Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java

+     * structures to indicate other modules aren't allowed to use these
+     * files in {@link FileAccessTree}s.
+     */
+    private final List<String> exclusivePaths;


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?

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.

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.

rjernst · 2025-02-21T00:47:03Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java

+                    if (fd.exclusive()) {
+                        List<Path> paths = fd.resolvePaths(pathLookup).toList();
+                        for (Path path : paths) {
+                            String pathStr = FileAccessTree.normalizePath(path);


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

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

I will change this based on previous comments.

prdoyle · 2025-02-21T14:25:19Z

...nt/src/main/java/org/elasticsearch/entitlement/initialization/EntitlementInitialization.java

-                            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),


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

I considered that, and I think you're right. I'll go ahead and add additional ofPaths.

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.

I did end up changing this to @rjernst's suggestion.

Yeah that's even better.

prdoyle · 2025-02-21T14:26:56Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java

+            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) {


There's no need to check equals because startsWith includes that case.

:) I will fix this.

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

prdoyle · 2025-02-21T14:28:11Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java

-    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) {


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

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.

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.

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

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

jdconrad · 2025-02-21T15:45:50Z

@rjernst and @prdoyle Thank you both for the reviews. I will continue working on this code.

exclusive path validation

jdconrad · 2025-02-25T22:02:54Z

@rjernst @prdoyle I have added tests and am ready for another round of reviews.

prdoyle

Mindboggling

prdoyle · 2025-02-25T22:05:49Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java

+                            + "] ["
+                            + currentExclusivePath.moduleName()
+                            + "] ["
+                            + currentExclusivePath.path()


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

prdoyle · 2025-02-25T22:14:15Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java

+            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())) {


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.

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 :)

prdoyle · 2025-02-25T22:22:40Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/FileAccessTree.java

-    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) {


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.

prdoyle · 2025-02-25T22:24:24Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java

        this.entitlementsModule = entitlementsModule;
        this.pathLookup = requireNonNull(pathLookup);
-        this.defaultFileAccess = FileAccessTree.of(FilesEntitlement.EMPTY, pathLookup);
+        this.defaultFileAccess = FileAccessTree.of("", "", FilesEntitlement.EMPTY, pathLookup, List.of());


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.

I switched this to use to the existing (unknown)

prdoyle · 2025-02-25T22:25:28Z

libs/entitlement/src/main/java/org/elasticsearch/entitlement/runtime/policy/PolicyManager.java

+        String componentName,
+        String moduleName,
+        List<Entitlement> entitlements,
+        List<ExclusiveFileEntitlement> exclusiveFileEntitlements


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

rjernst

LGTM

rjernst · 2025-02-26T23:30:18Z

...rc/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java

+            String modeAsString = (String) file.remove("mode");
+            String platformAsString = (String) file.remove("platform");
+            String ignoreUrlAsString = (String) file.remove("ignore_url");
+            Boolean exclusiveBoolean = (Boolean) file.remove("exclusive");


Can we have a better error message in case this is a string?

This adds an exclusive parameter for FilesEntitlement where a path can be made exclusive for a certain module. Should two modules attempt to both specify the same path as exclusive an exception is thrown.

elasticsearchmachine · 2025-02-27T01:37:44Z

💚 Backport successful

Status	Branch	Result
✅	8.18
✅	8.x
✅	9.0

This adds an exclusive parameter for FilesEntitlement where a path can be made exclusive for a certain module. Should two modules attempt to both specify the same path as exclusive an exception is thrown.

With the introduction of entitlements (elastic#120243) and exclusive file access (elastic#123087) it is no longer safe to watch a whole directory. In a lot of deployments, the parent directory for SSL config files will be the main config directory, which also contains exclusive files such as SAML realm metadata or File realm users. Watching that directory will cause entitlement warnings because it is not permissible for core/ssl-config to read files that are exclusively owned by the security module (or other modules)

With the introduction of entitlements (#120243) and exclusive file access (#123087) it is no longer safe to watch a whole directory. In a lot of deployments, the parent directory for SSL config files will be the main config directory, which also contains exclusive files such as SAML realm metadata or File realm users. Watching that directory will cause entitlement warnings because it is not permissible for core/ssl-config to read files that are exclusively owned by the security module (or other modules)

With the introduction of entitlements (elastic#120243) and exclusive file access (elastic#123087) it is no longer safe to watch a whole directory. In a lot of deployments, the parent directory for SSL config files will be the main config directory, which also contains exclusive files such as SAML realm metadata or File realm users. Watching that directory will cause entitlement warnings because it is not permissible for core/ssl-config to read files that are exclusively owned by the security module (or other modules)

jdconrad added 7 commits February 20, 2025 13:05

add dedup to file access tree

4d2f850

add exclusive to FilesEntitlement

6002f1f

add global exclusive variable

c79fb72

build and validate exclusive paths in PolicyManager

415a744

add exclusive paths to FileAccessTree constructor

8897f0b

build and throw for exclusive path access

8da4660

add some basic tests

381ed11

jdconrad added >non-issue v8.18.1 v8.19.0 v9.0.1 v9.1.0 :Core/Infra/Entitlements Entitlements infrastructure labels Feb 21, 2025

jdconrad requested a review from a team as a code owner February 21, 2025 00:33

elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 21, 2025

[CI] Auto commit changes from spotless

6f14915

jdconrad added the test-entitlements label Feb 21, 2025

rjernst reviewed Feb 21, 2025

View reviewed changes

prdoyle reviewed Feb 21, 2025

View reviewed changes

jdconrad added 9 commits February 21, 2025 10:18

Merge branch 'main' into exclusive2

c5b9f2a

remove conflict for main

fef7dc5

Merge branch 'main' into exclusive2

0978447

update to use path instead of string and component and module for

857ba64

exclusive path validation

checkpoint

a39b3a9

add withExclusive to FileData

b86d434

Merge branch 'main' into exclusive2

e839df5

updates

2b9570a

fix test

580c4a3

jdconrad and others added 5 commits February 25, 2025 11:50

Merge branch 'main' into exclusive2

b255a2c

add policy manager tests for exclusive

c5dbb64

[CI] Auto commit changes from spotless

3b648b8

more tests

b78ce2c

Merge branch 'main' into exclusive2

39b9ff7

jdconrad changed the title ~~[WIP] Add an exclusive parameter for files entitlements~~ Add an exclusive parameter for files entitlements Feb 25, 2025

prdoyle reviewed Feb 25, 2025

View reviewed changes

jdconrad added the auto-backport Automatically create backport pull requests when merged label Feb 25, 2025

jdconrad added 3 commits February 25, 2025 15:28

response to pr comments

d9de8c7

Merge branch 'main' into exclusive2

cc6cd44

Merge branch 'main' into exclusive2

f0b4136

rjernst approved these changes Feb 26, 2025

View reviewed changes

fix error messages for parsing types

7383ea8

jdconrad merged commit 3e3dfa4 into elastic:main Feb 27, 2025
22 checks passed

This was referenced Feb 27, 2025

[8.18] Add an exclusive parameter for files entitlements (#123087) #123562

Merged

[8.x] Add an exclusive parameter for files entitlements (#123087) #123563

Merged

[9.0] Add an exclusive parameter for files entitlements (#123087) #123564

Merged

tvernum mentioned this pull request Jun 20, 2025

Watch SSL files instead of directories #129738

Merged

Add an exclusive parameter for files entitlements #123087

Add an exclusive parameter for files entitlements #123087

Uh oh!

Conversation

jdconrad commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 21, 2025

Uh oh!

rjernst left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdconrad commented Feb 21, 2025

Uh oh!

jdconrad commented Feb 25, 2025

Uh oh!

prdoyle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prdoyle Feb 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

jdconrad commented Feb 21, 2025 •

edited

Loading

prdoyle Feb 25, 2025 •

edited

Loading