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.

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

Block specific config files from being read after startup #107481

Merged
merged 16 commits into from Apr 29, 2024

Conversation

thecoop
Copy link
Member

@thecoop thecoop commented Apr 15, 2024

Block specific config files from being read by anything in elasticsearch after startup. There's no reason for anything to read these files after node initialization.

@thecoop thecoop added >non-issue :Security/Security Security issues without another label :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown labels Apr 15, 2024
@thecoop thecoop marked this pull request as ready for review April 16, 2024 10:29
@thecoop thecoop requested a review from a team as a code owner April 16, 2024 10:29
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team labels Apr 16, 2024
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-security (Team:Security)

@elasticsearchmachine
Copy link
Collaborator

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

@thecoop thecoop added v7.17.21 auto-backport Automatically create backport pull requests when merged and removed Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team labels Apr 16, 2024
@elasticsearchmachine elasticsearchmachine added Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team labels Apr 16, 2024
@thecoop thecoop requested a review from rjernst April 16, 2024 10:32
@thecoop thecoop added >bug and removed >non-issue labels Apr 16, 2024
@elasticsearchmachine
Copy link
Collaborator

Hi @thecoop, I've created a changelog YAML for you.

@thecoop thecoop requested a review from tvernum April 16, 2024 13:52
Copy link
Contributor

@mosche mosche left a comment

Choose a reason for hiding this comment

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

Nice, lgtm 👍
Only wondering how much visibility we need here.

private static List<FilePermission> createForbiddenFilePermissions(Environment environment) throws IOException {
Permissions policy = new Permissions();
addSingleFilePath(policy, environment.configFile().resolve("elasticsearch.yml"), "read,readlink");
addSingleFilePath(policy, environment.configFile().resolve("jvm.options"), "read,readlink");
Copy link
Contributor

Choose a reason for hiding this comment

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

What about jvm.options.d ? I know we didn't mention it when we planned this work, but in my mind it was implied by jvm.options.

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've added a check for that too

@thecoop thecoop requested a review from tvernum April 23, 2024 09:05
@thecoop
Copy link
Member Author

thecoop commented Apr 24, 2024

@elasticmachine update branch

Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I have one main question about how these denied accesses are surfaced.

@@ -93,17 +106,16 @@ public boolean implies(ProtectionDomain domain, Permission permission) {
}
}

if (permission instanceof FilePermission) {
Copy link
Member

Choose a reason for hiding this comment

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

IIRC this was here to avoid the (potentially relatively expensive) implies call on a permission collection. Can we add this conditional back, and also guard the new check in the same way?

Copy link
Member Author

Choose a reason for hiding this comment

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

The first thing FilePermissionCollection does is check the type of the argument - which is only duplicated by having a check here. So I don't think it affects much?

my_stemmer_override:
type: stemmer_override
rules_path: "jvm.options"
- match: { status: 500 }
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 catch this in the same way FileWatcher was modified, to result in a 400 instead of a 500?

Copy link
Member Author

@thecoop thecoop Apr 26, 2024

Choose a reason for hiding this comment

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

I've changed it to a 400 response code

@thecoop thecoop requested a review from rjernst April 26, 2024 10:49
Copy link
Member

@rjernst rjernst left a comment

Choose a reason for hiding this comment

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

LGTM

@thecoop thecoop merged commit 177dc26 into elastic:main Apr 29, 2024
17 of 18 checks passed
@thecoop thecoop deleted the security-config-files branch April 29, 2024 08:58
@elasticsearchmachine
Copy link
Collaborator

💔 Backport failed

Status Branch Result
7.17 Commit could not be cherrypicked due to conflicts

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

thecoop added a commit to thecoop/elasticsearch that referenced this pull request Apr 29, 2024
Block specific config files from being accessed after startup (elastic#107481)

Some files should never be accessed by ES or plugin code once startup has completed. Use the security manager to block these files from being accessed by anything at all. The current blocked files are elasticsearch.yml, jvm.options, and the jvm.options.d directory.
thecoop added a commit that referenced this pull request Apr 30, 2024
Block specific config files from being accessed after startup (#107481)

Some files should never be accessed by ES or plugin code once startup has completed. Use the security manager to block these files from being accessed by anything at all. The current blocked files are elasticsearch.yml, jvm.options, and the jvm.options.d directory.
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 >bug :Core/Infra/Node Lifecycle Node startup, bootstrapping, and shutdown :Security/Security Security issues without another label Team:Core/Infra Meta label for core/infra team Team:Security Meta label for security team v7.17.22 v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants