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

Update to jakarta.mail-api 2.1.0 #496

Conversation

adamretter
Copy link
Contributor

@adamretter adamretter commented Oct 23, 2022

This updates to jakarta mail api 2.1.0 and Eclipse Angus as the new API implementation.

However this is not yet working... see eclipse-ee4j/angus-mail#37

@adamretter
Copy link
Contributor Author

The upshot is that this change requires Java 11 at runtime.

@marcelmay are there any plans to move to Java 11?

@marcelmay
Copy link
Member

Sorry @adamretter for the late reply.

I will roll a 2.x alpha 3 in the next few days, and then make Java 11 a requirement for GreenMail 2.x .

Copy link
Contributor

@mabartos mabartos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @adamretter! What's the current progress on this? :))

pom.xml Outdated
<dependency>
<groupId>org.eclipse.angus</groupId>
<artifactId>angus-activation</artifactId>
<version>1.0.0</version>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<version>1.0.0</version>
<version>2.0.0</version>

As @lukasj suggested here: eclipse-ee4j/angus-mail#37 (comment)

This was referenced Jan 18, 2023
@adamretter
Copy link
Contributor Author

@mabartos No progress on my part. Is something needed?

@adamretter
Copy link
Contributor Author

adamretter commented Feb 5, 2023

@mabartos @marcelmay I have now updated this for Eclipse Angus Mail 2.0.1 with is the Jakarta EE implementation of Jakarta Mail. However I am now seeing a couple of failing tests like:

Caused by: java.io.IOException: Can't verify identity of server: 127.0.0.1
	at org.eclipse.angus.mail.util.SocketFetcher.checkServerIdentity(SocketFetcher.java:699)
	at org.eclipse.angus.mail.util.SocketFetcher.configureSSLSocket(SocketFetcher.java:636)
	at org.eclipse.angus.mail.util.SocketFetcher.createSocket(SocketFetcher.java:402)
	at org.eclipse.angus.mail.util.SocketFetcher.getSocket(SocketFetcher.java:215)
	at org.eclipse.angus.mail.smtp.SMTPTransport.openServer(SMTPTransport.java:2208)

I am not sure how GreenMail sets up its SSL identity for the host... any pointers?

@mabartos
Copy link
Contributor

mabartos commented Feb 6, 2023

@adamretter Have you tried to execute these tests even before the upgrade? AFAIK, it's not possible to execute tests with TLS on a local machine. As in the code, there are no certs or other stuff required for the TLS establishment.

@adamretter
Copy link
Contributor Author

@mabartos Running mvn clean verify on master seems to show that all tests pass (before my changes).

@marcelmay
Copy link
Member

marcelmay commented Feb 6, 2023

@adamretter , @mabartos - I can have a look on the SSL issue this weekend.
GreenMail uses tweaks for local secure connections.

Configuring GreenMails DummySSLSocketFactory might do the trick, see socket fetch properties:
https://github.com/javaee/javamail/blob/master/mail/src/main/java/com/sun/mail/util/SocketFetcher.java#L87

@marcelmay
Copy link
Member

marcelmay commented Feb 12, 2023

It seems Angus Mail enables mail.<PROTOCOL>.ssl.checkserveridentity by default :
eclipse-ee4j/angus-mail#12

This breaks with JavaMail, as the default was always false:
From https://jakarta.ee/specifications/mail/1.6/apidocs/com/sun/mail/smtp/package-summary.html :

mail.smtp.ssl.checkserveridentity boolean If set to true, check the server identity as specified by RFC 2595. These additional checks based on the content of the server's certificate are intended to prevent man-in-the-middle attacks. Defaults to false.

To fix this, you'll need to set the property to false in GreenMail ServerSetup.java:

        if (isSecure()) {
            ....
            // Required for Angus Mail
            props.setProperty(MAIL_DOT + getProtocol() + ".ssl.checkserveridentity", "false");
        }

@adamretter
Copy link
Contributor Author

@marcelmay Awesome thanks! I have just added that change now :-)

@adamretter adamretter marked this pull request as ready for review February 14, 2023 11:14
@adamretter
Copy link
Contributor Author

@marcelmay would you be able to kick off the CI for this?

@adamretter
Copy link
Contributor Author

@marcelmay Thanks for kicking off the CI. I just wondered if you perhaps yet had any idea of a timeline for this change to be included and released in a version of GreenMail?

@marcelmay
Copy link
Member

I hope to roll 2.0 this weekend, and would also do an initial 2.1 "alpha" (=2.0 + this PR) then.

@marcelmay
Copy link
Member

GreenMail 2.0 is out, will do 2.1.0-alpha-1 including this PR next at the beginning of this week.

@marcelmay marcelmay merged commit ebbcbcf into greenmail-mail-test:master Mar 6, 2023
@marcelmay marcelmay added breaking change dependencies Pull requests that update a dependency file labels Mar 6, 2023
@marcelmay marcelmay added this to the 2.1.0-alpha-1 milestone Mar 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants