Repository: relayrides/pushy (java)

Issue:
P12Util didn't reject a keystore with multiple private keys
As [discussed on the mailing list](https://groups.google.com/d/msg/pushy-apns/3HonwEuUxYk/oBioFgMyFAAJ), it sounds like `P12Util` may not be rejecting all keystores with multiple private key entries.

Pull request:
Allow undefined behavior with multi-key P12 files (closes #292)
As discussed in #292, we can't reliably detect problematic P12 files with multiple private keys. Rather than advertise that we're checking for problematic cases (since we know we can't catch the case that users are most likely to encounter), we could just warn users that the behavior with a multi-key P12 file is undefined.

None of the options feel great to me, but I did want to open this for discussion. What do you all think?

Patch:
diff --git a/README.md b/README.md
index aa045bdc..612cbb6f 100644
--- a/README.md
+++ b/README.md
@@ -123,7 +123,7 @@ Pushy works with Java 7 and newer, but has some additional dependencies dependin
 
 The APNs protocol is built on top of the [HTTP/2 protocol](https://http2.github.io/). HTTP/2 is a relatively new protocol, and relies on some new developments that aren't yet wide-spread in the Java world. In particular:
 
-1. HTTP/2 depends on [ALPN](https://tools.ietf.org/html/rfc7301), a TLS extension for protocol negotiation. No version of Java has native ALPN support at this time. The ALPN requirement may be met either by [using a native SSL provider](#using-a-native-ssl-provider) or by [using Jetty's ALPN implementation](#using-jettys-alpn-implementation) under Java 7 or 8.
+1. HTTP/2 depends on [ALPN](https://tools.ietf.org/html/rfc7301), a TLS extension for protocol negotiation. No version of Java has native ALPN support at this time. The ALPN requirement may be met either by [using a native SSL provider](#using-a-native-ssl-provider) or by [using Jetty's ALPN implementation](#using-jettys-alpn-implementation) under OpenJDK 7 or 8.
 2. The HTTP/2 specification requires the use of [ciphers](https://httpwg.github.io/specs/rfc7540.html#rfc.section.9.2.2) that weren't introduced in Java until Java 8. Using a native SSL provider is the best way to meet this requirement under Java 7. A native SSL provider isn't a requirement under Java 8, but may still yield performance gains.
 
 Generally speaking, a native SSL provider is the best way to fulfill the system requirements imposed by HTTP/2 because installation is fairly straightforward, it works for Java 7 onward and generally offers better SSL performance than the JDK SSL provider.
@@ -148,9 +148,9 @@ Please note that Pushy requires netty-tcnative 1.1.33.Fork17 or newer. Additiona
 
 ### Using Jetty's ALPN implementation
 
-As an alternative to a native SSL provider, Jetty's ALPN implementation. Please note that if you're not using Oracle JDK 8 or newer (or if you're using a JDK other than Oracle's), you'll need to meet the cipher suite requirement separately; you may do so either by using a native SSL provider (which also fulfills the ALPN requirement) or by using another cryptography provider (which is beyond the scope of this document).
+As an alternative to a native SSL provider, you may use Jetty's ALPN implementation if you're using OpenJDK 8. Please note that if you're not using Java 8 or newer, you'll need to meet the cipher suite requirement separately; you may do so either by using a native SSL provider (which also fulfills the ALPN requirement) or by using another cryptography provider (which is beyond the scope of this document).
 
-Using Jetty's ALPN implementation is somewhat more complicated than using a native SSL provider. You'll need to choose a version of `alpn-boot` specific to the version (down to the update!) of the JDK you're using, and then add it to your *boot* class path (note that this is *not* the same as your regular classpath). [Detailed instructions](http://www.eclipse.org/jetty/documentation/current/alpn-chapter.html) are provided by Jetty.
+Using Jetty's ALPN implementation is somewhat more complicated than using a native SSL provider. You'll need to choose a version of `alpn-boot` specific to the version (down to the update!) of OpenJDK you're using, and then add it to your *boot* class path (note that this is *not* the same as your regular classpath). [Detailed instructions](http://www.eclipse.org/jetty/documentation/current/alpn-chapter.html) are provided by Jetty.
 
 If you know exactly which version of Java you'll be running, you can just add that specific version of `alpn-boot` to your boot class path. If your project might run on a number of different systems and if you use Maven, you can use the [`jetty-alpn-agent`](https://github.com/jetty-project/jetty-alpn-agent) in your `pom.xml`, which loads the correct `alpn-boot` JAR file for the current Java version on the fly.
 
diff --git a/pushy/script/generate-certs.sh b/pushy/script/generate-certs.sh
index 96e8a8dd..dbfedeef 100755
--- a/pushy/script/generate-certs.sh
+++ b/pushy/script/generate-certs.sh
@@ -37,6 +37,9 @@ done
 
 keytool -importkeystore -srckeystore multiple-keys.jks -destkeystore multiple-keys.p12 -srcstoretype JKS -deststoretype PKCS12 -srcstorepass pushy-test -deststorepass pushy-test
 
+# Generate a PKCS#12 with a certificate, but no private key
+openssl pkcs12 -export -in ca.pem -nokeys -out no-keys.p12 -password pass:pushy-test
+
 # Clean up intermediate files
 rm *.key
 rm multiple-keys.jks
diff --git a/pushy/src/main/java/com/relayrides/pushy/apns/ApnsClient.java b/pushy/src/main/java/com/relayrides/pushy/apns/ApnsClient.java
index 028a0357..12655349 100644
--- a/pushy/src/main/java/com/relayrides/pushy/apns/ApnsClient.java
+++ b/pushy/src/main/java/com/relayrides/pushy/apns/ApnsClient.java
@@ -28,11 +28,8 @@ import java.io.InputStream;
 import java.net.InetSocketAddress;
 import java.security.KeyStore.PrivateKeyEntry;
 import java.security.KeyStoreException;
-import java.security.NoSuchAlgorithmException;
 import java.security.PrivateKey;
-import java.security.UnrecoverableEntryException;
 import java.security.cert.Certificate;
-import java.security.cert.CertificateException;
 import java.security.cert.X509Certificate;
 import java.util.IdentityHashMap;
 import java.util.Map;
@@ -217,7 +214,8 @@ public class ApnsClient<T extends ApnsPushNotification> {
 
     /**
      * <p>Creates a new APNs client that will identify itself to the APNs gateway with the certificate and key from the
-     * given PKCS#12 file. The PKCS#12 file <em>must</em> contain a single certificate/private key pair.</p>
+     * given PKCS#12 file. The PKCS#12 file <em>must</em> contain a certificate/private key pair. If it contains more
+     * than one certificate/private key pair, the pair that will be used is undefined.</p>
      *
      * <p>Clients created using this method will use a default, internally-managed event loop group. Once the client
      * has been disconnected via the {@link ApnsClient#disconnect()} method, its event loop will be shut down and the
@@ -241,7 +239,8 @@ public class ApnsClient<T extends ApnsPushNotification> {
 
     /**
      * <p>Creates a new APNs client that will identify itself to the APNs gateway with the certificate and key from the
-     * given PKCS#12 file. The PKCS#12 file <em>must</em> contain a single certificate/private key pair.</p>
+     * given PKCS#12 file. The PKCS#12 file <em>must</em> contain a certificate/private key pair. If it contains more
+     * than one certificate/private key pair, the pair that will be used is undefined.</p>
      *
      * <p>Clients created using this method will use the provided event loop group, which may be useful in cases where
      * multiple clients are running in parallel. If a client is created using this method, it is the responsibility of
@@ -268,7 +267,7 @@ public class ApnsClient<T extends ApnsPushNotification> {
     /**
      * <p>Creates a new APNs client that will identify itself to the APNs gateway with the certificate and key from the
      * given PKCS#12 input stream. The PKCS#12 input stream <em>must</em> contain a single certificate/private key
-     * pair.</p>
+     * pair. If it contains more than one certificate/private key pair, the pair that will be used is undefined.</p>
      *
      * <p>Clients created using this method will use a default, internally-managed event loop group. Once the client
      * has been disconnected via the {@link ApnsClient#disconnect()} method, its event loop will be shut down and the
@@ -291,7 +290,7 @@ public class ApnsClient<T extends ApnsPushNotification> {
     /**
      * <p>Creates a new APNs client that will identify itself to the APNs gateway with the certificate and key from the
      * given PKCS#12 input stream. The PKCS#12 input stream <em>must</em> contain a single certificate/private key
-     * pair.</p>
+     * pair. If it contains more than one certificate/private key pair, the pair that will be used is undefined.</p>
      *
      * <p>Clients created using this method will use the provided event loop group, which may be useful in cases where
      * multiple clients are running in parallel. If a client is created using this method, it is the responsibility of
@@ -370,7 +369,7 @@ public class ApnsClient<T extends ApnsPushNotification> {
         final PrivateKey privateKey;
 
         try {
-            final PrivateKeyEntry privateKeyEntry = P12Util.getPrivateKeyEntryFromP12InputStream(p12InputStream, password);
+            final PrivateKeyEntry privateKeyEntry = P12Util.getFirstPrivateKeyEntryFromP12InputStream(p12InputStream, password);
 
             final Certificate certificate = privateKeyEntry.getCertificate();
 
@@ -380,7 +379,7 @@ public class ApnsClient<T extends ApnsPushNotification> {
 
             x509Certificate = (X509Certificate) certificate;
             privateKey = privateKeyEntry.getPrivateKey();
-        } catch (final KeyStoreException | NoSuchAlgorithmException | UnrecoverableEntryException | CertificateException | IOException e) {
+        } catch (final KeyStoreException | IOException e) {
             throw new SSLException(e);
         }
 
diff --git a/pushy/src/main/java/com/relayrides/pushy/apns/P12Util.java b/pushy/src/main/java/com/relayrides/pushy/apns/P12Util.java
index 163b950b..950e9c84 100644
--- a/pushy/src/main/java/com/relayrides/pushy/apns/P12Util.java
+++ b/pushy/src/main/java/com/relayrides/pushy/apns/P12Util.java
@@ -11,17 +11,38 @@ import java.security.cert.CertificateException;
 import java.util.Enumeration;
 import java.util.Objects;
 
+/**
+ * Utility class for extracting private keys from P12 files.
+ *
+ * @author <a href="https://github.com/jchambers">Jon Chambers</a>
+ */
 class P12Util {
 
-    public static PrivateKeyEntry getPrivateKeyEntryFromP12InputStream(final InputStream p12InputStream, final String password) throws KeyStoreException, NoSuchAlgorithmException, UnrecoverableEntryException, CertificateException, IOException {
+    /**
+     * Returns the first private key entry found in the given keystore. If more than one private key is present, the
+     * key that is returned is undefined.
+     *
+     * @param p12InputStream an input stream for a PKCS#12 keystore
+     * @param password the password to be used to load the keystore and its entries; may be blank, but must not be
+     * {@code null}
+     *
+     * @return the first private key entry found in the given keystore
+     *
+     * @throws KeyStoreException if a private key entry could not be extracted from the given keystore for any reason
+     * @throws IOException if the given input stream could not be read for any reason
+     */
+    public static PrivateKeyEntry getFirstPrivateKeyEntryFromP12InputStream(final InputStream p12InputStream, final String password) throws KeyStoreException, IOException {
         Objects.requireNonNull(password, "Password may be blank, but must not be null.");
 
         final KeyStore keyStore = KeyStore.getInstance("PKCS12");
-        keyStore.load(p12InputStream, password.toCharArray());
 
-        final Enumeration<String> aliases = keyStore.aliases();
-        KeyStore.PrivateKeyEntry privateKeyEntry = null;
+        try {
+            keyStore.load(p12InputStream, password.toCharArray());
+        } catch (NoSuchAlgorithmException | CertificateException e) {
+            throw new KeyStoreException(e);
+        }
 
+        final Enumeration<String> aliases = keyStore.aliases();
         final KeyStore.PasswordProtection passwordProtection = new KeyStore.PasswordProtection(password.toCharArray());
 
         while (aliases.hasMoreElements()) {
@@ -30,24 +51,20 @@ class P12Util {
             KeyStore.Entry entry;
 
             try {
-                entry = keyStore.getEntry(alias, passwordProtection);
-            } catch (final UnsupportedOperationException e) {
-                entry = keyStore.getEntry(alias, null);
+                try {
+                    entry = keyStore.getEntry(alias, passwordProtection);
+                } catch (final UnsupportedOperationException e) {
+                    entry = keyStore.getEntry(alias, null);
+                }
+            } catch (final UnrecoverableEntryException | NoSuchAlgorithmException e) {
+                throw new KeyStoreException(e);
             }
 
             if (entry instanceof KeyStore.PrivateKeyEntry) {
-                if (privateKeyEntry != null) {
-                    throw new KeyStoreException("Key store must contain exactly one private key entry.");
-                }
-
-                privateKeyEntry = (PrivateKeyEntry) entry;
+                return (PrivateKeyEntry) entry;
             }
         }
 
-        if (privateKeyEntry == null) {
-            throw new KeyStoreException("Key store must contain exactly one private key entry.");
-        }
-
-        return privateKeyEntry;
+        throw new KeyStoreException("Key store did not contain any private key entries.");
     }
 }


Test patch:
diff --git a/pushy/src/test/java/com/relayrides/pushy/apns/ApnsClientTest.java b/pushy/src/test/java/com/relayrides/pushy/apns/ApnsClientTest.java
index 3a738b24..a7691cf0 100644
--- a/pushy/src/test/java/com/relayrides/pushy/apns/ApnsClientTest.java
+++ b/pushy/src/test/java/com/relayrides/pushy/apns/ApnsClientTest.java
@@ -268,7 +268,7 @@ public class ApnsClientTest {
         // We're happy here as long as nothing throws an exception
         try (final InputStream p12InputStream = ApnsClientTest.class.getResourceAsStream(SINGLE_TOPIC_CLIENT_KEYSTORE_FILENAME)) {
             final PrivateKeyEntry privateKeyEntry =
-                    P12Util.getPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
+                    P12Util.getFirstPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
 
             new ApnsClient<SimpleApnsPushNotification>(
                     (X509Certificate) privateKeyEntry.getCertificate(), privateKeyEntry.getPrivateKey(), KEYSTORE_PASSWORD);
@@ -281,7 +281,7 @@ public class ApnsClientTest {
         try (final InputStream p12InputStream = ApnsClientTest.class.getResourceAsStream(SINGLE_TOPIC_CLIENT_KEYSTORE_UNPROTECTED_FILENAME)) {
 
             final PrivateKeyEntry privateKeyEntry =
-                    P12Util.getPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
+                    P12Util.getFirstPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
 
             new ApnsClient<SimpleApnsPushNotification>(
                     (X509Certificate) privateKeyEntry.getCertificate(), privateKeyEntry.getPrivateKey(), null);
@@ -747,7 +747,7 @@ public class ApnsClientTest {
 
     private static SslContext getSslContextForTestClient(final String p12Filename, final String password) throws NoSuchAlgorithmException, KeyStoreException, IOException, CertificateException, UnrecoverableEntryException {
         try (final InputStream p12InputStream = ApnsClientTest.class.getResourceAsStream(p12Filename)) {
-            final PrivateKeyEntry privateKeyEntry = P12Util.getPrivateKeyEntryFromP12InputStream(p12InputStream, password);
+            final PrivateKeyEntry privateKeyEntry = P12Util.getFirstPrivateKeyEntryFromP12InputStream(p12InputStream, password);
 
             return SslContextBuilder.forClient()
                     .sslProvider(OpenSsl.isAlpnSupported() ? SslProvider.OPENSSL : SslProvider.JDK)
diff --git a/pushy/src/test/java/com/relayrides/pushy/apns/MockApnsServer.java b/pushy/src/test/java/com/relayrides/pushy/apns/MockApnsServer.java
index 42da9efd..8a4e62f6 100644
--- a/pushy/src/test/java/com/relayrides/pushy/apns/MockApnsServer.java
+++ b/pushy/src/test/java/com/relayrides/pushy/apns/MockApnsServer.java
@@ -54,7 +54,7 @@ public class MockApnsServer {
     public MockApnsServer(final EventLoopGroup eventLoopGroup) {
         final SslContext sslContext;
         try (final InputStream caInputStream = this.getClass().getResourceAsStream(CA_CERTIFICATE_FILENAME)) {
-            final PrivateKeyEntry privateKeyEntry = P12Util.getPrivateKeyEntryFromP12InputStream(
+            final PrivateKeyEntry privateKeyEntry = P12Util.getFirstPrivateKeyEntryFromP12InputStream(
                     MockApnsServer.class.getResourceAsStream(SERVER_KEYSTORE), SERVER_KEYSTORE_PASSWORD);
 
             sslContext = SslContextBuilder.forServer(privateKeyEntry.getPrivateKey(), (X509Certificate) privateKeyEntry.getCertificate())
diff --git a/pushy/src/test/java/com/relayrides/pushy/apns/P12UtilTest.java b/pushy/src/test/java/com/relayrides/pushy/apns/P12UtilTest.java
index aa4f5176..05afc6c3 100644
--- a/pushy/src/test/java/com/relayrides/pushy/apns/P12UtilTest.java
+++ b/pushy/src/test/java/com/relayrides/pushy/apns/P12UtilTest.java
@@ -12,6 +12,7 @@ public class P12UtilTest {
 
     private static final String SINGLE_TOPIC_CLIENT_KEYSTORE_FILENAME = "/single-topic-client.p12";
     private static final String MULTIPLE_KEY_KEYSTORE_FILENAME = "/multiple-keys.p12";
+    private static final String NO_KEY_KEYSTORE_FILENAME = "/no-keys.p12";
 
     private static final String KEYSTORE_PASSWORD = "pushy-test";
 
@@ -19,19 +20,26 @@ public class P12UtilTest {
     public void testGetPrivateKeyEntryFromP12InputStream() throws Exception {
         try (final InputStream p12InputStream = P12UtilTest.class.getResourceAsStream(SINGLE_TOPIC_CLIENT_KEYSTORE_FILENAME)) {
             final PrivateKeyEntry privateKeyEntry =
-                    P12Util.getPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
+                    P12Util.getFirstPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
 
             assertNotNull(privateKeyEntry);
         }
     }
 
-    @Test(expected = KeyStoreException.class)
+    @Test
     public void testGetPrivateKeyEntryFromP12InputStreamWithMultipleKeys() throws Exception {
         try (final InputStream p12InputStream = P12UtilTest.class.getResourceAsStream(MULTIPLE_KEY_KEYSTORE_FILENAME)) {
             final PrivateKeyEntry privateKeyEntry =
-                    P12Util.getPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
+                    P12Util.getFirstPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
 
             assertNotNull(privateKeyEntry);
         }
     }
+
+    @Test(expected = KeyStoreException.class)
+    public void testGetPrivateKeyEntryFromP12InputStreamWithNoKeys() throws Exception {
+        try (final InputStream p12InputStream = P12UtilTest.class.getResourceAsStream(NO_KEY_KEYSTORE_FILENAME)) {
+            P12Util.getFirstPrivateKeyEntryFromP12InputStream(p12InputStream, KEYSTORE_PASSWORD);
+        }
+    }
 }