Indexify CCR internal actions for RCS 2.0 #95264

ywangd · 2023-04-17T06:11:51Z

This PR adds CCR support for RCS 2.0 by pairing its internal actions with indices actions which are safe to use across different clusters.

.../main/java/org/elasticsearch/xpack/ccr/action/repositories/GetCcrRestoreFileChunkAction.java

ywangd · 2023-04-17T08:56:12Z

...main/java/org/elasticsearch/xpack/ccr/action/repositories/ClearCcrRestoreSessionRequest.java

+    @Override
+    public String[] indices() {
+        if (shardId == null) {
+            return null;
+        } else {
+            return new String[] { shardId.getIndexName() };
+        }
+    }


Using just the index name from the shard works because CCR does not support aliases. Otherwise it could be a lot of trouble to keep the original indices.

ywangd · 2023-04-17T08:57:23Z

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

+                    final String effectiveAction = RCS_ACTION_NAME_LOOKUP.getOrDefault(action, action);
+                    if (false == effectiveAction.equals(action)) {
+                        logger.info("switching action from [{}] to [{}]", action, effectiveAction);
+                    }
+                    sendWithCrossClusterAccessHeaders(crossClusterAccessHeaders, connection, effectiveAction, request, options, handler);


Rewrite the request here to use the indices actions instead of internal actions to honor indices privileges. This also means, for existing remote cluster model, it will keep using the internal actions.

ywangd · 2023-04-17T08:58:50Z

...main/java/org/elasticsearch/xpack/ccr/action/repositories/ClearCcrRestoreSessionRequest.java

+        if (out.getTransportVersion().onOrAfter(TransportVersion.V_8_8_0)) {
+            shardId.writeTo(out);
+        }


Since RCS 2.0 is after 8.8, dropping this field for older versions is not an issue at all.

elasticsearchmachine · 2023-04-24T07:07:17Z

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

ywangd · 2023-04-24T07:10:08Z

...a/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityCcrFcActionAuthorizationIT.java

