feat(core): support trusted Bedrock identity sessions#57
Merged
Conversation
… and admission cleanup
…re documentation baseline
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.
Intent
Recover Connect Java PR #57's two approved Bedrock admission boundary blockers: use one Guice/platform-owned coordinator for raw Watch/libp2p admissions behind opaque consume-once generation tokens; keep raw sessions out of SessionProposal, LocalSession.Context, players and reflection/Gson-reachable graphs; remove static ADMISSION_OWNERS and compatibility-created registries/executors; preserve constructor descriptors as threadless direct-raw compatibility paths; keep the existing Moxy GeyserMC#464 protocol, verifier, API, fixtures and platform behavior unchanged; prove both transports, generation-safe slow verification replacement/expiry/close behavior, lifecycle cleanup and legacy compatibility. Do not alter Moxy, docs, deployments, dependencies, releases, or default behavior.
What Changed
ConnectApi, and apply trusted profiles on Spigot and Velocity.Risk Assessment
✅ Low: The focused remediation correctly waits for coordinator cleanup, and the full branch review found no remaining material intent, lifecycle, isolation, or compatibility risks.
Testing
After base-to-target mapping and replacing an auxiliary JShell evidence attempt that hit a cached compiler-plugin conflict, focused tests, complete core/Velocity suites, the multi-module build, ABI/scope audits, and direct non-UI transport/lifecycle transcripts all succeeded; generated output was removed and only intentional evidence-test edits remain.
Evidence: Transport admission transcript
LIBP2P: ACCEPTED, same-stream; WATCH: admitted. Both used opaque tokens with no private envelope exposed through proposals or staged players.Evidence: Generation and lifecycle transcript
Demonstrates opaque monotonic tokens, consume-once verification, stale-verification rejection after replacement/expiry/close, and platform cleanup.Evidence: Constructor compatibility descriptors
Compiled descriptors retain legacy and coordinator-aware constructors for LocalSession, WatchClient, BedrockIdentityEnforcer, ConnectPlatform, and SpigotPlatform.Evidence: Forbidden-scope audit
No docs, deployments, dependency manifests, release automation, or production ADMISSION_OWNERS changed/present.Pipeline
Updates from git push no-mistakes
✅ **intent** - passed
✅ No issues found.
✅ **Rebase** - passed
✅ No issues found.
🔧 **Review** - 5 issues found → auto-fixed (3) ✅
core/src/main/java/com/minekube/connect/api/SimpleConnectApi.java:51- The required criterion says raw sessions must stay out of “reflection/Gson-reachable graphs,” but the globally exposed SimpleConnectApi directly references the coordinator, whose admissions map contains raw Session objects. Reflection or Gson traversal of the API implementation can therefore reach every pending envelope. Move admission staging/discarding out of the public API object and retain only a claims-only lookup capability there.core/src/main/java/com/minekube/connect/bedrock/BedrockAdmissionCoordinator.java:80- Generation replacement is ordered by stage time, not proposal generation: this filter ignores older admissions whose player is still null. An older Watch proposal delayed in prepare() can therefore stage after a newer libp2p proposal, cancel the newer generation, and become authoritative. This contradicts the required generation-safe replacement behavior; register and supersede generations when proposals are created.core/src/main/java/com/minekube/connect/bedrock/VerifiedBedrockIdentityRegistry.java:34- The required single-coordinator boundary is incomplete: VerifiedBedrockIdentityRegistry still independently accepts raw Sessions, retains private admission profiles, and creates a second cleanup executor. Production no longer calls this path, but tests preserve it. Make the registry claims-only and remove its obsolete admission/TTL machinery, subject to confirming this public class is not an intended compatibility surface.core/src/main/java/com/minekube/connect/bedrock/VerifiedBedrockIdentityRegistry.java:29- Verified claims are keyed only by the Edge-provided session ID, while SimpleConnectApi uses value equality to decide whether a player is current. Reused session IDs can therefore make a different player—or a stale equal ConnectPlayer instance—retrieve another generation’s claims. Bind claims to the exact public ConnectPlayer identity and check players.get(uuid) == player.core/src/main/java/com/minekube/connect/bedrock/BedrockAdmissionCoordinator.java:61- Rejecting a proposal before stage() does not cancel its coordinator admission; the raw signed envelope and one scheduled task remain until the 30-second TTL. Invalid Watch proposals and libp2p starter failures can accumulate sensitive raw state under load. Add a rejection hook that consumes/cancels the token immediately.🔧 Fix: Harden Bedrock admission generation and claims isolation
2 issues (1 error, 1 warning) still open:
core/src/main/java/com/minekube/connect/bedrock/BedrockAdmissionCoordinator.java:60- The required intent says admissions must use “opaque consume-once generation tokens” with “generation-safe … replacement/expiry/close behavior,” but tokens are created only when private identity properties exist. A coordinator-created tokenless proposal bypasses generation, expiry, and closed-state checks in stage(), so an older legacy Watch proposal can stage after a newer libp2p proposal—or after coordinator shutdown. Track every coordinator-created transport proposal; reserve the tokenless direct-raw path for compatibility constructors.core/src/main/java/com/minekube/connect/network/netty/LocalSession.java:214- The displaced player returned by addPlayer() is ignored. For the same UUID with different session IDs, coordinator supersession does not remove the older admission, allowing its raw envelope or stale verification to remain until channel cleanup/TTL. Capture the previous player and discard it through the coordinator, relocating the cleanup removed from SimpleConnectApi.🔧 Fix: Tokenize all admissions and clean displaced players
1 warning still open:
core/src/test/java/com/minekube/connect/network/netty/LocalSessionBedrockIdentityTest.java:177- The latch is released inside addPlayer(), before LocalSession performs admissionCoordinator.discard(displaced). The test thread can therefore inspect the admissions map before cleanup completes, making this required race test flaky. Synchronize on observing the discard itself before asserting coordinator state.🔧 Fix: Synchronize displaced-admission test on coordinator cleanup
✅ Re-checked - no issues remain.
✅ **Test** - passed
✅ No issues found.
Base-to-target inspection withgit diff --stat 9946252506ec5e27a57ab37ac728b29a584dbefc..a3411e1cb6b51f05a1a1f6dc38976d1d6634eeefandgit diff --name-status ...Focused admission suite via./gradlew :core:test --tests 'com.minekube.connect.bedrock.BedrockAdmissionCoordinatorTest' --tests 'com.minekube.connect.bedrock.BedrockIdentityEnforcerTest' --tests 'com.minekube.connect.bedrock.ConnectPlatformBedrockIdentityLifecycleTest' --tests 'com.minekube.connect.bedrock.SimpleConnectApiBedrockIdentityTest' --tests 'com.minekube.connect.network.netty.LocalSessionBedrockIdentityTest' --tests 'com.minekube.connect.watch.SessionProposalTest' --tests 'com.minekube.connect.watch.WatchClientTest' --tests 'com.minekube.connect.tunnel.p2p.Libp2pSessionMapperTest' --tests 'com.minekube.connect.tunnel.p2p.Libp2pSessionResponderTest' --tests 'com.minekube.connect.tunnel.p2p.Libp2pRuntimeTest' --stacktraceTransport evidence via./gradlew :core:test --tests 'com.minekube.connect.watch.WatchClientTest.privateWatchSessionUsesCoordinatorTokenAndSanitizedProposal' --tests 'com.minekube.connect.tunnel.p2p.Libp2pSessionResponderTest.privateOfferUsesCoordinatorTokenAndSanitizedProposal' --infoGeneration/lifecycle evidence via focusedBedrockAdmissionCoordinatorTest, slow-verificationBedrockIdentityEnforcerTest, andConnectPlatformBedrockIdentityLifecycleTestselectors with--infoComplete module tests via./gradlew :core:test :velocity:test --stacktraceBroader multi-platform verification via./gradlew build --stacktraceBinary compatibility audit usingjavap -s -publicon the five compatibility-sensitive classesForbidden-scope and static-owner audit using target-diff path filtering andrg -n 'ADMISSION_OWNERS'across production sourcesCleanup via./gradlew cleanfollowed by removal of Gradle-generated worktree caches;git diff --checkpassed✅ **Document** - passed
✅ No issues found.
✅ **Lint** - passed
✅ No issues found.
✅ **Push** - passed
✅ No issues found.