Update CertificateRequestCallback usage to match new tcnative

Motiviation:

Previously the way how CertificateRequestCallback was working had some issues which could cause memory leaks and segfaults. Due of this tcnative code was updated to change the signature of the method provided by the interface.

Modifications:

Update CertificateRequestCallback implementations to match new interface signature.

Result:

No more segfaults / memory leaks when using boringssl or openssl >= 1.1.0

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

@@ -16,6 +16,7 @@
 package io.netty.handler.ssl;
 
 import io.netty.buffer.ByteBufAllocator;
+import org.apache.tomcat.jni.CertificateRequestedCallback;
 import org.apache.tomcat.jni.SSL;
 
 import javax.net.ssl.SSLException;
@@ -84,9 +85,46 @@ class OpenSslKeyMaterialManager {
         }
     }
 
-    void setKeyMaterial(ReferenceCountedOpenSslEngine engine, String[] keyTypes,
+    CertificateRequestedCallback.KeyMaterial keyMaterial(ReferenceCountedOpenSslEngine engine, String[] keyTypes,
                                                          X500Principal[] issuer) throws SSLException {
-        setKeyMaterial(engine.sslPointer(), chooseClientAlias(engine, keyTypes, issuer));
+        String alias = chooseClientAlias(engine, keyTypes, issuer);
+        long keyBio = 0;
+        long keyCertChainBio = 0;
+        long pkey = 0;
+        long certChain = 0;
+
+        try {
+            // TODO: Should we cache these and so not need to do a memory copy all the time ?
+            X509Certificate[] certificates = keyManager.getCertificateChain(alias);
+            if (certificates == null || certificates.length == 0) {
+                return null;
+            }
+
+            PrivateKey key = keyManager.getPrivateKey(alias);
+            keyCertChainBio = toBIO(certificates);
+            certChain = SSL.parseX509Chain(keyCertChainBio);
+            if (key != null) {
+                keyBio = toBIO(key);
+                pkey = SSL.parsePrivateKey(keyBio, password);
+            }
+            CertificateRequestedCallback.KeyMaterial material = new CertificateRequestedCallback.KeyMaterial(
+                    certChain, pkey);
+
+            // Reset to 0 so we do not free these. This is needed as the client certificate callback takes ownership
+            // of both the key and the certificate if they are returned from this method, and thus must not
+            // be freed here.
+            certChain = pkey = 0;
+            return material;
+        } catch (SSLException e) {
+            throw e;
+        } catch (Exception e) {
+            throw new SSLException(e);
+        } finally {
+            freeBio(keyBio);
+            freeBio(keyCertChainBio);
+            SSL.freePrivateKey(pkey);
+            SSL.freeX509Chain(certChain);
+        }
     }
 
     private void setKeyMaterial(long ssl, String alias) throws SSLException {
@@ -96,10 +134,13 @@ class OpenSslKeyMaterialManager {
 
         try {
             // TODO: Should we cache these and so not need to do a memory copy all the time ?
-            PrivateKey key = keyManager.getPrivateKey(alias);
             X509Certificate[] certificates = keyManager.getCertificateChain(alias);
+            if (certificates == null || certificates.length == 0) {
+                return;
+            }
+
+            PrivateKey key = keyManager.getPrivateKey(alias);
 
-            if (certificates != null && certificates.length != 0) {
             // Only encode one time
             PemEncoded encoded = PemX509Certificate.toPEM(ByteBufAllocator.DEFAULT, true, certificates);
             try {
@@ -116,7 +157,6 @@ class OpenSslKeyMaterialManager {
             } finally {
                 encoded.release();
             }
-            }
         } catch (SSLException e) {
             throw e;
         } catch (Exception e) {

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

@@ -234,7 +234,7 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted
         }
 
         @Override
-        public void requested(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals) {
+        public KeyMaterial requested(long ssl, byte[] keyTypeBytes, byte[][] asn1DerEncodedPrincipals) {
             final ReferenceCountedOpenSslEngine engine = engineMap.get(ssl);
             try {
                 final Set<String> keyTypesSet = supportedClientKeyTypes(keyTypeBytes);
@@ -248,12 +248,13 @@ public final class ReferenceCountedOpenSslClientContext extends ReferenceCounted
                         issuers[i] = new X500Principal(asn1DerEncodedPrincipals[i]);
                     }
                 }
-                keyManagerHolder.setKeyMaterial(engine, keyTypes, issuers);
+                return keyManagerHolder.keyMaterial(engine, keyTypes, issuers);
             } catch (Throwable cause) {
                 logger.debug("request of key failed", cause);
                 SSLHandshakeException e = new SSLHandshakeException("General OpenSslEngine problem");
                 e.initCause(cause);
                 engine.handshakeException = e;
+                return null;
             }
         }
 

pom.xml

@@ -224,7 +224,7 @@
     <!-- Fedora-"like" systems. This is currently only used for the netty-tcnative dependency -->
     <os.detection.classifierWithLikes>fedora</os.detection.classifierWithLikes>
     <tcnative.artifactId>netty-tcnative</tcnative.artifactId>
-    <tcnative.version>1.1.33.Fork20</tcnative.version>
+    <tcnative.version>1.1.33.Fork21</tcnative.version>
     <tcnative.classifier>${os.detected.classifier}</tcnative.classifier>
     <epoll.classifier>${os.detected.name}-${os.detected.arch}</epoll.classifier>
     <logging.config>${project.basedir}/../common/src/test/resources/logback-test.xml</logging.config>