Hot-reloadable remote cluster credentials #102798

n1v0lg · 2023-11-30T08:56:50Z

This PR enables RCS 2.0 remote clusters to be configured without the need to restart nodes. It works as the follows (assuming both clusters are already running):

Get a cross-cluster API key for accessing the remote cluster

Add cross-cluster API key to keystores of the local cluster, e.g.

echo -n xxx | ./bin/elasticsearch-keystore add cluster.remote.my.credentials -x

Call ReloadSecureSettings API on the local cluster

Configure RCS 2.0 remote cluster should now just work for the local cluster, e.g.

PUT /_cluster/settings
{"persistent":{"cluster":{"remote":{"my":{"seeds":["127.0.0.1:9443"]}}}}}

This PR does not include functionality to automatically re-build connections on secure settings reload. I will add this in a follow up PR.

The high level technical approach is to maintain a credentials manager class and use this to attach credentials for connections to remote clusters. This comment also provides more context on some lower level details.

Relates: #98120
Relates: ES-6764

elasticsearchmachine · 2023-11-30T08:57:38Z

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

…s2-reload

n1v0lg · 2023-12-07T15:44:45Z

@elasticmachine update branch

n1v0lg · 2023-12-07T16:13:25Z

CI failure is unrelated:

Execution failed for task ':server:test'.
> Process 'Gradle Test Executor 237' finished with non-zero exit value 134
  This problem might be caused by incorrect test process configuration.
  For more on test execution, please refer to https://docs.gradle.org/8.5/userguide/java_testing.html#sec:test_execution in the Gradle documentation.

I thought this was fixed by: #103112 though... I'll dig into this but it should not prevent a review.

ChrisHegarty · 2023-12-07T17:18:46Z

CI failure is unrelated:

Execution failed for task ':server:test'.
> Process 'Gradle Test Executor 237' finished with non-zero exit value 134
  This problem might be caused by incorrect test process configuration.
  For more on test execution, please refer to https://docs.gradle.org/8.5/userguide/java_testing.html#sec:test_execution in the Gradle documentation.

I thought this was fixed by: #103112 though... I'll dig into this but it should not prevent a review.

The fix for 103112 was not in your merge with main 5f91ae7. It should be there now with your subsequent merge with main, c865955. If you see an issue now, then we have a serious problem.

elasticsearchmachine · 2023-12-07T17:19:41Z

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

jakelandis

LGTM - nice work !

A couple nitpicks (nits:) (feel free to ignore entirely) and a couple non-blocking comments.

...sticsearch/xpack/security/action/settings/TransportReloadRemoteClusterCredentialsAction.java

jakelandis · 2023-12-07T21:13:32Z

...main/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptor.java

                    logger.trace("No cluster credentials are configured for remote cluster [{}]", remoteClusterAlias);
                    return Optional.empty();
                }

