Skip to content
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

Merged
Merged

Conversation

qqmyers
Copy link
Member

@qqmyers qqmyers commented Mar 7, 2025

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:

@qqmyers qqmyers changed the title MailTest: try longer key MailTest Fix Mar 7, 2025
@qqmyers qqmyers marked this pull request as ready for review March 7, 2025 21:21
Copy link
Member

@pdurbin pdurbin left a 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! 🚀

@pdurbin pdurbin added this to the 6.6 milestone Mar 7, 2025
@ofahimIQSS ofahimIQSS self-assigned this Mar 7, 2025
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.
@poikilotherm
Copy link
Contributor

poikilotherm commented Mar 7, 2025

@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 CN 🙄 So I went ahead and fixed that, also adding an OpenSSL config file to make the creation process less manual. I hope you don't mind me being a bit pushy with forcing the changes into your branch.

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.

@ofahimIQSS
Copy link
Contributor

Tests Passed - Merging PR.

@ofahimIQSS ofahimIQSS merged commit 0af33b8 into IQSS:develop Mar 9, 2025
11 checks passed
@ofahimIQSS ofahimIQSS removed their assignment Mar 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Merged 🚀
Development

Successfully merging this pull request may close these issues.

Integration Test Failure After Payara Upgrade (Stable / JDK 17) - MailSessionProducerIT
4 participants