Undo reflective modification of SunPKCS11-NSS algorithms mapping (#46)

tjcelaya · web-flow · commit 3c3093d86b53 · 2017-12-16T13:01:57.000-08:00
* Resolves #45 by undoing the reflective modification of SunPKCS11-NSS algorithms mapping and providing a KeyPairLoader#getKeyPair which accepts a provider name. * Checkstyle * Explicit check for requested provider potentially not being available and the relevant test case * Add a BC-specific test * Clean up some more
diff --git a/.gitignore b/.gitignore
@@ -81,6 +81,8 @@ local.properties
 # TeXlipse plugin
 .texlipse
 
+# OS X
+.DS_Store
 
 ### Maven ###
 target/
diff --git a/common/src/main/java/com/joyent/http/signature/KeyLoadException.java b/common/src/main/java/com/joyent/http/signature/KeyLoadException.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright (c) 2017, Joyent, Inc. All rights reserved.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/.
+ */
+package com.joyent.http.signature;
+
+/**
+ * Exception that can occur while loading a {@link java.security.KeyPair}.
+ *
+ * @since 4.0.4
+ * @author <a href="https://github.com/tjcelaya">Tomas Celayac</a>
+ */
+public class KeyLoadException extends HttpSignatureException {
+
+    private static final long serialVersionUID = 3842266217250311085L;
+
+    /**
+     * Creates a new exception with the specified message.
+     * @param message Message to embed
+     */
+    public KeyLoadException(final String message) {
+        super(message);
+    }
+}
diff --git a/common/src/main/java/com/joyent/http/signature/KeyPairLoader.java b/common/src/main/java/com/joyent/http/signature/KeyPairLoader.java
@@ -7,12 +7,12 @@
  */
 package com.joyent.http.signature;
 
-import com.joyent.http.signature.crypto.NssBridgeKeyConverter;
 import org.bouncycastle.jce.provider.BouncyCastleProvider;
 import org.bouncycastle.openssl.PEMDecryptorProvider;
 import org.bouncycastle.openssl.PEMEncryptedKeyPair;
 import org.bouncycastle.openssl.PEMKeyPair;
 import org.bouncycastle.openssl.PEMParser;
+import org.bouncycastle.openssl.jcajce.JcaPEMKeyConverter;
 import org.bouncycastle.openssl.jcajce.JcePEMDecryptorProviderBuilder;
 
 import java.io.BufferedReader;
@@ -26,6 +26,7 @@
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.security.KeyPair;
+import java.security.Provider;
 import java.security.Security;
 
 
@@ -35,18 +36,66 @@
  */
 public final class KeyPairLoader {
 
+    /**
+     * Provider name for libnss.
+     */
+    public static final String PROVIDER_PKCS11_NSS = "SunPKCS11-NSS";
 
-    @SuppressWarnings("checkstyle:javadocmethod")
-    private KeyPairLoader() {
-    }
+    /**
+     * Provider name for Bouncy Castle.
+     */
+    public static final String PROVIDER_BOUNCY_CASTLE = "BC";
+
+    /**
+     * The key format converter to use when reading key pairs and libnss is enabled (or specifically requested).
+     */
+    private static final JcaPEMKeyConverter CONVERTER_PKCS11_NSS;
+
+    /**
+     * The key format converter to use when libnss is disabled (or BC is specifically requested).
+     */
+    private static final JcaPEMKeyConverter CONVERTER_BOUNCY_CASTLE;
 
     /**
-     * The key format converter to use when reading key pairs.
+     * Set of security providers users can request.
      */
-    private static final NssBridgeKeyConverter CONVERTER =
-        new NssBridgeKeyConverter();
-    {
-        CONVERTER.setProvider("BC");
+    @SuppressWarnings({"checkstyle:JavaDocVariable", "checkstyle:JavadocMethod"})
+    public enum DesiredSecurityProvider {
+        BC(PROVIDER_BOUNCY_CASTLE),
+        NSS(PROVIDER_PKCS11_NSS);
+
+        private final String providerCode;
+
+         DesiredSecurityProvider(final String providerCode) {
+            this.providerCode = providerCode;
+        }
+
+        @Override
+        public String toString() {
+             return providerCode;
+        }
+    }
+
+    static {
+        final Provider providerPkcs11NSS = Security.getProvider(PROVIDER_PKCS11_NSS);
+
+        if (providerPkcs11NSS != null) {
+            CONVERTER_PKCS11_NSS = new JcaPEMKeyConverter().setProvider(PROVIDER_PKCS11_NSS);
+        } else {
+            CONVERTER_PKCS11_NSS = null;
+        }
+
+        final Provider providerBouncyCastle = Security.getProvider(PROVIDER_BOUNCY_CASTLE);
+
+        if (providerBouncyCastle == null) {
+            Security.addProvider(new BouncyCastleProvider());
+        }
+
+        CONVERTER_BOUNCY_CASTLE = new JcaPEMKeyConverter().setProvider(PROVIDER_BOUNCY_CASTLE);
+    }
+
+    @SuppressWarnings("checkstyle:javadocmethod")
+    private KeyPairLoader() {
     }
 
     /**
@@ -150,36 +199,71 @@ public static KeyPair getKeyPair(final byte[] pKeyBytes, final char[] password)
     /**
      * Read KeyPair from an input stream, optionally using password.
      *
-     * @param is private key content as a stream
+     * @param is       private key content as a stream
+     * @param password password associated with key
+     * @return public/private keypair object
+     * @throws IOException If unable to read the private key from the string
+     */
+    public static KeyPair getKeyPair(final InputStream is,
+                                     final char[] password) throws IOException {
+        return getKeyPair(is, password, null);
+    }
+
+    /**
+     * Read KeyPair from an input stream, optionally using password and desired Security Provider. Most implementations
+     * should continue calling the one and two-argument methods
+     *
+     * @param is       private key content as a stream
      * @param password password associated with key
+     * @param provider security provider to use when loading the key
      * @return public/private keypair object
      * @throws IOException If unable to read the private key from the string
      */
     public static KeyPair getKeyPair(final InputStream is,
-                              final char[] password) throws IOException {
+                                     final char[] password,
+                                     final DesiredSecurityProvider provider) throws IOException {
+        final Object pemObject;
         try (InputStreamReader isr = new InputStreamReader(is, StandardCharsets.US_ASCII);
              BufferedReader br = new BufferedReader(isr);
              PEMParser pemParser = new PEMParser(br)) {
 
+            pemObject = pemParser.readObject();
+        }
+
+        final PEMKeyPair pemKeyPair;
+
+        if (pemObject instanceof PEMEncryptedKeyPair) {
             if (password == null) {
-                Security.addProvider(new BouncyCastleProvider());
-                final Object object = pemParser.readObject();
-                return CONVERTER.getKeyPair((PEMKeyPair) object);
-            } else {
-                PEMDecryptorProvider decProv = new JcePEMDecryptorProviderBuilder().build(password);
-
-                Object object = pemParser.readObject();
-
-                final KeyPair kp;
-                if (object instanceof PEMEncryptedKeyPair) {
-                    kp = CONVERTER.getKeyPair(((PEMEncryptedKeyPair) object).decryptKeyPair(decProv));
-                } else {
-                    kp = CONVERTER.getKeyPair((PEMKeyPair) object);
-                }
-
-                return kp;
+                throw new KeyLoadException("Loaded key is encrypted but no password was supplied.");
             }
+
+            final PEMDecryptorProvider decryptorProvider = new JcePEMDecryptorProviderBuilder().build(password);
+            final PEMEncryptedKeyPair encryptedPemObject = ((PEMEncryptedKeyPair) pemObject);
+            pemKeyPair = encryptedPemObject.decryptKeyPair(decryptorProvider);
+        } else if (pemObject instanceof PEMKeyPair) {
+            if (password != null) {
+                throw new KeyLoadException("Loaded key is not encrypted but a password was supplied.");
+            }
+
+            pemKeyPair = (PEMKeyPair) pemObject;
+        } else {
+            throw new KeyLoadException("Unexpected PEM object loaded: " + pemObject.getClass().getCanonicalName());
         }
+
+        // throw if the user has specifically requested NSS and it is unavailable
+        if (provider != null && provider.equals(DesiredSecurityProvider.NSS) && CONVERTER_PKCS11_NSS == null) {
+            throw new KeyLoadException(PROVIDER_PKCS11_NSS + " provider requested but unavailable. "
+                    + "Is java.security configured correctly?");
+        }
+
+        // Attempt to load with NSS if it is available and requested (or no provider was specified)
+        final boolean attemptPKCS11NSS = provider == null || provider.equals(DesiredSecurityProvider.NSS);
+
+        if (CONVERTER_PKCS11_NSS != null && attemptPKCS11NSS) {
+            return CONVERTER_PKCS11_NSS.getKeyPair(pemKeyPair);
+        }
+
+        return CONVERTER_BOUNCY_CASTLE.getKeyPair(pemKeyPair);
     }
 
 }
diff --git a/common/src/main/java/com/joyent/http/signature/crypto/NssBridgeKeyConverter.java b/common/src/main/java/com/joyent/http/signature/crypto/NssBridgeKeyConverter.java
@@ -7,14 +7,8 @@
  */
 package com.joyent.http.signature.crypto;
 
-import org.bouncycastle.asn1.x9.X9ObjectIdentifiers;
 import org.bouncycastle.openssl.jcajce.JcaPEMKeyConverter;
 
-import java.lang.reflect.Field;
-import java.lang.reflect.Method;
-import java.security.Provider;
-import java.security.Security;
-
 /**
  * There is an unfortunate naming discrepancy between BouncyCastle and
  * other security providers over the name of ECDSA.  BouncyCastle uses
@@ -31,35 +25,6 @@
  * @see <a href="http://docs.oracle.com/javase/8/docs/technotes/guides/security/p11guide.html">PKCS#11 Reference Guide</a>
  * @see <a href="https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS">Network Security Services</a>
  */
+@Deprecated
 public class NssBridgeKeyConverter extends JcaPEMKeyConverter {
-
-    {
-        Provider[] providers = Security.getProviders();
-        try {
-            if (providers == null || providers.length == 0) {
-                /* A lack of any security providers should be
-                 * an "impossible" condition.  But if it occurs we print
-                 * to stderr instead of throwing in a static
-                 * initializer.
-                 */
-                System.err.println("Unable to configure ECDSA, no security providers present");
-            } else if (providers[0].getName().equals("SunPKCS11-NSS")) {
-                /* JcaPEMKeyConverter maintains an internal mapping of
-                 * algorithms identifies to string codes. If SunPKCS11-NSS
-                 * is the most preferred provider, reflection is used to
-                 * adjust that mapping to match the expectations of
-                 * SunPKCS11.
-                 */
-                Field fieldDefinition = JcaPEMKeyConverter.class.getDeclaredField("algorithms");
-                fieldDefinition.setAccessible(true);
-                Object fieldValue = fieldDefinition.get(null);
-                Method put = fieldValue.getClass().getDeclaredMethod("put", Object.class, Object.class);
-                put.invoke(fieldValue, X9ObjectIdentifiers.id_ecPublicKey, "EC");
-            }
-        } catch (ReflectiveOperationException e) {
-            System.err.println("SunPKCS11-NSS is preferred security provider, "
-                               + "but failed to enable for ECDSA via reflection");
-            e.printStackTrace();
-        }
-    }
 }
diff --git a/common/src/test/java/com/joyent/http/signature/KeyPairLoaderTest.java b/common/src/test/java/com/joyent/http/signature/KeyPairLoaderTest.java
@@ -1,30 +1,40 @@
 package com.joyent.http.signature;
 
+import com.joyent.http.signature.KeyPairLoader.DesiredSecurityProvider;
 import org.bouncycastle.openssl.PEMEncryptor;
 import org.bouncycastle.openssl.jcajce.JcaMiscPEMGenerator;
 import org.bouncycastle.openssl.jcajce.JcaPEMWriter;
 import org.bouncycastle.openssl.jcajce.JcePEMEncryptorBuilder;
 import org.testng.Assert;
 import org.testng.AssertJUnit;
+import org.testng.SkipException;
 import org.testng.annotations.Test;
 
+import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
 import java.io.OutputStreamWriter;
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.security.KeyPair;
 import java.security.KeyPairGenerator;
 import java.security.NoSuchAlgorithmException;
+import java.security.Security;
 import java.util.UUID;
 
+import static com.joyent.http.signature.KeyPairLoader.PROVIDER_PKCS11_NSS;
+
 @Test
 public class KeyPairLoaderTest {
 
     private static final String RSA_HEADER = "-----BEGIN RSA PRIVATE KEY-----";
 
+    private static final ClassLoader CLASS_LOADER = KeyPairLoaderTest.class.getClassLoader();
+
     public void willThrowOnNullInputs() {
         Assert.assertThrows(() ->
                 KeyPairLoader.getKeyPair((String) null, null));
@@ -86,6 +96,50 @@ public void canLoadPasswordProtectedKeyFromFile() throws Exception {
         compareKeyContents(keyPair, loadedKeyPair);
     }
 
+    public void canLoadKeyPairUsingSpecifiedProvider() throws Exception {
+
+        if (Security.getProvider(PROVIDER_PKCS11_NSS) == null) {
+            throw new SkipException(PROVIDER_PKCS11_NSS + " provider is missing.");
+        }
+
+        for (final String keyId : SignerTestUtil.keys.keySet()) {
+            final SignerTestUtil.TestKeyResource keyResource = SignerTestUtil.keys.get(keyId);
+            final KeyPair bouncyKeyPair = loadTestKeyPair(keyResource.resourcePath, DesiredSecurityProvider.BC);
+            final String bouncyAlgo = bouncyKeyPair.getPrivate().getAlgorithm().toUpperCase();
+            String classAlgoName = bouncyAlgo;
+            if (bouncyAlgo.equals("ECDSA")) {
+                classAlgoName = "EC";
+            }
+
+            Assert.assertEquals(KeyFingerprinter.md5Fingerprint(bouncyKeyPair), keyResource.md5Fingerprint);
+            Assert.assertTrue(bouncyKeyPair.getPrivate().getClass().getSimpleName().contains("BC" + classAlgoName + "Private"));
+            Assert.assertTrue(bouncyKeyPair.getPublic().getClass().getSimpleName().contains("BC" + classAlgoName + "Public"));
+
+            final String nssAlgo = bouncyKeyPair.getPrivate().getAlgorithm().toUpperCase();
+            Assert.assertEquals(bouncyAlgo, nssAlgo);
+
+            final KeyPair nssKeyPair = loadTestKeyPair(keyResource.resourcePath, DesiredSecurityProvider.NSS);
+            Assert.assertEquals(KeyFingerprinter.md5Fingerprint(nssKeyPair), keyResource.md5Fingerprint);
+            Assert.assertTrue(nssKeyPair.getPrivate().getClass().getSimpleName().contains("P11" + classAlgoName));
+            Assert.assertTrue(nssKeyPair.getPublic().getClass().getSimpleName().contains("P11" + classAlgoName));
+
+        }
+    }
+
+    public void willThrowWhenPkcs11IsRequestedButUnavailable() throws Exception {
+        if (Security.getProvider(PROVIDER_PKCS11_NSS) != null) {
+            throw new SkipException("PKCS11 provider is available, can't perform skip test");
+        }
+
+        final KeyPair keyPair = generateKeyPair();
+        final byte[] serializedKey = serializePrivateKey(keyPair, null);
+
+        Assert.assertTrue(new String(serializedKey, StandardCharsets.UTF_8).startsWith(RSA_HEADER));
+
+        Assert.assertThrows(KeyLoadException.class, () ->
+                KeyPairLoader.getKeyPair(new ByteArrayInputStream(serializedKey), null, DesiredSecurityProvider.NSS));
+    }
+
     // TEST UTILITY METHODS
 
     private KeyPair generateKeyPair() throws NoSuchAlgorithmException {
@@ -122,4 +176,13 @@ private void compareKeyContents(KeyPair expectedKeyPair, KeyPair actualKeyPair)
                 actualKeyPair.getPublic().getEncoded());
     }
 
+    private KeyPair loadTestKeyPair(final String resourcePath,
+                                    final DesiredSecurityProvider provider) throws IOException {
+        final KeyPair loadedKeyPair;
+
+        try (final InputStream inputKey = CLASS_LOADER.getResourceAsStream(resourcePath)) {
+            loadedKeyPair = KeyPairLoader.getKeyPair(inputKey, null, provider);
+        }
+        return loadedKeyPair;
+    }
 }
diff --git a/common/src/test/java/com/joyent/http/signature/SignerTestUtil.java b/common/src/test/java/com/joyent/http/signature/SignerTestUtil.java
@@ -13,7 +13,6 @@
 import java.util.HashMap;
 import java.util.Map;
 
-
 public class SignerTestUtil {
     @SuppressWarnings("serial")
     public static final Map<String,TestKeyResource> keys = new HashMap<String,TestKeyResource>() {{
@@ -56,7 +55,7 @@ public static KeyPair testKeyPair(String keyId) throws IOException {
         final ClassLoader loader = SignerTestUtil.class.getClassLoader();
 
         try (InputStream is = loader.getResourceAsStream(keys.get(keyId).resourcePath)) {
-            KeyPair classPathPair = KeyPairLoader.getKeyPair(is, null);
+            KeyPair classPathPair = KeyPairLoader.getKeyPair(is, null, null);
             return classPathPair;
         }
     }
diff --git a/microbench/src/main/java/com/joyent/http/signature/BenchmarkDSASigner.java b/microbench/src/main/java/com/joyent/http/signature/BenchmarkDSASigner.java
diff --git a/microbench/src/main/java/com/joyent/http/signature/BenchmarkECDSASigner.java b/microbench/src/main/java/com/joyent/http/signature/BenchmarkECDSASigner.java
diff --git a/microbench/src/main/java/com/joyent/http/signature/BenchmarkRSASigner.java b/microbench/src/main/java/com/joyent/http/signature/BenchmarkRSASigner.java

Original file line number	Diff line number	Diff line change
`@@ -13,7 +13,6 @@`
`13`	`13`	`import java.util.HashMap;`
`14`	`14`	`import java.util.Map;`
`15`	`15`
`16`		`-`
`17`	`16`	`public class SignerTestUtil {`
`18`	`17`	`@SuppressWarnings("serial")`
`19`	`18`	`public static final Map<String,TestKeyResource> keys = new HashMap<String,TestKeyResource>() {{`
`@@ -56,7 +55,7 @@ public static KeyPair testKeyPair(String keyId) throws IOException {`
`56`	`55`	`final ClassLoader loader = SignerTestUtil.class.getClassLoader();`
`57`	`56`
`58`	`57`	`try (InputStream is = loader.getResourceAsStream(keys.get(keyId).resourcePath)) {`
`59`		`- KeyPair classPathPair = KeyPairLoader.getKeyPair(is, null);`
	`58`	`+ KeyPair classPathPair = KeyPairLoader.getKeyPair(is, null, null);`
`60`	`59`	`return classPathPair;`
`61`	`60`	`}`
`62`	`61`	`}`