SSLSocket related improvements

Mavenfile

@@ -82,9 +82,12 @@ plugin :clean do
                  'failOnError' =>  'false' )
 end
 
-jar 'org.jruby:jruby-core', '9.2.0.0', :scope => :provided
+jruby_compile_compat = '9.2.1.0' # due load_ext can use 9.2.0.0
+jar 'org.jruby:jruby-core', jruby_compile_compat, :scope => :provided
 # for invoker generated classes we need to add javax.annotation when on Java > 8
 jar 'javax.annotation:javax.annotation-api', '1.3.1', :scope => :compile
+# a test dependency to provide digest and other stdlib bits, needed when loading OpenSSL in Java unit tests
+jar 'org.jruby:jruby-stdlib', jruby_compile_compat, :scope => :test
 jar 'junit:junit', '[4.13.1,)', :scope => :test
 
 # NOTE: to build on Java 11 - installing gems fails (due old jossl) with:

pom.xml

@@ -98,7 +98,7 @@ DO NOT MODIFY - GENERATED CODE
     <dependency>
       <groupId>org.jruby</groupId>
       <artifactId>jruby-core</artifactId>
-      <version>9.2.0.0</version>
+      <version>9.2.1.0</version>
       <scope>provided</scope>
     </dependency>
     <dependency>
@@ -107,6 +107,12 @@ DO NOT MODIFY - GENERATED CODE
       <version>1.3.1</version>
       <scope>compile</scope>
     </dependency>
+    <dependency>
+      <groupId>org.jruby</groupId>
+      <artifactId>jruby-stdlib</artifactId>
+      <version>9.2.1.0</version>
+      <scope>test</scope>
+    </dependency>
     <dependency>
       <groupId>junit</groupId>
       <artifactId>junit</artifactId>
@@ -275,6 +281,7 @@ DO NOT MODIFY - GENERATED CODE
         <configuration>
           <source>1.8</source>
           <target>1.8</target>
+          <release>8</release>
           <encoding>UTF-8</encoding>
           <debug>true</debug>
           <showWarnings>true</showWarnings>

src/main/java/org/jruby/ext/openssl/SSLSocket.java

@@ -141,14 +141,15 @@ private static CallSite callSite(final CallSite[] sites, final CallSiteIndex ind
         return sites[ index.ordinal() ];
     }
 
+    private static final ByteBuffer EMPTY_DATA = ByteBuffer.allocate(0).asReadOnlyBuffer();
+
     private SSLContext sslContext;
     private SSLEngine engine;
     private RubyIO io;
 
-    private ByteBuffer appReadData;
-    private ByteBuffer netReadData;
-    private ByteBuffer netWriteData;
-    private final ByteBuffer dummy = ByteBuffer.allocate(0); // could be static
+    ByteBuffer appReadData;
+    ByteBuffer netReadData;
+    ByteBuffer netWriteData;
 
     private boolean initialHandshake = false;
     private transient long initializeTime;
@@ -539,8 +540,18 @@ public void wakeup() {
         }
     }
 
