Refactor remote cluster interceptor and tests #135747
Conversation
- Converted `ServerTransportFilter` to interface with the default implementation. - Refactoried `RemoteClusterTransportInterceptor` to allow optionally providing only a custom remote cluster transport profile filter. - Split transport interceptor and filter tests
slobodanadamovic
added
>refactoring
:Security/Security
elasticsearchmachine
added
the
serverless-linked
slobodanadamovic
changed the title
|
Pinging @elastic/es-security (Team:Security) |
slobodanadamovic
changed the title
.../java/org/elasticsearch/xpack/security/rcs/extension/TestRemoteClusterSecurityExtension.java
Outdated
Show resolved
Hide resolved
| import static org.elasticsearch.xpack.security.authc.CrossClusterAccessHeaders.CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY; | ||
|
|
||
| final class CrossClusterAccessServerTransportFilter extends ServerTransportFilter { | ||
| final class CrossClusterAccessServerTransportFilter extends DefaultServerTransportFilter implements ServerTransportFilter { |
There was a problem hiding this comment.
implements ServerTransportFilter is implicit since CrossClusterAccessServerTransportFilter is a DefaultServerTransportFilter that in turn implements ServerTransportFilter.
Might be out of scope for this PR, but the inheritance here is a little confusing. CrossClusterAccessServerTransportFilter is only overriding the authenticate method which leads me to believe that the code in CrossClusterAccessServerTransportFilter actually belongs in CrossClusterAccessAuthenticationService and this class could be dropped for that reason.
There was a problem hiding this comment.
That's a very good point. Thanks for bringing this up. I think the end goal could be to have a single ServerTransportFilter implementation that accept a custom authentication service (CrossClusterAccessAuthenticationService or AuthenticationService). I like this idea, but that might be more involved at this point as it would require more refactoring. As a first step towards that, I could start by reverting the interface introduction, and later following up with generalizing the authentication part. WDYT?
There was a problem hiding this comment.
I see, I didn't realize they were different interfaces, yes that makes it more difficult. One more thing that could be done in this PR is to move the logic into the CrossClusterAccessAuthenticationService and just do the authenticate call. I'll leave it up to you, might also make sense to not increase the scope.
There was a problem hiding this comment.
Yeah, them being different interfaces with a different number of parameters makes it not that straightforward, but doable. In the interest of unblocking downstream work, I'd prefer to get back to this in a followup.
.../java/org/elasticsearch/xpack/security/transport/CrossClusterAccessTransportInterceptor.java
Show resolved
Hide resolved
.../java/org/elasticsearch/xpack/security/rcs/extension/TestRemoteClusterSecurityExtension.java
Outdated
Show resolved
Hide resolved
.../java/org/elasticsearch/xpack/security/transport/CrossClusterAccessTransportInterceptor.java
Outdated
Show resolved
Hide resolved
| import static org.elasticsearch.xpack.security.authc.CrossClusterAccessHeaders.CROSS_CLUSTER_ACCESS_CREDENTIALS_HEADER_KEY; | ||
|
|
||
| final class CrossClusterAccessServerTransportFilter extends ServerTransportFilter { | ||
| final class CrossClusterAccessServerTransportFilter extends DefaultServerTransportFilter implements ServerTransportFilter { |
There was a problem hiding this comment.
I see, I didn't realize they were different interfaces, yes that makes it more difficult. One more thing that could be done in this PR is to move the logic into the CrossClusterAccessAuthenticationService and just do the authenticate call. I'll leave it up to you, might also make sense to not increase the scope.
| @Override | ||
| public Map<String, ServerTransportFilter> getProfileTransportFilters( | ||
| Map<String, SslProfile> profileConfigurations, | ||
| public Optional<ServerTransportFilter> getRemoteProfileTransportFilter( |
There was a problem hiding this comment.
Returning Optional<ServerTransportFilter> is more of a preference here. We could as well return ServerTransportFilter and annotate the method as @Nullable
jfreden
left a comment
jfreden
left a comment
There was a problem hiding this comment.
Nice job on this! You probably want to wait for a review from someone familiar with CPS too, but LGTM!
…-transport-refactoring # Conflicts: # x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/transport/SecurityServerTransportInterceptorTests.java
slobodanadamovic
added
the
auto-merge-without-approval
4 participants
Initially, the
RemoteClusterTransportInterceptor#getProfileTransportFiltersrequired remote-cluster security extensions to provide transport filters for all transport profiles. The method was too generic and not specific to only_remote_clustertransport 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"ServerTransportFilterin order for security to work.Followups to:
ServerTransportFilterto interface with a default implementation.RemoteClusterTransportInterceptorto allow optionally providing only a custom remote cluster transport filter. It's no longer required from RCS extensions to provide the defaultServerTransportFilterimplementation.ServerTransportFilterTestsbecame abstract and got split into two tests:CrossClusterAccessServerTransportFilterTestsandDefaultServerTransportFilterTestsSecurityServerTransportInterceptorTestsinto its ownCrossClusterAccessTransportInterceptorTestsclass