Introduce remote cluster security interceptor and authenticator #134245

slobodanadamovic · 2025-09-05T20:02:34Z

This refactoring is meant to be purely structural, with no functional changes.
It introduces two interfaces:

RemoteClusterAuthenticationService - extracted based on the existing CrossClusterAccessAuthenticationService
RemoteClusterTransportInterceptor - which aims to abstract and move all "cross-cluster access" logic from SecurityServerTransportInterceptor

This is prerequisite for making remote cluster security logic pluggable (which I plan to add in a followup PR).

Relates to ES-12801

Note: The PR can also be reviewed commit-by-commit. I've tried to make each commit as small as possible and self-sustained.
Note2: Test refactoring will be handled in a followup PR.

- define a new interface based on CrossClusterAccessAuthenticationService - hide method that accepts ApiKeyCredentials as it's only used for testing

…onService

…teClusterTransportInterceptor

…REPLACEMENTS

…rceptor

…lters method

…adContext

…lias

…ll new constructor

…rceptor-refactoring

slobodanadamovic · 2025-09-07T20:41:19Z

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

+ * Allows to provide remote cluster interception that's capable of intercepting remote connections
+ * both on the receiver and the sender side.
+ */
+public interface RemoteClusterTransportInterceptor {


I'm open to any naming suggestions here. I went with using RemoteCluster prefix for both new interfaces as it felt consistent.

I think it's a good name

elasticsearchmachine · 2025-09-07T20:54:49Z

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

tvernum

I haven't looked at the detail of the interceptors yet. Will try to finish that today

tvernum · 2025-09-08T06:34:01Z

...src/main/java/org/elasticsearch/xpack/security/authc/RemoteClusterAuthenticationService.java

+     * @param listener callback to receive {@code null} on successful authentication,
+     *                 or an exception on authentication failure
+     */
+    void authenticateHeaders(Map<String, String> headers, ActionListener<Void> listener);


I'm a little bit confused about the contract of this method.

The name of the method implies that is does authentication.

The javadoc implies that it doesn't actually do authentication (or isn't required to?).

The implementation in CrossClusterAccessAuthenticationService.tryAuthenticate appears to require and authenticate the API Key, but not set the Authentication.

I'm not quite sure what to make of that.

Perhaps the issue is what "if headers contain valid remote cluster credentials" actually means. I would read that and assume it was just checking whether the headers exist, but I think it might mean it does more than that. So, I guess my question is what doesn't it do? Is it just that it doesn't create an Authentication object?

Perhaps the issue is what "if headers contain valid remote cluster credentials" actually means.

Yeah, this is confusing. It's vague because the "validation" may mean different for cross-project than for RCS 2.0.

The order of method calls is:

read only headers on the remote cluster port and call authenticateHeaders

consume the full request body and call authenticate

The authenticateHeaders method validates and verifies the cross cluster credentials based on the received headers. It's called early during request processing, and only for requests on the remote cluster port. We only read headers (body is not consumed yet) at the stage when this method is called. This means that required cross cluster headers must be present and credentials must be valid. Fro RCS 2.0, there is no authentication nor role building happening at this stage, hence the without the overhead of full authentication processing comment.

But thinking further, this may not hold true in all use cases. This method could chose to build authentication object and stash it in the thread context, but it's not required at this phase. It gets required when we call ServerTransportFilter to process inbound message.

Also, I leaned on the fact that all communication is going over RCS port and REMOTE_CLUSTER_PROFILE. This is explicit in SecurityNetty4Transport, but implicit when authenticateHeaders is called. I can try to make that contract clearer. And also make it explicit and change RemoteClusterTransportInterceptor#getProfileTransportFilters to provide a single filter for the REMOTE_CLUSTER_PROFILE only. Currently, it's pretty generic as I wanted to keep flexibility to inject different transport filters for other profiles. We may not need to be able to do it, but I couldn't foresee it at this stage. Let me know what you think.

For RCS 2.0, there is no authentication

