-
Notifications
You must be signed in to change notification settings - Fork 501
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MailTest Fix #11319
MailTest Fix #11319
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests in maven_unit_test.yml are passing now. Nice comment too. Thanks, @qqmyers! Approved! 🚀
It is not necessary to use 4096 bit, as the error was about the wrong common name in the cert. Detailed explanation: Before, the cert presented to Jakarta Mail neither contained a commonName=localhost nor subjectAltName=localhost. This worked before with Payara 6.2024.6 because Eclipse Angus Mail 1.0.0 was still in use (see list of managed dependencies on the 6.2024.6 BOM). Certificate host identity check was introduced as default=on with Eclipse Angus Mail 1.1.0 (see their release notes). Now that Payara 6.2025.2 is in use on develop, we upgraded to Eclipse Angus Mail 2.3.0, making the error appear.
Including an openssl.conf makes it much easier and less error prone to generate the certs. Rewriting the necessary shell command steps in the test comment to document the process.
Without the explicit addition of which port to use for the HTTP based waiting strategy, someone may end up at random with the SMTP port being picked. This will obviously fail and result in aborted tests, making results flaky. Pinning the port down should prevent this for good.
@qqmyers thanks for looking into this! You sparked my interest on a late evening to look into this in my spare time. I was very curious why the RSA key length would be the crucial thing to fix here, so I tried to find changes in Eclipse Angus Mail to corroborate that. Turns out when I added the certificate back in the day I made a rookie mistake by using the wrong In the same go I fixed the problem of Testcontainers waiting on the wrong port which was a bump in the road for me. Hopefully now this won't affect anyone else. |
Tests Passed - Merging PR. |
What this PR does / why we need it: Somewhere in the update it looks like the test certificate is now being checked to make sure the common name matches the host. In the test, that is "localhost". The original cert had "example.org". This PR creates a new cert/key pair that has localhost as the common name and uses a longer key (based off an earlier guess that short keys might also cause failures in the next 10 years of the cert's life.
Which issue(s) this PR closes:
Special notes for your reviewer:
Suggestions on how to test this: Verify the test passes
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
Is there a release notes update needed for this change?:
Additional documentation: