From c54a4bb666cdeef41a71ed8eeb5ddbe7c5ccc337 Mon Sep 17 00:00:00 2001 From: Alejandro Abdelnur Date: Tue, 13 May 2014 18:29:40 +0000 Subject: [PATCH] HADOOP-10583. bin/hadoop key throws NPE with no args and assorted other fixups. (clamb via tucu) git-svn-id: https://svn.apache.org/repos/asf/hadoop/common/trunk@1594320 13f79535-47bb-0310-9956-ffa450edef68 --- .../hadoop-common/CHANGES.txt | 2 + .../apache/hadoop/crypto/key/KeyProvider.java | 71 ++++---- .../apache/hadoop/crypto/key/KeyShell.java | 168 +++++++++--------- .../crypto/key/kms/KMSClientProvider.java | 9 +- .../hadoop/crypto/key/TestKeyShell.java | 4 +- 5 files changed, 137 insertions(+), 117 deletions(-) diff --git a/hadoop-common-project/hadoop-common/CHANGES.txt b/hadoop-common-project/hadoop-common/CHANGES.txt index 0c5f7fb6ea..36fe52b7b5 100644 --- a/hadoop-common-project/hadoop-common/CHANGES.txt +++ b/hadoop-common-project/hadoop-common/CHANGES.txt @@ -326,6 +326,8 @@ Trunk (Unreleased) HADOOP-10431. Change visibility of KeyStore.Options getter methods to public. (tucu) + HADOOP-10583. bin/hadoop key throws NPE with no args and assorted other fixups. (clamb via tucu) + OPTIMIZATIONS HADOOP-7761. Improve the performance of raw comparisons. (todd) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyProvider.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyProvider.java index 17ce33cfdb..0b031c0493 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyProvider.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyProvider.java @@ -27,9 +27,7 @@ import java.security.NoSuchAlgorithmException; import java.text.MessageFormat; import java.util.Date; -import java.util.LinkedHashMap; import java.util.List; -import java.util.Map; import com.google.gson.stream.JsonReader; import com.google.gson.stream.JsonWriter; @@ -176,22 +174,26 @@ protected int addVersion() { protected byte[] serialize() throws IOException { ByteArrayOutputStream buffer = new ByteArrayOutputStream(); JsonWriter writer = new JsonWriter(new OutputStreamWriter(buffer)); - writer.beginObject(); - if (cipher != null) { - writer.name(CIPHER_FIELD).value(cipher); + try { + writer.beginObject(); + if (cipher != null) { + writer.name(CIPHER_FIELD).value(cipher); + } + if (bitLength != 0) { + writer.name(BIT_LENGTH_FIELD).value(bitLength); + } + if (created != null) { + writer.name(CREATED_FIELD).value(created.getTime()); + } + if (description != null) { + writer.name(DESCRIPTION_FIELD).value(description); + } + writer.name(VERSIONS_FIELD).value(versions); + writer.endObject(); + writer.flush(); + } finally { + writer.close(); } - if (bitLength != 0) { - writer.name(BIT_LENGTH_FIELD).value(bitLength); - } - if (created != null) { - writer.name(CREATED_FIELD).value(created.getTime()); - } - if (description != null) { - writer.name(DESCRIPTION_FIELD).value(description); - } - writer.name(VERSIONS_FIELD).value(versions); - writer.endObject(); - writer.flush(); return buffer.toByteArray(); } @@ -207,23 +209,27 @@ protected Metadata(byte[] bytes) throws IOException { int versions = 0; String description = null; JsonReader reader = new JsonReader(new InputStreamReader - (new ByteArrayInputStream(bytes))); - reader.beginObject(); - while (reader.hasNext()) { - String field = reader.nextName(); - if (CIPHER_FIELD.equals(field)) { - cipher = reader.nextString(); - } else if (BIT_LENGTH_FIELD.equals(field)) { - bitLength = reader.nextInt(); - } else if (CREATED_FIELD.equals(field)) { - created = new Date(reader.nextLong()); - } else if (VERSIONS_FIELD.equals(field)) { - versions = reader.nextInt(); - } else if (DESCRIPTION_FIELD.equals(field)) { - description = reader.nextString(); + (new ByteArrayInputStream(bytes))); + try { + reader.beginObject(); + while (reader.hasNext()) { + String field = reader.nextName(); + if (CIPHER_FIELD.equals(field)) { + cipher = reader.nextString(); + } else if (BIT_LENGTH_FIELD.equals(field)) { + bitLength = reader.nextInt(); + } else if (CREATED_FIELD.equals(field)) { + created = new Date(reader.nextLong()); + } else if (VERSIONS_FIELD.equals(field)) { + versions = reader.nextInt(); + } else if (DESCRIPTION_FIELD.equals(field)) { + description = reader.nextString(); + } } + reader.endObject(); + } finally { + reader.close(); } - reader.endObject(); this.cipher = cipher; this.bitLength = bitLength; this.created = created; @@ -310,7 +316,6 @@ public abstract KeyVersion getKeyVersion(String versionName */ public abstract List getKeys() throws IOException; - /** * Get key metadata in bulk. * @param names the names of the keys to get diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyShell.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyShell.java index 908179d034..3d56640e11 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyShell.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/KeyShell.java @@ -23,9 +23,6 @@ import java.security.InvalidParameterException; import java.security.NoSuchAlgorithmException; import java.util.List; -import java.util.Map; - -import javax.crypto.KeyGenerator; import org.apache.hadoop.conf.Configuration; import org.apache.hadoop.conf.Configured; @@ -93,41 +90,54 @@ public int run(String[] args) throws Exception { */ private int init(String[] args) throws IOException { for (int i = 0; i < args.length; i++) { // parse command line + boolean moreTokens = (i < args.length - 1); if (args[i].equals("create")) { - String keyName = args[++i]; + String keyName = "--help"; + if (moreTokens) { + keyName = args[++i]; + } + command = new CreateCommand(keyName); - if (keyName.equals("--help")) { + if ("--help".equals(keyName)) { printKeyShellUsage(); return -1; } } else if (args[i].equals("delete")) { - String keyName = args[++i]; + String keyName = "--help"; + if (moreTokens) { + keyName = args[++i]; + } + command = new DeleteCommand(keyName); - if (keyName.equals("--help")) { + if ("--help".equals(keyName)) { printKeyShellUsage(); return -1; } } else if (args[i].equals("roll")) { - String keyName = args[++i]; + String keyName = "--help"; + if (moreTokens) { + keyName = args[++i]; + } + command = new RollCommand(keyName); - if (keyName.equals("--help")) { + if ("--help".equals(keyName)) { printKeyShellUsage(); return -1; } - } else if (args[i].equals("list")) { + } else if ("list".equals(args[i])) { command = new ListCommand(); - } else if (args[i].equals("--size")) { + } else if ("--size".equals(args[i]) && moreTokens) { getConf().set(KeyProvider.DEFAULT_BITLENGTH_NAME, args[++i]); - } else if (args[i].equals("--cipher")) { + } else if ("--cipher".equals(args[i]) && moreTokens) { getConf().set(KeyProvider.DEFAULT_CIPHER_NAME, args[++i]); - } else if (args[i].equals("--provider")) { + } else if ("--provider".equals(args[i]) && moreTokens) { userSuppliedProvider = true; getConf().set(KeyProviderFactory.KEY_PROVIDER_PATH, args[++i]); - } else if (args[i].equals("--metadata")) { + } else if ("--metadata".equals(args[i])) { getConf().setBoolean(LIST_METADATA, true); - } else if (args[i].equals("-i") || (args[i].equals("--interactive"))) { + } else if ("-i".equals(args[i]) || ("--interactive".equals(args[i]))) { interactive = true; - } else if (args[i].equals("--help")) { + } else if ("--help".equals(args[i])) { printKeyShellUsage(); return -1; } else { @@ -136,6 +146,12 @@ private int init(String[] args) throws IOException { return -1; } } + + if (command == null) { + printKeyShellUsage(); + return -1; + } + return 0; } @@ -143,8 +159,7 @@ private void printKeyShellUsage() { out.println(USAGE_PREFIX + COMMANDS); if (command != null) { out.println(command.getUsage()); - } - else { + } else { out.println("=========================================================" + "======"); out.println(CreateCommand.USAGE + ":\n\n" + CreateCommand.DESC); @@ -174,8 +189,7 @@ protected KeyProvider getKeyProvider() { providers = KeyProviderFactory.getProviders(getConf()); if (userSuppliedProvider) { provider = providers.get(0); - } - else { + } else { for (KeyProvider p : providers) { if (!p.isTransient()) { provider = p; @@ -190,7 +204,7 @@ protected KeyProvider getKeyProvider() { } protected void printProviderWritten() { - out.println(provider.getClass().getName() + " has been updated."); + out.println(provider + " has been updated."); } protected void warnIfTransientProvider() { @@ -206,12 +220,12 @@ protected void warnIfTransientProvider() { private class ListCommand extends Command { public static final String USAGE = - "list [--provider] [--metadata] [--help]"; + "list [--provider ] [--metadata] [--help]"; public static final String DESC = - "The list subcommand displays the keynames contained within \n" + - "a particular provider - as configured in core-site.xml or " + - "indicated\nthrough the --provider argument.\n" + - "If the --metadata option is used, the keys metadata will be printed"; + "The list subcommand displays the keynames contained within\n" + + "a particular provider as configured in core-site.xml or\n" + + "specified with the --provider argument. --metadata displays\n" + + "the metadata."; private boolean metadata = false; @@ -220,9 +234,9 @@ public boolean validate() { provider = getKeyProvider(); if (provider == null) { out.println("There are no non-transient KeyProviders configured.\n" - + "Consider using the --provider option to indicate the provider\n" - + "to use. If you want to list a transient provider then you\n" - + "you MUST use the --provider argument."); + + "Use the --provider option to specify a provider. If you\n" + + "want to list a transient provider then you must use the\n" + + "--provider argument."); rc = false; } metadata = getConf().getBoolean(LIST_METADATA, false); @@ -231,12 +245,12 @@ public boolean validate() { public void execute() throws IOException { try { - List keys = provider.getKeys(); - out.println("Listing keys for KeyProvider: " + provider.toString()); + final List keys = provider.getKeys(); + out.println("Listing keys for KeyProvider: " + provider); if (metadata) { - Metadata[] meta = + final Metadata[] meta = provider.getKeysMetadata(keys.toArray(new String[keys.size()])); - for(int i=0; i < meta.length; ++i) { + for (int i = 0; i < meta.length; ++i) { out.println(keys.get(i) + " : " + meta[i]); } } else { @@ -245,7 +259,7 @@ public void execute() throws IOException { } } } catch (IOException e) { - out.println("Cannot list keys for KeyProvider: " + provider.toString() + out.println("Cannot list keys for KeyProvider: " + provider + ": " + e.getMessage()); throw e; } @@ -258,11 +272,10 @@ public String getUsage() { } private class RollCommand extends Command { - public static final String USAGE = "roll [--provider] [--help]"; + public static final String USAGE = "roll [--provider ] [--help]"; public static final String DESC = - "The roll subcommand creates a new version of the key specified\n" + - "through the argument within the provider indicated using\n" + - "the --provider argument"; + "The roll subcommand creates a new version for the specified key\n" + + "within the provider indicated using the --provider argument\n"; String keyName = null; @@ -274,15 +287,14 @@ public boolean validate() { boolean rc = true; provider = getKeyProvider(); if (provider == null) { - out.println("There are no valid KeyProviders configured.\n" - + "Key will not be rolled.\n" - + "Consider using the --provider option to indicate the provider" - + " to use."); + out.println("There are no valid KeyProviders configured. The key\n" + + "has not been rolled. Use the --provider option to specify\n" + + "a provider."); rc = false; } if (keyName == null) { - out.println("There is no keyName specified. Please provide the" + - "mandatory . See the usage description with --help."); + out.println("Please provide a .\n" + + "See the usage description by using --help."); rc = false; } return rc; @@ -290,10 +302,9 @@ public boolean validate() { public void execute() throws NoSuchAlgorithmException, IOException { try { - Metadata md = provider.getMetadata(keyName); warnIfTransientProvider(); out.println("Rolling key version from KeyProvider: " - + provider.toString() + " for key name: " + keyName); + + provider + "\n for key name: " + keyName); try { provider.rollNewVersion(keyName); out.println(keyName + " has been successfully rolled."); @@ -301,12 +312,12 @@ public void execute() throws NoSuchAlgorithmException, IOException { printProviderWritten(); } catch (NoSuchAlgorithmException e) { out.println("Cannot roll key: " + keyName + " within KeyProvider: " - + provider.toString()); + + provider); throw e; } } catch (IOException e1) { out.println("Cannot roll key: " + keyName + " within KeyProvider: " - + provider.toString()); + + provider); throw e1; } } @@ -318,11 +329,11 @@ public String getUsage() { } private class DeleteCommand extends Command { - public static final String USAGE = "delete [--provider] [--help]"; + public static final String USAGE = "delete [--provider ] [--help]"; public static final String DESC = - "The delete subcommand deletes all of the versions of the key\n" + - "specified as the argument from within the provider\n" + - "indicated through the --provider argument"; + "The delete subcommand deletes all versions of the key\n" + + "specified by the argument from within the\n" + + "provider specified --provider."; String keyName = null; boolean cont = true; @@ -335,23 +346,21 @@ public DeleteCommand(String keyName) { public boolean validate() { provider = getKeyProvider(); if (provider == null) { - out.println("There are no valid KeyProviders configured.\n" - + "Nothing will be deleted.\n" - + "Consider using the --provider option to indicate the provider" - + " to use."); + out.println("There are no valid KeyProviders configured. Nothing\n" + + "was deleted. Use the --provider option to specify a provider."); return false; } if (keyName == null) { - out.println("There is no keyName specified. Please provide the" + - "mandatory . See the usage description with --help."); + out.println("There is no keyName specified. Please specify a " + + ". See the usage description with --help."); return false; } if (interactive) { try { cont = ToolRunner .confirmPrompt("You are about to DELETE all versions of " - + "the key: " + keyName + " from KeyProvider " - + provider.toString() + ". Continue?:"); + + " key: " + keyName + " from KeyProvider " + + provider + ". Continue?:"); if (!cont) { out.println("Nothing has been be deleted."); } @@ -367,7 +376,7 @@ public boolean validate() { public void execute() throws IOException { warnIfTransientProvider(); out.println("Deleting key: " + keyName + " from KeyProvider: " - + provider.toString()); + + provider); if (cont) { try { provider.deleteKey(keyName); @@ -375,7 +384,7 @@ public void execute() throws IOException { provider.flush(); printProviderWritten(); } catch (IOException e) { - out.println(keyName + "has NOT been deleted."); + out.println(keyName + " has not been deleted."); throw e; } } @@ -388,16 +397,16 @@ public String getUsage() { } private class CreateCommand extends Command { - public static final String USAGE = "create [--cipher] " + - "[--size] [--provider] [--help]"; + public static final String USAGE = + "create [--cipher ] [--size ]\n" + + " [--provider ] [--help]"; public static final String DESC = - "The create subcommand creates a new key for the name specified\n" + - "as the argument within the provider indicated through\n" + - "the --provider argument. You may also indicate the specific\n" + - "cipher through the --cipher argument. The default for cipher is\n" + - "currently \"AES/CTR/NoPadding\". The default keysize is \"256\".\n" + - "You may also indicate the requested key length through the --size\n" + - "argument."; + "The create subcommand creates a new key for the name specified\n" + + "by the argument within the provider specified by the\n" + + "--provider argument. You may specify a cipher with the --cipher\n" + + "argument. The default cipher is currently \"AES/CTR/NoPadding\".\n" + + "The default keysize is 256. You may specify the requested key\n" + + "length using the --size argument.\n"; String keyName = null; @@ -409,15 +418,14 @@ public boolean validate() { boolean rc = true; provider = getKeyProvider(); if (provider == null) { - out.println("There are no valid KeyProviders configured.\nKey" + - " will not be created.\n" - + "Consider using the --provider option to indicate the provider" + - " to use."); + out.println("There are no valid KeyProviders configured. No key\n" + + " was created. You can use the --provider option to specify\n" + + " a provider to use."); rc = false; } if (keyName == null) { - out.println("There is no keyName specified. Please provide the" + - "mandatory . See the usage description with --help."); + out.println("Please provide a . See the usage description" + + " with --help."); rc = false; } return rc; @@ -432,13 +440,13 @@ public void execute() throws IOException, NoSuchAlgorithmException { provider.flush(); printProviderWritten(); } catch (InvalidParameterException e) { - out.println(keyName + " has NOT been created. " + e.getMessage()); + out.println(keyName + " has not been created. " + e.getMessage()); throw e; } catch (IOException e) { - out.println(keyName + " has NOT been created. " + e.getMessage()); + out.println(keyName + " has not been created. " + e.getMessage()); throw e; } catch (NoSuchAlgorithmException e) { - out.println(keyName + " has NOT been created. " + e.getMessage()); + out.println(keyName + " has not been created. " + e.getMessage()); throw e; } } diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java index 1bbbf9d876..ff30f86de3 100644 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/crypto/key/kms/KMSClientProvider.java @@ -126,7 +126,6 @@ public static T checkNotNull(T o, String name) return o; } - public static String checkNotEmpty(String s, String name) throws IllegalArgumentException { checkNotNull(s, name); @@ -140,6 +139,13 @@ public static String checkNotEmpty(String s, String name) private String kmsUrl; private SSLFactory sslFactory; + @Override + public String toString() { + final StringBuilder sb = new StringBuilder("KMSClientProvider["); + sb.append(kmsUrl).append("]"); + return sb.toString(); + } + public KMSClientProvider(URI uri, Configuration conf) throws IOException { Path path = unnestUri(uri); URL url = path.toUri().toURL(); @@ -515,5 +521,4 @@ public void flush() throws IOException { public static String buildVersionName(String name, int version) { return KeyProvider.buildVersionName(name, version); } - } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyShell.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyShell.java index 4783fff58b..ae6938eb68 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyShell.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/crypto/key/TestKeyShell.java @@ -121,7 +121,7 @@ public void testInvalidKeySize() throws Exception { ks.setConf(new Configuration()); rc = ks.run(args1); assertEquals(-1, rc); - assertTrue(outContent.toString().contains("key1 has NOT been created.")); + assertTrue(outContent.toString().contains("key1 has not been created.")); } @Test @@ -134,7 +134,7 @@ public void testInvalidCipher() throws Exception { ks.setConf(new Configuration()); rc = ks.run(args1); assertEquals(-1, rc); - assertTrue(outContent.toString().contains("key1 has NOT been created.")); + assertTrue(outContent.toString().contains("key1 has not been created.")); } @Test