I think this is where the semantic mismatch occurs. For me, verifying the api key (which, unless I'm horribly confused, is done in authenticateHeaders) is authentication. That's the primary credential that determines whether this is an authenticated request.

We don't determine the identity of the end-user, their roles (intersection with the API key) or construct an Authentication object, but in my mental model, those are side-effects of the actual "authentication" which is checking the validity of the API key.

I don't want to bike shed this, or argue about precise terminology, but it seems we draw the box around "authentication" differently, and your mindset is "unless we've done all the things we typically do during authentication, then we haven't done authentication" and mine is "if we've verified credentials, that's authentication".

So calling it authenticateHeaders but saying it doesn't do "full authentication" is confusing to me, I don't know what that means.

Calling it authenticateConnection would be more meaningful to me, at least in the context of RCS2.0, but maybe that's not a terminology that fits with CPS (I'm not sure what you think we'll do in that method for CPS).

I think semantics are important and this should have read as For RCS 2.0, there is no authentication object nor role building happening at this stage.

I tried to make distinction between authenticateHeaders and authenticate methods, but failed to name correctly the other typical things we do after we execute authentication. Which could be summed up as "building authentication informations and stashing them in the thread context".

Both methods are called for each request. I don't have full picture on why this was implemented the way it is, but I'm assuming it has to do with how thread context is handled at these different points in time.

We may need to do more refactoring here, because we execute authentication two times for the same RCS request. This may have not been a big of an issue for RCS 2.0, but I think it will be for CPS.

We may need to do more refactoring here, because we execute authentication two times for the same RCS request. This may have not been a big of an issue for RCS 2.0, but I think it will be for CPS.

I think we can defer this conversation until we know what the CPS version looks like. That's going to require coming up with a clearer concept of why we have 2 methods and what we're supposed to do in each one, and trying to name the methods now is probably going to be a waste of effort.

tvernum · 2025-09-08T10:26:53Z

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

+    TransportInterceptor.AsyncSender interceptSender(TransportInterceptor.AsyncSender sender);
+
+    /**
+     * This method returns {@code true} if the {@code connection} is targeting a remote cluster.


Suggested change

* This method returns {@code true} if the {@code connection} is targeting a remote cluster.

* This method returns {@code true} if the outbound {@code connection} is targeting a remote cluster.

It should always be an outbound connection (if my mental model of this stuff is correct) but I think it's helpful for the javadoc to be explicit about that.

tvernum · 2025-09-08T10:27:10Z

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

+ * Allows to provide remote cluster interception that's capable of intercepting remote connections
+ * both on the receiver and the sender side.
+ */
+public interface RemoteClusterTransportInterceptor {


I think it's a good name

tvernum · 2025-09-08T10:27:58Z

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

+    /**
+     * Returns {@code true} if any of the remote cluster access headers are in the security context.
+     * This method is used to assert we don't have access headers already in the security context,
+     * before we even run remote cluster intercepts. Serves as a sanity check that we properly clear


Suggested change

* before we even run remote cluster intercepts. Serves as a sanity check that we properly clear

* before we even run remote cluster intercepts. Serves as an integrity check that we properly clear

tvernum · 2025-09-08T10:48:24Z

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

+        XPackLicenseState licenseState,
+        SecurityContext securityContext,
+        ThreadPool threadPool,
+        Settings settings


Nit: It feels like these arguments are in a random order. Or at least, it's a completely different order than they were in SecurityServerTransportInterceptor

They are in different order. I did't pay attention on preserving the order.
I can see how it could have been helpful to review if they were. I can reorder them.

tvernum · 2025-09-08T11:13:29Z

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

    private SecurityContext securityContext;
    private ClusterService clusterService;
    private MockLicenseState mockLicenseState;
+    private DestructiveOperations destructiveOperations;


Did you intentionally decide not to split up this test case? It feels like it should become 2 sets of tests, but maybe that's a followup PR.

It was intentional to avoid too many changes. I will split them in a followup PR.

…tic#134245) This refactoring is meant to be purely structural, with no functional changes. It introduces two interfaces: - `RemoteClusterAuthenticationService` - extracted based on the existing `CrossClusterAccessAuthenticationService` - `RemoteClusterTransportInterceptor` - which aims to abstract and move all "cross-cluster access" logic from `SecurityServerTransportInterceptor` This is prerequisite for making remote cluster security logic pluggable (which I plan to add in a followup PR). Relates to ES-12801

Initially, the `RemoteClusterTransportInterceptor#getProfileTransportFilters` required remote-cluster security extensions to provide transport filters for all transport profiles. The method was too generic and not specific to only `_remote_cluster` transport profile. This meant that RCS extensions were free to decide which filter they wanted to "override" with its custom transport filter implementation. This turned out to be unnecessary, because RCS extensions only ever need to provide a custom implementation for the remote cluster profile. This refactoring removes the need to provide the "default" `ServerTransportFilter` in order for security to work. Followups to: - #134785 (comment) - #134785 (comment) - #134245 (comment) --- - Converted `ServerTransportFilter` to interface with a default implementation. - Refactored `RemoteClusterTransportInterceptor` to allow optionally providing only a custom remote cluster transport filter. It's no longer required from RCS extensions to provide the default `ServerTransportFilter` implementation. - Split transport interceptor and filter tests: - `ServerTransportFilterTests` became abstract and got split into two tests: `CrossClusterAccessServerTransportFilterTests` and `DefaultServerTransportFilterTests` - Cross-cluster access tests got extracted from `SecurityServerTransportInterceptorTests` into its own `CrossClusterAccessTransportInterceptorTests`class

slobodanadamovic added 3 commits September 5, 2025 21:28

add RemoteClusterTransportInterceptor interface

063f212

add RemoteClusterAuthenticationService interface

6e6f79f

- define a new interface based on CrossClusterAccessAuthenticationService - hide method that accepts ApiKeyCredentials as it's only used for testing

change SecurityNetty4Transport to depend on RemoteClusterAuthenticati…

9d5d6b3

…onService

slobodanadamovic self-assigned this Sep 5, 2025

elasticsearchmachine added the v9.2.0 label Sep 5, 2025

slobodanadamovic added >refactoring :Security/Security Security issues without another label Team:Security Meta label for security team and removed v9.2.0 labels Sep 5, 2025

slobodanadamovic added 21 commits September 5, 2025 22:09

add empty CrossClusterAccessTransportInterceptor that implements Remo…

d3f2df9

…teClusterTransportInterceptor

construct RemoteClusterTransportInterceptor in security interceptor

90a6c54

move interceptForCrossClusterAccessRequests and RCS_INTERNAL_ACTIONS_…

9b25155

…REPLACEMENTS

move profile intialization logic into CrossClusterAccessTransportInte…

5018173

…rceptor

inline profile filters intitialization and remove initializeProfileFi…

cbebc26

…lters method

change order of parameters in shouldRemoveParentAuthorizationFromThre…

1325d3b

…adContext

replace Optional<String> with boolean

f07cd23

implement isRemoteClusterConnection and invoke it instead resolving a…

9a21578

…lias

delegate last call to remoteClusterCredentialsResolver and remove it

201fcad

remove other unused fields from SecurityServerTransportInterceptor

179b339

move RemoteClusterCredentials record

b493336

move remote access headers check into RemoteClusterTransportInterceptor

71e94a4

add new SecurityServerTransportInterceptor constructor

9b5aa27

construct RemoteClusterTransportInterceptor in security plugin and ca…

4d54614

…ll new constructor

move to using new constructor part 1

99d7319

move to using new constructor part 2

4981394

move to using new constructor part 3

af00bcd

spotless and remove repeat annotation

35f2a44

Merge branch 'main' of github.com:elastic/elasticsearch into rcs-inte…

de29062

…rceptor-refactoring

introduce destructiveOperations field in test class

b7b8aa7

rename tryAuthenticate to authenticateHeaders

3dd666f

slobodanadamovic changed the title ~~Refactor remote cluster security interceptor and authenticator~~ Introduce remote cluster security interceptor and authenticator Sep 7, 2025

slobodanadamovic commented Sep 7, 2025

View reviewed changes

renaming and javadoc

7450a83

slobodanadamovic requested a review from tvernum September 7, 2025 20:52

slobodanadamovic added the test-full-bwc Trigger full BWC version matrix tests label Sep 7, 2025

slobodanadamovic marked this pull request as ready for review September 7, 2025 20:54

inline interceptForCrossClusterAccessRequests method

84ed0b5

slobodanadamovic added the v9.2.0 label Sep 7, 2025

tvernum reviewed Sep 8, 2025

View reviewed changes

tvernum approved these changes Sep 8, 2025

View reviewed changes

slobodanadamovic and others added 2 commits September 9, 2025 10:28

address review feedback

75aa5ca

Merge branch 'main' into rcs-interceptor-refactoring

25d558c

slobodanadamovic removed the test-full-bwc Trigger full BWC version matrix tests label Sep 9, 2025

Merge branch 'main' into rcs-interceptor-refactoring

2d96a32

slobodanadamovic merged commit 53ef68f into elastic:main Sep 9, 2025
41 checks passed

slobodanadamovic mentioned this pull request Sep 30, 2025

Refactor remote cluster interceptor and tests #135747

Merged

	* This method returns {@code true} if the {@code connection} is targeting a remote cluster.
	* This method returns {@code true} if the outbound {@code connection} is targeting a remote cluster.

	* before we even run remote cluster intercepts. Serves as a sanity check that we properly clear
	* before we even run remote cluster intercepts. Serves as an integrity check that we properly clear

Introduce remote cluster security interceptor and authenticator #134245

Introduce remote cluster security interceptor and authenticator #134245

Uh oh!

Conversation

slobodanadamovic commented Sep 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slobodanadamovic Sep 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Sep 7, 2025

Uh oh!

tvernum left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tvernum Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slobodanadamovic Sep 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

slobodanadamovic commented Sep 5, 2025 •

edited

Loading

slobodanadamovic Sep 7, 2025 •

edited

Loading

tvernum Sep 8, 2025 •

edited

Loading

slobodanadamovic Sep 8, 2025 •

edited

Loading