Do not route the internal (v3Internal) API through the external router#16154
Open
evilgensec wants to merge 2 commits into
Open
Do not route the internal (v3Internal) API through the external router#16154evilgensec wants to merge 2 commits into
evilgensec wants to merge 2 commits into
Conversation
The /v3Internal API is for service-to-service calls and its handlers do not perform per-request authorization, relying on callers presenting an internal credential. RouterPathLookup only special-cased the public /v3 prefix and fell through to APP_FABRIC_HTTP for everything else, so the external NettyRouter forwarded /v3Internal requests from any authenticated external user to app-fabric, which applies no internal-only gate. Internal callers reach these endpoints over service discovery (RemoteClient against the APP_FABRIC_HTTP service), not the external router, so the router can stop routing /v3Internal, matching the existing handling of /v3/metadata-internals.
There was a problem hiding this comment.
Code Review
This pull request prevents external routing of internal APIs by returning DONT_ROUTE when the request path starts with the internal API version token, and updates the corresponding unit tests. Feedback suggests adding a defensive check for empty uriParts to avoid throwing an ArrayIndexOutOfBoundsException when routing empty paths.
…athLookup.java Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
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.
Problem
The
/v3InternalAPI is an internal, service-to-service surface: its handlers (for exampleFileFetcherHttpHandlerInternal,AppStateHandler,PreferencesHttpHandlerInternal,AppLifecycleHttpHandlerInternal) do not callaccessEnforcer.enforce(...)and rely on callers presenting an internal credential.RouterPathLookup.getRoutingServiceonly special-cases the public/v3prefix and otherwise falls through toreturn APP_FABRIC_HTTP. The externalNettyRoutertherefore forwards/v3Internal/...requests to app-fabric for any authenticated external user, and the app-fabric pipeline does not require an internal credential for those paths. A low-privileged user can then call the internal API across namespaces — for exampleGET /v3Internal/location/<absolute-path>(FileFetcherHttpHandlerInternal→Locations.getLocationFromAbsolutePath) reads files rooted at the filesystem root, and the app-state / preferences / app-lifecycle internal handlers read or modify another namespace's data.Internal callers do not use the external router for these paths — they go through service discovery (
RemoteClientagainstConstants.Service.APP_FABRIC_HTTP, e.g.RemoteArtifactManager), so the router can stop routing/v3Internalwithout affecting internal traffic.Change
RouterPathLookupreturnsDONT_ROUTEfor any path whose first segment isv3Internal, mirroring the existing treatment of/v3/metadata-internals.RouterPathLookupTestis updated accordingly.