Fix remote cluster credential secure settings reload by n1v0lg · Pull Request #111535 · elastic/elasticsearch

n1v0lg · 2024-08-02T08:19:22Z

Due to the cluster:admin/xpack/security action name prefix, the internal action cluster:admin/xpack/security/remote_cluster_credentials/reload to reload remote cluster credentials fails for users that have the manage cluster privilege. This does not align with our documentation and the overall permission requirements for reloading secure settings.

This PR renames the action to match the manage privilege. Since this is a local-only action there are no BWC concerns with this rename.

Fixes: #111543

elasticsearchmachine · 2024-08-02T08:20:08Z

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

…v0lg/elasticsearch into fix-reload-secure-settings-privileges

n1v0lg · 2024-08-02T10:08:33Z

...elasticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java

            final List<Exception> exceptions = new ArrayList<>();
            // broadcast the new settings object (with the open embedded keystore) to all reloadable plugins
            pluginsService.filterPlugins(ReloadablePlugin.class).forEach(p -> {
+                logger.debug("Reloading plugin [" + p.getClass().getSimpleName() + "]");


Seems useful for future debugging...

elasticsearchmachine · 2024-08-02T13:02:50Z

Hi @n1v0lg, I've updated the changelog YAML for you.

elasticsearchmachine · 2024-08-02T13:02:50Z

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

n1v0lg · 2024-08-02T13:08:25Z

@elasticmachine update branch

jakelandis

Question about where we should mark it as the system context.

jakelandis · 2024-08-02T17:10:08Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

+        // Run this action in system context -- it was authorized upstream and should not be tied to end-user permissions
+        final ThreadContext ctx = getClient().threadPool().getThreadContext();
+        assert ctx != null : "Thread context must be set for reload call";
+        try (ThreadContext.StoredContext ignore = ctx.stashContext()) {


Can we stash the context and mark as system context from TransportNodesReloadSecureSettingsAction instead of here ? It feels off that we can call this method directly from anywhere that we can reference Security#reload and the work will be done in the system context, possibly having never run any authz.

Since the expectation is that work is implicitly allowed as a sub action from TransportNodesReloadSecureSettingsAction, I think having TransportNodesReloadSecureSettingsAction mark it as the system context is more correct since. It would also apply to all other sub reload actions, but that I think is also more correct since the permissions to use TransportNodesReloadSecureSettingsAction need to converge to a single permission.

Alternatively, I think we could change RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION to an actual "internal:" action, helping to re-inforce that this is sub action / implementation detail of another action. But that would I prefer keeping the action name as-is and ensuring that the flip to system context is a bit more isolated to it's usage.

I included it here primarily because it's an implementation detail that it's an action to begin with -- most other reload calls happen at the "service" layer and are not subject to authz at all -- it would be the same here if we had access to the right services within the Security plugin (we'd just call RemoteClusterService.reload(...) without the action workaround, but unfortunately, RemoteClusterService is not available in Security. To me it actually feels more isolated to have it here.

However, I don't feel strongly -- TransportNodesReloadSecureSettingsAction is a fine place to switch to system context (though we'll have to double-check that we're not somehow switching back out of it along the way) -- let me know if you strongly prefer TransportNodesReloadSecureSettingsAction instead and I pull the switch over there.

Discussed on Slack -- switching to system context in TransportNodesReloadSecureSettingsAction has several downsides.

Since Security#reload is a public method that could (in theory) be called in a context that did not ensure proper authz it's however better to also avoid the system context switch inside reload.

To accommodate this, we've decided to simply rename the action to have a prefix that's covered by manage. Since it's a local-only action there are not BWC concerns with this.

…v0lg/elasticsearch into fix-reload-secure-settings-privileges

jakelandis

LGTM, thanks for the iterations.

(super nitpick: the PR description "Due to a missing system context switch,..." part is a bit misleading now)

n1v0lg · 2024-08-09T08:09:15Z

@elasticmachine update branch

Due to the `cluster:admin/xpack/security` action name prefix, the internal action `cluster:admin/xpack/security/remote_cluster_credentials/reload` to reload remote cluster credentials fails for users that have the `manage` cluster privilege. This does not align with our documentation and the overall permission requirements for reloading secure settings. This PR renames the action to match the `manage` privilege. Since this is a local-only action there are no BWC concerns with this rename. Fixes: elastic#111543

elasticsearchmachine · 2024-08-09T09:08:40Z

💚 Backport successful

Status	Branch	Result
✅	8.15

* upstream/main: (22 commits) Prune changelogs after 8.15.0 release Bump versions after 8.15.0 release EIS integration (elastic#111154) Skip LOOKUP/INLINESTATS cases unless on snapshot (elastic#111755) Always enforce strict role validation (elastic#111056) Mute org.elasticsearch.xpack.esql.analysis.VerifierTests testUnsupportedAndMultiTypedFields elastic#111753 [ML] Force time shift integration test (elastic#111620) ESQL: Add tests for sort, where with unsupported type (elastic#111737) [ML] Force time shift documentation (elastic#111668) Fix remote cluster credential secure settings reload (elastic#111535) ESQL: Fix for overzealous validation in case of invalid mapped fields (elastic#111475) Pass allow security manager flag in gradle test policy setup plugin (elastic#111725) Rename streamContent/Separator to bulkContent/Separator (elastic#111716) Mute org.elasticsearch.tdigest.ComparisonTests testSparseGaussianDistribution elastic#111721 Remove 8.14 from branches.json Only emit product origin in deprecation log if present (elastic#111683) Forward port release notes for v8.15.0 (elastic#111714) [ES|QL] Combine Disjunctive CIDRMatch (elastic#111501) ESQL: Remove qualifier from attrs (elastic#110581) Force using the last centroid during merging (elastic#111644) ... # Conflicts: # server/src/main/java/org/elasticsearch/TransportVersions.java # x-pack/plugin/inference/src/main/java/org/elasticsearch/xpack/inference/InferenceNamedWriteablesProvider.java

Due to the `cluster:admin/xpack/security` action name prefix, the internal action `cluster:admin/xpack/security/remote_cluster_credentials/reload` to reload remote cluster credentials fails for users that have the `manage` cluster privilege. This does not align with our documentation and the overall permission requirements for reloading secure settings. This PR renames the action to match the `manage` privilege. Since this is a local-only action there are no BWC concerns with this rename. Fixes: elastic#111543

Due to the `cluster:admin/xpack/security` action name prefix, the internal action `cluster:admin/xpack/security/remote_cluster_credentials/reload` to reload remote cluster credentials fails for users that have the `manage` cluster privilege. This does not align with our documentation and the overall permission requirements for reloading secure settings. This PR renames the action to match the `manage` privilege. Since this is a local-only action there are no BWC concerns with this rename. Fixes: #111543 Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>

Fix reload secure settings privileges

564f63c

n1v0lg added >bug :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Aug 2, 2024

n1v0lg self-assigned this Aug 2, 2024

elasticsearchmachine added the v8.16.0 label Aug 2, 2024

Update docs/changelog/111535.yaml

85fccbe

n1v0lg added 2 commits August 2, 2024 11:34

Log and tests

b89c507

Merge branch 'fix-reload-secure-settings-privileges' of github.com:n1…

eda3711

…v0lg/elasticsearch into fix-reload-secure-settings-privileges

n1v0lg added v8.15.0 auto-backport Automatically create backport pull requests when merged labels Aug 2, 2024

n1v0lg commented Aug 2, 2024

View reviewed changes

Merge branch 'main' into fix-reload-secure-settings-privileges

f248cc3

n1v0lg changed the title ~~Fix reload secure settings privileges~~ Fix remote cluster credential secure settings reload Aug 2, 2024

n1v0lg added 5 commits August 2, 2024 12:54

Fix tests

89381dd

Changelog

1dec187

Fix changelog

f6d905a

REST its

b7d560f

New req instances

3211c9a

n1v0lg marked this pull request as ready for review August 2, 2024 13:02

elasticsearchmachine added the Team:Security Meta label for security team label Aug 2, 2024

Update docs/changelog/111535.yaml

cbde0d4

n1v0lg requested a review from jakelandis August 2, 2024 13:06

Merge branch 'main' into fix-reload-secure-settings-privileges

bfdf2eb

jakelandis requested changes Aug 2, 2024

View reviewed changes

n1v0lg added 2 commits August 5, 2024 18:59

Merge branch 'main' into fix-reload-secure-settings-privileges

7cab1de

Rename action instead

52639f3

n1v0lg added 2 commits August 5, 2024 19:09

Merge branch 'fix-reload-secure-settings-privileges' of github.com:n1…

4a70f8f

…v0lg/elasticsearch into fix-reload-secure-settings-privileges

Operator reg

a2bb171

n1v0lg requested a review from a team as a code owner August 5, 2024 18:55

n1v0lg added 2 commits August 5, 2024 21:52

Undo

b9669f1

Merge branch 'main' into fix-reload-secure-settings-privileges

26d0b32

n1v0lg requested a review from jakelandis August 8, 2024 09:04

jakelandis approved these changes Aug 8, 2024

View reviewed changes

n1v0lg added v8.15.1 and removed v8.15.0 labels Aug 9, 2024

Merge branch 'main' into fix-reload-secure-settings-privileges

80d5c66

n1v0lg added test-update-serverless auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) labels Aug 9, 2024

elasticsearchmachine merged commit d99f871 into elastic:main Aug 9, 2024

n1v0lg deleted the fix-reload-secure-settings-privileges branch August 9, 2024 09:07

n1v0lg mentioned this pull request Aug 9, 2024

[8.15] Fix remote cluster credential secure settings reload (#111535) #111738

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix remote cluster credential secure settings reload #111535

Fix remote cluster credential secure settings reload #111535
elasticsearchmachine merged 19 commits intoelastic:mainfrom
n1v0lg:fix-reload-secure-settings-privileges

n1v0lg commented Aug 2, 2024 •

edited

Loading

Uh oh!

elasticsearchmachine commented Aug 2, 2024

Uh oh!

n1v0lg Aug 2, 2024

Uh oh!

elasticsearchmachine commented Aug 2, 2024

Uh oh!

elasticsearchmachine commented Aug 2, 2024

Uh oh!

n1v0lg commented Aug 2, 2024

Uh oh!

jakelandis left a comment

Uh oh!

jakelandis Aug 2, 2024

Uh oh!

n1v0lg Aug 2, 2024

Uh oh!

n1v0lg Aug 5, 2024

Uh oh!

jakelandis left a comment

Uh oh!

n1v0lg commented Aug 9, 2024

Uh oh!

elasticsearchmachine commented Aug 9, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

n1v0lg commented Aug 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticsearchmachine commented Aug 2, 2024

Uh oh!

n1v0lg Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Aug 2, 2024

Uh oh!

elasticsearchmachine commented Aug 2, 2024

Uh oh!

n1v0lg commented Aug 2, 2024

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

jakelandis Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

n1v0lg Aug 2, 2024

Choose a reason for hiding this comment

Uh oh!

n1v0lg Aug 5, 2024

Choose a reason for hiding this comment

Uh oh!

jakelandis left a comment

Choose a reason for hiding this comment

Uh oh!

n1v0lg commented Aug 9, 2024

Uh oh!

elasticsearchmachine commented Aug 9, 2024

💚 Backport successful

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

n1v0lg commented Aug 2, 2024 •

edited

Loading