Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add platform property to files entitlement #123212

Merged
merged 8 commits into from
Feb 24, 2025
Merged

Add platform property to files entitlement #123212

merged 8 commits into from
Feb 24, 2025

Conversation

rjernst
Copy link
Member

@rjernst rjernst commented Feb 22, 2025 •
edited by ldematte
Loading

Some file paths are OS specific. This commit adds a platform property to each file in a files entitlement that can be used to limit that file to a specific platform.

Relates to ES-10912

@rjernst
Add platform property to files entitlement
1e23a29 
Some file paths are OS specific. This commit adds a `platform` property
to each file in a files entitlement that can be used to limit that file
to a specific platform.
@rjernst rjernst added >refactoring auto-backport Automatically create backport pull requests when merged v8.18.1 v8.19.0 v9.0.1 v9.1.0 :Core/Infra/Entitlements Entitlements infrastructure labels Feb 22, 2025
@rjernst rjernst requested a review from a team as a code owner February 22, 2025 21:42
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Feb 22, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@rjernst rjernst added the test-entitlements Trigger CI checks using security manager replacement label Feb 23, 2025
ldematte
ldematte reviewed Feb 24, 2025
View reviewed changes
Copy link
Contributor

@ldematte ldematte left a comment

Choose a reason for hiding this comment

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

It looks good; my main concern is around where we check and how we can have an intermediate, non "final" FileData. Should we just have overloads of ofRelativePath etc. with the platform?
Also related: should we always enforce that we have a platform with absolute paths? Our current usage of it seems to say so, and I can't think of an absolute path that makes sense across all platforms.

...rc/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java Outdated
if (relativePathAsString != null) {
if (baseDir == null) {
throw new PolicyValidationException("files entitlement with a 'relative_path' must specify 'relative_to'");
}

Path relativePath = Path.of(relativePathAsString);
if (relativePath.isAbsolute()) {
if ((platform == null || platform.isCurrent()) && relativePath.isAbsolute()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I kind of liked the fluent interface, but this means that validation is moved here, at policy parsing; this would not catch any error in the server policy, for example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Scratch that, it seems we don't want to do it due to libraries checking for absolute paths regardless of the OS (netty). I pushed a change for that, to skip the absolute/relative check if the path is for "any" platform (null).

@ldematte
Copy link
Contributor

Our current usage of it seems to say so

I stand corrected: I saw your message on Netty. That would be a case for platform = null & absolute paths, and (unfortunately), skip absolute vs relative in that case :(

prdoyle
prdoyle approved these changes Feb 24, 2025
View reviewed changes
...rc/main/java/org/elasticsearch/entitlement/runtime/policy/entitlements/FilesEntitlement.java
if (os.startsWith("Linux")) {
return LINUX;
} else if (os.startsWith("Mac OS")) {
return MACOS;
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect we'll want a UNIX covering both LINUX and MACOS at some point.

Copy link
Member Author

Choose a reason for hiding this comment

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

I had a version of this (calling it POSIX because these aren't actually unix), but I'm unsure it will actually be necessary because the location of system files on linux/mac are usually very different, even though they begin with /.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. FWIW the JDK calls it the UnixFileSystemProvider.

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

private static final Platform current = findCurrent();

private static Platform findCurrent() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Elasticsearch already have a way to determine the platform? (Are we really the first to need this?)

If it does, should we use that instead of parsing the os.name system property?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are several places we repeat this logic, including inside Lucene. Here we don't have access to Lucene classes. I originally had a comment in this change to move this logic to elasticsearch.core, but I removed it. We could consider moving it still, though, but as a followup.

@rjernst rjernst enabled auto-merge (squash) February 24, 2025 21:31
@rjernst rjernst merged commit 09a3ec1 into elastic:main Feb 24, 2025
22 checks passed
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
❌ 8.18 Commit could not be cherrypicked due to conflicts
❌ 8.x Commit could not be cherrypicked due to conflicts
❌ 9.0 Commit could not be cherrypicked due to conflicts

You can use sqren/backport to manually backport by running backport --upstream elastic/elasticsearch --pr 123212

@rjernst rjernst deleted the entitlements/platform_files branch February 24, 2025 22:56
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Feb 25, 2025
@rjernst @mosche @ldematte
Add platform property to files entitlement (elastic#123212)
ddf122d 
Some file paths are OS specific. This commit adds a `platform` property
to each file in a files entitlement that can be used to limit that file
to a specific platform.

Co-authored-by: Moritz Mack <mmack@apache.org>
Co-authored-by: Lorenzo Dematte <lorenzo.dematte@elastic.co>
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Feb 25, 2025
@rjernst @mosche @ldematte
Add platform property to files entitlement (elastic#123212)
93821b0 
Some file paths are OS specific. This commit adds a `platform` property
to each file in a files entitlement that can be used to limit that file
to a specific platform.

Co-authored-by: Moritz Mack <mmack@apache.org>
Co-authored-by: Lorenzo Dematte <lorenzo.dematte@elastic.co>
rjernst added a commit to rjernst/elasticsearch that referenced this pull request Feb 25, 2025
@rjernst @mosche @ldematte
Add platform property to files entitlement (elastic#123212)
3ae99e0 
Some file paths are OS specific. This commit adds a `platform` property
to each file in a files entitlement that can be used to limit that file
to a specific platform.

Co-authored-by: Moritz Mack <mmack@apache.org>
Co-authored-by: Lorenzo Dematte <lorenzo.dematte@elastic.co>
elasticsearchmachine pushed a commit that referenced this pull request Feb 25, 2025
@rjernst @mosche @ldematte
Add platform property to files entitlement (#123212) (#123323)
76fa38c 
Some file paths are OS specific. This commit adds a `platform` property
to each file in a files entitlement that can be used to limit that file
to a specific platform.

Co-authored-by: Moritz Mack <mmack@apache.org>
Co-authored-by: Lorenzo Dematte <lorenzo.dematte@elastic.co>
elasticsearchmachine pushed a commit that referenced this pull request Feb 25, 2025
@rjernst @mosche @ldematte
Add platform property to files entitlement (#123212) (#123324)
2137a66 
Some file paths are OS specific. This commit adds a `platform` property
to each file in a files entitlement that can be used to limit that file
to a specific platform.

Co-authored-by: Moritz Mack <mmack@apache.org>
Co-authored-by: Lorenzo Dematte <lorenzo.dematte@elastic.co>
elasticsearchmachine pushed a commit that referenced this pull request Feb 25, 2025
@rjernst @mosche @ldematte
Add platform property to files entitlement (#123212) (#123325)
975f1c7 
Some file paths are OS specific. This commit adds a `platform` property
to each file in a files entitlement that can be used to limit that file
to a specific platform.

Co-authored-by: Moritz Mack <mmack@apache.org>
Co-authored-by: Lorenzo Dematte <lorenzo.dematte@elastic.co>
@ldematte ldematte mentioned this pull request Feb 26, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prdoyle prdoyle prdoyle approved these changes

@mosche mosche mosche left review comments

@ldematte ldematte ldematte left review comments

Assignees
No one assigned
Labels
auto-backport Automatically create backport pull requests when merged :Core/Infra/Entitlements Entitlements infrastructure >refactoring Team:Core/Infra Meta label for core/infra team test-entitlements Trigger CI checks using security manager replacement v8.18.1 v8.19.0 v9.0.1 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rjernst @elasticsearchmachine @ldematte @prdoyle @mosche