+import static org.hamcrest.Matchers.hasSize;
+import static org.hamcrest.Matchers.is;
+
+public class RemoteClusterSecurityCcrFcActionAuthorizationIT extends ESRestTestCase {


This test class does not extends the abstract RCS base class because it only uses a single cluster. The single cluster is used as the leader and the test uses a TransportService to directly hit the remote server interface. This allows us to test requests and beheviours that should not happen with normal software execution environment, i.e. it simulates potentially malicious behaviours.

ywangd · 2023-04-24T07:19:06Z

...plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRestoreSourceService.java

+            if (false == getMetadata().contains(fileName)) {
+                throw new IllegalArgumentException("invalid file name [" + fileName + "]");
+            }


This logic here as well as the above ensureSessionShardIdConsistency method are behaviour change for existing CCR as well. They feel safe enough and are likely useful enhancements. We could have a distributed team member to take a look to be sure. Alternatively we could try conditionally disable these checks for RCS 1 leader clusters. But it may not be easy since this service layer does not have knowledge about the cluster connections.

I'm leaning towards not disabling them conditionally since as you say it would be complicated to do this; besides, these seem like reasonable validation steps.

As you suggest here it actually makes sense (and isn't too challenging) to avoid this for old-style actions.

One more unrelated sanity check: the filenames are for Lucene-maintained segment info files, with a fixed format of segment_N; having poked around the code a bit (e.g. this is the relevant ES-side part) I don't think there is a way for someone to maliciously (and manually) add a file with a name that would enable directory traversal, unless perhaps they had access to the file system; in that case though they would not benefit from a directory traversal attack to begin with. Was wondering if this is roughly what you have found as well.

Yes basically that. Also, I don't think it is possible to physically add a file with a name that enables directory traversal. The file sytsem won't allow such file name to exist in the first place.

ywangd · 2023-04-24T07:22:31Z

...in/core/src/main/java/org/elasticsearch/xpack/core/security/user/CrossClusterAccessUser.java

+                    RetentionLeaseActions.Add.ACTION_NAME,
+                    RetentionLeaseActions.Remove.ACTION_NAME,
+                    RetentionLeaseActions.Renew.ACTION_NAME,
+                    "indices:monitor/stats",
+                    "indices:internal/admin/ccr/restore/session/put",
+                    "indices:internal/admin/ccr/restore/session/clear",
+                    "internal:transport/proxy/indices:internal/admin/ccr/restore/session/clear",
+                    "indices:internal/admin/ccr/restore/file_chunk/get",
+                    "internal:transport/proxy/indices:internal/admin/ccr/restore/file_chunk/get"


I will have a separate PR for adding new consolidated CCR/CCS privileges.

n1v0lg

This is looking great. I made a first pass today -- only nits, and one sanity check so far. Will review the rest, including test tomorrow.

...src/main/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesAction.java

n1v0lg · 2023-04-24T11:30:34Z

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

+                // Cross cluster access user can perform has privilege check
+                if (authentication.isCrossClusterAccess() && HasPrivilegesAction.NAME.equals(action)) {
+                    assert request instanceof HasPrivilegesRequest;
+                    return ((Authentication) authentication.getAuthenticatingSubject()


Optional nit: getRemoteAuthenticationFromMetadata or getAuthenticationFromCrossClusterAccessMetadata or something similar could be a nice util to add to Authentication.

((Authentication) authentication.getAuthenticatingSubject().getMetadata().get(CROSS_CLUSTER_ACCESS_AUTHENTICATION_KEY)) feels pretty long and distracting, esp. because of the cast.

I considered that. But I was a bit hesitate to add such method to Authentication because we don't even have a getMetadata method for it since metadata right now is a subject level information. So for now I added getAuthenticationFromCrossClusterAccessMetadata as a static method to the Authentication class. We can iterate in future to further rationalize how Authentication information should be distributed.

...java/org/elasticsearch/xpack/security/transport/CrossClusterAccessServerTransportFilter.java

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

n1v0lg · 2023-04-24T20:13:45Z

.../main/java/org/elasticsearch/xpack/ccr/action/repositories/ClearCcrRestoreSessionAction.java

            this.ccrRestoreService = ccrRestoreService;
        }

        @Override
        protected void doExecute(Task task, ClearCcrRestoreSessionRequest request, ActionListener<ActionResponse.Empty> listener) {
+            final ShardId shardId = request.getShardId();
+            // It can be null if the request is sent from an old node with the internal action
+            if (shardId != null) {


Sanity check: if shardId is null, for example for ClearCcrRestoreSessionRequest, then authorization is guaranteed to fail for the new-style action (indices:internal/admin/ccr/restore/session/clear) here:

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

Lines 149 to 151 in 3bd0032

if (indices == null || indices.length == 0) {

throw new IllegalArgumentException("the action " + action + " requires explicit index names, but none were provided");

}

For the old-style action (internal:admin/ccr/restore/session/clear) on the other hand, we won't hit the above IAE because authz won't go through the regular authz flow.

Is my understanding correct here? If so, I wonder if it makes sense to fail early for new style actions if shardId is null, e.g., by making doExecute or parts of it abstract, and overriding for InternalTransportAction and TransportAction accordingly. After all the action is guaranteed to fail.

Apologies if I'm missing something here. I ran out of time getting to the bottom of this today.

What you said makes perfect sense! Thanks for the prompt. I was focusing on minimize the changes of different transport actions and forgot about that we could actual leverage the difference. In this case, you are absolutely right that we should fail for null in the new TransportAction and only have this leniency in the internal one.

This also prompts me to think twice about this comment. Since we have separate transport actions for different RCS models, we could lift the check up into the transport action so that it does not affect existing model. This is my current preference because conditional check for the ShardId here would also make the change not affect existing model. It would be more consistent if both ShardId and fileName checks either affect the existing model or not at all. What do you think?

Nice, glad I was onto something. Yup, I'm ++ on lifting the other check to the transport action as well. It's good to minimize the impact on the old model and we can always retrofit the check if we deem it worthwhile in the future.

n1v0lg

I've gone through the test code as well. All looks good. Once the adjustments around separating old-style from new-style actions are made, I will take another quick look but from my perspective this is basically ready to ship!

.../src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityCcrIT.java

...est/java/org/elasticsearch/xpack/security/action/user/TransportHasPrivilegesActionTests.java

n1v0lg · 2023-04-25T12:28:57Z

...plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRestoreSourceService.java

+            if (false == getMetadata().contains(fileName)) {
+                throw new IllegalArgumentException("invalid file name [" + fileName + "]");
+            }


As you suggest here it actually makes sense (and isn't too challenging) to avoid this for old-style actions.

n1v0lg · 2023-04-25T13:28:43Z

...plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRestoreSourceService.java

+            if (false == getMetadata().contains(fileName)) {
+                throw new IllegalArgumentException("invalid file name [" + fileName + "]");
+            }


One more unrelated sanity check: the filenames are for Lucene-maintained segment info files, with a fixed format of segment_N; having poked around the code a bit (e.g. this is the relevant ES-side part) I don't think there is a way for someone to maliciously (and manually) add a file with a name that would enable directory traversal, unless perhaps they had access to the file system; in that case though they would not benefit from a directory traversal attack to begin with. Was wondering if this is roughly what you have found as well.

…ecurity/transport/SecurityServerTransportInterceptor.java Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>

ywangd · 2023-04-26T07:45:31Z

...plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRestoreSourceService.java

+    public void ensureSessionShardIdConsistency(String sessionUUID, ShardId shardId) {
+        final RestoreSession restore = onGoingRestores.get(sessionUUID);
+        if (restore == null) {
+            logger.debug("could not get session [{}] because session not found", sessionUUID);
+            throw new IllegalArgumentException("session [" + sessionUUID + "] not found");
+        }
+        final ShardId sessionShardId = restore.indexShard.shardId();
+        if (false == sessionShardId.equals(shardId)) {
+            throw new IllegalArgumentException(
+                "session [" + sessionUUID + "] shardId [" + sessionShardId + "] does not match requested shardId [" + shardId + "]"
+            );
+        }
+    }
+
+    public void ensureFileNameIsKnownToSession(String sessionUUID, String fileName) throws IOException {
+        final RestoreSession restore = onGoingRestores.get(sessionUUID);
+        if (restore == null) {
+            logger.debug("could not get session [{}] because session not found", sessionUUID);
+            throw new IllegalArgumentException("session [" + sessionUUID + "] not found");
+        }
+        // Ensure no file system traversal is possible by only allow file names known to the restore session
+        if (false == restore.getMetadata().contains(fileName)) {
+            throw new IllegalArgumentException("invalid file name [" + fileName + "]");
+        }
+    }


I am not sure whether these methods need to be sychronized. I will seek advices from the distributed team.

It's been advised that synchronization are not necessary for these methods since they do not mutate state.

ywangd · 2023-04-26T07:52:14Z

.../src/javaRestTest/java/org/elasticsearch/xpack/remotecluster/RemoteClusterSecurityCcrIT.java

+        // query cluster error cases - aliases not supported
+        {
+            final Request putCcrRequest = new Request("PUT", "/follower-index-3/_ccr/follow?wait_for_active_shards=1");
+            putCcrRequest.setJsonEntity("""
+                {
+                  "remote_cluster": "my_remote_cluster",
+                  "leader_index": "shared-alias"
+                }""");
+            final ResponseException e = expectThrows(ResponseException.class, () -> performRequestWithCcrUser(putCcrRequest));
+            assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400));
+            assertThat(e.getMessage(), containsString("cannot follow [shared-alias], because it is a ALIAS"));


Add a negative test case specifically for aliases since the indices action has an implicit assumption that the permission must be granted on the concrete indices.

.../java/org/elasticsearch/xpack/ccr/action/repositories/ClearCcrRestoreSessionActionTests.java

ywangd · 2023-04-27T01:25:22Z

...plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRestoreSourceService.java

+                final Engine.IndexCommitRef commitRef = indexShard.acquireSafeIndexCommit();
+                final Set<String> fileNames = Set.copyOf(commitRef.getIndexCommit().getFileNames());
+                restore = new RestoreSession(sessionUUID, indexShard, commitRef, fileNames, scheduleTimeout(sessionUUID));


Storing the known list of file names on RestoreSession creation time so that later checks for known file name is lightweight.
Thanks to @Tim-Brooks for the suggestion!

n1v0lg

LGTM 🚀

.../main/java/org/elasticsearch/xpack/ccr/action/repositories/ClearCcrRestoreSessionAction.java

...plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRestoreSourceService.java

Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>

ywangd · 2023-04-28T00:54:04Z

Failures are not related. I raised
#95644
#95645

ywangd · 2023-04-28T00:54:34Z

@elasticmachine run elasticsearch-ci/part-2

ywangd · 2023-04-28T00:54:58Z

@elasticmachine run elasticsearch-ci/part-2-fips

ywangd · 2023-04-28T01:51:21Z

@elasticmachine run elasticsearch-ci/part-2

ywangd added 3 commits April 17, 2023 15:58

Indexify CCR internal actions for RCS 2.0

c71dbe6

This PR adds CCR support for RCS 2.0 by pairing its internal actions with indices actions which are safe to use across different clusters.

comments

033682f

tweak

020b514

elasticsearchmachine added the v8.8.0 label Apr 17, 2023

ywangd added 2 commits April 17, 2023 16:44

forbidden api

1f756ff

suppress forbidden

db468fe

ywangd commented Apr 17, 2023

View reviewed changes

.../main/java/org/elasticsearch/xpack/ccr/action/repositories/GetCcrRestoreFileChunkAction.java Outdated Show resolved Hide resolved

ywangd commented Apr 17, 2023

View reviewed changes

ywangd added 7 commits April 18, 2023 11:38

better option to prevent file system traversal

8f0ff4c

tweak

0590134

Merge remote-tracking branch 'origin/main' into rcs2-ccr-support

a6cfbee

use centralized version constant

d128a79

Tests for changes to CCR actions and requests

2e15e3d

Test ccr internal action rewrite

7c85a1d

Add tests for FC side privilege enforcements for CCR actions

8738acb

ywangd added :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC >non-issue labels Apr 24, 2023

ywangd marked this pull request as ready for review April 24, 2023 07:06

elasticsearchmachine added the Team:Security Meta label for security team label Apr 24, 2023

ywangd commented Apr 24, 2023

View reviewed changes

ywangd requested a review from n1v0lg April 24, 2023 07:19

ywangd commented Apr 24, 2023

View reviewed changes

tweak

f191a68

n1v0lg reviewed Apr 24, 2023

View reviewed changes

n1v0lg reviewed Apr 25, 2023

View reviewed changes

Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…

5e966bd

…ecurity/transport/SecurityServerTransportInterceptor.java Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>

ywangd added 5 commits April 26, 2023 13:25

address feedback

feb2f9b

Merge remote-tracking branch 'origin/main' into rcs2-ccr-support

ebae9c6

fix compilation

de5ee1a

relocate restore session consistency check

c054de1

tweak

dcc434d

ywangd commented Apr 26, 2023

View reviewed changes

tweak

5c8565d

ywangd requested a review from n1v0lg April 26, 2023 07:57

ywangd commented Apr 26, 2023

View reviewed changes

.../java/org/elasticsearch/xpack/ccr/action/repositories/ClearCcrRestoreSessionActionTests.java Show resolved Hide resolved

gmarouli added v8.9.0 and removed v8.8.0 labels Apr 26, 2023

Store fileNames to avoid reading metadata again which could be heavy

710fcb5

ywangd commented Apr 27, 2023

View reviewed changes

n1v0lg approved these changes Apr 27, 2023

View reviewed changes

.../main/java/org/elasticsearch/xpack/ccr/action/repositories/ClearCcrRestoreSessionAction.java Outdated Show resolved Hide resolved

...plugin/ccr/src/main/java/org/elasticsearch/xpack/ccr/repository/CcrRestoreSourceService.java Outdated Show resolved Hide resolved

ywangd and others added 5 commits April 28, 2023 09:52

Apply suggestions from code review

5b7ffb8

Co-authored-by: Nikolaj Volgushev <n1v0lg@users.noreply.github.com>

tweak

ff1431b

Merge remote-tracking branch 'origin/main' into rcs2-ccr-support

b7bb1bc

update version constant

da63603

last tweak

bdf24d0

ywangd added the auto-merge Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Apr 28, 2023

elasticsearchmachine merged commit a2c9c0b into elastic:main Apr 28, 2023

ywangd deleted the rcs2-ccr-support branch April 28, 2023 02:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indexify CCR internal actions for RCS 2.0 #95264

Indexify CCR internal actions for RCS 2.0 #95264

ywangd commented Apr 17, 2023

ywangd Apr 17, 2023

ywangd Apr 17, 2023 •

edited

ywangd Apr 17, 2023

elasticsearchmachine commented Apr 24, 2023

ywangd Apr 24, 2023

ywangd Apr 24, 2023

n1v0lg Apr 24, 2023

n1v0lg Apr 25, 2023

n1v0lg Apr 25, 2023

ywangd Apr 26, 2023

ywangd Apr 24, 2023

n1v0lg left a comment •

edited

n1v0lg Apr 24, 2023

ywangd Apr 26, 2023

n1v0lg Apr 24, 2023

ywangd Apr 24, 2023

n1v0lg Apr 25, 2023

n1v0lg left a comment

n1v0lg Apr 25, 2023

n1v0lg Apr 25, 2023

ywangd Apr 26, 2023

ywangd Apr 27, 2023

ywangd Apr 26, 2023

ywangd Apr 27, 2023

n1v0lg left a comment

ywangd commented Apr 28, 2023

ywangd commented Apr 28, 2023

ywangd commented Apr 28, 2023

ywangd commented Apr 28, 2023

	if (indices == null \|\| indices.length == 0) {
	throw new IllegalArgumentException("the action " + action + " requires explicit index names, but none were provided");
	}

Indexify CCR internal actions for RCS 2.0 #95264

Indexify CCR internal actions for RCS 2.0 #95264

Conversation

ywangd commented Apr 17, 2023

Choose a reason for hiding this comment

ywangd Apr 17, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticsearchmachine commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg left a comment

Choose a reason for hiding this comment

ywangd commented Apr 28, 2023

ywangd commented Apr 28, 2023

ywangd commented Apr 28, 2023

ywangd commented Apr 28, 2023

ywangd Apr 17, 2023 •

edited

n1v0lg left a comment •

edited