Move initialization of KeyManagerFactory, TrustManagerFactory to server

[JohnPhillips31416]

Move initialization of KeyManagerFactory, TrustManagerFactory to server initialization. This avoids reading the keystore file twice on every ssl request, and also fixes (#2299) a filehandle leak from reading the keystore file without closing it properly.

[MSP-Greg]

LGTM & thanks. I cancelled the CI, as truffleruby-head is failing, and freezes on macOS. All The JRuby passed with this, but I'm not a good choice for reviewing Java code...

[JohnPhillips31416]

The pr does 2 things: I close the filehandles after use, and I move the keystore operations from initialize, which is called on each ssl request, to server, which is called on add_ssl_listener. The question is whether moving the code in this way is valid for all reasonable/known use cases.

[eregon]

@MSP-Greg Could you @-mention me if you see CI issues on TruffleRuby? This particular hang should be solved in the latest nightly.

[MSP-Greg]

@eregon

Of course. It did pass the latest CI: https://github.com/puma/puma/actions/runs/159351533

[MSP-Greg]

@eregon

@JohnPhillips31416 has done a good (and appreciated) job with this.

Care to review it? Everything he's said re Puma makes sense, and you are associated with that Java group (@oracle Labs ), right? Being a Windows type, filehandle leaks are a pita. Thanks.

[eregon]

This touches the JRuby native extension, so I'm not really familiar with it.
Maybe @headius could review it?

[MSP-Greg]

@JohnPhillips31416

Got a question and a plea for help. Working on CI, and trying to get the SSL tests working on JRuby, as several are bypassed. There is a method Puma::MiniSSL::Engine#init? which is used in Puma::MiniSSL::Socket#should_drop_bytes?, and it's missing in the Java MiniSSL implementation. The C implementation:

puma/ext/puma_http11/mini_ssl.c

Lines 419 to 425 in e2b4193

	VALUE engine_init(VALUE self) {
	ms_conn* conn;
	Data_Get_Struct(self, ms_conn, conn);
	return SSL_in_init(conn->ssl) ? Qtrue : Qfalse;
	}

The man page for the OpenSSL call is https://www.openssl.org/docs/man1.1.1/man3/SSL_in_init.html, and it's used to determine the handshake status.

You may have also seen logs showing the missing method error.

I noticed several calls to engine.getHandshakeStatus() in MiniSSL.java, but that's about as far as I'd like to take it.

If you have time, could you look at adding the method?

JFYI, there are some constants missing re available SSL protocols. Ideally they would query for whether the protocol is available, but for the time being, I may add them to minissl.rb based on the current protocols available in Java. With TLSv1.3 and OpenSSL 3, that may be changing in the future...

[JohnPhillips31416]

@MSP-Greg this is actually discussed in #1125, we're running with the monkey patch described there. I can try to do a PR which encodes this logic into the minissl ruby code.

[MSP-Greg]

@JohnPhillips31416

Thanks. Not knowing Java very well, nor knowing in what way it processes a handshake, I added the following to minissl.rb in one of the JRuby conditionals:

      class Engine
        def init?
          true
        end
      end

Since JRuby SSL CI has been bypassed in the past, I've now got it enabled, and the above seems to work. But. it's logging some errors that I'm not sure should be, all related to clients using protocols that are not accepted by the server.

Any idea if this is a really bad idea?

[JohnPhillips31416]

@MSP-Greg seems reasonable.

[headius]

The Java patch looks ok to me. JRuby only supports Java 8 or higher, so you could use the "try-with-resources" syntax to safely handle those FileInputStream:

try (FileInputStream fis = new FileInputStream(...)) {
  // code code code
} // fis will be closed automatically without an explicit finally block

[JohnPhillips31416]

@headius unfortunately Puma is built with Java 1.5 syntax

[headius]

@JohnPhillips31416 So let's change that?

diff --git a/Rakefile b/Rakefile
index 231be50b..c839b013 100644
--- a/Rakefile
+++ b/Rakefile
@@ -49,6 +49,8 @@ else
   # Java (JRuby)
   Rake::JavaExtensionTask.new("puma_http11", gemspec) do |ext|
     ext.lib_dir = "lib/puma"
+    ext.source_version = "1.7"
+    ext.target_version = "1.7"
   end
 end

All supported versions of JRuby require Java 8 or higher. JRuby 9.0, released 5 years ago, required Java 7.

You have to go back to the long-unsupported JRuby 1.7.x to get Java 6 support, and since those versions only support Ruby 1.9.3 features I doubt Puma would even still work.

rake-compiler really should default to 1.8 but I suppose 1.7 is fine for any 9.0.x stragglers.

[headius]

PR to update rake-compiler to 1.7: rake-compiler/rake-compiler#172

[nateberkopec]

I think with the rake-compiler change merged, this is OK to merge I believe?

[MSP-Greg]

PR #2355 updates rake-compiler to "~> 1.1.1" in Gemfile. 1.1.1 includes the fix mentioned above by @headius, so this is ready to merge?

[JohnPhillips31416]

LGTM

[nateberkopec]

Thanks for all the help everyone!