Add support for PKCS#1 keys by floragunn · Pull Request #7451 · netty/netty

floragunn · 2017-11-29T09:45:21Z

Motivation:

Currently netty only support PKCS#8 keys,
but PKCS#1 is still used in the wild out there.
This only works if Bouncy Castle (BC) is available in the
classpath.

Modifications:

Until now netty implements the extraction of a key from a PEM
itself. All this code remains and is still valid.
Before the original code is called we look if BC is available.
If not then we do just nothing. If yes we use BC to extract the
private key from the PEM. If that fails we fallback to the original code.
Add unittests for unencrypted and encrypted PKCS#1 PEM formatted keys.

Result:

Fixes #7323.

Motivation: Currently netty only support PKCS#8 keys, but PKCS#1 is still used in the wild out there. This only works if Bouncy Castle (BC) is available in the classpath. Modifications: - Until now netty implements the extraction of a key from a PEM itself. All this code remains and is still valid. - Before the original code is called we look if BC is available. If not then we do just nothing. If yes we use BC to extract the private key from the PEM. If that fails we fallback to the original code. - Add unittests for unencrypted and encrypted PKCS#1 PEM formatted keys. Result: Fixes netty#7323.

normanmaurer · 2017-11-29T10:46:09Z

@floragunncom please fix the test error: https://garage.netty.io/teamcity/viewLog.html?buildId=26426&buildTypeId=pulls_netty&guest=1

