You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-commits@hadoop.apache.org by st...@apache.org on 2017/09/05 13:48:32 UTC
[1/2] hadoop git commit: HADOOP-14820 Wasb mkdirs security checks
inconsistent with HDFS. Contributed by Sivaguru Sankaridurg
Repository: hadoop
Updated Branches:
refs/heads/branch-2 80516b3de -> f382f6625
refs/heads/trunk 5dba54596 -> 792eff9ea
HADOOP-14820 Wasb mkdirs security checks inconsistent with HDFS.
Contributed by Sivaguru Sankaridurg
Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo
Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/792eff9e
Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/792eff9e
Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/792eff9e
Branch: refs/heads/trunk
Commit: 792eff9ea70da2c6e0ff5a1b177a51e7b2fb96eb
Parents: 5dba545
Author: Steve Loughran <st...@apache.org>
Authored: Tue Sep 5 14:16:57 2017 +0100
Committer: Steve Loughran <st...@apache.org>
Committed: Tue Sep 5 14:16:57 2017 +0100
----------------------------------------------------------------------
.../hadoop/fs/azure/NativeAzureFileSystem.java | 9 ++-
.../TestNativeAzureFileSystemAuthorization.java | 64 ++++++++++++++++++++
.../TestAzureFileSystemInstrumentation.java | 18 +++---
3 files changed, 77 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hadoop/blob/792eff9e/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
----------------------------------------------------------------------
diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
index 2abc6c6..0bde124 100644
--- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
+++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
@@ -2425,13 +2425,13 @@ public class NativeAzureFileSystem extends FileSystem {
private Path getAncestor(Path f) throws IOException {
- for (Path current = f.getParent(), parent = current.getParent();
+ for (Path current = f, parent = current.getParent();
parent != null; // Stop when you get to the root
current = parent, parent = current.getParent()) {
String currentKey = pathToKey(current);
FileMetadata currentMetadata = store.retrieveMetadata(currentKey);
- if (currentMetadata != null) {
+ if (currentMetadata != null && currentMetadata.isDir()) {
Path ancestor = keyToPath(currentMetadata.getKey());
LOG.debug("Found ancestor {}, for path: {}", ancestor.toString(), f.toString());
return ancestor;
@@ -2448,7 +2448,6 @@ public class NativeAzureFileSystem extends FileSystem {
public boolean mkdirs(Path f, FsPermission permission, boolean noUmask) throws IOException {
-
LOG.debug("Creating directory: {}", f.toString());
if (containsColon(f)) {
@@ -2459,6 +2458,10 @@ public class NativeAzureFileSystem extends FileSystem {
Path absolutePath = makeAbsolute(f);
Path ancestor = getAncestor(absolutePath);
+ if (absolutePath.equals(ancestor)) {
+ return true;
+ }
+
performAuthCheck(ancestor, WasbAuthorizationOperations.WRITE, "mkdirs", absolutePath);
PermissionStatus permissionStatus = null;
http://git-wip-us.apache.org/repos/asf/hadoop/blob/792eff9e/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java
----------------------------------------------------------------------
diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java
index 91d6ebb..a3f2843 100644
--- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java
+++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java
@@ -604,6 +604,70 @@ public class TestNativeAzureFileSystemAuthorization
}
/**
+ * Positive test for mkdirs -p with existing hierarchy
+ * @throws Throwable
+ */
+ @Test
+ public void testMkdirsWithExistingHierarchyCheckPositive1() throws Throwable {
+
+ Path testPath = new Path("/testMkdirsWithExistingHierarchyCheckPositive1");
+
+ authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+ fs.updateWasbAuthorizer(authorizer);
+
+ try {
+ fs.mkdirs(testPath);
+ ContractTestUtils.assertIsDirectory(fs, testPath);
+
+ /* Don't need permissions to create a directory that already exists */
+ authorizer.deleteAllAuthRules();
+
+ fs.mkdirs(testPath);
+ ContractTestUtils.assertIsDirectory(fs, testPath);
+ }
+ finally {
+ allowRecursiveDelete(fs, testPath.toString());
+ fs.delete(testPath, true);
+ }
+ }
+
+ @Test
+ public void testMkdirsWithExistingHierarchyCheckPositive2() throws Throwable {
+
+ Path testPath = new Path("/testMkdirsWithExistingHierarchyCheckPositive2");
+ Path childPath1 = new Path(testPath, "1");
+ Path childPath2 = new Path(childPath1, "2");
+ Path childPath3 = new Path(childPath2, "3");
+
+ authorizer.addAuthRule("/",
+ WasbAuthorizationOperations.WRITE.toString(), true);
+
+ authorizer.addAuthRule(childPath1.toString(),
+ WasbAuthorizationOperations.WRITE.toString(), true);
+
+ fs.updateWasbAuthorizer(authorizer);
+
+ try {
+ fs.mkdirs(childPath1);
+ ContractTestUtils.assertIsDirectory(fs, childPath1);
+
+ // Path already exists => no-op.
+ fs.mkdirs(testPath);
+ ContractTestUtils.assertIsDirectory(fs, testPath);
+
+ // Path already exists => no-op.
+ fs.mkdirs(childPath1);
+ ContractTestUtils.assertIsDirectory(fs, childPath1);
+
+ // Check permissions against existing ancestor childPath1
+ fs.mkdirs(childPath3);
+ ContractTestUtils.assertIsDirectory(fs, childPath3);
+ } finally {
+ allowRecursiveDelete(fs, testPath.toString());
+ fs.delete(testPath, true);
+ }
+ }
+ /**
* Negative test for mkdirs access check
* @throws Throwable
*/
http://git-wip-us.apache.org/repos/asf/hadoop/blob/792eff9e/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java
----------------------------------------------------------------------
diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java
index a1a021b..818a844 100644
--- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java
+++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java
@@ -108,17 +108,13 @@ public class TestAzureFileSystemInstrumentation {
// Create a directory
assertTrue(fs.mkdirs(new Path("a")));
- // At the time of writing, it takes 1 request to create the actual directory,
- // plus 2 requests per level to check that there's no blob with that name and
- // 1 request per level above to create it if it doesn't exist.
- // So for the path above (/user/<name>/a), it takes 2 requests each to check
- // there's no blob called /user, no blob called /user/<name> and no blob
- // called /user/<name>/a, and then 3 request for the creation of the three
- // levels, and then 2 requests for checking/stamping the version of AS,
- // totaling 11.
- // Also, there's the initial 1 request for container check so total is 12.
- // The getAncestor call at the very beginning adds another 4 calls, totalling 16.
- base = assertWebResponsesInRange(base, 1, 16);
+ // At the time of writing
+ // getAncestor uses 2 calls for each folder level /user/<name>/a
+ // plus 1 call made by checkContainer
+ // mkdir checks the hierarchy with 2 calls per level
+ // mkdirs calls storeEmptyDir to create the empty folder, which makes 5 calls
+ // For a total of 7 + 6 + 5 = 18 web responses
+ base = assertWebResponsesInRange(base, 1, 18);
assertEquals(1,
AzureMetricsTestUtil.getLongCounterValue(getInstrumentation(), WASB_DIRECTORIES_CREATED));
---------------------------------------------------------------------
To unsubscribe, e-mail: common-commits-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-commits-help@hadoop.apache.org
[2/2] hadoop git commit: HADOOP-14820 Wasb mkdirs security checks
inconsistent with HDFS. Contributed by Sivaguru Sankaridurg
Posted by st...@apache.org.
HADOOP-14820 Wasb mkdirs security checks inconsistent with HDFS.
Contributed by Sivaguru Sankaridurg
Project: http://git-wip-us.apache.org/repos/asf/hadoop/repo
Commit: http://git-wip-us.apache.org/repos/asf/hadoop/commit/f382f662
Tree: http://git-wip-us.apache.org/repos/asf/hadoop/tree/f382f662
Diff: http://git-wip-us.apache.org/repos/asf/hadoop/diff/f382f662
Branch: refs/heads/branch-2
Commit: f382f662559cb631914f886e77a2f5fef8b10d7a
Parents: 80516b3
Author: Steve Loughran <st...@apache.org>
Authored: Tue Sep 5 14:16:57 2017 +0100
Committer: Steve Loughran <st...@apache.org>
Committed: Tue Sep 5 14:18:43 2017 +0100
----------------------------------------------------------------------
.../hadoop/fs/azure/NativeAzureFileSystem.java | 9 ++-
.../TestNativeAzureFileSystemAuthorization.java | 64 ++++++++++++++++++++
.../TestAzureFileSystemInstrumentation.java | 18 +++---
3 files changed, 77 insertions(+), 14 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/hadoop/blob/f382f662/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
----------------------------------------------------------------------
diff --git a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
index b0b872d..01b957d 100644
--- a/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
+++ b/hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java
@@ -2423,13 +2423,13 @@ public class NativeAzureFileSystem extends FileSystem {
private Path getAncestor(Path f) throws IOException {
- for (Path current = f.getParent(), parent = current.getParent();
+ for (Path current = f, parent = current.getParent();
parent != null; // Stop when you get to the root
current = parent, parent = current.getParent()) {
String currentKey = pathToKey(current);
FileMetadata currentMetadata = store.retrieveMetadata(currentKey);
- if (currentMetadata != null) {
+ if (currentMetadata != null && currentMetadata.isDir()) {
Path ancestor = keyToPath(currentMetadata.getKey());
LOG.debug("Found ancestor {}, for path: {}", ancestor.toString(), f.toString());
return ancestor;
@@ -2446,7 +2446,6 @@ public class NativeAzureFileSystem extends FileSystem {
public boolean mkdirs(Path f, FsPermission permission, boolean noUmask) throws IOException {
-
LOG.debug("Creating directory: {}", f.toString());
if (containsColon(f)) {
@@ -2457,6 +2456,10 @@ public class NativeAzureFileSystem extends FileSystem {
Path absolutePath = makeAbsolute(f);
Path ancestor = getAncestor(absolutePath);
+ if (absolutePath.equals(ancestor)) {
+ return true;
+ }
+
performAuthCheck(ancestor, WasbAuthorizationOperations.WRITE, "mkdirs", absolutePath);
PermissionStatus permissionStatus = null;
http://git-wip-us.apache.org/repos/asf/hadoop/blob/f382f662/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java
----------------------------------------------------------------------
diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java
index 33f7a45..c04080f 100644
--- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java
+++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java
@@ -604,6 +604,70 @@ public class TestNativeAzureFileSystemAuthorization
}
/**
+ * Positive test for mkdirs -p with existing hierarchy
+ * @throws Throwable
+ */
+ @Test
+ public void testMkdirsWithExistingHierarchyCheckPositive1() throws Throwable {
+
+ Path testPath = new Path("/testMkdirsWithExistingHierarchyCheckPositive1");
+
+ authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+ fs.updateWasbAuthorizer(authorizer);
+
+ try {
+ fs.mkdirs(testPath);
+ ContractTestUtils.assertIsDirectory(fs, testPath);
+
+ /* Don't need permissions to create a directory that already exists */
+ authorizer.deleteAllAuthRules();
+
+ fs.mkdirs(testPath);
+ ContractTestUtils.assertIsDirectory(fs, testPath);
+ }
+ finally {
+ allowRecursiveDelete(fs, testPath.toString());
+ fs.delete(testPath, true);
+ }
+ }
+
+ @Test
+ public void testMkdirsWithExistingHierarchyCheckPositive2() throws Throwable {
+
+ Path testPath = new Path("/testMkdirsWithExistingHierarchyCheckPositive2");
+ Path childPath1 = new Path(testPath, "1");
+ Path childPath2 = new Path(childPath1, "2");
+ Path childPath3 = new Path(childPath2, "3");
+
+ authorizer.addAuthRule("/",
+ WasbAuthorizationOperations.WRITE.toString(), true);
+
+ authorizer.addAuthRule(childPath1.toString(),
+ WasbAuthorizationOperations.WRITE.toString(), true);
+
+ fs.updateWasbAuthorizer(authorizer);
+
+ try {
+ fs.mkdirs(childPath1);
+ ContractTestUtils.assertIsDirectory(fs, childPath1);
+
+ // Path already exists => no-op.
+ fs.mkdirs(testPath);
+ ContractTestUtils.assertIsDirectory(fs, testPath);
+
+ // Path already exists => no-op.
+ fs.mkdirs(childPath1);
+ ContractTestUtils.assertIsDirectory(fs, childPath1);
+
+ // Check permissions against existing ancestor childPath1
+ fs.mkdirs(childPath3);
+ ContractTestUtils.assertIsDirectory(fs, childPath3);
+ } finally {
+ allowRecursiveDelete(fs, testPath.toString());
+ fs.delete(testPath, true);
+ }
+ }
+ /**
* Negative test for mkdirs access check
* @throws Throwable
*/
http://git-wip-us.apache.org/repos/asf/hadoop/blob/f382f662/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java
----------------------------------------------------------------------
diff --git a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java
index a1a021b..818a844 100644
--- a/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java
+++ b/hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java
@@ -108,17 +108,13 @@ public class TestAzureFileSystemInstrumentation {
// Create a directory
assertTrue(fs.mkdirs(new Path("a")));
- // At the time of writing, it takes 1 request to create the actual directory,
- // plus 2 requests per level to check that there's no blob with that name and
- // 1 request per level above to create it if it doesn't exist.
- // So for the path above (/user/<name>/a), it takes 2 requests each to check
- // there's no blob called /user, no blob called /user/<name> and no blob
- // called /user/<name>/a, and then 3 request for the creation of the three
- // levels, and then 2 requests for checking/stamping the version of AS,
- // totaling 11.
- // Also, there's the initial 1 request for container check so total is 12.
- // The getAncestor call at the very beginning adds another 4 calls, totalling 16.
- base = assertWebResponsesInRange(base, 1, 16);
+ // At the time of writing
+ // getAncestor uses 2 calls for each folder level /user/<name>/a
+ // plus 1 call made by checkContainer
+ // mkdir checks the hierarchy with 2 calls per level
+ // mkdirs calls storeEmptyDir to create the empty folder, which makes 5 calls
+ // For a total of 7 + 6 + 5 = 18 web responses
+ base = assertWebResponsesInRange(base, 1, 18);
assertEquals(1,
AzureMetricsTestUtil.getLongCounterValue(getInstrumentation(), WASB_DIRECTORIES_CREATED));
---------------------------------------------------------------------
To unsubscribe, e-mail: common-commits-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-commits-help@hadoop.apache.org