Skip to content

Conversation

@mosche
Copy link
Contributor

@mosche mosche commented Mar 10, 2025

This will rethrow the NoSuchFileException when encountering an invalid symbolic link when following links during file (read) entitlement checks.

Relates to #124133 (ES-11019)

@mosche mosche requested a review from a team as a code owner March 10, 2025 13:23
@elasticsearchmachine elasticsearchmachine added v9.1.0 needs:triage Requires assignment of a team area label labels Mar 10, 2025
@mosche mosche added auto-backport Automatically create backport pull requests when merged v8.18.1 v8.19.0 v9.0.1 :Core/Infra/Entitlements Entitlements infrastructure >refactoring and removed needs:triage Requires assignment of a team area label labels Mar 10, 2025
@elasticsearchmachine elasticsearchmachine added the Team:Core/Infra Meta label for core/infra team label Mar 10, 2025
@elasticsearchmachine
Copy link
Collaborator

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

@mosche
Copy link
Contributor Author

mosche commented Mar 10, 2025

The test failure is interesting, in the non modular case EntitledActions does not work. So all non modular denied tests where we need to set something up likely fail due to a NotEntitledException that originates from there rather than the actual code under test.

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.

Overall looks good. Left only 1 question really, and 1 optional (if possible) improvement.

This is going to conflict massively with #124429, lets see who gets in first :)

try {
checkFileRead(callerClass, path, false);
} catch (NoSuchFileException e) {
assert false : "NoSuchFileException should only be thrown when following links";
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the right approach for where we are now; if we go forward with the idea of moving towards exceptions already in the method contract (e.g. IOExceptions for File operations) we will revisit this with all the other methods


// path
void checkPathToRealPath(Class<?> callerClass, Path that, LinkOption... options);
void checkPathToRealPath(Class<?> callerClass, Path that, LinkOption... options) throws NoSuchFileException;
Copy link
Contributor

Choose a reason for hiding this comment

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

Possibly for future work: should we add a check that we match the target method's checked exceptions? We would not like to use an incorrect exception type by mistake, it might end up with another "VerifyError"-style problem again.

Copy link
Contributor

Choose a reason for hiding this comment

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

IIRC the throws clause doesn't affect bytecode, so it can't cause a verification error. It's only there for javac.

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.

LGTM

@mosche mosche added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Mar 11, 2025
@elasticsearchmachine elasticsearchmachine merged commit c26d195 into elastic:main Mar 11, 2025
17 checks passed
@mosche mosche deleted the entitlements/NoSuchFileExceptionForInvalidLinks branch March 11, 2025 09:38
mosche added a commit to mosche/elasticsearch that referenced this pull request Mar 11, 2025
…hecking file entitlements (elastic#124483)

This will rethrow the `NoSuchFileException` when encountering an invalid
symbolic link when following links during file (read) entitlement
checks.

Relates to elastic#124133
(ES-11019)
@elasticsearchmachine
Copy link
Collaborator

💚 Backport successful

Status Branch Result
8.18
8.x
9.0

mosche added a commit to mosche/elasticsearch that referenced this pull request Mar 11, 2025
…hecking file entitlements (elastic#124483)

This will rethrow the `NoSuchFileException` when encountering an invalid
symbolic link when following links during file (read) entitlement
checks.

Relates to elastic#124133
(ES-11019)
mosche added a commit to mosche/elasticsearch that referenced this pull request Mar 11, 2025
…hecking file entitlements (elastic#124483)

This will rethrow the `NoSuchFileException` when encountering an invalid
symbolic link when following links during file (read) entitlement
checks.

Relates to elastic#124133
(ES-11019)
elasticsearchmachine pushed a commit that referenced this pull request Mar 11, 2025
…hecking file entitlements (#124483) (#124543)

This will rethrow the `NoSuchFileException` when encountering an invalid
symbolic link when following links during file (read) entitlement
checks.

Relates to #124133
(ES-11019)
elasticsearchmachine pushed a commit that referenced this pull request Mar 11, 2025
…hecking file entitlements (#124483) (#124541)

This will rethrow the `NoSuchFileException` when encountering an invalid
symbolic link when following links during file (read) entitlement
checks.

Relates to #124133
(ES-11019)
elasticsearchmachine pushed a commit that referenced this pull request Mar 11, 2025
…hecking file entitlements (#124483) (#124542)

This will rethrow the `NoSuchFileException` when encountering an invalid
symbolic link when following links during file (read) entitlement
checks.

Relates to #124133
(ES-11019)
albertzaharovits pushed a commit to albertzaharovits/elasticsearch that referenced this pull request Mar 13, 2025
…hecking file entitlements (elastic#124483)

This will rethrow the `NoSuchFileException` when encountering an invalid
symbolic link when following links during file (read) entitlement
checks.

Relates to elastic#124133
(ES-11019)
jfreden pushed a commit to jfreden/elasticsearch that referenced this pull request Mar 13, 2025
…hecking file entitlements (elastic#124483)

This will rethrow the `NoSuchFileException` when encountering an invalid
symbolic link when following links during file (read) entitlement
checks.

Relates to elastic#124133
(ES-11019)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

auto-backport Automatically create backport pull requests when merged auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) :Core/Infra/Entitlements Entitlements infrastructure >refactoring Team:Core/Infra Meta label for core/infra team v8.18.1 v8.19.0 v9.0.1 v9.1.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants