HDFS-6771. Require specification of an encryption key when creating an encryption zone. (wang)

git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/branches/fs-encryption@1614519 13f79535-47bb-0310-9956-ffa450edef68

hadoop-hdfs-project/hadoop-hdfs/CHANGES-fs-encryption.txt

@@ -65,6 +65,9 @@ fs-encryption (Unreleased)
     HDFS-6509. Create a special /.reserved/raw directory for raw access to
     encrypted data. (clamb via wang)
 
+    HDFS-6771. Require specification of an encryption key when creating
+    an encryption zone. (wang)
+
   OPTIMIZATIONS
 
   BUG FIXES

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/client/HdfsAdmin.java

@@ -231,22 +231,16 @@ public RemoteIterator<CachePoolEntry> listCachePools() throws IOException {
   }
 
   /**
-   * Create an encryption zone rooted at an empty existing directory. An
+   * Create an encryption zone rooted at an empty existing directory, using the
-   * encryption zone has an associated encryption key used when reading and
+   * specified encryption key. An encryption zone has an associated encryption
-   * writing files within the zone. An existing key can be specified,
+   * key used when reading and writing files within the zone.
-   * else a new key will be generated for the encryption zone.
-   *
-   * @param path The path of the root of the encryption zone. Must refer to
-   *             an empty, existing directory.
-   *
-   * @param keyName Optional name of key available at the KeyProvider. If null,
-   *                then a key is generated.
-   *
-   * @throws IOException if there was a general IO exception
    *
+   * @param path    The path of the root of the encryption zone. Must refer to
+   *                an empty, existing directory.
+   * @param keyName Name of key available at the KeyProvider.
+   * @throws IOException            if there was a general IO exception
    * @throws AccessControlException if the caller does not have access to path
-   *
+   * @throws FileNotFoundException  if the path does not exist
-   * @throws FileNotFoundException if the path does not exist
    */
   public void createEncryptionZone(Path path, String keyName)
     throws IOException, AccessControlException, FileNotFoundException {

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java

@@ -8459,22 +8459,17 @@ AclStatus getAclStatus(String src) throws IOException {
   }
 
   /**
-   * Create an encryption zone on directory src. If provided,
+   * Create an encryption zone on directory src using the specified key.
-   * will use an existing key, else will generate a new key.
-   *
-   * @param src the path of a directory which will be the root of the
-   * encryption zone. The directory must be empty.
-   *
-   * @param keyNameArg an optional name of a key in the configured
-   * KeyProvider. If this is null, then a a new key is generated.
-   *
-   * @throws AccessControlException if the caller is not the superuser.
    *
+   * @param src     the path of a directory which will be the root of the
+   *                encryption zone. The directory must be empty.
+   * @param keyName name of a key which must be present in the configured
+   *                KeyProvider.
+   * @throws AccessControlException  if the caller is not the superuser.
    * @throws UnresolvedLinkException if the path can't be resolved.
-   *
+   * @throws SafeModeException       if the Namenode is in safe mode.
-   * @throws SafeModeException if the Namenode is in safe mode.
    */
-  void createEncryptionZone(final String src, String keyNameArg)
+  void createEncryptionZone(final String src, final String keyName)
     throws IOException, UnresolvedLinkException,
       SafeModeException, AccessControlException {
     final CacheEntry cacheEntry = RetryCache.waitForCompletion(retryCache);
@@ -8482,8 +8477,6 @@ void createEncryptionZone(final String src, String keyNameArg)
       return; // Return previous response
     }
 
-    boolean createdKey = false;
-    String keyName = keyNameArg;
     boolean success = false;
     try {
       if (provider == null) {
@@ -8492,22 +8485,20 @@ void createEncryptionZone(final String src, String keyNameArg)
             " since no key provider is available.");
       }
       if (keyName == null || keyName.isEmpty()) {
-        keyName = UUID.randomUUID().toString();
+        throw new IOException("Must specify a key name when creating an " +
-        createNewKey(keyName, src);
+            "encryption zone");
-        createdKey = true;
+      }
-      } else {
+      KeyVersion keyVersion = provider.getCurrentKey(keyName);
-        KeyVersion keyVersion = provider.getCurrentKey(keyName);
+      if (keyVersion == null) {
-        if (keyVersion == null) {
+        /*
-          /*
+         * It would be nice if we threw something more specific than
-           * It would be nice if we threw something more specific than
+         * IOException when the key is not found, but the KeyProvider API
-           * IOException when the key is not found, but the KeyProvider API
+         * doesn't provide for that. If that API is ever changed to throw
-           * doesn't provide for that. If that API is ever changed to throw
+         * something more specific (e.g. UnknownKeyException) then we can
-           * something more specific (e.g. UnknownKeyException) then we can
+         * update this to match it, or better yet, just rethrow the
-           * update this to match it, or better yet, just rethrow the
+         * KeyProvider's exception.
-           * KeyProvider's exception.
+         */
-           */
+        throw new IOException("Key " + keyName + " doesn't exist.");
-          throw new IOException("Key " + keyName + " doesn't exist.");
-        }
       }
       createEncryptionZoneInt(src, keyName, cacheEntry != null);
       success = true;
@@ -8516,10 +8507,6 @@ void createEncryptionZone(final String src, String keyNameArg)
       throw e;
     } finally {
       RetryCache.setState(cacheEntry, success);
-      if (!success && createdKey) {
-        /* Unwind key creation. */
-        provider.deleteKey(keyName);
-      }
     }
   }
 
@@ -8550,40 +8537,6 @@ private void createEncryptionZoneInt(final String srcArg, String keyName,
     logAuditEvent(true, "createEncryptionZone", srcArg, null, resultingStat);
   }
 
-  /**
-   * Create a new key on the KeyProvider for an encryption zone.
-   *
-   * @param keyNameArg name of the key
-   * @param src path of the encryption zone.
-   * @return KeyVersion of the created key
-   * @throws IOException
-   */
-  private KeyVersion createNewKey(String keyNameArg, String src)
-    throws IOException {
-    Preconditions.checkNotNull(keyNameArg);
-    Preconditions.checkNotNull(src);
-    final StringBuilder sb = new StringBuilder("hdfs://");
-    if (nameserviceId != null) {
-      sb.append(nameserviceId);
-    }
-    sb.append(src);
-    if (!src.endsWith("/")) {
-      sb.append('/');
-    }
-    sb.append(keyNameArg);
-    final String keyName = sb.toString();
-    providerOptions.setDescription(keyName);
-    providerOptions.setBitLength(codec.getCipherSuite()
-        .getAlgorithmBlockSize()*8);
-    KeyVersion version = null;
-    try {
-      version = provider.createKey(keyNameArg, providerOptions);
-    } catch (NoSuchAlgorithmException e) {
-      throw new IOException(e);
-    }
-    return version;
-  }
-
   List<EncryptionZone> listEncryptionZones() throws IOException {
     boolean success = false;
     checkSuperuserPrivilege();

hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/CryptoAdmin.java

@@ -124,7 +124,7 @@ public String getName() {
 
     @Override
     public String getShortUsage() {
-      return "[" + getName() + " [-keyName <keyName>] -path <path> " + "]\n";
+      return "[" + getName() + " -keyName <keyName> -path <path> " + "]\n";
     }
 
     @Override
@@ -133,7 +133,7 @@ public String getLongUsage() {
       listing.addRow("<path>", "The path of the encryption zone to create. " +
         "It must be an empty directory.");
       listing.addRow("<keyName>", "Name of the key to use for the " +
-          "encryption zone. A new key will be generated if unspecified.");
+          "encryption zone.");
       return getShortUsage() + "\n" +
         "Create a new encryption zone.\n\n" +
         listing.toString();
@@ -149,6 +149,10 @@ public int run(Configuration conf, List<String> args) throws IOException {
 
       final String keyName =
           StringUtils.popOptionWithArgument("-keyName", args);
+      if (keyName == null) {
+        System.err.println("You must specify a key name with -keyName.");
+        return 1;
+      }
 
       if (!args.isEmpty()) {
         System.err.println("Can't understand argument: " + args.get(0));

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestCryptoAdminCLI.java

@@ -48,7 +48,7 @@
 import org.junit.Test;
 import org.xml.sax.SAXException;
 
-public class TestCryptoCLI  extends CLITestHelperDFS {
+public class TestCryptoAdminCLI extends CLITestHelperDFS {
   protected MiniDFSCluster dfsCluster = null;
   protected FileSystem fs = null;
   protected String namenode = null;

hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java

@@ -68,12 +68,13 @@ public class TestEncryptionZones {
   private HdfsAdmin dfsAdmin;
   private DistributedFileSystem fs;
   private File testRootDir;
+  private final String TEST_KEY = "testKey";
 
   protected FileSystemTestWrapper fsWrapper;
   protected FileContextTestWrapper fcWrapper;
 
   @Before
-  public void setup() throws IOException {
+  public void setup() throws Exception {
     conf = new HdfsConfiguration();
     fsHelper = new FileSystemTestHelper();
     // Set up java key store
@@ -93,6 +94,8 @@ public void setup() throws IOException {
     // else the updates do not get flushed properly
     fs.getClient().provider = cluster.getNameNode().getNamesystem()
         .getProvider();
+    // Create a test key
+    createKey(TEST_KEY);
   }
 
   @After
@@ -143,6 +146,8 @@ private void createKey(String keyName)
       throws NoSuchAlgorithmException, IOException {
     KeyProvider provider = cluster.getNameNode().getNamesystem().getProvider();
     final KeyProvider.Options options = KeyProvider.options(conf);
+    options.setDescription(keyName);
+    options.setBitLength(128);
     provider.createKey(keyName, options);
     provider.flush();
   }
@@ -155,7 +160,7 @@ public void testBasicOperations() throws Exception {
     /* Test failure of create EZ on a directory that doesn't exist. */
     final Path zone1 = new Path("/zone1");
     try {
-      dfsAdmin.createEncryptionZone(zone1, null);
+      dfsAdmin.createEncryptionZone(zone1, TEST_KEY);
       fail("expected /test doesn't exist");
     } catch (IOException e) {
       assertExceptionContains("cannot find", e);
@@ -163,13 +168,13 @@ public void testBasicOperations() throws Exception {
 
     /* Normal creation of an EZ */
     fsWrapper.mkdir(zone1, FsPermission.getDirDefault(), true);
-    dfsAdmin.createEncryptionZone(zone1, null);
+    dfsAdmin.createEncryptionZone(zone1, TEST_KEY);
     assertNumZones(++numZones);
     assertZonePresent(null, zone1.toString());
 
     /* Test failure of create EZ on a directory which is already an EZ. */
     try {
-      dfsAdmin.createEncryptionZone(zone1, null);
+      dfsAdmin.createEncryptionZone(zone1, TEST_KEY);
     } catch (IOException e) {
       assertExceptionContains("already in an encryption zone", e);
     }
@@ -178,7 +183,7 @@ public void testBasicOperations() throws Exception {
     final Path zone1Child = new Path(zone1, "child");
     fsWrapper.mkdir(zone1Child, FsPermission.getDirDefault(), false);
     try {
-      dfsAdmin.createEncryptionZone(zone1Child, null);
+      dfsAdmin.createEncryptionZone(zone1Child, TEST_KEY);
       fail("EZ in an EZ");
     } catch (IOException e) {
       assertExceptionContains("already in an encryption zone", e);
@@ -189,7 +194,7 @@ public void testBasicOperations() throws Exception {
     final Path notEmptyChild = new Path(notEmpty, "child");
     fsWrapper.mkdir(notEmptyChild, FsPermission.getDirDefault(), true);
     try {
-      dfsAdmin.createEncryptionZone(notEmpty, null);
+      dfsAdmin.createEncryptionZone(notEmpty, TEST_KEY);
       fail("Created EZ on an non-empty directory with folder");
     } catch (IOException e) {
       assertExceptionContains("create an encryption zone", e);
@@ -199,7 +204,7 @@ public void testBasicOperations() throws Exception {
     /* create EZ on a folder with a file fails */
     fsWrapper.createFile(notEmptyChild);
     try {
-      dfsAdmin.createEncryptionZone(notEmpty, null);
+      dfsAdmin.createEncryptionZone(notEmpty, TEST_KEY);
       fail("Created EZ on an non-empty directory with file");
     } catch (IOException e) {
       assertExceptionContains("create an encryption zone", e);
@@ -215,6 +220,21 @@ public void testBasicOperations() throws Exception {
     } catch (IOException e) {
       assertExceptionContains("doesn't exist.", e);
     }
+
+    /* Test failure of empty and null key name */
+    try {
+      dfsAdmin.createEncryptionZone(zone2, "");
+      fail("created a zone with empty key name");
+    } catch (IOException e) {
+      assertExceptionContains("Must specify a key name when creating", e);
+    }
+    try {
+      dfsAdmin.createEncryptionZone(zone2, null);
+      fail("created a zone with null key name");
+    } catch (IOException e) {
+      assertExceptionContains("Must specify a key name when creating", e);
+    }
+
     assertNumZones(1);
 
     /* Test success of creating an EZ when they key exists. */
@@ -235,7 +255,7 @@ public Object run() throws Exception {
         final HdfsAdmin userAdmin =
             new HdfsAdmin(FileSystem.getDefaultUri(conf), conf);
         try {
-          userAdmin.createEncryptionZone(nonSuper, null);
+          userAdmin.createEncryptionZone(nonSuper, TEST_KEY);
           fail("createEncryptionZone is superuser-only operation");
         } catch (AccessControlException e) {
           assertExceptionContains("Superuser privilege is required", e);
@@ -247,7 +267,7 @@ public Object run() throws Exception {
     // Test success of creating an encryption zone a few levels down.
     Path deepZone = new Path("/d/e/e/p/zone");
     fsWrapper.mkdir(deepZone, FsPermission.getDirDefault(), true);
-    dfsAdmin.createEncryptionZone(deepZone, null);
+    dfsAdmin.createEncryptionZone(deepZone, TEST_KEY);
     assertNumZones(++numZones);
     assertZonePresent(null, deepZone.toString());
   }
@@ -266,10 +286,10 @@ public void testListEncryptionZonesAsNonSuperUser() throws Exception {
     final Path allPath = new Path(testRoot, "accessall");
 
     fsWrapper.mkdir(superPath, new FsPermission((short) 0700), true);
-    dfsAdmin.createEncryptionZone(superPath, null);
+    dfsAdmin.createEncryptionZone(superPath, TEST_KEY);
 
     fsWrapper.mkdir(allPath, new FsPermission((short) 0707), true);
-    dfsAdmin.createEncryptionZone(allPath, null);
+    dfsAdmin.createEncryptionZone(allPath, TEST_KEY);
 
     user.doAs(new PrivilegedExceptionAction<Object>() {
       @Override
@@ -294,7 +314,7 @@ private void doRenameEncryptionZone(FSTestWrapper wrapper) throws Exception {
     final Path pathFoo = new Path(testRoot, "foo");
     final Path pathFooBaz = new Path(pathFoo, "baz");
     wrapper.mkdir(pathFoo, FsPermission.getDirDefault(), true);
-    dfsAdmin.createEncryptionZone(pathFoo, null);
+    dfsAdmin.createEncryptionZone(pathFoo, TEST_KEY);
     wrapper.mkdir(pathFooBaz, FsPermission.getDirDefault(), true);
     try {
       wrapper.rename(pathFooBaz, testRoot);
@@ -331,7 +351,7 @@ public void testReadWrite() throws Exception {
     // Create the first enc file
     final Path zone = new Path("/zone");
     fs.mkdirs(zone);
-    dfsAdmin.createEncryptionZone(zone, null);
+    dfsAdmin.createEncryptionZone(zone, TEST_KEY);
     final Path encFile1 = new Path(zone, "myfile");
     DFSTestUtil.createFile(fs, encFile1, len, (short) 1, 0xFEED);
     // Read them back in and compare byte-by-byte
@@ -364,7 +384,7 @@ public void testCipherSuiteNegotiation() throws Exception {
         new HdfsAdmin(FileSystem.getDefaultUri(conf), conf);
     final Path zone = new Path("/zone");
     fs.mkdirs(zone);
-    dfsAdmin.createEncryptionZone(zone, null);
+    dfsAdmin.createEncryptionZone(zone, TEST_KEY);
     // Create a file in an EZ, which should succeed
     DFSTestUtil
         .createFile(fs, new Path(zone, "success1"), 0, (short) 1, 0xFEED);
@@ -434,7 +454,7 @@ public void testCreateEZWithNoProvider() throws Exception {
     /* Normal creation of an EZ */
     fsWrapper.mkdir(zone1, FsPermission.getDirDefault(), true);
     try {
-      dfsAdmin.createEncryptionZone(zone1, null);
+      dfsAdmin.createEncryptionZone(zone1, TEST_KEY);
       fail("expected exception");
     } catch (IOException e) {
       assertExceptionContains("since no key provider is available", e);

hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCryptoConf.xml

@@ -50,7 +50,7 @@
       <description>Test create ez, dir doesn't exist</description>
       <test-commands>
         <command>-fs NAMENODE -ls /test</command>-
-        <crypto-admin-command>-createZone -path /test</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /test -keyName myKey</crypto-admin-command>
       </test-commands>
       <cleanup-commands>
       </cleanup-commands>
@@ -67,8 +67,8 @@
       <test-commands>
         <command>-fs NAMENODE -mkdir /foo</command>
         <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /foo</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /foo -keyName myKey</crypto-admin-command>
-        <crypto-admin-command>-createZone -path /foo</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /foo -keyName myKey</crypto-admin-command>
       </test-commands>
       <cleanup-commands>
         <command>-fs NAMENODE -rmdir /foo</command>
@@ -81,32 +81,14 @@
       </comparators>
     </test>
 
-    <test>
-      <description>Test success of create ez in which a key is created</description>
-      <test-commands>
-        <command>-fs NAMENODE -mkdir /foo</command>
-        <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /foo</crypto-admin-command>
-      </test-commands>
-      <cleanup-commands>
-        <command>-fs NAMENODE -rmdir /foo</command>
-      </cleanup-commands>
-      <comparators>
-        <comparator>
-          <type>SubstringComparator</type>
-          <expected-output>Added encryption zone /foo</expected-output>
-        </comparator>
-      </comparators>
-    </test>
-
     <test>
       <description>Test failure of Create EZ operation in an existing EZ.</description>
       <test-commands>
         <command>-fs NAMENODE -mkdir /foo</command>
         <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /foo</crypto-admin-command>
+        <crypto-admin-command>-createZone -keyName myKey -path /foo</crypto-admin-command>
         <command>-fs NAMENODE -mkdir /foo/bar</command>
-        <crypto-admin-command>-createZone -path /foo/bar</crypto-admin-command>
+        <crypto-admin-command>-createZone -keyName myKey -path /foo/bar</crypto-admin-command>
       </test-commands>
       <cleanup-commands>
         <command>-fs NAMENODE -rmdir /foo/bar</command>
@@ -126,7 +108,7 @@
         <command>-fs NAMENODE -mkdir /foo</command>
         <command>-fs NAMENODE -touchz /foo/bar</command>
         <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /foo</crypto-admin-command>
+        <crypto-admin-command>-createZone -keyName myKey -path /foo</crypto-admin-command>
       </test-commands>
       <cleanup-commands>
         <command>-fs NAMENODE -rm /foo/bar</command>
@@ -159,19 +141,31 @@
     </test>
 
     <test>
-      <description>Test success of creating an EZ when the key exists.</description>
+      <description>Test failure of creating an EZ no path is specified.</description>
       <test-commands>
-        <command>-fs NAMENODE -mkdir /foo</command>
+        <crypto-admin-command>-createZone -keyName blahKey</crypto-admin-command>
-        <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /foo -keyName mykey</crypto-admin-command>
       </test-commands>
       <cleanup-commands>
-        <command>-fs NAMENODE -rmdir /foo</command>
       </cleanup-commands>
       <comparators>
         <comparator>
           <type>SubstringComparator</type>
-          <expected-output>Added encryption zone /foo</expected-output>
+          <expected-output>You must specify a path</expected-output>
+        </comparator>
+      </comparators>
+    </test>
+
+    <test>
+      <description>Test failure of creating an EZ no key is specified.</description>
+      <test-commands>
+        <crypto-admin-command>-createZone -path /foo</crypto-admin-command>
+      </test-commands>
+      <cleanup-commands>
+      </cleanup-commands>
+      <comparators>
+        <comparator>
+          <type>SubstringComparator</type>
+          <expected-output>You must specify a key name</expected-output>
         </comparator>
       </comparators>
     </test>
@@ -183,7 +177,7 @@
         <command>-fs NAMENODE -mkdir /foo/bar</command>
         <command>-fs NAMENODE -mkdir /foo/bar/baz</command>
         <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /foo/bar/baz</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /foo/bar/baz -keyName myKey</crypto-admin-command>
       </test-commands>
       <cleanup-commands>
         <command>-fs NAMENODE -rmdir /foo/bar/baz</command>
@@ -204,8 +198,8 @@
         <command>-fs NAMENODE -mkdir /src</command>
         <command>-fs NAMENODE -mkdir /dst</command>
         <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /src</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /src -keyName myKey</crypto-admin-command>
-        <crypto-admin-command>-createZone -path /dst</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /dst -keyName myKey</crypto-admin-command>
         <command>-fs NAMENODE -mkdir /src/subdir</command>
         <command>-fs NAMENODE -mv /src/subdir /dst</command>-
       </test-commands>
@@ -228,7 +222,7 @@
         <command>-fs NAMENODE -mkdir /src</command>
         <command>-fs NAMENODE -mkdir /dst</command>
         <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /dst</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /dst -keyName myKey</crypto-admin-command>
         <command>-fs NAMENODE -mv /src /dst</command>-
       </test-commands>
       <cleanup-commands>
@@ -249,7 +243,7 @@
         <command>-fs NAMENODE -mkdir /src</command>
         <command>-fs NAMENODE -mkdir /dst</command>
         <command>-fs NAMENODE -ls /</command>-
-        <crypto-admin-command>-createZone -path /src</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /src -keyName myKey</crypto-admin-command>
         <command>-fs NAMENODE -mv /src /dst</command>-
       </test-commands>
       <cleanup-commands>
@@ -268,7 +262,7 @@
       <description>Test success of renaming file intra-EZ</description>
       <test-commands>
         <command>-fs NAMENODE -mkdir /src</command>
-        <crypto-admin-command>-createZone -path /src</crypto-admin-command>
+        <crypto-admin-command>-createZone -path /src -keyName myKey</crypto-admin-command>
         <command>-fs NAMENODE -mkdir /src/subdir1</command>
         <command>-fs NAMENODE -mkdir /src/subdir2</command>
         <command>-fs NAMENODE -mv /src/subdir1 /src/subdir2</command>-