support full certificate chain PEM for cert_pem: parameter to ssl_bind

[copiousfreetime]

Description

Bring the cert_pem: parameter into parity with the cert: parameter to ssl_bind.

Currently the cert: parameter allows for a full certificate chain file to be used, and the cert_pem: only allows a certificate. This patch allows the cert_pem: parameter to be be a full certificate chain String.

See #3172 for an in depth discussion of the issue.

Closes #3172

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.
  • I need guidance on how to add a test for this
• My pull request is 100 lines added/removed or less so that it can be easily reviewed.
• 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.

[MSP-Greg]

@copiousfreetime

Thanks for working on this. Not sure if you've thought about tests. I've got two commits on https://github.com/MSP-Greg/puma/commits/00-3174, I believe they add what's needed. Thoughts?

EDIT: I just noticed "I need guidance on how to add a test for this". Added the two commits, one adds files, the other adds tests.

The tests load the the files in four different ways, connect an SSLSocket, then check what certs it shows from the server (peer).

[MSP-Greg]

Re the effect of this PR, after cherry-picking the two last commits (which only have test changes) onto master, then running test_puma_server_ssl.rb, the following error occurs:

  1) Failure:
TestPumaSSLCertChain#test_chain_cert_string_without_ca [/puma/test/test_puma_server_ssl.rb:583]:
--- expected
+++ actual
@@ -1 +1 @@
-["anchor.lcl.host", "SubCA", "AnchorCA"]
+["anchor.lcl.host"]

[copiousfreetime]

Thanks for those tests! That's the result I was looking for. And that's what I would expect on master, that's what I was seeing when doing the commandline testing.

One thing we probably want to do is use different certs. Those anchor from my example are set to expire in 1 year, so probably not good for test data. We'll have some unexpected test failures next June - which will make everyone unhappy.

I'll take a look at getting better test data, or generating it.

[MSP-Greg]

Glad those helped. I didn't want to assume that an openssl exe was in PATH, so I used Ruby SSLSockets.

I like to point out the failures/errors in master, helps when one has totally forgotten what change/feature the PR adds.

Those anchor from my example are set to expire in 1 year

Glad you remembered that, I didn't look.

I'll take a look at getting better test data, or generating it.

Thanks. If you could, adding the code to generate them in the comments/notes is helpful.

[copiousfreetime]

@MSP-Greg I think we're all good - added generate_chain_test.rb and brought all the naming conventions in line with the client_certs directory. This does add the certificate_authority gem as development dependency for help in generating the intermediate signing cert. That gem might be useful elsewhere for development too.

I also addedtest_example_cert_expiration.rb that checks the expiration dates of all the certs in the repo that I could find. So we should get a 30 day notice from CI if the certs are about to expire.

This test did find that the certs in examples/CA all expired a while ago. And I wasn't able to find if they are actually used for something. This might be a cleanup for another PR, but thought I would mention it here.

[dentarg]

This does add the certificate_authority gem as development dependency for help in generating the intermediate signing cert. That gem might be useful elsewhere for development too.

If we only use that gem in generate_chain_test.rb we can do an inline gemfile in that script instead of adding it to the general Gemfile. It is an awesome and probably underused feature of Bundler :)

[copiousfreetime]

This does add the certificate_authority gem as development dependency for help in generating the intermediate signing cert. That gem might be useful elsewhere for development too.

If we only use that gem in generate_chain_test.rb we can do an inline gemfile in that script instead of adding it to the general Gemfile. It is an awesome and probably underused feature of Bundler :)

Good call - I'll make that change - I use inline bundler all the time.

[MSP-Greg]

@nateberkopec

A few days ago I added a 'feature' label. Now, I'm not sure if it should be 'feature' or 'bug'. I'm now thinking it should be 'bug'.

Currently, when using DSL#ssl_bind, the following two code lines should be equivalent if the cert file contains multiple certs, but they are not, as cert_pem fails, since it only reads the first cert (example of CI failure above):

cert_pem: path_to_cert
cert_pem: File.read(path_to_cert)

This PR aligns the functionality of the two.

Thoughts?