From 415223548d84cd17979a0cff05f87f1fc3beb7f2 Mon Sep 17 00:00:00 2001 From: Andrew Wang Date: Tue, 29 Jul 2014 23:39:38 +0000 Subject: [PATCH] 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/CHANGES-fs-encryption.txt | 3 + .../apache/hadoop/hdfs/client/HdfsAdmin.java | 22 ++--- .../hdfs/server/namenode/FSNamesystem.java | 93 +++++-------------- .../apache/hadoop/hdfs/tools/CryptoAdmin.java | 8 +- ...CryptoCLI.java => TestCryptoAdminCLI.java} | 2 +- .../hadoop/hdfs/TestEncryptionZones.java | 50 +++++++--- .../src/test/resources/testCryptoConf.xml | 66 ++++++------- 7 files changed, 106 insertions(+), 138 deletions(-) rename hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/{TestCryptoCLI.java => TestCryptoAdminCLI.java} (98%) diff --git a/hadoop-hdfs-project/hadoop-hdfs/CHANGES-fs-encryption.txt b/hadoop-hdfs-project/hadoop-hdfs/CHANGES-fs-encryption.txt index d6b4e5b1f1..2531a4e404 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/CHANGES-fs-encryption.txt +++ b/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 diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/client/HdfsAdmin.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/client/HdfsAdmin.java index be3ac51cfe..0a22d9dd3f 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/client/HdfsAdmin.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/client/HdfsAdmin.java @@ -231,22 +231,16 @@ public RemoteIterator listCachePools() throws IOException { } /** - * Create an encryption zone rooted at an empty existing directory. An - * encryption zone has an associated encryption key used when reading and - * writing files within the zone. An existing key can be specified, - * 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 + * Create an encryption zone rooted at an empty existing directory, using the + * specified encryption key. An encryption zone has an associated encryption + * key used when reading and writing files within the zone. * + * @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 { diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java index 7b29cedf9f..dde19b9bd2 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java +++ b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/server/namenode/FSNamesystem.java @@ -8457,24 +8457,19 @@ AclStatus getAclStatus(String src) throws IOException { readUnlock(); } } - + /** - * Create an encryption zone on directory src. If provided, - * 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. + * Create an encryption zone on directory src using the specified key. * + * @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(); - createNewKey(keyName, src); - createdKey = true; - } else { - KeyVersion keyVersion = provider.getCurrentKey(keyName); - if (keyVersion == null) { - /* - * It would be nice if we threw something more specific than - * IOException when the key is not found, but the KeyProvider API - * doesn't provide for that. If that API is ever changed to throw - * something more specific (e.g. UnknownKeyException) then we can - * update this to match it, or better yet, just rethrow the - * KeyProvider's exception. - */ - throw new IOException("Key " + keyName + " doesn't exist."); - } + throw new IOException("Must specify a key name when creating an " + + "encryption zone"); + } + KeyVersion keyVersion = provider.getCurrentKey(keyName); + if (keyVersion == null) { + /* + * It would be nice if we threw something more specific than + * IOException when the key is not found, but the KeyProvider API + * doesn't provide for that. If that API is ever changed to throw + * something more specific (e.g. UnknownKeyException) then we can + * update this to match it, or better yet, just rethrow the + * KeyProvider's exception. + */ + 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 listEncryptionZones() throws IOException { boolean success = false; checkSuperuserPrivilege(); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/CryptoAdmin.java b/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/CryptoAdmin.java index c0155fcb72..28aaef2e45 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/main/java/org/apache/hadoop/hdfs/tools/CryptoAdmin.java +++ b/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 ] -path " + "]\n"; + return "[" + getName() + " -keyName -path " + "]\n"; } @Override @@ -133,7 +133,7 @@ public String getLongUsage() { listing.addRow("", "The path of the encryption zone to create. " + "It must be an empty directory."); listing.addRow("", "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 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)); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestCryptoCLI.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestCryptoAdminCLI.java similarity index 98% rename from hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestCryptoCLI.java rename to hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestCryptoAdminCLI.java index 1b4468e545..1c83829102 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/cli/TestCryptoCLI.java +++ b/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; diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java b/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java index c722922860..e1fb878139 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/java/org/apache/hadoop/hdfs/TestEncryptionZones.java +++ b/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() { @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); diff --git a/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCryptoConf.xml b/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCryptoConf.xml index 2ff2f20ae4..ebbf773b13 100644 --- a/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCryptoConf.xml +++ b/hadoop-hdfs-project/hadoop-hdfs/src/test/resources/testCryptoConf.xml @@ -50,7 +50,7 @@ Test create ez, dir doesn't exist -fs NAMENODE -ls /test- - -createZone -path /test + -createZone -path /test -keyName myKey @@ -67,8 +67,8 @@ -fs NAMENODE -mkdir /foo -fs NAMENODE -ls /- - -createZone -path /foo - -createZone -path /foo + -createZone -path /foo -keyName myKey + -createZone -path /foo -keyName myKey -fs NAMENODE -rmdir /foo @@ -81,32 +81,14 @@ - - Test success of create ez in which a key is created - - -fs NAMENODE -mkdir /foo - -fs NAMENODE -ls /- - -createZone -path /foo - - - -fs NAMENODE -rmdir /foo - - - - SubstringComparator - Added encryption zone /foo - - - - Test failure of Create EZ operation in an existing EZ. -fs NAMENODE -mkdir /foo -fs NAMENODE -ls /- - -createZone -path /foo + -createZone -keyName myKey -path /foo -fs NAMENODE -mkdir /foo/bar - -createZone -path /foo/bar + -createZone -keyName myKey -path /foo/bar -fs NAMENODE -rmdir /foo/bar @@ -126,7 +108,7 @@ -fs NAMENODE -mkdir /foo -fs NAMENODE -touchz /foo/bar -fs NAMENODE -ls /- - -createZone -path /foo + -createZone -keyName myKey -path /foo -fs NAMENODE -rm /foo/bar @@ -159,19 +141,31 @@ - Test success of creating an EZ when the key exists. + Test failure of creating an EZ no path is specified. - -fs NAMENODE -mkdir /foo - -fs NAMENODE -ls /- - -createZone -path /foo -keyName mykey + -createZone -keyName blahKey - -fs NAMENODE -rmdir /foo SubstringComparator - Added encryption zone /foo + You must specify a path + + + + + + Test failure of creating an EZ no key is specified. + + -createZone -path /foo + + + + + + SubstringComparator + You must specify a key name @@ -183,7 +177,7 @@ -fs NAMENODE -mkdir /foo/bar -fs NAMENODE -mkdir /foo/bar/baz -fs NAMENODE -ls /- - -createZone -path /foo/bar/baz + -createZone -path /foo/bar/baz -keyName myKey -fs NAMENODE -rmdir /foo/bar/baz @@ -204,8 +198,8 @@ -fs NAMENODE -mkdir /src -fs NAMENODE -mkdir /dst -fs NAMENODE -ls /- - -createZone -path /src - -createZone -path /dst + -createZone -path /src -keyName myKey + -createZone -path /dst -keyName myKey -fs NAMENODE -mkdir /src/subdir -fs NAMENODE -mv /src/subdir /dst- @@ -228,7 +222,7 @@ -fs NAMENODE -mkdir /src -fs NAMENODE -mkdir /dst -fs NAMENODE -ls /- - -createZone -path /dst + -createZone -path /dst -keyName myKey -fs NAMENODE -mv /src /dst- @@ -249,7 +243,7 @@ -fs NAMENODE -mkdir /src -fs NAMENODE -mkdir /dst -fs NAMENODE -ls /- - -createZone -path /src + -createZone -path /src -keyName myKey -fs NAMENODE -mv /src /dst- @@ -268,7 +262,7 @@ Test success of renaming file intra-EZ -fs NAMENODE -mkdir /src - -createZone -path /src + -createZone -path /src -keyName myKey -fs NAMENODE -mkdir /src/subdir1 -fs NAMENODE -mkdir /src/subdir2 -fs NAMENODE -mv /src/subdir1 /src/subdir2-