Fix remote cluster credential secure settings reload #111535
Conversation
|
Hi @n1v0lg, I've created a changelog YAML for you. |
…v0lg/elasticsearch into fix-reload-secure-settings-privileges
n1v0lg
added
v8.15.0
auto-backport
| @@ -123,6 +123,7 @@ protected NodesReloadSecureSettingsResponse.NodeResponse nodeOperation( | |||
| 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() + "]"); | |||
There was a problem hiding this comment.
Seems useful for future debugging...
n1v0lg
changed the title
|
Hi @n1v0lg, I've updated the changelog YAML for you. |
|
Pinging @elastic/es-security (Team:Security) |
|
@elasticmachine update branch |
| // 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()) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
|
@elasticmachine update branch |
n1v0lg
added
test-update-serverless
auto-merge-without-approval
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
💚 Backport successful
|
* 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: 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 <[email protected]>
Due to the
cluster:admin/xpack/securityaction name prefix, the internal actioncluster:admin/xpack/security/remote_cluster_credentials/reloadto reload remote cluster credentials fails for users that have themanagecluster 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
manageprivilege. Since this is a local-only action there are no BWC concerns with this rename.Fixes: #111543