Tighten path containment in Live Migration file route

[nvharikrishna]

Making this change as part of CEP-40.

CASSSIDECAR-479 Tightens the path check on the Live Migration file route so that symlinks inside a data directory cannot point to files outside the configured directory.

Earlier, the route only checked the URL text for .. patterns. That check does not catch symlinks, so a symlink within the data dir could still resolve to a file elsewhere on disk. The route now also resolves the path with toRealPath() and confirms the resolved path stays inside the configured base directory.

[yifan-c]

I know that the LiveMigrationInstanceMetadataUtil is a static helper already. But the static map BASE_DIR_CANONICAL catches my attention. I have 2 suggestions.

1. It is common in this project to inject util via guice. Can we do the same, instead of an unmanaged global object out side of guice?
2. To harden the assumption on the size, can we set a size limit and emit warning if exceeding certain size (say 10) when inserting into the map.

[nvharikrishna]

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.

[yifan-c]

Removing the cache sounds good.

I am ok with the the utility class w/o managing its own static/global resources. Just to share the info re: guice + utility class, it is possible. Those are the utility/helper classes using Guice in the codebase.

• CacheFactory — factory for building caches
• DigestVerifierFactory — factory for DigestVerifier instances
• FileStreamer — streams files to HTTP responses
• InstanceMetadataFetcher — retrieves instance info from instanceId/hostname
• SSTableImporter — performs SSTable imports into a Cassandra instance
• SSTableUploader — handles SSTable uploads
• SSTableUploadsPathBuilder — builds paths to the SSTable uploads staging dir
• SidecarClientProvider — Guice Provider for the singleton client
• snapshots.SnapshotPathBuilder — builds/validates snapshot paths on a host
• restore.RestoreJobUtil — restore job helper utilities
• concurrent.ExecutorPools — provides shared executor/thread pools

[yifan-c]

Is it expected? 404 -> 400

[nvharikrishna]

Thanks for pointing it out. There is some ambiguity w.r.t usage of IllegalArgumentException. Changed it to BAD_REQUEST as resolveLexically throws it for malformed URL. This usage is overlapping with file/dir that don’t exist. Introduced new exception to bring clear separation between malformed URLs and well constructed URLs, but file/directory doesn't exist so that existing behavior is preserved.

[yifan-c]

looks like toRealPath already throws NoSuchFileException when the file does not exist. The above check is redundant.

@throws IOException – if the file does not exist or an I/O error occurs

[nvharikrishna]

@throws IOException – if the file does not exist or an I/O error occurs

Documentation says IOException, and implementation may throw just IOException and not NoSuchFileException. So, checking for file existence and throwing NoSuchFileException explicitly.

[frankgh]

+1 Looks good. I have one minor suggestion

[frankgh]

NIT, we can simplify this a bit:

Suggested change

	ResolvedPath resolved;
	try
	{
	localFile = LiveMigrationInstanceMetadataUtil.localPath(normalizedPath, instanceMeta).toString();
	resolved = LiveMigrationInstanceMetadataUtil.resolveLexically(normalizedPath, instanceMeta);
	}
	catch (UnknownMigrationPrefixException e)
	{
	// The URL is well-formed but matches no configured live-migration directory on this
	// instance, so it addresses no resource here - report it as not found.
	rc.fail(wrapHttpException(HttpResponseStatus.NOT_FOUND, e.getMessage(), e));
	return;
	}
	catch (IllegalArgumentException e)
	{
	LOGGER.warn("Invalid path", e);
	rc.response().setStatusCode(HttpResponseStatus.NOT_FOUND.code()).end();
	// URL must have been malformed, report it as bad request
	rc.fail(wrapHttpException(HttpResponseStatus.BAD_REQUEST, e.getMessage(), e));
	return;
	}
	ResolvedPath resolved = LiveMigrationInstanceMetadataUtil.resolveLexically(normalizedPath, instanceMeta);

and then handle with the override:

    @Override
    protected void processFailure(Throwable cause, RoutingContext context, String host, SocketAddress remoteAddress, Void request)
    {
        if (cause instanceof UnknownMigrationPrefixException)
        {
            context.fail(wrapHttpException(HttpResponseStatus.NOT_FOUND, cause.getMessage(), cause));
        }
        else
        {
            super.processFailure(cause, context, host, remoteAddress, request);
        }
    }

The IllegalArgumentException is already handled correctly in org.apache.cassandra.sidecar.handlers.AbstractHandler#determineHttpException