Repository navigation
Conversation
A peer that can reach the listener could name any path the process can read as the source_obj_id of a kGetObj, and any path it can write as the dest_obj_id of a kPutObj. The service now takes a base directory at Initialize() and rejects every object id that does not resolve into it. Resolution follows symlinks in the part of the path that exists and normalizes the rest, so "..", "." and symlinks planted inside the base directory cannot lead out of it. The containment check is component-wise, so "/base2/x" is not inside "/base". The resolved path is the one the service then opens, renames or creates, so what was checked is what is used. base_dir itself is created if missing and must be a directory. Enforcement points: - HandleGetObjRequest: the peer's source_obj_id; kError on rejection, the connection stays usable. - HandleDataReceive: dest_obj_id, the one place every write goes through, for a peer's Put and for the local half of a Get; the payload is drained and kError sent, as for the other early failures. - AsyncGet: the local dest_obj_id is checked before anything is sent, so a bad id fails the future at once instead of after a full transfer. The Python binding makes base_dir a required argument. In C++ an empty base_dir means the current working directory, which is what the existing tests rely on with their relative object ids. Tests: unit tests for ResolvePathWithinBaseDir (inside, not yet existing, dot segments, traversal, sibling prefix, base itself, empty, symlink file and directory escapes, symlink staying inside, relative ids, overlong component), and P2P tests for a rejected Put and Get over a raw socket (absolute, "..", symlink) with the socket still serving ids inside, a fast-failing AsyncGet that never contacts the peer, the working-directory default, a base directory given as a symlink, and Initialize() creating a missing base directory and refusing a regular file.
…fer service ReplicationManager.initialize() takes base_dir and hands it to the TransferService it creates, so replication can only read and write checkpoint objects under the ML Flashpoint base path. It is required unless a pre-configured replication_transfer_service is given, in which case that service's own base directory applies. The NeMo wrapper passes flashpoint_base_container. The transfer-service tests switch their working directory to tmp_path and use it as the base directory, so the relative object ids they use stay inside it and no test files are left in the repository. New tests cover a Put to a destination outside the receiver's base directory, a Get of a file outside the peer's base directory, a Get whose local destination is outside the base directory, the ValueError when base_dir is missing, and the pre-configured service path that needs no base_dir.
… service The previous commits let an empty base_dir mean the current working directory. That is one more implicit way to end up with a service whose reach depends on where the process happened to be started. base_dir is now the first parameter of Initialize(), has no default, and an empty value makes Initialize() return -1 like any other unusable base directory. The Python binding is bound directly to the C++ method, so it reads initialize(base_dir, listen_port=0, threads=16, conn_pool_per_peer=16, global_rank=-1). A caller that still passes the port first gets a TypeError instead of an unconfined service. The C++ tests pass their working directory explicitly (CwdBaseDir()), which is where they create their object files by relative name. The test that covered the working-directory default now covers relative ids with the working directory as the base; a new unit test covers the empty base_dir.
Python Code Coverage Summary
Minimum allowed line rate is |
C++ Code Coverage Summary
Minimum allowed line rate is |
This branch has not been deployed
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
1 participant
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.
Follow-up to #112 (b/565101350). Draft: the
ReplicationManagerAPI question under "Open questions" needs a decision before this is ready for review.Problem
source_obj_idanddest_obj_idare raw paths. A peer that can reach the listener can ask for any file the process can read (kGetObj) and write any path the process can write (kPutObj). #112 closed the SSRF; this PR closes the file access.Fix
TransferService::Initialize()takes a base directory (explicit, required, no default), and every object id must resolve into it:ResolvePathWithinBaseDir()(transfer_helpers.h) implements the check. The resolved path is the one the service opens, creates or renames, so what was checked is what is used. The base directory is canonicalized too (a symlinked base works), created if missing, and must be a directory.HandleGetObjRequestsource_obj_idkError, socket stays usableHandleDataReceivedest_obj_id: the peer's for a Put, the local one for a Get. The single write path.kError, socket stays usable (same as the other early failures)AsyncGetdest_obj_id, before anything is sentNot checked locally:
AsyncPut'sdest_obj_idandAsyncGet'ssource_obj_idare paths on the peer, which checks them against its own base directory.API change
base_diris explicit and required everywhere; there is no default such as the working directory.Initialize(base_dir, listen_port = 0, threads = 16, conn_pool_per_peer = 16, global_rank = -1). An emptybase_dir, or one that cannot be created or is not a directory, makesInitialize()return-1. Relative object ids are still resolved against the working directory (asopen(2)would), then confined.TransferService.initialize(base_dir, listen_port=0, threads=16, conn_pool_per_peer=16, global_rank=-1), bound directly to the C++ method. Callers that passed the port positionally get aTypeError(anintis not abase_dir), not a silently unconfined service.ReplicationManager.initialize(..., base_dir=None): required (ValueError) unlessreplication_transfer_serviceis provided, in which case that service's own base directory applies.wrap_trainer_and_auto_resume_with_mlflashpointpassesflashpoint_base_container.docs/user-guide.mdsnippets updated.CwdBaseDir()), which is where they create their object files by relative name.Open questions
base_diracceptable forReplicationManager.initialize()(public API, breaking for direct callers), or should it fall back to something derived from theCheckpointObjectManager(for example the parent ofpool_dir_path)? I chose explicit and required: the fallback would be a hidden coupling, and a missing value should not silently widen access.Verification
transfer_service_test: full suite twice, all green (excl.MLFLogSinkTest.*, pre-existing/unrelated); the 19 new tests pass--gtest_repeat=5; test cwd and/tmpleft clean.pytest tests/replication tests/adapter/nemo/test_wrapper_util.py: 105 passed (5 new).ruff check/ruff format --checkclean;clang-format --dry-run --Werrorclean onsrc.Tests
ResolvePathWithinBaseDirTest.*(12).., absolute outside, sibling prefix, base itself, empty, symlinked file and symlinked directory escapes rejected; symlink staying inside followed; relative ids resolved against cwd; overlong component is an error, not an acceptPutOutsideBaseDirIsRejectedAndSocketStaysUsable..destinations getkError, nothing (not even.tmp) is written, the same socket then completes a Put insideGetOutsideBaseDirIsRejectedAndSocketStaysUsable..and a symlink planted inside the base getkError; the same socket then serves a file insideGetWithLocalDestinationOutsideBaseDirFailsBeforeContactingPeerRelativeIdsAreConfinedToWorkingDirectoryBase,BaseDirGivenAsSymlinkIsResolved,InitializeCreatesBaseDirAndRejectsNonDirectory,InitializeFailsWithEmptyBaseDirInitialize()creating/refusing the directory, and-1for an empty basetest_async_put_outside_base_dir_is_rejected,test_async_get_of_object_outside_peer_base_dir_is_rejected,test_async_get_with_local_destination_outside_base_dir_fails_fast,test_initialize_requires_base_dir_when_creating_transfer_service,test_initialize_accepts_preconfigured_transfer_service_without_base_dirReplicationManager