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

Fix remote cluster credential secure settings reload #111535

Merged
merged 19 commits into from
Aug 9, 2024
Merged

Fix remote cluster credential secure settings reload #111535

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
564f63c
Fix reload secure settings privileges
n1v0lg Aug 2, 2024
85fccbe
Update docs/changelog/111535.yaml
n1v0lg Aug 2, 2024
b89c507
Log and tests
n1v0lg Aug 2, 2024
eda3711
Merge branch 'fix-reload-secure-settings-privileges' of github.com:n1…
n1v0lg Aug 2, 2024
f248cc3
Merge branch 'main' into fix-reload-secure-settings-privileges
n1v0lg Aug 2, 2024
89381dd
Fix tests
n1v0lg Aug 2, 2024
1dec187
Changelog
n1v0lg Aug 2, 2024
f6d905a
Fix changelog
n1v0lg Aug 2, 2024
b7d560f
REST its
n1v0lg Aug 2, 2024
3211c9a
New req instances
n1v0lg Aug 2, 2024
cbde0d4
Update docs/changelog/111535.yaml
n1v0lg Aug 2, 2024
bfdf2eb
Merge branch 'main' into fix-reload-secure-settings-privileges
elasticmachine Aug 2, 2024
7cab1de
Merge branch 'main' into fix-reload-secure-settings-privileges
n1v0lg Aug 5, 2024
52639f3
Rename action instead
n1v0lg Aug 5, 2024
4a70f8f
Merge branch 'fix-reload-secure-settings-privileges' of github.com:n1…
n1v0lg Aug 5, 2024
a2bb171
Operator reg
n1v0lg Aug 5, 2024
b9669f1
Undo
n1v0lg Aug 5, 2024
26d0b32
Merge branch 'main' into fix-reload-secure-settings-privileges
n1v0lg Aug 8, 2024
80d5c66
Merge branch 'main' into fix-reload-secure-settings-privileges
elasticmachine Aug 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/111535.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 111535
summary: Fix reload secure settings privileges
area: Authorization
type: bug
issues: []
1 change: 1 addition & 0 deletions ...sticsearch/action/admin/cluster/node/reload/TransportNodesReloadSecureSettingsAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() + "]");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems useful for future debugging...

try {
p.reload(settingsWithKeystore);
} catch (final Exception e) {
Expand Down
9 changes: 8 additions & 1 deletion ...est/java/org/elasticsearch/xpack/remotecluster/AbstractRemoteClusterSecurityTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import org.apache.http.HttpHost;
import org.apache.http.client.methods.HttpPost;
import org.elasticsearch.client.Request;
import org.elasticsearch.client.RequestOptions;
import org.elasticsearch.client.Response;
import org.elasticsearch.client.RestClient;
import org.elasticsearch.client.RestClientBuilder;
Expand Down Expand Up @@ -48,6 +49,7 @@ public abstract class AbstractRemoteClusterSecurityTestCase extends ESRestTestCa
protected static final String USER = "test_user";
protected static final SecureString PASS = new SecureString("x-pack-test-password".toCharArray());
protected static final String REMOTE_SEARCH_USER = "remote_search_user";
protected static final String MANAGE_USER = "manage_user";
protected static final String REMOTE_METRIC_USER = "remote_metric_user";
protected static final String REMOTE_TRANSFORM_USER = "remote_transform_user";
protected static final String REMOTE_SEARCH_ROLE = "remote_search";
Expand Down Expand Up @@ -220,7 +222,7 @@ protected void removeRemoteClusterCredentials(String clusterAlias, MutableSettin
private void reloadSecureSettings() throws IOException {
final Request request = new Request("POST", "/_nodes/reload_secure_settings");
request.setJsonEntity("{\"secure_settings_password\":\"" + KEYSTORE_PASSWORD + "\"}");
final Response reloadResponse = adminClient().performRequest(request);
final Response reloadResponse = performRequestWithManageUser(request);
assertOK(reloadResponse);
final Map<String, Object> map = entityAsMap(reloadResponse);
assertThat(map.get("nodes"), instanceOf(Map.class));
Expand All @@ -233,6 +235,11 @@ private void reloadSecureSettings() throws IOException {
}
}

private Response performRequestWithManageUser(final Request request) throws IOException {
request.setOptions(RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", headerFromRandomAuthMethod(MANAGE_USER, PASS)));
return client().performRequest(request);
}

protected void putRemoteClusterSettings(
String clusterAlias,
ElasticsearchCluster targetFulfillingCluster,
Expand Down
1 change: 1 addition & 0 deletions ...a/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityReloadCredentialsRestIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,7 @@ public class RemoteClusterSecurityReloadCredentialsRestIT extends AbstractRemote
})
.rolesFile(Resource.fromClasspath("roles.yml"))
.user(REMOTE_SEARCH_USER, PASS.toString(), "read_remote_shared_logs", false)
.user(MANAGE_USER, PASS.toString(), "manage_role", false)
.build();
}

Expand Down
3 changes: 3 additions & 0 deletions x-pack/plugin/security/qa/multi-cluster/src/javaRestTest/resources/roles.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,3 +38,6 @@ ccr_user_role:
- names: [ 'leader-index', 'shared-*', 'metrics-*' ]
privileges: [ 'cross_cluster_replication' ]
clusters: [ "*" ]

manage_role:
cluster: [ 'manage' ]
20 changes: 13 additions & 7 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2202,13 +2202,19 @@ private void reloadRemoteClusterCredentials(Settings settingsWithKeystore) {
return;
}

final PlainActionFuture<ActionResponse.Empty> future = new UnsafePlainActionFuture<>(ThreadPool.Names.GENERIC);
getClient().execute(
ActionTypes.RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION,
new TransportReloadRemoteClusterCredentialsAction.Request(settingsWithKeystore),
future
);
future.actionGet();
// 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()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

ctx.markAsSystemContext();
final PlainActionFuture<ActionResponse.Empty> future = new UnsafePlainActionFuture<>(ThreadPool.Names.GENERIC);
getClient().execute(
ActionTypes.RELOAD_REMOTE_CLUSTER_CREDENTIALS_ACTION,
new TransportReloadRemoteClusterCredentialsAction.Request(settingsWithKeystore),
future
);
future.actionGet();
}
}

