From f2efd68c396e9b02fed5c594943c8be920243a06 Mon Sep 17 00:00:00 2001
From: Norman Maurer <norman_maurer@apple.com>
Date: Wed, 15 Jun 2016 12:04:29 +0200
Subject: [PATCH] Correctly support SSLSession.getId() when using OpenSslEngine

Motivation:

At the moment SSLSession.getId() may always return an empty byte array when OpenSSLEngine is used. This is as we not set SSL_OP_NO_TICKET on the SSLContext and so SSL_SESSION_get_id(...) will return an session id with length of 0 if tickets are not used.

Modifications:

- Set SSL_OP_NO_TICKET by default and only clear it if the user requests the usage of session tickets.
- Add unit test

Result:

Ensure consistent behavior between different SSLEngine implementations.
---
 .../io/netty/handler/ssl/OpenSslContext.java  |  5 ++++
 .../handler/ssl/OpenSslSessionContext.java    | 12 ++++-----
 .../io/netty/handler/ssl/SSLEngineTest.java   | 25 +++++++++++++++++++
 3 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
index dff05b67d7..3abba9181a 100644
--- a/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslContext.java
@@ -191,6 +191,11 @@ public abstract class OpenSslContext extends SslContext {
                 SSLContext.setOptions(ctx, SSL.SSL_OP_SINGLE_ECDH_USE);
                 SSLContext.setOptions(ctx, SSL.SSL_OP_SINGLE_DH_USE);
                 SSLContext.setOptions(ctx, SSL.SSL_OP_NO_SESSION_RESUMPTION_ON_RENEGOTIATION);
+                // Disable ticket support by default to be more inline with SSLEngineImpl of the JDK.
+                // This also let SSLSession.getId() work the same way for the JDK implementation and the OpenSSLEngine.
+                // If tickets are supported SSLSession.getId() will only return an ID on the server-side if it could
+                // make use of tickets.
+                SSLContext.setOptions(ctx, SSL.SSL_OP_NO_TICKET);
 
                 // We need to enable SSL_MODE_ACCEPT_MOVING_WRITE_BUFFER as the memory address may change between
                 // calling OpenSSLEngine.wrap(...).
diff --git a/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java b/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java
index 45b646a701..bcc76159ff 100644
--- a/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java
+++ b/handler/src/main/java/io/netty/handler/ssl/OpenSslSessionContext.java
@@ -15,6 +15,8 @@
  */
 package io.netty.handler.ssl;
 
+import io.netty.util.internal.ObjectUtil;
+import org.apache.tomcat.jni.SSL;
 import org.apache.tomcat.jni.SSLContext;
 import org.apache.tomcat.jni.SessionTicketKey;
 
@@ -60,9 +62,8 @@ public abstract class OpenSslSessionContext implements SSLSessionContext {
      */
     @Deprecated
     public void setTicketKeys(byte[] keys) {
-        if (keys == null) {
-            throw new NullPointerException("keys");
-        }
+        ObjectUtil.checkNotNull(keys, "keys");
+        SSLContext.clearOptions(context.ctx, SSL.SSL_OP_NO_TICKET);
         SSLContext.setSessionTicketKeys(context.ctx, keys);
     }
 
@@ -70,9 +71,8 @@ public abstract class OpenSslSessionContext implements SSLSessionContext {
      * Sets the SSL session ticket keys of this context.
      */
     public void setTicketKeys(OpenSslSessionTicketKey... keys) {
-        if (keys == null) {
-            throw new NullPointerException("keys");
-        }
+        ObjectUtil.checkNotNull(keys, "keys");
+        SSLContext.clearOptions(context.ctx, SSL.SSL_OP_NO_TICKET);
         SessionTicketKey[] ticketKeys = new SessionTicketKey[keys.length];
         for (int i = 0; i < ticketKeys.length; i++) {
             ticketKeys[i] = keys[i].key;
diff --git a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
index 6b3acee740..1d5ccde22a 100644
--- a/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
+++ b/handler/src/test/java/io/netty/handler/ssl/SSLEngineTest.java
@@ -332,6 +332,31 @@ public abstract class SSLEngineTest {
         assertFalse(session.isValid());
     }
 
+    @Test
+    public void testSSLSessionId() throws Exception {
+        final SslContext clientContext = SslContextBuilder.forClient()
+                .trustManager(InsecureTrustManagerFactory.INSTANCE)
+                .sslProvider(sslProvider())
+                .build();
+        SelfSignedCertificate ssc = new SelfSignedCertificate();
+        SslContext serverContext = SslContextBuilder.forServer(ssc.certificate(), ssc.privateKey())
+                .sslProvider(sslProvider())
+                .build();
+        SSLEngine clientEngine = clientContext.newEngine(UnpooledByteBufAllocator.DEFAULT);
+        SSLEngine serverEngine = serverContext.newEngine(UnpooledByteBufAllocator.DEFAULT);
+
+        // Before the handshake the id should have length == 0
+        assertEquals(0, clientEngine.getSession().getId().length);
+        assertEquals(0, serverEngine.getSession().getId().length);
+
+        handshake(clientEngine, serverEngine);
+
+        // After the handshake the id should have length > 0
+        assertNotEquals(0, clientEngine.getSession().getId().length);
+        assertNotEquals(0, serverEngine.getSession().getId().length);
+        assertArrayEquals(clientEngine.getSession().getId(), serverEngine.getSession().getId());
+    }
+
     protected void testEnablingAnAlreadyDisabledSslProtocol(String[] protocols1, String[] protocols2) throws Exception {
         SSLEngine sslEngine = null;
         try {