websocket sync handshakes#6386
Open
Aias00 wants to merge 3 commits into
Open
Conversation
WebSocket sync currently exposes the full bootstrap data stream after an anonymous upgrade, including APP_AUTH records needed by gateway bootstrap. This change keeps the sync payload shape intact and moves the trust decision to the handshake by requiring a shared sync token from bootstrap clients. Admin validates the X-Shenyu-Sync-Token header before copying session attributes into the endpoint context. Bootstrap clients read the same token from shenyu.sync.websocket.token and send it with each WebSocket connection. The token is masked from WebsocketConfig.toString() so operational logging does not reveal the secret. Constraint: APP_AUTH.appSecret must remain in sync payloads because bootstrap sign-plugin behavior depends on it Rejected: Remove appSecret from APP_AUTH sync data | breaks bootstrap consumers that need the secret to verify signed requests Rejected: Require admin-user JWT on websocket upgrade | couples bootstrap nodes to user login/session token lifecycle Confidence: high Scope-risk: moderate Directive: Do not remove /websocket from the Shiro whitelist unless bootstrap has a non-user auth path through that filter Tested: ./mvnw -pl shenyu-admin,shenyu-sync-data-center/shenyu-sync-data-websocket -am -DfailIfNoTests=false -Dskip.checkstyle=true -DskipLicense=true -Dspotless.check.skip=true -Djacoco.skip=true -Dtest=WebsocketConfiguratorTest,WebsocketSyncPropertiesTest,WebsocketConfigTest test Tested: ./mvnw -pl shenyu-admin,shenyu-sync-data-center/shenyu-sync-data-websocket -am -DskipTests -DfailIfNoTests=false -DskipLicense=true -Dspotless.check.skip=true -Djacoco.skip=true package Tested: git diff --check Not-tested: End-to-end admin/bootstrap websocket connection with a live shared token
Contributor
There was a problem hiding this comment.
Pull request overview
This PR tightens Shenyu’s WebSocket-based sync channel by requiring a shared X-Shenyu-Sync-Token header during the JSR-356 handshake, with matching configuration added to both admin and bootstrap so sync clients can authenticate at connection time.
Changes:
- Enforce a required
X-Shenyu-Sync-Tokenduring admin WebSocket handshake (WebsocketConfigurator), with tests for missing/invalid/blank configuration cases. - Add
shenyu.sync.websocket.tokenconfiguration to admin and bootstrap, and propagate the token as a WebSocket client header from bootstrap. - Mask the configured token in
WebsocketConfig.toString()and extend tests to cover token behavior.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| shenyu-sync-data-center/shenyu-sync-data-websocket/src/test/java/org/apache/shenyu/plugin/sync/data/websocket/config/WebsocketConfigTest.java | Extends config tests for token and verifies token is masked in toString(). |
| shenyu-sync-data-center/shenyu-sync-data-websocket/src/main/java/org/apache/shenyu/plugin/sync/data/websocket/WebsocketSyncDataService.java | Adds token (and origin) to WebSocket client handshake headers via a shared createClient helper. |
| shenyu-sync-data-center/shenyu-sync-data-websocket/src/main/java/org/apache/shenyu/plugin/sync/data/websocket/config/WebsocketConfig.java | Adds token field and masks it in toString(). |
| shenyu-common/src/main/java/org/apache/shenyu/common/constant/Constants.java | Introduces X-Shenyu-Sync-Token header constant. |
| shenyu-bootstrap/src/main/resources/application.yml | Adds bootstrap-side shenyu.sync.websocket.token env-based configuration. |
| shenyu-admin/src/test/java/org/apache/shenyu/admin/listener/websocket/WebsocketConfiguratorTest.java | Adds/updates handshake tests to validate sync token enforcement. |
| shenyu-admin/src/test/java/org/apache/shenyu/admin/config/properties/WebsocketSyncPropertiesTest.java | Extends properties test coverage for the new token property. |
| shenyu-admin/src/main/resources/application.yml | Adds admin-side shenyu.sync.websocket.token env-based configuration. |
| shenyu-admin/src/main/java/org/apache/shenyu/admin/listener/websocket/WebsocketConfigurator.java | Enforces sync token check during the handshake using constant-time comparison. |
| shenyu-admin/src/main/java/org/apache/shenyu/admin/config/properties/WebsocketSyncProperties.java | Adds token property to bind admin configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
111
to
115
| public String toString() { | ||
| return "WebsocketConfig{" | ||
| + "urls='" | ||
| + urls | ||
| + ", allowOrigin='" |
| @Test | ||
| public void testToString() { | ||
| String toString = "WebsocketConfig{urls='%s, allowOrigin='%s}"; | ||
| String toString = "WebsocketConfig{urls='%s, allowOrigin='%s, token='******}"; |
Comment on lines
+716
to
+720
| /** | ||
| * X-Shenyu-Sync-Token. | ||
| */ | ||
| String SHENYU_WEBSOCKET_SYNC_TOKEN = "X-Shenyu-Sync-Token"; | ||
|
|
Websocket endpoint configurators can be created by the JSR-356 container outside Spring autowiring, so handshake auth now resolves sync properties through the same SpringBeanUtils fallback already used by origin checks. The test and e2e runtimes also configure the same sync token on both admin and bootstrap sides so the new mandatory handshake token does not break CI scenarios. Constraint: WebSocket handshake configurator instances are not guaranteed to be Spring-autowired. Constraint: The token is mandatory when websocket sync is enabled, so test runtimes must configure both admin and bootstrap. Rejected: Make admin accept blank tokens in test runtimes | would weaken the security behavior being added by the PR. Confidence: high Scope-risk: moderate Directive: Keep admin and bootstrap websocket sync token values paired in compose, k8s, and local integration-test resources. Tested: ./mvnw -pl shenyu-admin,shenyu-sync-data-center/shenyu-sync-data-websocket -am -DfailIfNoTests=false -Dskip.checkstyle=true -DskipLicense=true -Dspotless.check.skip=true -Djacoco.skip=true -Dtest=WebsocketConfiguratorTest,WebsocketConfigTest test Tested: ../mvnw -pl shenyu-integrated-test-sdk-http -am -DskipTests -DfailIfNoTests=false -Dskip.checkstyle=true -DskipLicense=true -Dspotless.check.skip=true -Djacoco.skip=true package (from shenyu-integrated-test/) Tested: docker compose config for shenyu-integrated-test-sdk-http and shenyu-sync-websocket compose files Tested: ruby YAML parse for representative e2e k8s config files Tested: git diff --check Not-tested: Full Docker e2e/IT containers locally
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
X-Shenyu-Sync-Tokenduring the handshake before admin accepts the connection.shenyu.sync.websocket.tokento admin and bootstrap config so bootstrap nodes send the shared sync token.APP_AUTH.appSecretin sync payloads for bootstrap behavior and mask the new token inWebsocketConfig.toString().Notes
/websocketfrom the Shiro whitelist. Bootstrap nodes are not admin UI users, so the node trust check is enforced in the JSR-356 handshake configurator instead.SHENYU_SYNC_WEBSOCKET_TOKENvalue on admin and bootstrap.Make sure that:
./mvnw clean install -Dmaven.javadoc.skip=true.Test Plan
./mvnw -pl shenyu-admin,shenyu-sync-data-center/shenyu-sync-data-websocket -am -DfailIfNoTests=false -Dskip.checkstyle=true -DskipLicense=true -Dspotless.check.skip=true -Djacoco.skip=true -Dtest=WebsocketConfiguratorTest,WebsocketSyncPropertiesTest,WebsocketConfigTest test./mvnw -pl shenyu-admin,shenyu-sync-data-center/shenyu-sync-data-websocket -am -DskipTests -DfailIfNoTests=false -DskipLicense=true -Dspotless.check.skip=true -Djacoco.skip=true packagegit diff --check origin/master..HEAD