YARN-9132. Added file permission check for auxiliary services manifest file.

           Contributed by Billie Rinaldi

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/main/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/AuxServices.java

@@ -41,6 +41,7 @@ import java.util.regex.Pattern;
 import com.fasterxml.jackson.databind.DeserializationFeature;
 import com.fasterxml.jackson.databind.ObjectMapper;
 import org.apache.hadoop.fs.FSDataInputStream;
+import org.apache.hadoop.security.authorize.AccessControlList;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.records.AuxServiceConfiguration;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.records.AuxServiceFile;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.records.AuxServiceRecord;
@@ -481,6 +482,35 @@ public class AuxServices extends AbstractService
     loadManifest(getConfig(), true);
   }
 
+  private boolean checkManifestPermissions(FileStatus status) throws
+      IOException {
+    if ((status.getPermission().toShort() & 0022) != 0) {
+      LOG.error("Manifest file and parents must not be writable by group or " +
+          "others. The current Permission of " + status.getPath() + " is " +
+          status.getPermission());
+      return false;
+    }
+    Path parent = status.getPath().getParent();
+    if (parent == null) {
+      return true;
+    }
+    return checkManifestPermissions(manifestFS.getFileStatus(parent));
+  }
+
+  private boolean checkManifestOwnerAndPermissions(FileStatus status) throws
+      IOException {
+    AccessControlList yarnAdminAcl = new AccessControlList(getConfig().get(
+        YarnConfiguration.YARN_ADMIN_ACL,
+        YarnConfiguration.DEFAULT_YARN_ADMIN_ACL));
+    if (!yarnAdminAcl.isUserAllowed(
+        UserGroupInformation.createRemoteUser(status.getOwner()))) {
+      LOG.error("Manifest must be owned by YARN admin: " + manifest);
+      return false;
+    }
+
+    return checkManifestPermissions(status);
+  }
+
   /**
    * Reads the manifest file if it is configured, exists, and has not been
    * modified since the last read.
@@ -494,14 +524,20 @@ public class AuxServices extends AbstractService
       return null;
     }
     if (!manifestFS.exists(manifest)) {
-      LOG.info("Manifest file " + manifest + " doesn't exist");
+      LOG.warn("Manifest file " + manifest + " doesn't exist");
       return null;
     }
     FileStatus status;
     try {
       status = manifestFS.getFileStatus(manifest);
     } catch (FileNotFoundException e) {
-      LOG.info("Manifest file " + manifest + " doesn't exist");
+      LOG.warn("Manifest file " + manifest + " doesn't exist");
+      return null;
+    }
+    if (!status.isFile()) {
+      LOG.warn("Manifest file " + manifest + " is not a file");
+    }
+    if (!checkManifestOwnerAndPermissions(status)) {
       return null;
     }
     if (status.getModificationTime() == manifestModifyTS) {
@@ -721,6 +757,9 @@ public class AuxServices extends AbstractService
       serviceMap.clear();
       serviceRecordMap.clear();
       serviceMetaData.clear();
+      if (manifestFS != null) {
+        manifestFS.close();
+      }
       if (manifestReloadTimer != null) {
         manifestReloadTimer.cancel();
       }

hadoop-yarn-project/hadoop-yarn/hadoop-yarn-server/hadoop-yarn-server-nodemanager/src/test/java/org/apache/hadoop/yarn/server/nodemanager/containermanager/TestAuxServices.java

@@ -34,9 +34,11 @@ import static org.mockito.Mockito.verify;
 
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.databind.ObjectMapper;
+import org.apache.hadoop.security.UserGroupInformation;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.records.AuxServiceRecord;
 import org.apache.hadoop.yarn.server.nodemanager.containermanager.records.AuxServiceRecords;
 import org.junit.After;
+import org.junit.Assume;
 import org.junit.Before;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
@@ -834,4 +836,53 @@ public class TestAuxServices {
 
     aux.stop();
   }
+
+  @Test
+  public void testAuxServicesManifestPermissions() throws IOException {
+    Assume.assumeTrue(useManifest);
+    Configuration conf = getABConf();
+    FileSystem fs = FileSystem.get(conf);
+    fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
+        .createImmutable((short) 0777));
+    AuxServices aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE);
+    aux.init(conf);
+    assertEquals(0, aux.getServices().size());
+
+    fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
+        .createImmutable((short) 0775));
+    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE);
+    aux.init(conf);
+    assertEquals(0, aux.getServices().size());
+
+    fs.setPermission(new Path(manifest.getAbsolutePath()), FsPermission
+        .createImmutable((short) 0755));
+    fs.setPermission(new Path(rootDir.getAbsolutePath()), FsPermission
+        .createImmutable((short) 0775));
+    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE);
+    aux.init(conf);
+    assertEquals(0, aux.getServices().size());
+
+    fs.setPermission(new Path(rootDir.getAbsolutePath()), FsPermission
+        .createImmutable((short) 0755));
+    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE);
+    aux.init(conf);
+    assertEquals(2, aux.getServices().size());
+
+    conf.set(YarnConfiguration.YARN_ADMIN_ACL, "");
+    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE);
+    aux.init(conf);
+    assertEquals(0, aux.getServices().size());
+
+    conf.set(YarnConfiguration.YARN_ADMIN_ACL, UserGroupInformation
+        .getCurrentUser().getShortUserName());
+    aux = new AuxServices(MOCK_AUX_PATH_HANDLER,
+        MOCK_CONTEXT, MOCK_DEL_SERVICE);
+    aux.init(conf);
+    assertEquals(2, aux.getServices().size());
+  }
 }