Skip to content

Preserve authentication for self-referential CCS - #6473

Closed
cwperks wants to merge 1 commit into
opensearch-project:mainfrom
cwperks:fix/ccs-self-remote-auth
Closed

Preserve authentication for self-referential CCS#6473
cwperks wants to merge 1 commit into
opensearch-project:mainfrom
cwperks:fix/ccs-self-remote-auth

Conversation

@cwperks

@cwperks cwperks commented Sep 5, 2026

Copy link
Copy Markdown
Member

Fixes #5846

Summary

  • identify direct local transport requests by the exact local DiscoveryNode instance rather than DiscoveryNode.equals
  • serialize user context for equal-but-distinct remote node representations, including self-referential CCS
  • add regression coverage for a remote connection whose DiscoveryNode equals, but is not identical to, the local node

Validation

  • ./gradlew spotlessJavaCheck test --tests org.opensearch.security.transport.SecurityInterceptorTests --tests org.opensearch.security.transport.RestoringTransportResponseHandlerTests
  • reproduced with two OpenSearch 3.7.0 clusters and both self-referential and external remote aliases
  • before fix: self CCS returned HTTP 500 in 5/5 requests; external CCS returned HTTP 200
  • after identity-based fix: self CCS returned HTTP 200 in 5/5 requests; external CCS returned HTTP 200 in 3/3 requests

Note

  • ./gradlew precommit reaches an unrelated existing forbidden-API failure in the sample resource plugin for URL.openStream()

Identify direct local requests using the TransportService connection instead of relying on DiscoveryNode equality. This ensures remote TCP requests targeting the same node serialize their user context.

Signed-off-by: Craig Perkins <craig5008@gmail.com>
@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🧪 PR contains tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review

Possible NPE

Objects.requireNonNull(connection) will throw a NullPointerException if connection is null, whereas the previous logic tolerated a null-check indirectly via localNode != null && localNode.equals(connection.getNode()). If any caller passes a null connection (previously guarded when localNode was null), this now throws instead of proceeding. Verify all sendRequestDecorate call paths guarantee non-null connection, or restore a defensive null check.

final boolean isSameNodeRequest = Objects.requireNonNull(connection) == localNodeConnectionProvider.apply(localNode)
    && !isStreamChannel;
Potential startup ordering issue

GuiceHolder::getLocalNodeConnection is passed to SecurityInterceptor during createComponents, but GuiceHolder.transportService is only assigned when Guice instantiates GuiceHolder. If SecurityInterceptor is invoked before GuiceHolder is constructed, transportService will be null and the provider will return null, meaning the same-node optimization silently degrades (serialization always occurs for local requests). This may be intended fallback behavior but is worth confirming.

remoteClusterIdentityPolicy,
GuiceHolder::getLocalNodeConnection

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Avoid NPE in same-node connection check

Using Objects.requireNonNull(connection) will throw NPE if connection is ever null,
which could break call paths that previously tolerated a null connection (the prior
check localNode != null && localNode.equals(connection.getNode()) also would NPE,
but only when localNode was non-null). More importantly, comparing via reference
equality (==) with the result of localNodeConnectionProvider.apply(localNode) will
be false when the provider returns null (e.g., before TransportService is
initialized or when localNode is null), which is the desired behavior — but the
explicit requireNonNull is unnecessary and may mask legitimate null-connection
scenarios. Consider a null-safe comparison instead.

src/main/java/org/opensearch/security/transport/SecurityInterceptor.java [178-179]

-final boolean isSameNodeRequest = Objects.requireNonNull(connection) == localNodeConnectionProvider.apply(localNode)
-    && !isStreamChannel;
+final Connection localConnection = localNode == null ? null : localNodeConnectionProvider.apply(localNode);
+final boolean isSameNodeRequest = connection != null && localConnection != null && connection == localConnection && !isStreamChannel;
Suggestion importance[1-10]: 5

__

Why: The suggestion raises a valid concern about Objects.requireNonNull(connection) potentially throwing NPE where the previous code handled it, but in practice connection is unlikely to be null in this code path. The null-safe alternative is a minor defensive improvement.

Low

@cwperks cwperks closed this Sep 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Cross Cluster Search to same node fails auth

1 participant