-                return remoteClusterCredentials;
+                return Optional.of(
+                    new RemoteClusterCredentials(remoteClusterAlias, ApiKeyService.withApiKeyPrefix(remoteClusterCredentials.toString()))


nit: can we reuse RemoteClusterAliasWithCredentials to avoid different records that serve a similar purpose with only minor distinction and keeps credential in a SecureString. Could probably promote RemoteClusterAliasWithCredentials to a top level object and add a method to add the prefix (and method to check if it prefixed) and return a new record.

I wanted to do this originally, but the ApiKey prefix logic is a security concept, and the method lives in xpack, which makes it inaccessible in the core remote cluster code. I think it makes sense to keep the two different records, as they conceptually belong to two different domains and we're translating between these here.

jakelandis · 2023-12-07T21:16:40Z

server/src/main/java/org/elasticsearch/transport/RemoteConnectionManager.java

+    public record RemoteClusterAliasWithCredentials(String clusterAlias, @Nullable SecureString credentials) {
+        @Override
+        public String toString() {
+            return "RemoteClusterAliasWithCredentials{clusterAlias='" + clusterAlias + "', credentials='::es_redacted::'}";


++ and really surprised that we don't do this automatically with SecureString#toString() (we should fix that outside of this PR)

It's trappy for sure, but that's currently how to translate between SecureString and a regular string (e.g., for sending cross cluster access headers, or when dealing with third party deps that don't have a concept of SecureString). Not saying, we shouldn't change it -- just that it's going to be a slightly trickier refactor.

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

jakelandis · 2023-12-07T22:49:31Z

...ernalClusterTest/java/org/elasticsearch/xpack/security/ReloadRemoteClusterCredentialsIT.java

+            // Update credentials and ensure they are used
+            final String updatedCredentials = randomAlphaOfLength(41);
+            writeCredentialsToKeyStore(updatedCredentials);
+            reloadSecureSettings();


I'm not sure I fully understand the relationship between connection profile and the need to rebuild the connection. Once the connection is established (TCP + handshake) it should remain open and if need to re-establish will use the new credential. Per request will use the new credential. I assumed that the rebuilding the connection was for 1) ensure we disconnect any old connections that we established with the old credential (more clean up than anything) 2) so that we can error or at least warn if the new credential is not valid on the reload call. I can't remember if it is possible to explictly change the underlying transport profile (https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-network.html#transport-profiles) but we intentionally didn't document the one used for RCS 2.0 to discourage direct configuration there and any related options that influence the underlying transport profile are exposed as dedicated settings and are not related to the credential reload.

EDIT: this in reference to #102798 (comment) ... not sure why GH decided add the comment outside of the reply to that comment.

n1v0lg · 2023-12-08T09:04:36Z

@ChrisHegarty sorry I missed your comment, yup, it looks like that build did not include your fix; after merging main again the build passed just fine. Thanks for investigating this!

n1v0lg · 2023-12-08T14:03:31Z

@elasticmachine update branch bitte

This reverts commit 7ded906.

Reverts #102798

…103211)" This reverts commit 9db4cb4.

…103215) Brings back #102798, with a tweak to avoid tripping on a blocking operation. Only change compared to the original PR is 4072fac

This PR builds on #102798 by adding automatic remote connection rebuilding on cluster credentials changes. In particular, we rebuild a remote cluster connection if a credential for the associated cluster is newly added (i.e., we are moving from RCS 1.0 -> RCS 2.0) or removed (moving from RCS 2.0 -> RCS 1.0). A connection rebuild allows us to associate the correct profile (`_remote_server` in case of RCS 2.0, or "regular" transport profile for RCS 1.0) without requiring end-users to manually update remote cluster settings via a settings update call. More context on connection rebuilding also in this [comment](https://github.com/elastic/elasticsearch/pull/102798/files#r1420454541). Relates: ES-6764

ywangd and others added 11 commits August 2, 2023 13:33

working prototype

57c5b83

fix tests for remote connections

6c43819

fix security tests

6b5b50a

tweak

1c70ac8

Merge branch 'main' into rcs2-remote-cluster-no-restart

bfed01d

WIP poc rcs reloadable keys

d4f35bb

Merge branch 'main' into rcs2-reload

8ffd630

Hot reload remote cluster credentials

93485f8

WIP integ test

13da6f7

Merge branch 'main' into rcs2-reload

31714e7

Tweaks

4c086c0

n1v0lg added >enhancement :Security/Security Security issues without another label labels Nov 30, 2023

n1v0lg self-assigned this Nov 30, 2023

elasticsearchmachine added the v8.12.0 label Nov 30, 2023

Update docs/changelog/102798.yaml

e46a98c

n1v0lg added 13 commits November 30, 2023 10:05

License headers

174c390

Delete

24fd9a8

Merge branch 'rcs2-reload' of github.com:n1v0lg/elasticsearch into rc…

f477953

…s2-reload

Store creds with connection

324f84e

Fix tests

ac53bcb

Merge branch 'main' into rcs2-reload

7a2e956

Gnarly test with refactoring

a95b016

Remove unused

a32d375

Clean up

99b6d4c

Test update

a6f0912

TODO

e348ef6

Clear

b0a515d

Clean up and randomized connection

f15d588

Merge branch 'main' into rcs2-reload

5f91ae7

Merge branch 'main' into rcs2-reload

c865955

n1v0lg marked this pull request as ready for review December 7, 2023 17:19

elasticsearchmachine added the Team:Security Meta label for security team label Dec 7, 2023

jakelandis approved these changes Dec 7, 2023

View reviewed changes

Address review feedback

cd95799

elasticmachine and others added 2 commits December 9, 2023 01:03

Merge branch 'main' into rcs2-reload

6fb39b0

Nit

31fed5f

n1v0lg added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Dec 8, 2023

n1v0lg added 2 commits December 8, 2023 15:14

Grammar still

197b2cf

One more

d03e1d8

elasticsearchmachine merged commit 7ded906 into elastic:main Dec 8, 2023
19 checks passed

n1v0lg deleted the rcs2-reload branch December 8, 2023 15:09

n1v0lg added a commit that referenced this pull request Dec 8, 2023

Revert "Hot-reloadable remote cluster credentials (#102798)"

795fb8b

This reverts commit 7ded906.

n1v0lg mentioned this pull request Dec 8, 2023

Revert "Hot-reloadable remote cluster credentials" #103211

Merged

elasticsearchmachine pushed a commit that referenced this pull request Dec 8, 2023

Revert "Hot-reloadable remote cluster credentials (#102798)" (#103211)

9db4cb4

Reverts #102798

n1v0lg added a commit that referenced this pull request Dec 8, 2023

Revert "Revert "Hot-reloadable remote cluster credentials (#102798)" (#…

6007cf8

…103211)" This reverts commit 9db4cb4.

n1v0lg mentioned this pull request Dec 8, 2023

Hot-reloadable remote cluster credentials (with blocking call fix) #103215

Merged

n1v0lg mentioned this pull request Dec 20, 2023

Re-build remote cluster connections on credential changes #103460

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hot-reloadable remote cluster credentials #102798

Hot-reloadable remote cluster credentials #102798

n1v0lg commented Nov 30, 2023 •

edited

Loading

elasticsearchmachine commented Nov 30, 2023

n1v0lg commented Dec 7, 2023

n1v0lg commented Dec 7, 2023

ChrisHegarty commented Dec 7, 2023

elasticsearchmachine commented Dec 7, 2023

jakelandis left a comment

jakelandis Dec 7, 2023

n1v0lg Dec 8, 2023

jakelandis Dec 7, 2023

n1v0lg Dec 8, 2023

jakelandis Dec 7, 2023 •

edited

Loading

n1v0lg commented Dec 8, 2023

n1v0lg commented Dec 8, 2023

Hot-reloadable remote cluster credentials #102798

Hot-reloadable remote cluster credentials #102798

Conversation

n1v0lg commented Nov 30, 2023 • edited Loading

elasticsearchmachine commented Nov 30, 2023

n1v0lg commented Dec 7, 2023

n1v0lg commented Dec 7, 2023

ChrisHegarty commented Dec 7, 2023

elasticsearchmachine commented Dec 7, 2023

jakelandis left a comment

Choose a reason for hiding this comment

jakelandis Dec 7, 2023

Choose a reason for hiding this comment

n1v0lg Dec 8, 2023

Choose a reason for hiding this comment

jakelandis Dec 7, 2023

Choose a reason for hiding this comment

n1v0lg Dec 8, 2023

Choose a reason for hiding this comment

jakelandis Dec 7, 2023 • edited Loading

Choose a reason for hiding this comment

n1v0lg commented Dec 8, 2023

n1v0lg commented Dec 8, 2023

n1v0lg commented Nov 30, 2023 •

edited

Loading

jakelandis Dec 7, 2023 •

edited

Loading