normanmaurer · 2017-11-29T11:16:34Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+    private static final InternalLogger logger = InternalLoggerFactory.getInstance(BouncyCastlePemReader.class);
+    private static Boolean available;
+
+    public static boolean isBcAvailable() {


remove Bc ?

normanmaurer · 2017-11-29T11:16:48Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+        return available;
+    }
+
+    private static void tryLoadingBc() {


Remove Bc

normanmaurer · 2017-11-29T11:18:15Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+            if (pemParser != null) {
+                try {
+                    pemParser.close();
+                } catch (Exception e) {


e -> ignored

what else can we do here instead of ignoring?

@floragunncom I only asked to rename e to ignored ;)

You may also want to log something in to debug level

changed to "ignored"

normanmaurer · 2017-11-29T11:18:40Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+        return new PEMParser(new FileReader(keyFile));
+    }
+
+    private static PEMParser getParser(InputStream keyInputStream) {


s/getParser/newParser/

normanmaurer · 2017-11-29T11:18:43Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+        }
+    }
+
+    private static PEMParser getParser(File keyFile) throws FileNotFoundException {


s/getParser/newParser/

normanmaurer · 2017-11-29T11:19:34Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+        try {
+            return getPrivateKey(getParser(keyFile), keyPassword);
+        } catch (Exception e) {
+            logger.debug("Unable to extract private key due to " + e);


change to: logger.debug("Unable to extract private key", e);

This way we also get a stacktrace

normanmaurer · 2017-11-29T11:19:39Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+        try {
+            return getPrivateKey(getParser(keyInputStream), keyPassword);
+        } catch (Exception e) {
+            logger.debug("Unable to extract private key due to " + e);


change to: logger.debug("Unable to extract private key", e);

This way we also get a stacktrace

normanmaurer · 2017-11-29T11:20:42Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                    logger.debug("Bouncy Castle provider available");
+                    return Boolean.TRUE;
+                } catch (Exception e) {
+                    logger.debug("Cannot load Bouncy Castle provider");


Change to: logger.debug("Cannot load Bouncy Castle provider", e);

normanmaurer · 2017-11-29T11:21:29Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+    }
+
+    static {
+        isBcAvailable();


call tryLoadingBc

normanmaurer

only a few more small things

normanmaurer · 2017-12-05T09:44:29Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+    private static Boolean available;
+
+    public static boolean isAvailable() {
+        tryLoading();


You can remove this as tryLoading() is called in static {...}

normanmaurer · 2017-12-05T09:44:43Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+    }
+
+    private static void tryLoading() {
+        if (available != null) {


remove this as it should only be called one time.

normanmaurer · 2017-12-05T09:45:00Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                try {
+                    Class<Provider> bcProviderClass
+                      = (Class<Provider>) Class.forName("org.bouncycastle.jce.provider.BouncyCastleProvider"
+                              , true, this.getClass().getClassLoader());


move the , on the line above.

normanmaurer · 2017-12-05T09:45:17Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+
+    private static PrivateKey getPrivateKey(PEMParser pemParser, String keyPassword) throws PEMException, IOException,
+            PKCSException, OperatorCreationException {
+


nit: remove empty line

jaymode · 2017-12-05T21:19:18Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                      = (Class<Provider>) Class.forName("org.bouncycastle.jce.provider.BouncyCastleProvider",
+                              true, this.getClass().getClassLoader());
+                    if (Security.getProvider("BC") == null) {
+                        Security.addProvider(bcProviderClass.newInstance());


Even if bouncycastle is on the classpath, I do not think a library like Netty should be installing this provider for everything in the JVM. I'd suggest passing an instance of the provider to methods as needed.

Thats indeed a valid argument, i will change this

carl-mastrangelo · 2017-12-07T00:43:04Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                    Class<Provider> bcProviderClass
+                      = (Class<Provider>) Class.forName("org.bouncycastle.jce.provider.BouncyCastleProvider",
+                              true, this.getClass().getClassLoader());
+                    bcProvider = bcProviderClass.newInstance();


getConstructor().newInstance()

carl-mastrangelo · 2017-12-07T00:43:59Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                    logger.debug("Bouncy Castle provider available");
+                    return Boolean.TRUE;
+                } catch (Exception e) {
+                    logger.debug("Cannot load Bouncy Castle provider", e);


Exception / Errors should be saved, likely in some sort of bouncyCastleUnavailabilityCause field for later use.

+1. Look at the native transports for an example of this (e.g. KQueue)

carl-mastrangelo · 2017-12-07T00:46:33Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+    private static Boolean available;
+
+    static {
+        tryLoading();


nit: doing this in a static init block leads to problems. If any exceptions are thrown, they are lost and reinterpreted as ClassNotFoundExceptions. Avoid if possible. You can delay till the first method call.

carl-mastrangelo · 2017-12-07T00:46:51Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+    }
+
+    public static PrivateKey getPrivateKey(InputStream keyInputStream, String keyPassword) {
+        try {


check is available first? Maybe throw the unavailability cause here?

carl-mastrangelo · 2017-12-07T00:47:12Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+        });
+    }
+
+    public static PrivateKey getPrivateKey(InputStream keyInputStream, String keyPassword) {


The return value needs to be documented.

also document the parameters too please.

carl-mastrangelo · 2017-12-07T00:47:57Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                try {
+                    pemParser.close();
+                } catch (Exception ignored) {
+                    // ignore


This should be logged. debug level is probably okay.

Scottmitch · 2017-12-20T02:50:34Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+        });
+    }
+
+    public static PrivateKey getPrivateKey(InputStream keyInputStream, String keyPassword) {


also document the parameters too please.

Scottmitch · 2017-12-20T02:51:03Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+        }
+    }
+
+    private static PrivateKey getPrivateKey(PEMParser pemParser, String keyPassword) throws PEMException, IOException,


IOException covers PEMException, and so you can remove PEMException.

Scottmitch · 2017-12-20T02:52:15Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+            PKCSException, OperatorCreationException {
+        try {
+            Object object = pemParser.readObject();
+            logger.debug("Parsed PEM object of type " + object.getClass().getName() + " and assume key is "


is this debug statement necessary? if so can you use the slf4j logging format to avoid always doing string concatenation operations (e.g. "Parsed PEM object of type {} ...", object.getClass().getName()

Scottmitch · 2017-12-20T02:52:57Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                } else if (object instanceof PEMKeyPair) {
+                    pk = converter.getKeyPair((PEMKeyPair) object).getPrivate();
+                } else {
+                    logger.debug("Unable to handle PEM object of type "


use slf4j's formatting syntax to avoid always doing string concatenation.

Scottmitch · 2017-12-20T02:53:01Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                    pk = converter.getPrivateKey(((PKCS8EncryptedPrivateKeyInfo) object)
+                            .decryptPrivateKeyInfo(pkcs8InputDecryptorProvider));
+                } else {
+                    logger.debug("Unable to handle PEM object of type " + object.getClass() + " as a encrypted key");


use slf4j's formatting syntax to avoid always doing string concatenation.

Scottmitch · 2017-12-20T02:58:58Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                    logger.debug("Bouncy Castle provider available");
+                    return Boolean.TRUE;
+                } catch (Exception e) {
+                    logger.debug("Cannot load Bouncy Castle provider", e);


+1. Look at the native transports for an example of this (e.g. KQueue)

Scottmitch · 2017-12-20T03:00:16Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+public final class BouncyCastlePemReader {
+
+    private static final InternalLogger logger = InternalLoggerFactory.getInstance(BouncyCastlePemReader.class);
+    private static Provider bcProvider;


if these variables are initialized in a static context, then we should be able to make them final.

Scottmitch · 2017-12-20T03:01:28Z

handler/src/main/java/io/netty/handler/ssl/util/BouncyCastlePemReader.java

+                            .setProvider(bcProvider)
+                            .build(keyPassword.toCharArray());
+                    pk = converter.getKeyPair(((PEMEncryptedKeyPair) object).decryptKeyPair(decProv)).getPrivate();
+                } else if (object instanceof PKCS8EncryptedPrivateKeyInfo) {


does this mean that BouncyCastle will be used for PKCS8 keys too?

Scottmitch · 2017-12-20T03:02:13Z

handler/src/main/java/io/netty/handler/ssl/SslContext.java

        }
+
+        // try BC first, if this fail fallback to original key extraction
+        // process


formatting: move this comment onto the previous line

Scottmitch · 2017-12-20T03:02:17Z

handler/src/main/java/io/netty/handler/ssl/SslContext.java

        }
+
+        // try BC first, if this fail fallback to original key extraction
+        // process


formatting: move this comment onto the previous line

normanmaurer · 2017-12-21T06:48:44Z

@floragunncom can you please address comments of @Scottmitch and @carl-mastrangelo and ping me once done. Thanks!

normanmaurer · 2018-01-23T14:32:33Z

@floragunncom ping

floragunn · 2018-01-26T10:53:17Z

no cycles at the moment but will come back to this in the next weeks

normanmaurer · 2018-05-04T11:53:30Z

@floragunncom any update ?

normanmaurer · 2018-09-04T09:56:57Z

Let me close this due inactivity. Please reopen once you have time again.

4x0v7 · 2019-09-27T12:06:05Z

Hi there. I pieced together some code to import PKCS#1 RSA keys into a KeyStore. Could this be useful here?

njhill · 2019-09-30T23:34:39Z

Thanks @4x0v7 I think it would be a nice addition, would you be willing to open a PR?

4x0v7 · 2019-10-02T07:58:19Z

@njhill Yes I can do. The implementation I did only supports PKCS#1 RSA private keys, and doesn't use bouncycastle

normanmaurer · 2019-10-07T06:07:18Z

@4x0v7 yep please open a PR and let us know once ready for review :)

Motivation: Motivated by a stale [PR](#7451) that was closed. This change adds support for keys in the PKCS#1 format. Currently netty only supports PKCS#8 keys. Modification: This change introduces a class called `BouncyCastlePemReader` which is only used if BouncyCastle is available on the classpath and uses BouncyCastle's PEMParser to parse the private keys. See list of supported types [here](https://www.bouncycastle.org/docs/pkixdocs1.5on/org/bouncycastle/openssl/PEMParser.html). Tests are added in `SSLContextTest` including tests with PKCS#8 keys (encrypted + unencrypted) to show these working with BouncyCastle. Result: Fixes [#7323](#7323) Signed-off-by: Craig Perkins <cwperx@amazon.com>

Fix failing testcase

fd98fce

normanmaurer requested changes Nov 29, 2017

View reviewed changes

Incorporate requested changes

68cf541

normanmaurer requested changes Dec 5, 2017

View reviewed changes

Incorporate requested changes

a1cfa98

jaymode reviewed Dec 5, 2017

View reviewed changes

Do not register BC provider

60de03f

normanmaurer approved these changes Dec 6, 2017

View reviewed changes

normanmaurer requested review from Scottmitch, carl-mastrangelo, ejona86 and nmittler December 6, 2017 19:54

carl-mastrangelo reviewed Dec 7, 2017

View reviewed changes

Scottmitch requested changes Dec 20, 2017

View reviewed changes

normanmaurer force-pushed the 4.1 branch from 6499ce1 to 4c1e0f5 Compare January 23, 2018 10:34

normanmaurer closed this Sep 4, 2018

cwperks mentioned this pull request Aug 5, 2022

Support for pkcs1 #12670

Merged


		private static PrivateKey getPrivateKey(PEMParser pemParser, String keyPassword) throws PEMException, IOException,
		PKCSException, OperatorCreationException {

Uh oh!

Conversation

floragunn commented Nov 29, 2017

Uh oh!

normanmaurer commented Nov 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

normanmaurer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment