Tighten path containment in Live Migration file route #367
There are no files selected for viewing
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,9 @@ | |
|
|
||
| package org.apache.cassandra.sidecar.livemigration; | ||
|
|
||
| import java.io.IOException; | ||
| import java.nio.file.Files; | ||
| import java.nio.file.NoSuchFileException; | ||
| import java.nio.file.Path; | ||
| import java.nio.file.Paths; | ||
| import java.util.ArrayList; | ||
|
|
@@ -28,12 +31,15 @@ | |
| import java.util.Map; | ||
| import java.util.Objects; | ||
| import java.util.Set; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentMap; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import org.apache.cassandra.sidecar.cluster.instance.InstanceMetadata; | ||
| import org.apache.cassandra.sidecar.common.ApiEndpointsV1; | ||
| import org.apache.cassandra.sidecar.exceptions.LiveMigrationExceptions.UnknownMigrationPrefixException; | ||
| import org.apache.cassandra.sidecar.handlers.livemigration.LiveMigrationDirType; | ||
| import org.jetbrains.annotations.NotNull; | ||
|
|
||
|
|
@@ -58,6 +64,14 @@ public class LiveMigrationInstanceMetadataUtil | |
| { | ||
| private static final Logger LOGGER = LoggerFactory.getLogger(LiveMigrationInstanceMetadataUtil.class); | ||
|
|
||
| /** | ||
| * Caches the canonical (symlinks resolved) form of each base directory keyed by its lexical | ||
| * path. Base directories come from {@link InstanceMetadata}, which is fixed at startup, so the | ||
| * canonical form is stable for the process lifetime. The set of distinct base directories is | ||
| * bounded by configuration (~5–10 per instance), so no eviction is required. | ||
| */ | ||
| private static final ConcurrentMap<Path, Path> BASE_DIR_CANONICAL = new ConcurrentHashMap<>(); | ||
|
|
||
| /** | ||
| * Encapsulates all metadata for a specific directory instance in live migration. | ||
| * Each descriptor represents one physical directory with its associated index, URL route, and placeholder. | ||
|
|
@@ -263,14 +277,52 @@ public static Map<String, Set<String>> placeholderDirsMap(InstanceMetadata insta | |
| } | ||
|
|
||
| /** | ||
| * Resolves a live migration file download URL to a local path. Performs only a lexical | ||
| * containment check; symlinks are not resolved and the file is not required to exist. | ||
| * Suitable for destination-side callers that place files into operator-controlled directories. | ||
| * Source-side callers that serve existing files in response to remote requests must additionally | ||
| * call {@link ResolvedPath#verifyContainment()} on the returned object before using the path. | ||
| * | ||
| * @param fileUrl Live migration file download URL | ||
| * @param metadata Cassandra instance metadata | ||
| * @return lexically-resolved local path for given live migration file download URL | ||
| * @throws IllegalArgumentException if the URL is malformed or the lexical path escapes | ||
| * the base directory | ||
| */ | ||
| public static Path localPath(@NotNull String fileUrl, | ||
| @NotNull InstanceMetadata metadata) | ||
| { | ||
| return resolveLexically(fileUrl, metadata).resolvedPath(); | ||
| } | ||
|
|
||
| private static Path canonicalBaseDir(Path baseDir) throws IOException | ||
| { | ||
| Path cached = BASE_DIR_CANONICAL.get(baseDir); | ||
| if (cached != null) | ||
| { | ||
| return cached; | ||
| } | ||
| Path canonical = baseDir.toRealPath(); | ||
| Path existing = BASE_DIR_CANONICAL.putIfAbsent(baseDir, canonical); | ||
| return existing != null ? existing : canonical; | ||
| } | ||
|
|
||
| /** | ||
| * Lexically resolves a live migration file download URL to a {@link ResolvedPath}. Performs only | ||
| * string-level validation; the filesystem is not touched and the file is not required to exist. | ||
| * | ||
| * @param fileUrl Live migration file download URL | ||
| * @param metadata Cassandra instance metadata | ||
| * @return the lexically-resolved {@link ResolvedPath} | ||
| * @throws IllegalArgumentException if the URL is malformed - it contains a relative traversal | ||
| * segment ({@code /../}) or lexically escapes the configured base directory | ||
| * @throws UnknownMigrationPrefixException if the URL does not match any configured live-migration directory | ||
| * prefix; the URL is well-formed but addresses no resource on this | ||
| * instance. This is a subtype of {@link IllegalArgumentException}, so | ||
| * callers that only care about "bad URL" need not distinguish it | ||
| */ | ||
| public static ResolvedPath resolveLexically(@NotNull String fileUrl, | ||
| @NotNull InstanceMetadata metadata) | ||
| { | ||
| Objects.requireNonNull(fileUrl, "fileUrl cannot be null"); | ||
| Objects.requireNonNull(metadata, "metadata cannot be null"); | ||
|
|
@@ -299,11 +351,66 @@ public static Path localPath(@NotNull String fileUrl, | |
| throw new IllegalArgumentException(errorMessage); | ||
| } | ||
|
|
||
| return new ResolvedPath(baseDir, resolvedPath); | ||
| } | ||
| } | ||
|
|
||
| LOGGER.warn("File url {} does not match any configured live-migration directory prefix.", fileUrl); | ||
| throw new UnknownMigrationPrefixException("File url " + fileUrl + " is unknown."); | ||
| } | ||
|
|
||
| /** | ||
| * Lexical resolution of a live-migration URL: the configured base directory paired with the | ||
| * local path it maps to. | ||
| */ | ||
| public static final class ResolvedPath | ||
| { | ||
| private final Path baseDir; | ||
| private final Path resolvedPath; | ||
|
|
||
| ResolvedPath(Path baseDir, Path resolvedPath) | ||
| { | ||
| this.baseDir = baseDir; | ||
| this.resolvedPath = resolvedPath; | ||
| } | ||
|
|
||
| public Path resolvedPath() | ||
| { | ||
| return resolvedPath; | ||
| } | ||
|
|
||
| /** | ||
| * Verifies that the source-side file represented by this {@code ResolvedPath} exists and | ||
| * that its canonical (symlinks resolved) path stays inside the canonical base directory. | ||
| * Use this on the source side after | ||
| * {@link LiveMigrationInstanceMetadataUtil#resolveLexically(String, InstanceMetadata)} to | ||
| * enforce that the file the operator is about to serve cannot escape the configured | ||
| * directory through a symlink. Callers continue to use {@link #resolvedPath()} (the | ||
| * lexical form) for exclusion matching, logging, and serving, since exclusion patterns | ||
| * and operator-facing logs are configured against the lexical form. | ||
| * | ||
| * <p>Performs blocking filesystem I/O - must be called from a worker thread, not the event | ||
| * loop. Throws {@link NoSuchFileException} before any canonical-path resolution runs, so | ||
| * callers can distinguish "file missing" from "path escapes via symlink". | ||
| * | ||
| * @throws NoSuchFileException if the resolved file does not exist | ||
| * @throws IOException if {@link Path#toRealPath} fails for an I/O reason | ||
| * other than missing file | ||
| * @throws IllegalArgumentException if the canonical path escapes the base directory | ||
| */ | ||
| public void verifyContainment() throws IOException | ||
| { | ||
| if (!Files.exists(resolvedPath)) | ||
| { | ||
| throw new NoSuchFileException(resolvedPath.toString()); | ||
| } | ||
| Path canonical = resolvedPath.toRealPath(); | ||
|
Contributor
There was a problem hiding this comment. looks like
Contributor
Author
There was a problem hiding this comment.
Documentation says IOException, and implementation may throw just IOException and not NoSuchFileException. So, checking for file existence and throwing |
||
| if (!canonical.startsWith(canonicalBaseDir(baseDir))) | ||
| { | ||
| LOGGER.error("Resolved path escapes base directory for {}", resolvedPath); | ||
| throw new IllegalArgumentException("Resolved path escapes base directory"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static Map<String, String> migrationUrlLocalDirMap(InstanceMetadata instanceMetadata) | ||
|
|
||
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know that the
LiveMigrationInstanceMetadataUtilis a static helper already. But the static mapBASE_DIR_CANONICALcatches my attention. I have 2 suggestions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is a utility class and all its methods are static, so Guice cannot be used. Compared to all other work done during downloading, I think the save by this cache is very minimal. For simplicity, I can remove the cache entirely.