-    private static final int READ_WOULD_BLOCK_RESULT = Integer.MIN_VALUE + 1;
-    private static final int WRITE_WOULD_BLOCK_RESULT = Integer.MIN_VALUE + 2;
+    // Legitimate return values are -1 (EOF) and >= 0 (byte counts), so any value < -1 is safely in sentinel territory.
+    private static final int READ_WOULD_BLOCK_RESULT  = -2;
+    private static final int WRITE_WOULD_BLOCK_RESULT = -3;
+
+    private static boolean isWouldBlockResult(final int result) {
+        return result < -1;
+    }
+
+    private RubySymbol wouldBlockSymbol(final int result) {
+        assert isWouldBlockResult(result) : "unexpected result: " + result;
+        return getRuntime().newSymbol(result == READ_WOULD_BLOCK_RESULT ? "wait_readable" : "wait_writable");
+    }
 
     private static void readWouldBlock(final Ruby runtime, final boolean exception, final int[] result) {
         if ( exception ) throw newSSLErrorWaitReadable(runtime, "read would block");
@@ -552,10 +563,6 @@ private static void writeWouldBlock(final Ruby runtime, final boolean exception,
         result[0] = WRITE_WOULD_BLOCK_RESULT;
     }
 
-    private void doHandshake(final boolean blocking) throws IOException {
-        doHandshake(blocking, true);
-    }
-
     // might return :wait_readable | :wait_writable in case (true, false)
     private IRubyObject doHandshake(final boolean blocking, final boolean exception) throws IOException {
         while (true) {
@@ -577,7 +584,11 @@ private IRubyObject doHandshake(final boolean blocking, final boolean exception)
                 doTasks();
                 break;
             case NEED_UNWRAP:
-                if (readAndUnwrap(blocking) == -1 && handshakeStatus != SSLEngineResult.HandshakeStatus.FINISHED) {
+                int unwrapResult = readAndUnwrap(blocking, exception);
+                if (isWouldBlockResult(unwrapResult)) {
+                    return wouldBlockSymbol(unwrapResult);
+                }
+                if (unwrapResult == -1 && handshakeStatus != SSLEngineResult.HandshakeStatus.FINISHED) {
                     throw new SSLHandshakeException("Socket closed");
                 }
                 // during initialHandshake, calling readAndUnwrap that results UNDERFLOW does not mean writable.
@@ -613,7 +624,7 @@ private IRubyObject doHandshake(final boolean blocking, final boolean exception)
 
     private void doWrap(final boolean blocking) throws IOException {
         netWriteData.clear();
-        SSLEngineResult result = engine.wrap(dummy, netWriteData);
+        SSLEngineResult result = engine.wrap(EMPTY_DATA.duplicate(), netWriteData);
         netWriteData.flip();
         handshakeStatus = result.getHandshakeStatus();
         status = result.getStatus();
@@ -688,7 +699,9 @@ public int write(ByteBuffer src, boolean blocking) throws SSLException, IOExcept
             if ( netWriteData.hasRemaining() ) {
                 flushData(blocking);
             }
-            netWriteData.clear();
+            // use compact() to preserve any encrypted bytes that flushData could not send (non-blocking partial write)
+            // clear() would discard them, corrupting the TLS record stream:
+            netWriteData.compact();
             final SSLEngineResult result = engine.wrap(src, netWriteData);
             if ( result.getStatus() == SSLEngineResult.Status.CLOSED ) {
                 throw getRuntime().newIOError("closed SSL engine");
@@ -703,12 +716,16 @@ public int write(ByteBuffer src, boolean blocking) throws SSLException, IOExcept
     }
 
     public int read(final ByteBuffer dst, final boolean blocking) throws IOException {
+        return read(dst, blocking, true);
+    }
+
+    private int read(final ByteBuffer dst, final boolean blocking, final boolean exception) throws IOException {
         if ( initialHandshake ) return 0;
         if ( engine.isInboundDone() ) return -1;
 
         if ( ! appReadData.hasRemaining() ) {
-            int appBytesProduced = readAndUnwrap(blocking);
-            if (appBytesProduced == -1 || appBytesProduced == 0) {
+            final int appBytesProduced = readAndUnwrap(blocking, exception);
+            if (appBytesProduced == -1 || appBytesProduced == 0 || isWouldBlockResult(appBytesProduced)) {
                 return appBytesProduced;
             }
         }
@@ -718,7 +735,15 @@ public int read(final ByteBuffer dst, final boolean blocking) throws IOException
         return limit;
     }
 
-    private int readAndUnwrap(final boolean blocking) throws IOException {
+    /**
+     * @param blocking whether to block on I/O
+     * @param exception when false, returns {@link #READ_WOULD_BLOCK_RESULT} or
+     *                  {@link #WRITE_WOULD_BLOCK_RESULT} instead of throwing if the
+     *                  post-handshake processing would block
+     * @return application bytes available, -1 on EOF/close, 0 when no app data
+     *         produced, or a WOULD_BLOCK sentinel when would-block with exception=false
+     */
+    private int readAndUnwrap(final boolean blocking, final boolean exception) throws IOException {
         final int bytesRead = socketChannelImpl().read(netReadData);
         if ( bytesRead == -1 ) {
             if ( ! netReadData.hasRemaining() ||
@@ -767,7 +792,11 @@ private int readAndUnwrap(final boolean blocking) throws IOException {
                 handshakeStatus == SSLEngineResult.HandshakeStatus.NEED_TASK ||
                 handshakeStatus == SSLEngineResult.HandshakeStatus.NEED_WRAP ||
                 handshakeStatus == SSLEngineResult.HandshakeStatus.FINISHED ) ) {
-            doHandshake(blocking);
+            IRubyObject ex = doHandshake(blocking, exception);
+            if ( ex != null ) { // :wait_readable | :wait_writable
+                // TODO needs refactoring to avoid Symbol -> int -> Symbol
+                return "wait_writable".equals(ex.asJavaString()) ? WRITE_WOULD_BLOCK_RESULT : READ_WOULD_BLOCK_RESULT;
+            }
         }
         return appReadData.remaining();
     }
@@ -792,7 +821,7 @@ private void doShutdown() throws IOException {
         }
         netWriteData.clear();
         try {
-            engine.wrap(dummy, netWriteData); // send close (after sslEngine.closeOutbound)
+            engine.wrap(EMPTY_DATA.duplicate(), netWriteData); // send close (after sslEngine.closeOutbound)
         }
         catch (SSLException e) {
             debug(getRuntime(), "SSLSocket.doShutdown", e);
@@ -830,6 +859,14 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l
         }
 
         try {
+            // Flush any pending encrypted write data before reading.
+            // After write_nonblock, encrypted bytes may remain in netWriteData that haven't been sent to the server.
+            // If we read without flushing, the server may not have received the complete request
+            // (e.g. net/http POST body) and will not send a response.
+            if ( engine != null && netWriteData.hasRemaining() ) {
+                flushData(blocking);
+            }
+
             // So we need to make sure to only block when there is no data left to process
             if ( engine == null || ! ( appReadData.hasRemaining() || netReadData.position() > 0 ) ) {
                 final Object ex = waitSelect(SelectionKey.OP_READ, blocking, exception);
@@ -843,19 +880,28 @@ private IRubyObject sysreadImpl(final ThreadContext context, final IRubyObject l
                 if ( engine == null ) {
                     read = socketChannelImpl().read(dst);
                 } else {
-                    read = read(dst, blocking);
+                    read = read(dst, blocking, exception);
                 }
 
-                if ( read == -1 ) {
-                    if ( exception ) throw runtime.newEOFError();
-                    return context.nil;
+                switch ( read ) {
+                    case -1 :
+                        if ( exception ) throw runtime.newEOFError();
+                        return context.nil;
+                    // Post-handshake processing (e.g. TLS 1.3 NewSessionTicket) signaled would-block
+                    case READ_WOULD_BLOCK_RESULT :
+                        return runtime.newSymbol("wait_readable");
+                    case WRITE_WOULD_BLOCK_RESULT :
+                        return runtime.newSymbol("wait_writable");
                 }
 
-                if ( read == 0 && status == SSLEngineResult.Status.BUFFER_UNDERFLOW ) {
-                    // If we didn't get any data back because we only read in a partial TLS record,
-                    // instead of spinning until the rest comes in, call waitSelect to either block
-                    // until the rest is available, or throw a "read would block" error if we are in
-                    // non-blocking mode.
+                if ( read == 0 && netReadData.position() == 0 ) {
+                    // If we didn't get any data back and there is no buffered network data left to process,
+                    // wait for more data from the network instead of spinning until it arrives.
+                    // In blocking mode this blocks; in non-blocking mode it raises/returns "read would block".
+                    //
+                    // We check netReadData.position() rather than status == BUFFER_UNDERFLOW because readAndUnwrap
+                    // may have successfully consumed a non-application record (e.g. a TLS 1.3 NewSessionTicket)
+                    // leaving status == OK with zero app bytes produced and nothing left in the network buffer.
                     final Object ex = waitSelect(SelectionKey.OP_READ, blocking, exception);
                     if ( ex instanceof IRubyObject ) return (IRubyObject) ex; // :wait_readable
                 }

src/test/java/org/jruby/ext/openssl/OpenSSLHelper.java

@@ -0,0 +1,70 @@
+package org.jruby.ext.openssl;
+
+import org.jruby.Ruby;
+import org.jruby.runtime.ThreadContext;
+
+import java.io.ByteArrayOutputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.InputStream;
+import java.nio.charset.StandardCharsets;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;
+
+abstract class OpenSSLHelper {
+
+    protected Ruby runtime;
+
+    void setUpRuntime() throws ClassNotFoundException {
+        runtime = Ruby.newInstance();
+        loadOpenSSL(runtime);
+    }
+
+    void tearDownRuntime() {
+        if (runtime != null) runtime.tearDown(false);
+    }
+
+    protected void loadOpenSSL(final Ruby runtime) throws ClassNotFoundException {
+        // prepend lib/ so openssl.rb + jopenssl/ are loaded instead of bundled OpenSSL in jruby-stdlib
+        final String libDir = new File("lib").getAbsolutePath();
+        runtime.evalScriptlet("$LOAD_PATH.unshift '" + libDir + "'");
+        runtime.evalScriptlet("require 'openssl'");
+
+        // sanity: verify openssl was loaded from the project, not jruby-stdlib :
+        final String versionFile = new File(libDir, "jopenssl/version.rb").getAbsolutePath();
+        final String expectedVersion = runtime.evalScriptlet(
+                "File.read('" + versionFile + "').match( /.*\\sVERSION\\s*=\\s*['\"](.*)['\"]/ )[1]")
+                .toString();
+        final String loadedVersion = runtime.evalScriptlet("JOpenSSL::VERSION").toString();
+        assertEquals("OpenSSL must be loaded from project (got version " + loadedVersion +
+                "), not from jruby-stdlib", expectedVersion, loadedVersion);
+
+        // Also check the Java extension classes were resolved from the project, not jruby-stdlib :
+        final String classOrigin = runtime.getJRubyClassLoader()
+                .loadClass("org.jruby.ext.openssl.OpenSSL")
+                .getProtectionDomain().getCodeSource().getLocation().toString();
+        assertTrue("OpenSSL.class (via JRuby classloader) come from project, got: " + classOrigin,
+                classOrigin.endsWith("/pkg/classes/"));
+    }
+
+    // HELPERS
+
+    public ThreadContext currentContext() {
+        return runtime.getCurrentContext();
+    }
+
+    public static String readResource(final String resource) {
+        int n;
+        try (InputStream in = SSLSocketTest.class.getResourceAsStream(resource)) {
+            if (in == null) throw new IllegalArgumentException(resource + " not found on classpath");
+
+            ByteArrayOutputStream out = new ByteArrayOutputStream();
+            byte[] buf = new byte[8192];
+            while ((n = in.read(buf)) != -1) out.write(buf, 0, n);
+            return new String(out.toByteArray(), StandardCharsets.UTF_8);
+        } catch (IOException e) {
+            throw new IllegalStateException("failed to load" + resource, e);
+        }
+    }
+}