Add support for PEM files

[hgeraldino]

Proposed Changes

Adds support for PEM files, as requested in #1837

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bugfix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)
• Documentation (correction or otherwise)
• Cosmetics (whitespace, appearance)

Checklist

Put an x in the boxes that apply. You can also fill these out after creating
the PR. If you're unsure about any of them, don't hesitate to ask on the
mailing list. We're here to help! This is simply a reminder of what we are
going to look for before merging your code.

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• All tests pass locally with my changes
• I have added tests that prove my fix is effective or that my feature works
• I have added necessary documentation (if appropriate)
• Any dependent changes have been merged and published in related repositories

Further Comments

Change is rather simple. It copies the PemReader (ASF licensed) from Apache ZooKeeper and extends the ConnectionFactoryConfigurator so it supports a new keystore type (PEM)

[lukebakken]

I don't understand why PemReader.java is included in this PR when, as far as I can tell, all the code we need is in TlsTestUtils.java

[acogoluegnes]

I don't understand why PemReader.java is included in this PR when, as far as I can tell, all the code we need is in TlsTestUtils.java

The PEM reader is necessary to parse the certificates and keys in the PEM, it strips up the header and footer, decode the base 64 representation, etc. This is not done in the TlsTestUtils class. The JDK handles the actual certificate/key loading.

[acogoluegnes]

@hgeraldino Thanks for the contribution. CodeQL flagged the regex from the PEM reader, so I updated them. I ran the PEM reader test suite from Zookeeper with the updated regex to make sure I did not break anything. Feel free to stash my commit.

[lukebakken]

The PEM reader is necessary to parse the certificates and keys in the PEM, it strips up the header and footer, decode the base 64 representation, etc. This is not done in the TlsTestUtils class

CertificateFactory does this, even with multiple certs in the same PEM. I will have a project demonstrating that within an hour or two.

The only reason I'm harping on this is that this PR introduces a lot of unnecessary code.

UPDATE: https://github.com/lukebakken/rabbitmq-java-client-1840

Well, I learned something - as soon as you have a private key in your file, the existing Java lib can't read it /sad trombone

Score one for public_key:pem_decode/1

[lukebakken]

Actually, should it be possible for this new code to replace / augment the utility class used in tests?

[acogoluegnes]

@lukebakken I was referring to private keys, not certificates. I'm not aware of JDK utility that can load a private key from its PEM file without the work that the PEM reader class does.

[lukebakken]

@acogoluegnes yep, I updated my comment as soon as I discovered that in my testing 😸

@hgeraldino I have taken your code and created a branch in my fork of this repository:

https://github.com/lukebakken/rmq-rabbitmq-java-client/tree/rabbitmq-java-client-1840

Here is a comparison of my branch with yours: link

Please note that your PR branch is main, which is not the best practice when opening pull requests on GitHub. If this proves to be an issue we can re-create your PR and include your commits from a topic branch.

I spent some time having the genie review your pull request, and had it create a summary document of what it found. Some of the suggestions have already been implemented (like the regex suggestion). The other suggestion about silently handling certain exceptions seems worthwhile to address.

I also integrated your changes into the existing TlsTestUtils class.

If you'd like to use my work, feel free to add my fork as an upstream, and merge/cherry-pick whatever commits you'd like into your own work. Otherwise, I'd be happy to take your contribution to conclusion. Either way, thanks a lot.

[hgeraldino]

@acogoluegnes yep, I updated my comment as soon as I discovered that in my testing 😸

@hgeraldino I have taken your code and created a branch in my fork of this repository:

https://github.com/lukebakken/rmq-rabbitmq-java-client/tree/rabbitmq-java-client-1840

Here is a comparison of my branch with yours: link

Please note that your PR branch is main, which is not the best practice when opening pull requests on GitHub. If this proves to be an issue we can re-create your PR and include your commits from a topic branch.

I spent some time having the genie review your pull request, and had it create a summary document of what it found. Some of the suggestions have already been implemented (like the regex suggestion). The other suggestion about silently handling certain exceptions seems worthwhile to address.

I also integrated your changes into the existing TlsTestUtils class.

If you'd like to use my work, feel free to add my fork as an upstream, and merge/cherry-pick whatever commits you'd like into your own work. Otherwise, I'd be happy to take your contribution to conclusion. Either way, thanks a lot.

Thanks @lukebakken

I cherrypicked your commit that adds support for PEM files to TlsTestUtils. I also adopted some of the suggestions from the AI-generated doc (some of the APIs it suggested are only available on newer JDK versions)

Please take another look when you have a chance

Thanks!

[lukebakken]

@hgeraldino just FYI I don't see any changes to TlsTestUtils in this PR 🤔

[hgeraldino]

@hgeraldino just FYI I don't see any changes to TlsTestUtils in this PR 🤔

🤦 - they're now

[michaelklishin]

I assume we want to ship this in 5.x?

[acogoluegnes]

@michaelklishin Yes, it should not be a problem to backport.

[hgeraldino]

I'd be extremely grateful if this is backported to the current release branch, so we don't have to maintain an internally patched version of the client 🙏 🙏

Thanks a lot for being so receptive :)