Skip to content

fix(sync): validate trusted WebSocket master redirects before forwarding credentials (#8222) - #8242

Merged
Aias00 merged 2 commits into
apache:masterfrom
konlue:fix/trusted-master-redirect
Oct 30, 2026
Merged

Aias00 merged 2 commits into
apache:masterfrom
konlue:fix/trusted-master-redirect

Conversation

@konlue

@konlue konlue commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Fixes #8222.

Problem

An advertised cluster master is currently trusted whenever it uses ws / wss and has a host. A trusted wss:// seed can therefore redirect the gateway to another host and even to plaintext ws://, and the same X-Shenyu-Sync-Token is attached to the new handshake. Because shenyu.cluster.schema defaults to http, a TLS-terminated Admin also advertises a plaintext master by default.

Change

  • New MasterTargetPolicy validates every advertised master before a client — and therefore its credential headers — is constructed:
    • Trusted origins are the current connection, already-connected clients, configured seed origins and an explicit shenyu.sync.websocket.trusted-master-hosts list (host, host:port, or ws(s)://host:port).
    • A wss connection never follows a plaintext ws advertisement unless the operator sets shenyu.sync.websocket.allow-insecure-master-redirect=true. ws → wss upgrades remain allowed.
    • A rejected destination is logged by scheme/host/port only; the current trusted follower is kept, no client is created and no token is forwarded. Malformed URIs keep the existing fallback behavior.
  • Seeds remain trusted by configuration. Master promotion, context paths, rediscovery and bounded reconnect/backoff are unchanged.
  • Compatibility: The default policy trusts only seed/current origins. Deployments whose Admin advertises other cluster nodes must list all node origins in the seed URLs or the new allowlist. TLS-terminated Admins should set shenyu.cluster.schema=https so the advertised master is wss; an explicit insecure opt-in is required for plaintext.

Tests

  • Policy unit tests cover seed/current/allowlist origins, host-only and host:port entries, different ports, ws ↔ wss transitions, the insecure opt-in, seeds without a scheme, and invalid schemes/missing current context.
  • Service tests assert an untrusted advertised master and a wss → ws downgrade construct no client and do not close the trusted follower; an allowlisted master and the explicit insecure opt-in construct a client whose headers still carry the sync token.
  • The real-socket redirect test now explicitly allowlists the advertised master port and keeps its token/Origin/namespace/port assertions; config getter/equality tests cover the new properties.

Verification

shenyu-sync-data-websocket: 161/161
starter-sync-data-websocket: 4/4
checkstyle: clean

The check runs only when a master advertisement is evaluated, not on request or sync-data paths; no DNS, remote authorization or per-frame work is added.

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one IPv6 allowlist matching issue in the new master-target policy. A standalone probe compiled the production policy unchanged and demonstrated that valid bracketed IPv6 host and host:port entries are rejected, while the equivalent URI entry is accepted.

@konlue

konlue commented Oct 30, 2026

Copy link
Copy Markdown
Contributor Author

Thanks — the IPv6 mismatch is real. URI.getHost() keeps the brackets for an IPv6 literal, while the bracketed allowlist branch stripped them before comparing, so [2001:db8::1] and [2001:db8::1]:9095 were rejected even though the full-URI form worked.

Fixed by normalizing the host representation in one place: Origin now lowercases and strips surrounding brackets, so bracketed host, bracketed host:port and full ws(s)://[addr]:port/... entries all compare against the same value; a different address still rejects.

Added ipv6TrustedHostsNormalizeBrackets: the three accepted IPv6 forms plus a different-address negative control. The previous code reproduces false, false, true for the three forms; now the first three pass and the negative stays false.

Verification:

shenyu-sync-data-websocket: 162/162 (policy 10/10)
starter-sync-data-websocket: 4/4
checkstyle: clean

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The IPv6 allowlist finding is fixed: origin hosts and bracketed configured entries now use the same normalized representation. Reviewed trust boundaries, ports and transport downgrade handling; no remaining actionable finding.

Validation: 37 WebSocket policy/configuration/redirect/service tests passed, including bracketed IPv6 host, host:port and URI cases. CI was still pending at the initial review snapshot. Apache RAT passed locally. Scope: source review and targeted verification; full integration/e2e suites were not run locally for this review.

@Aias00
Aias00 merged commit 58a7881 into apache:master Oct 30, 2026
56 checks passed
@Aias00 Aias00 added this to the 2.7.2 milestone Nov 13, 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][Sync] Validate trusted WebSocket master redirects before forwarding credentials

2 participants