HADOOP-14845. Azure wasb: getFileStatus not making any auth check.

Contributed by Sivaguru Sankaridurg

hadoop-tools/hadoop-azure/src/main/java/org/apache/hadoop/fs/azure/NativeAzureFileSystem.java

@@ -281,7 +281,7 @@ public class NativeAzureFileSystem extends FileSystem {
      * @param fs file system on which a file is written.
      * @throws IOException Thrown when fail to write file.
      */
-    public void writeFile(FileSystem fs) throws IOException {
+    public void writeFile(NativeAzureFileSystem fs) throws IOException {
       Path path = getRenamePendingFilePath();
       LOG.debug("Preparing to write atomic rename state to {}", path.toString());
       OutputStream output = null;
@@ -290,7 +290,7 @@ public class NativeAzureFileSystem extends FileSystem {
 
       // Write file.
       try {
-        output = fs.create(path);
+        output = fs.createInternal(path, FsPermission.getFileDefault(), false, null);
         output.write(contents.getBytes(Charset.forName("UTF-8")));
       } catch (IOException e) {
         throw new IOException("Unable to write RenamePending file for folder rename from "
@@ -555,7 +555,7 @@ public class NativeAzureFileSystem extends FileSystem {
       if (!sourceFolderGone) {
         // Make sure the target folder exists.
         Path dst = fullPath(dstKey);
-        if (!fs.exists(dst)) {
+        if (!fs.existsInternal(dst)) {
           fs.mkdirs(dst);
         }
 
@@ -1658,6 +1658,10 @@ public class NativeAzureFileSystem extends FileSystem {
     // At this point, we have exclusive access to the source folder
     // via the lease, so we will not conflict with an active folder
     // rename operation.
+    //
+    // In the secure case, the call to exists will happen in the context
+    // of the user that initiated the operation. In this case, we should
+    // do the auth-check against ranger for the path.
     if (!exists(parent)) {
       try {
 
@@ -1753,6 +1757,30 @@ public class NativeAzureFileSystem extends FileSystem {
 
     performAuthCheck(ancestor, WasbAuthorizationOperations.WRITE, "create", absolutePath);
 
+    return createInternal(f, permission, overwrite, parentFolderLease);
+  }
+
+
+  /**
+   * This is the version of the create call that is meant for internal usage.
+   * This version is not public facing and does not perform authorization checks.
+   * It is used by the public facing create call and by FolderRenamePending to
+   * create the internal -RenamePending.json file.
+   * @param f the path to a file to be created.
+   * @param permission for the newly created file.
+   * @param overwrite specifies if the file should be overwritten.
+   * @param parentFolderLease lease on the parent folder.
+   * @return the output stream used to write data into the newly created file .
+   * @throws IOException if an IO error occurs while attempting to delete the
+   * path.
+   *
+   */
+  protected FSDataOutputStream createInternal(Path f, FsPermission permission,
+                                    boolean overwrite,
+                                    SelfRenewingLease parentFolderLease)
+      throws FileAlreadyExistsException, IOException {
+
+    Path absolutePath = makeAbsolute(f);
     String key = pathToKey(absolutePath);
 
     FileMetadata existingMetadata = store.retrieveMetadata(key);
@@ -2170,6 +2198,49 @@ public class NativeAzureFileSystem extends FileSystem {
 
     // Capture the absolute path and the path to key.
     Path absolutePath = makeAbsolute(f);
+
+    if (!isRenamePendingFile(absolutePath)) {
+      Path ancestor = getAncestor(absolutePath);
+      if (ancestor.equals(absolutePath) && !ancestor.equals(new Path("/"))) {
+        performAuthCheck(ancestor.getParent(), WasbAuthorizationOperations.READ,
+            "getFileStatus", absolutePath);
+      }
+      else {
+        performAuthCheck(ancestor, WasbAuthorizationOperations.READ,
+            "getFileStatus", absolutePath);
+      }
+    }
+
+    return getFileStatusInternal(f);
+  }
+
+  /**
+   * Checks if a given path exists in the filesystem.
+   * Calls getFileStatusInternal and has the same costs
+   * as the public facing exists call.
+   * This internal version of the exists call does not perform
+   * authorization checks, and is used internally by various filesystem
+   * operations that need to check if the parent/ancestor/path exist.
+   * The idea is to avoid having to configure authorization policies for
+   * these internal calls.
+   * @param f the path to a file or directory.
+   * @return true if path exists; otherwise false.
+   * @throws IOException if an IO error occurs while attempting to check
+   * for existence of the path.
+   *
+   */
+  protected boolean existsInternal(Path f) throws IOException {
+    try {
+      this.getFileStatusInternal(f);
+      return true;
+    } catch (FileNotFoundException fnfe) {
+      return false;
+    }
+  }
+
+  private FileStatus getFileStatusInternal(Path f) throws FileNotFoundException, IOException {
+
+    Path absolutePath = makeAbsolute(f);
     String key = pathToKey(absolutePath);
     if (key.length() == 0) { // root always exists
       return newDirectory(null, absolutePath);
@@ -2235,7 +2306,7 @@ public class NativeAzureFileSystem extends FileSystem {
     // Check if there is a -RenamePending.json file for this folder, and if so,
     // redo the rename.
     Path absoluteRenamePendingFile = renamePendingFilePath(f);
-    if (exists(absoluteRenamePendingFile)) {
+    if (existsInternal(absoluteRenamePendingFile)) {
       FolderRenamePending pending =
           new FolderRenamePending(absoluteRenamePendingFile, this);
       pending.redo();

hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorization.java

@@ -18,6 +18,7 @@
 
 package org.apache.hadoop.fs.azure;
 
+import java.io.FileNotFoundException;
 import java.security.PrivilegedExceptionAction;
 
 import org.apache.hadoop.conf.Configuration;
@@ -26,6 +27,7 @@ import org.apache.hadoop.fs.FSDataOutputStream;
 import org.apache.hadoop.fs.Path;
 import org.apache.hadoop.fs.contract.ContractTestUtils;
 import org.apache.hadoop.security.UserGroupInformation;
+import org.apache.hadoop.test.GenericTestUtils;
 import org.apache.hadoop.util.StringUtils;
 
 import org.junit.Assume;
@@ -116,6 +118,7 @@ public class TestNativeAzureFileSystemAuthorization
     Path testPath = new Path(parentDir, "test.dat");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule("/", WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -141,6 +144,8 @@ public class TestNativeAzureFileSystemAuthorization
     Path testPath = new Path(parentDir, "test.dat");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -167,7 +172,9 @@ public class TestNativeAzureFileSystemAuthorization
 
     setExpectedFailureMessage("create", testPath);
 
-    authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -193,6 +200,7 @@ public class TestNativeAzureFileSystemAuthorization
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
     authorizer.addAuthRule(testPath.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -296,6 +304,7 @@ public class TestNativeAzureFileSystemAuthorization
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); /* to create parentDir */
     authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.WRITE.toString(), true); /* for rename */
+    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -326,6 +335,7 @@ public class TestNativeAzureFileSystemAuthorization
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); /* to create parent dir */
     authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.WRITE.toString(), false);
+    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -358,6 +368,8 @@ public class TestNativeAzureFileSystemAuthorization
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); /* to create parent dir */
     authorizer.addAuthRule(parentSrcDir.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
     authorizer.addAuthRule(parentDstDir.toString(), WasbAuthorizationOperations.WRITE.toString(), false);
+    authorizer.addAuthRule(parentSrcDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
+    authorizer.addAuthRule(parentDstDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -387,6 +399,8 @@ public class TestNativeAzureFileSystemAuthorization
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); /* to create parent dirs */
     authorizer.addAuthRule(parentSrcDir.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
     authorizer.addAuthRule(parentDstDir.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(parentSrcDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
+    authorizer.addAuthRule(parentDstDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -494,6 +508,7 @@ public class TestNativeAzureFileSystemAuthorization
     Path testPath = new Path(parentDir, "test.dat");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
     try {
       fs.create(testPath);
@@ -517,7 +532,8 @@ public class TestNativeAzureFileSystemAuthorization
 
     setExpectedFailureMessage("delete", testPath);
 
-    authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
     try {
       fs.create(testPath);
@@ -526,7 +542,8 @@ public class TestNativeAzureFileSystemAuthorization
 
       /* Remove permissions for delete to force failure */
       authorizer.deleteAllAuthRules();
-      authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), false);
+      authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.WRITE.toString(), false);
+      authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
       fs.updateWasbAuthorizer(authorizer);
 
       fs.delete(testPath, false);
@@ -534,7 +551,8 @@ public class TestNativeAzureFileSystemAuthorization
     finally {
       /* Restore permissions to force a successful delete */
       authorizer.deleteAllAuthRules();
-      authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+      authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
+      authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
       fs.updateWasbAuthorizer(authorizer);
 
       fs.delete(testPath, false);
@@ -551,9 +569,12 @@ public class TestNativeAzureFileSystemAuthorization
   public void testFileDeleteAccessWithIntermediateFoldersCheckPositive() throws Throwable {
 
     Path parentDir = new Path("/testDeleteIntermediateFolder");
-    Path testPath = new Path(parentDir, "1/2/test.dat");
+    Path childPath = new Path(parentDir, "1/2");
+    Path testPath = new Path(childPath, "test.dat");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true); // for create and delete
+    authorizer.addAuthRule(childPath.toString(), WasbAuthorizationOperations.READ.toString(), true);
+    authorizer.addAuthRule("/", WasbAuthorizationOperations.READ.toString(), true);
     authorizer.addAuthRule("/testDeleteIntermediateFolder*",
         WasbAuthorizationOperations.WRITE.toString(), true); // for recursive delete
     fs.updateWasbAuthorizer(authorizer);
@@ -571,13 +592,14 @@ public class TestNativeAzureFileSystemAuthorization
   }
 
   /**
-   * Positive test for getFileStatus. No permissions are required for getting filestatus.
+   * Positive test for getFileStatus.
    * @throws Throwable
    */
   @Test
   public void testGetFileStatusPositive() throws Throwable {
 
     Path testPath = new Path("/");
+    authorizer.addAuthRule("/", WasbAuthorizationOperations.READ.toString(), true);
     ContractTestUtils.assertIsDirectory(fs, testPath);
   }
 
@@ -591,6 +613,8 @@ public class TestNativeAzureFileSystemAuthorization
     Path testPath = new Path("/testMkdirsAccessCheckPositive/1/2/3");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -613,6 +637,8 @@ public class TestNativeAzureFileSystemAuthorization
     Path testPath = new Path("/testMkdirsWithExistingHierarchyCheckPositive1");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -621,6 +647,8 @@ public class TestNativeAzureFileSystemAuthorization
 
       /* Don't need permissions to create a directory that already exists */
       authorizer.deleteAllAuthRules();
+      authorizer.addAuthRule(testPath.getParent().toString(),
+          WasbAuthorizationOperations.READ.toString(), true); // for assert
 
       fs.mkdirs(testPath);
       ContractTestUtils.assertIsDirectory(fs, testPath);
@@ -645,6 +673,13 @@ public class TestNativeAzureFileSystemAuthorization
     authorizer.addAuthRule(childPath1.toString(),
         WasbAuthorizationOperations.WRITE.toString(), true);
 
+    authorizer.addAuthRule(childPath1.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
+    authorizer.addAuthRule(childPath3.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
+
     fs.updateWasbAuthorizer(authorizer);
 
     try {
@@ -718,6 +753,7 @@ public class TestNativeAzureFileSystemAuthorization
     final Path testPath = new Path("/testSetOwnerNegative");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(testPath.toString(), WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     String owner = null;
@@ -752,9 +788,11 @@ public class TestNativeAzureFileSystemAuthorization
     final Path testPath = new Path("/testSetOwnerPositive");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
-    final String newOwner = "newowner";
+    final String newOwner = "user2";
     final String newGroup = "newgroup";
 
     UserGroupInformation authorisedUser = UserGroupInformation.createUserForTesting(
@@ -797,6 +835,8 @@ public class TestNativeAzureFileSystemAuthorization
 
     authorizer.init(conf);
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     final String newOwner = "newowner";
@@ -843,6 +883,8 @@ public class TestNativeAzureFileSystemAuthorization
 
     authorizer.init(conf);
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     String owner = null;
@@ -868,4 +910,53 @@ public class TestNativeAzureFileSystemAuthorization
       fs.delete(testPath, false);
     }
   }
+
+  /** Test to ensure that the internal RenamePending mechanism
+   * does not make authorization calls.
+   */
+  @Test
+  public void testRenamePendingAuthorizationCalls() throws Throwable {
+    Path testPath = new Path("/testRenamePendingAuthorizationCalls");
+    Path srcPath = new Path(testPath, "srcPath");
+    Path dstPath = new Path(testPath, "dstPath");
+    Path srcFilePath = new Path(srcPath, "file.txt");
+    Path dstFilePath = new Path(dstPath, "file.txt");
+
+    authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
+    /* Remove nextline after fixing createInternal from FolderRenamePending */
+    authorizer.addAuthRule(testPath.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(srcPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
+    authorizer.addAuthRule(dstFilePath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
+    fs.updateWasbAuthorizer(authorizer);
+
+    try {
+      fs.create(srcFilePath);
+
+      String srcKey = fs.pathToKey(srcPath);
+      String dstKey = fs.pathToKey(dstPath);
+
+      // Create a -RenamePendingFile
+      NativeAzureFileSystem.FolderRenamePending renamePending =
+          new NativeAzureFileSystem.FolderRenamePending(srcKey, dstKey, null, fs);
+      renamePending.writeFile(fs);
+
+      // Initiate the pending-rename
+      fs.getFileStatus(srcPath);
+    } catch (FileNotFoundException fnfe) {
+      // This is expected because getFileStatus would complete the pending "rename"
+      // represented by the -RenamePending file.
+      GenericTestUtils.assertExceptionContains(
+          srcPath.toString() + ": No such file or directory.", fnfe
+      );
+
+      // The pending rename should have completed
+      ContractTestUtils.assertPathExists(fs,
+          "dstFilePath does not exist -- pending rename failed", dstFilePath);
+    } finally {
+      allowRecursiveDelete(fs, testPath.toString());
+      fs.delete(testPath, true);
+    }
+  }
 }

hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestNativeAzureFileSystemAuthorizationWithOwner.java

@@ -49,10 +49,13 @@ public class TestNativeAzureFileSystemAuthorizationWithOwner
     Path testPath = new Path(parentDir, "test.data");
 
     authorizer.addAuthRule("/", WasbAuthorizationOperations.WRITE.toString(), true);
-    authorizer.addAuthRule(testPath.toString(), WasbAuthorizationOperations.READ.toString(), true);
-    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.WRITE.toString(), true);
+    authorizer.addAuthRule(parentDir.toString(),
+        WasbAuthorizationOperations.WRITE.toString(), true);
     // additional rule used for assertPathExists
-    authorizer.addAuthRule(parentDir.toString(), WasbAuthorizationOperations.READ.toString(), true);
+    authorizer.addAuthRule(testPath.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
+    authorizer.addAuthRule(parentDir.getParent().toString(),
+        WasbAuthorizationOperations.READ.toString(), true);
     fs.updateWasbAuthorizer(authorizer);
 
     try {

hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/TestWasbRemoteCallHelper.java

@@ -358,7 +358,7 @@ public class TestWasbRemoteCallHelper
 
     performop(mockHttpClient);
 
-    int expectedNumberOfInvocations = isAuthorizationCachingEnabled ? 1 : 2;
+    int expectedNumberOfInvocations = isAuthorizationCachingEnabled ? 2 : 3;
     Mockito.verify(mockHttpClient, times(expectedNumberOfInvocations)).execute(Mockito.argThat(new HttpGetForServiceLocal()));
     Mockito.verify(mockHttpClient, times(expectedNumberOfInvocations)).execute(Mockito.argThat(new HttpGetForService2()));
   }

hadoop-tools/hadoop-azure/src/test/java/org/apache/hadoop/fs/azure/metrics/TestAzureFileSystemInstrumentation.java

@@ -350,9 +350,9 @@ public class TestAzureFileSystemInstrumentation {
     assertFalse(fs.exists(filePath));
     // At the time of writing this code it takes 2 requests/responses to
     // check existence, which seems excessive, plus initial request for
-    // container check.
+    // container check, plus 2 ancestor checks only in the secure case.
     logOpResponseCount("Checking file existence for non-existent file", base);
-    base = assertWebResponsesInRange(base, 1, 3);
+    base = assertWebResponsesInRange(base, 1, 5);
 
     // Create an empty file
     assertTrue(fs.createNewFile(filePath));
@@ -361,7 +361,7 @@ public class TestAzureFileSystemInstrumentation {
     // Check existence again
     assertTrue(fs.exists(filePath));
     logOpResponseCount("Checking file existence for existent file", base);
-    base = assertWebResponsesInRange(base, 1, 2);
+    base = assertWebResponsesInRange(base, 1, 4);
 
     // Delete the file
     assertEquals(0, AzureMetricsTestUtil.getLongCounterValue(getInstrumentation(), WASB_FILES_DELETED));