context_builder.rb - require openssl if verify_mode != 'none'

[MSP-Greg]

Description

Current code in MiniSSL::Socket#peercert contains the following line:

puma/lib/puma/minissl.rb

Line 194 in 188f5da

@peercert = OpenSSL::X509::Certificate.new raw

The above statement only runs when MiniSSL::Context#verify_mode is not equal to MiniSSL::VERIFY_NONE. So, we need to require openssl when needed.

Note that MiniSSL::Socket#peercert was added in PR #709 'ssl: Add Client Side Certificate Auth'. The cert subject is used in LogWriter#ssl_error, and the cert object is set to env['puma.peercert']. The app environment key doesn't seem to be 'publicly' listed.

Current tests only check this with an in-process server, and since the client sockets are ssl, openssl is loaded for the clients.

I added some additional spawned tests (Integration) that showed the problem, but that was using my updated test framework. We'll see...

Your checklist for this pull request

• I have reviewed the guidelines for contributing to this repository.
• I have added (or updated) appropriate tests if this PR fixes a bug or adds a feature.
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
• If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
• I have updated the documentation accordingly.
• All new and existing tests passed, including Rubocop.

[nateberkopec]

This seems like a weird place to put the require call, maybe I don't get it.

Why not require in #peercert?

[MSP-Greg]

Why not require in #peercert?

Don't have strong opinion, but I thought locating where it is happens on 'startup', rather than when the first request happens.

Also, in peercert, it will run on every request? Note that with VERIFY_NONE, even if a client includes a cert, MiniSSL::Engine#peercert is nil, which means that OpenSSL::X509::Certificate.new is never called.

[MSP-Greg]

@nateberkopec

Actions is kind of wacked right now.

I added comments to a few methods for this.

[dentarg]

I added some additional spawned tests (Integration) that showed the problem, but that was using my updated test framework. We'll see...

Would be nice to commit the tests with this change

[MSP-Greg]

Would be nice to commit the tests with this change

Ok, I added two test helper files from my test update.

1. test/helpers/constants.rb - this contains constants pertaining to Puma's test suite. I'd like to see test/helper.rb just contain code related to minitest. One feature is that one can swap all HOST addresses between IPv4 & IPv6 for testing. Haven't finished it yet, as it needs a way to toggle it with an ENV setting. It also checks system localhost addresses, and if they've been reconfigured, it will use those.

2. test/helpers/puma_socket.rb - this contains code to generate requests, etc. Currently, code is scattered throughout the test suite, and the methods here allow much more functionality than the current code.

The main idea is that there should be two helpers files, one based on the server related code in test/helpers/integration.rb with additions for ssl sockets, etc. Tentatively, I thought it could be called server_spawn.rb, because it handles spawning a server in a sub-process. There would be an additional file, maybe named server_in_process.rb, which would contain code to start a Puma::Server instance, similar to the code scattered through several test files. One example is test_puma_server.rb.

These two server files would share instance variable names with puma_socket.rb, allowing the test methods to no longer have deal with socket host/port values in Puma or client related code. The code does allow setting those, which could be used to test all localhost addresses, binding to more than one socket, etc.

Re this PR, if one checks out the third commit which is before any library file changes, the following is shown in the test listing:

2023-06-19 20:56:58 -0500 Read: #<NameError: uninitialized constant Puma::MiniSSL::Socket::OpenSSL>

The new test (TestIntegrationSSL#test_verify_client_cert_roundtrip) also fails.

[MSP-Greg]

I'd like to merge this, as this PR adds test/helpers/puma_socket.rb, which is a client socket lib for use in CI. Currently, client socket code is duplicated in several test files, and it causes all sorts of issues.

Once it's committed, I'll document the code in it, and it will show up in the Puma docs. I can start converting test files to use it, one PR at a time.

Many files in the test suite either create an 'in process' server via Server.new or they create a spawned server via a cli command.

The code for inproc servers is also scattered across several test files, and I'd like to consolidate that code into a file, maybe test/helpers/server_inproc.rb

Thoughts?

Off-topic There are many places in the test suite where read or gets is used on IO's. Both block, which causes issues with parallel testing.

[dentarg]

Sounds good to me

PumaSocket is a lot of code so I can't say I've studied it very well, but I trust your chops @MSP-Greg and I'm sure I will read it again in the future... :)