zabetak commented on code in PR #6851:
URL: https://github.com/apache/hive/pull/6851#discussion_r4347934616
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java:
##########
@@ -98,4 +102,58 @@ public void rework(HiveConf job, MapredWork work) throws
IOException {
work.getMapWork().removePathToPartitionInfo(toRemove);
}
}
+
+ /**
+ * Expands a symlink file line, accepting only matches under the symlink
file's directory or an allowed root.
+ */
+ static List<Path> resolveTargets(Configuration conf, Path symlinkFile,
List<Path> allowedRoots, String line)
+ throws IOException {
+ // Qualified against the default filesystem, so no filesystem is created
for an arbitrary scheme.
+ FileSystem defaultFs = FileSystem.get(conf);
+ List<Path> roots = new ArrayList<>(allowedRoots);
+ roots.add(qualify(defaultFs, symlinkFile.getParent()));
+ Path pattern = qualify(defaultFs, new Path(line));
+ // Matches are checked too, as a glob can climb out of the root.
+ checkAllowed(pattern, roots);
+ FileStatus[] statuses = pattern.getFileSystem(conf).globStatus(pattern);
+ if (statuses == null) {
+ return Collections.emptyList();
+ }
+ List<Path> matches = new ArrayList<>();
+ for (FileStatus status : statuses) {
+ Path match = qualify(defaultFs, status.getPath());
+ checkAllowed(match, roots);
+ // FileInputFormat globs the match again, so it must not contain glob
metacharacters.
+ if (GLOB_METACHARS.matcher(match.toUri().getPath()).find()) {
+ throw new IOException("Symlink target " + match + " contains glob
metacharacters");
+ }
+ matches.add(match);
+ }
+ return matches;
+ }
+
+ private static void checkAllowed(Path target, List<Path> roots) throws
IOException {
+ for (Path root : roots) {
+ if (FileUtils.isPathWithinSubtree(target, root)) {
+ return;
+ }
+ }
+ throw new IOException("Symlink target " + target + " is outside of the
allowed locations. Additional locations"
+ + " can be configured with " +
ConfVars.HIVE_SYMLINK_ALLOWED_TARGET_PATHS.varname);
+ }
+
+ static List<Path> getAllowedRoots(Configuration conf) throws IOException {
+ List<Path> allowedRoots = new ArrayList<>();
+ for (String root : HiveConf.getTrimmedStringsVar(conf,
ConfVars.HIVE_SYMLINK_ALLOWED_TARGET_PATHS)) {
+ if (!root.isEmpty()) {
+ Path rootPath = new Path(root);
+ allowedRoots.add(qualify(rootPath.getFileSystem(conf), rootPath));
+ }
+ }
+ return allowedRoots;
+ }
+
+ private static Path qualify(FileSystem fs, Path path) {
+ return new Path(path.makeQualified(fs.getUri(),
fs.getWorkingDirectory()).toUri().normalize());
+ }
Review Comment:
Do we really need this wrapper method? Can we simply inline
`path.makeQualified(fs.getUri(), fs.getWorkingDirectory())` where necessary?
The implementation of `Path#makeQualified(java.net.URI,
org.apache.hadoop.fs.Path)` seems to already handle uris and normalization?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java:
##########
@@ -98,4 +102,58 @@ public void rework(HiveConf job, MapredWork work) throws
IOException {
work.getMapWork().removePathToPartitionInfo(toRemove);
}
}
+
+ /**
+ * Expands a symlink file line, accepting only matches under the symlink
file's directory or an allowed root.
+ */
+ static List<Path> resolveTargets(Configuration conf, Path symlinkFile,
List<Path> allowedRoots, String line)
Review Comment:
I think we can drop the `symlinkFile` parameter and the logic associated
with it. If a symlinkFile content points to other files under the same
directory it seems like that we are gonna end up with a circular dependency.
Dropping this logic would make the code more readable and a bit more efficient
since we will not have to copy the `allowedRoots` for every line in the file.
##########
ql/src/test/org/apache/hadoop/hive/ql/io/TestSymlinkTextInputFormat.java:
##########
@@ -364,4 +372,185 @@ private void writeSymlinkFile(Path symlinkFile,
Path...paths)
}
writer.close();
}
+
+ @Test
+ public void testTargetOutsideOfAllowedPaths() throws IOException {
+ writeTextFile(new Path(dataDir1, "file1"), "dir1_file1_line1\n");
+ writeSymlinkFile(new Path(symlinkDir, "symlink_file"), new Path(dataDir1,
"file1"));
+
+ SymlinkTextInputFormat inputFormat = new SymlinkTextInputFormat();
+ FileInputFormat.setInputPaths(job, symlinkDir);
+
+ assertRejected(
+ assertThrows(IOException.class, () -> inputFormat.getSplits(job,
2)).getCause()
+ );
+ assertRejected(
+ assertThrows(IOException.class, () ->
inputFormat.getContentSummary(symlinkDir, job)).getCause()
+ );
+ }
+
+ @Test
+ public void testTargetEscapingAllowedPath() throws IOException {
+ writeTextFile(new Path(dataDir2, "file2"), "dir2_file2_line1\n");
+ writeTextFile(new Path(symlinkDir, "symlink_file"), dataDir1 +
"/../datadir2/file2\n");
+ job.set(ConfVars.HIVE_SYMLINK_ALLOWED_TARGET_PATHS.varname,
dataDir1.toString());
+
+ SymlinkTextInputFormat inputFormat = new SymlinkTextInputFormat();
+ FileInputFormat.setInputPaths(job, symlinkDir);
+
+ assertRejected(assertThrows(IOException.class, () ->
inputFormat.getSplits(job, 2)).getCause());
+ }
+
+ /** Rejected before any FileSystem is created for the unknown scheme. */
+ @Test
+ public void testTargetOnForeignFileSystem() throws IOException {
+ writeTextFile(new Path(symlinkDir, "symlink_file"),
"unknownfs://custom:1234/abc\n");
Review Comment:
How does this test verify that the rejection happens before the FileSystem
creation?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java:
##########
@@ -98,4 +102,58 @@ public void rework(HiveConf job, MapredWork work) throws
IOException {
work.getMapWork().removePathToPartitionInfo(toRemove);
}
}
+
+ /**
+ * Expands a symlink file line, accepting only matches under the symlink
file's directory or an allowed root.
+ */
+ static List<Path> resolveTargets(Configuration conf, Path symlinkFile,
List<Path> allowedRoots, String line)
+ throws IOException {
+ // Qualified against the default filesystem, so no filesystem is created
for an arbitrary scheme.
+ FileSystem defaultFs = FileSystem.get(conf);
Review Comment:
The callers of `resolveTargets` already have a `FileSystem` instance
available. Why do we need to create a new one and not pass the one already
available?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java:
##########
@@ -98,4 +102,58 @@ public void rework(HiveConf job, MapredWork work) throws
IOException {
work.getMapWork().removePathToPartitionInfo(toRemove);
}
}
+
+ /**
+ * Expands a symlink file line, accepting only matches under the symlink
file's directory or an allowed root.
+ */
+ static List<Path> resolveTargets(Configuration conf, Path symlinkFile,
List<Path> allowedRoots, String line)
+ throws IOException {
+ // Qualified against the default filesystem, so no filesystem is created
for an arbitrary scheme.
+ FileSystem defaultFs = FileSystem.get(conf);
+ List<Path> roots = new ArrayList<>(allowedRoots);
+ roots.add(qualify(defaultFs, symlinkFile.getParent()));
+ Path pattern = qualify(defaultFs, new Path(line));
+ // Matches are checked too, as a glob can climb out of the root.
+ checkAllowed(pattern, roots);
+ FileStatus[] statuses = pattern.getFileSystem(conf).globStatus(pattern);
+ if (statuses == null) {
+ return Collections.emptyList();
+ }
+ List<Path> matches = new ArrayList<>();
+ for (FileStatus status : statuses) {
+ Path match = qualify(defaultFs, status.getPath());
+ checkAllowed(match, roots);
+ // FileInputFormat globs the match again, so it must not contain glob
metacharacters.
+ if (GLOB_METACHARS.matcher(match.toUri().getPath()).find()) {
+ throw new IOException("Symlink target " + match + " contains glob
metacharacters");
+ }
+ matches.add(match);
+ }
+ return matches;
+ }
+
+ private static void checkAllowed(Path target, List<Path> roots) throws
IOException {
+ for (Path root : roots) {
+ if (FileUtils.isPathWithinSubtree(target, root)) {
+ return;
+ }
+ }
+ throw new IOException("Symlink target " + target + " is outside of the
allowed locations. Additional locations"
+ + " can be configured with " +
ConfVars.HIVE_SYMLINK_ALLOWED_TARGET_PATHS.varname);
+ }
+
+ static List<Path> getAllowedRoots(Configuration conf) throws IOException {
+ List<Path> allowedRoots = new ArrayList<>();
+ for (String root : HiveConf.getTrimmedStringsVar(conf,
ConfVars.HIVE_SYMLINK_ALLOWED_TARGET_PATHS)) {
+ if (!root.isEmpty()) {
+ Path rootPath = new Path(root);
+ allowedRoots.add(qualify(rootPath.getFileSystem(conf), rootPath));
+ }
+ }
+ return allowedRoots;
Review Comment:
It appears that we shouldn't modify this list so better use Guava's
`ImmutableList` or `Collections.unmodifiableList`
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymlinkTextInputFormat.java:
##########
@@ -192,8 +193,14 @@ private static void getTargetPathsFromSymlinksDirs(
fileSystem.open(symlink.getPath())));
String line;
while ((line = reader.readLine()) != null) {
- targetPaths.add(new Path(line));
- symlinkPaths.add(symlink.getPath());
+ List<Path> matches = resolveTargets(conf, symlink.getPath(),
allowedRoots, line);
+ if (matches.isEmpty()) {
+ throw new IOException("Symlink target " + line + " does not
match any path");
+ }
Review Comment:
Should it fail or just turn into a noop? The behavior does not seem
consistent with `SymbolicInputFormat#rework`.
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java:
##########
@@ -98,4 +102,58 @@ public void rework(HiveConf job, MapredWork work) throws
IOException {
work.getMapWork().removePathToPartitionInfo(toRemove);
}
}
+
+ /**
+ * Expands a symlink file line, accepting only matches under the symlink
file's directory or an allowed root.
+ */
+ static List<Path> resolveTargets(Configuration conf, Path symlinkFile,
List<Path> allowedRoots, String line)
+ throws IOException {
+ // Qualified against the default filesystem, so no filesystem is created
for an arbitrary scheme.
+ FileSystem defaultFs = FileSystem.get(conf);
+ List<Path> roots = new ArrayList<>(allowedRoots);
+ roots.add(qualify(defaultFs, symlinkFile.getParent()));
+ Path pattern = qualify(defaultFs, new Path(line));
+ // Matches are checked too, as a glob can climb out of the root.
+ checkAllowed(pattern, roots);
Review Comment:
1. Should we really have this check here? Can't we just keep the one inside
the for-loop?
2. Is it possible to have a pattern that at this point is invalid but after
glob expansion becomes valid?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymlinkTextInputFormat.java:
##########
@@ -192,8 +193,14 @@ private static void getTargetPathsFromSymlinksDirs(
fileSystem.open(symlink.getPath())));
String line;
while ((line = reader.readLine()) != null) {
- targetPaths.add(new Path(line));
- symlinkPaths.add(symlink.getPath());
Review Comment:
In the old code there was no glob expansion at this point and by navigating
a bit the caller hierarchy I don't see this triggering elsewhere. Does the new
code add more capabilities & new functionality for this format?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java:
##########
@@ -98,4 +102,58 @@ public void rework(HiveConf job, MapredWork work) throws
IOException {
work.getMapWork().removePathToPartitionInfo(toRemove);
}
}
+
+ /**
+ * Expands a symlink file line, accepting only matches under the symlink
file's directory or an allowed root.
+ */
+ static List<Path> resolveTargets(Configuration conf, Path symlinkFile,
List<Path> allowedRoots, String line)
+ throws IOException {
+ // Qualified against the default filesystem, so no filesystem is created
for an arbitrary scheme.
+ FileSystem defaultFs = FileSystem.get(conf);
+ List<Path> roots = new ArrayList<>(allowedRoots);
+ roots.add(qualify(defaultFs, symlinkFile.getParent()));
+ Path pattern = qualify(defaultFs, new Path(line));
+ // Matches are checked too, as a glob can climb out of the root.
+ checkAllowed(pattern, roots);
+ FileStatus[] statuses = pattern.getFileSystem(conf).globStatus(pattern);
Review Comment:
It seems that we already have a filesystem (i.e., `defaultFs`) why do we
need to create it again?
##########
ql/src/test/org/apache/hadoop/hive/ql/io/TestSymlinkTextInputFormat.java:
##########
@@ -364,4 +372,185 @@ private void writeSymlinkFile(Path symlinkFile,
Path...paths)
}
writer.close();
}
+
+ @Test
+ public void testTargetOutsideOfAllowedPaths() throws IOException {
+ writeTextFile(new Path(dataDir1, "file1"), "dir1_file1_line1\n");
+ writeSymlinkFile(new Path(symlinkDir, "symlink_file"), new Path(dataDir1,
"file1"));
+
+ SymlinkTextInputFormat inputFormat = new SymlinkTextInputFormat();
+ FileInputFormat.setInputPaths(job, symlinkDir);
+
+ assertRejected(
+ assertThrows(IOException.class, () -> inputFormat.getSplits(job,
2)).getCause()
+ );
+ assertRejected(
+ assertThrows(IOException.class, () ->
inputFormat.getContentSummary(symlinkDir, job)).getCause()
+ );
+ }
+
+ @Test
+ public void testTargetEscapingAllowedPath() throws IOException {
+ writeTextFile(new Path(dataDir2, "file2"), "dir2_file2_line1\n");
+ writeTextFile(new Path(symlinkDir, "symlink_file"), dataDir1 +
"/../datadir2/file2\n");
+ job.set(ConfVars.HIVE_SYMLINK_ALLOWED_TARGET_PATHS.varname,
dataDir1.toString());
+
+ SymlinkTextInputFormat inputFormat = new SymlinkTextInputFormat();
+ FileInputFormat.setInputPaths(job, symlinkDir);
+
+ assertRejected(assertThrows(IOException.class, () ->
inputFormat.getSplits(job, 2)).getCause());
+ }
+
+ /** Rejected before any FileSystem is created for the unknown scheme. */
+ @Test
+ public void testTargetOnForeignFileSystem() throws IOException {
+ writeTextFile(new Path(symlinkDir, "symlink_file"),
"unknownfs://custom:1234/abc\n");
+
+ SymlinkTextInputFormat inputFormat = new SymlinkTextInputFormat();
+ FileInputFormat.setInputPaths(job, symlinkDir);
+
+ assertRejected(assertThrows(IOException.class, () ->
inputFormat.getSplits(job, 2)).getCause());
+ }
+
+ @Test
+ public void testReworkKeepsTargetNextToSymlinkFile() throws IOException {
Review Comment:
What is this test about?
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java:
##########
@@ -98,4 +102,58 @@ public void rework(HiveConf job, MapredWork work) throws
IOException {
work.getMapWork().removePathToPartitionInfo(toRemove);
}
}
+
+ /**
+ * Expands a symlink file line, accepting only matches under the symlink
file's directory or an allowed root.
+ */
+ static List<Path> resolveTargets(Configuration conf, Path symlinkFile,
List<Path> allowedRoots, String line)
+ throws IOException {
+ // Qualified against the default filesystem, so no filesystem is created
for an arbitrary scheme.
Review Comment:
Don't understand very well what the comment means.
##########
ql/src/java/org/apache/hadoop/hive/ql/io/SymbolicInputFormat.java:
##########
@@ -98,4 +102,58 @@ public void rework(HiveConf job, MapredWork work) throws
IOException {
work.getMapWork().removePathToPartitionInfo(toRemove);
}
}
+
+ /**
+ * Expands a symlink file line, accepting only matches under the symlink
file's directory or an allowed root.
+ */
+ static List<Path> resolveTargets(Configuration conf, Path symlinkFile,
List<Path> allowedRoots, String line)
+ throws IOException {
+ // Qualified against the default filesystem, so no filesystem is created
for an arbitrary scheme.
+ FileSystem defaultFs = FileSystem.get(conf);
+ List<Path> roots = new ArrayList<>(allowedRoots);
+ roots.add(qualify(defaultFs, symlinkFile.getParent()));
+ Path pattern = qualify(defaultFs, new Path(line));
+ // Matches are checked too, as a glob can climb out of the root.
+ checkAllowed(pattern, roots);
+ FileStatus[] statuses = pattern.getFileSystem(conf).globStatus(pattern);
+ if (statuses == null) {
+ return Collections.emptyList();
+ }
+ List<Path> matches = new ArrayList<>();
+ for (FileStatus status : statuses) {
+ Path match = qualify(defaultFs, status.getPath());
+ checkAllowed(match, roots);
+ // FileInputFormat globs the match again, so it must not contain glob
metacharacters.
+ if (GLOB_METACHARS.matcher(match.toUri().getPath()).find()) {
+ throw new IOException("Symlink target " + match + " contains glob
metacharacters");
+ }
+ matches.add(match);
+ }
+ return matches;
+ }
+
+ private static void checkAllowed(Path target, List<Path> roots) throws
IOException {
+ for (Path root : roots) {
+ if (FileUtils.isPathWithinSubtree(target, root)) {
+ return;
+ }
+ }
+ throw new IOException("Symlink target " + target + " is outside of the
allowed locations. Additional locations"
+ + " can be configured with " +
ConfVars.HIVE_SYMLINK_ALLOWED_TARGET_PATHS.varname);
+ }
+
+ static List<Path> getAllowedRoots(Configuration conf) throws IOException {
+ List<Path> allowedRoots = new ArrayList<>();
+ for (String root : HiveConf.getTrimmedStringsVar(conf,
ConfVars.HIVE_SYMLINK_ALLOWED_TARGET_PATHS)) {
+ if (!root.isEmpty()) {
+ Path rootPath = new Path(root);
+ allowedRoots.add(qualify(rootPath.getFileSystem(conf), rootPath));
Review Comment:
Have you considered what should happen if the some root path is not valid or
doesn't exist. It seems that a misconfiguration of the property can make all
Symbolic tables unusable till an HS2 restart happens.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