public Map<String, String> getAuthContextForSlowLog() {
Expand Down
14 changes: 11 additions & 3 deletions x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/SecurityTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionModule;
import org.elasticsearch.action.ActionResponse;
import org.elasticsearch.action.support.PlainActionFuture;
import org.elasticsearch.client.internal.Client;
import org.elasticsearch.cluster.ClusterName;
import org.elasticsearch.cluster.ClusterState;
Expand Down Expand Up @@ -960,8 +959,11 @@ public <T> List<T> loadExtensions(Class<T> extensionPointType) {
public void testReload() throws Exception {
final Settings settings = Settings.builder().put("xpack.security.enabled", true).put("path.home", createTempDir()).build();

final PlainActionFuture<ActionResponse.Empty> value = new PlainActionFuture<>();
final ThreadPool threadPool = mock(ThreadPool.class);
threadContext = new ThreadContext(Settings.EMPTY);
when(threadPool.getThreadContext()).thenReturn(threadContext);
final Client mockedClient = mock(Client.class);
when(mockedClient.threadPool()).thenReturn(threadPool);

final JwtRealm mockedJwtRealm = mock(JwtRealm.class);
final List<ReloadableSecurityComponent> reloadableComponents = List.of(mockedJwtRealm);
Expand Down Expand Up @@ -992,11 +994,17 @@ protected List<ReloadableSecurityComponent> getReloadableSecurityComponents() {
verify(mockedJwtRealm).reload(same(inputSettings));
}

public void testReloadWithFailures() throws Exception {
public void testReloadWithFailures() {
final Settings settings = Settings.builder().put("xpack.security.enabled", true).put("path.home", createTempDir()).build();

final boolean failRemoteClusterCredentialsReload = randomBoolean();

final ThreadPool threadPool = mock(ThreadPool.class);
threadContext = new ThreadContext(Settings.EMPTY);
when(threadPool.getThreadContext()).thenReturn(threadContext);
final Client mockedClient = mock(Client.class);
when(mockedClient.threadPool()).thenReturn(threadPool);

final JwtRealm mockedJwtRealm = mock(JwtRealm.class);
final List<ReloadableSecurityComponent> reloadableComponents = List.of(mockedJwtRealm);
if (failRemoteClusterCredentialsReload) {
Expand Down