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

Add config support for 'verifyingServerIdentity' with SMTP, also: since Angus 1.1.0 (8.6.0) server identity checks are on by default and can be countered by mailerBuilder.verifyingServerIdentity(false) #495

Closed
narpetri opened this issue Feb 13, 2024 · 12 comments
Labels
dependencies Pull requests that update a dependency file maintenance Priority-High
Milestone

Comments

@narpetri
Copy link

Starting from version 8.6.0 everything stopped working.

My config (Kotlin):

    private fun connectMailBuilder() {
        mailBuilder = MailerBuilder
            .withSMTPServer(
                System.getenv("POSTFIX_HOST"),
                25,
                System.getenv("POSTFIX_USER"),
                System.getenv("POSTFIX_PASSWORD")
            )
            .withProperty("mail.smtp.ssl.enable", false)
            .withDebugLogging(false)
            .async()

        pooledMailer = mailBuilder
            .withConnectionPoolCoreSize(1)
            .withConnectionPoolMaxSize(2)
            .withConnectionPoolClaimTimeoutMillis(1)
            .withConnectionPoolExpireAfterMillis(59000)
            .withTransportModeLoggingOnly(false)
            .buildMailer()

        connect.set(true)
    }

When trying to connect I started getting this error:

Exception in thread "pool-1-thread-1" org.simplejavamail.smtpconnectionpool.TransportHandlingException: Error when trying to open connection to the server, session:
	{mail.smtp.user=utp1-postfix, mail.smtp.ssl.trust=*, mail.smtp.port=25, mail.smtp.auth=true, mail.smtp.starttls.enable=true, mail.smtp.host=00.000.000.000, simplejavamail.transportstrategy=SMTP, mail.smtp.ssl.enable=false, mail.smtp.connectiontimeout=60000, mail.transport.protocol=smtp, mail.smtp.writetimeout=60000, mail.smtp.starttls.required=false, mail.smtp.timeout=60000, SESSION_BASED_EMAIL_TO_MIME_MESSAGE_CONVERTER_KEY=SessionBasedEmailToMimeMessageConverter(session=jakarta.mail.Session@592a514a, operationalConfig=OperationalConfigImpl(async=true, properties={mail.smtp.ssl.enable=false}, sessionTimeout=60000, threadPoolSize=10, threadPoolKeepAliveTime=1, clusterKey=70f8eadc-ebaa-4bee-a059-c7c69049f7d9, connectionPoolCoreSize=1, connectionPoolMaxSize=2, connectionPoolClaimTimeoutMillis=1, connectionPoolExpireAfterMillis=59000, connectionPoolLoadBalancingStrategy=ROUND_ROBIN, transportModeLoggingOnly=false, debugLogging=false, disableAllClientValidation=false, sslHostsToTrust=[], trustAllSSLHost=true, verifyingServerIdentity=true, executorService=org.simplejavamail.internal.batchsupport.concurrent.NonJvmBlockingThreadPoolExecutor@5c29bb95[Running, pool size = 0, active threads = 0, queued tasks = 0, completed tasks = 0], executorServiceIsUserProvided=false, customMailer=null), emailGovernance=EmailGovernanceImpl(emailValidator=EmailValidator[validationRuleCount=3], emailDefaults=Email{
	id=null
	sentDate=null
	fromRecipient=null,
	replyToRecipients=[],
	bounceToRecipient=null,
	text='null',
	textHTML='null',
	textCalendar='null (method: null)',
	contentTransferEncoding='quoted-printable',
	subject='null',
	recipients=[]
}, emailOverrides=Email{
	id=null
	sentDate=null
	fromRecipient=null,
	replyToRecipients=[],
	bounceToRecipient=null,
	text='null',
	textHTML='null',
	textCalendar='null (method: null)',
	contentTransferEncoding='quoted-printable',
	subject='null',
	recipients=[]
}, maximumEmailSize=null))}
	at org.simplejavamail.smtpconnectionpool.TransportAllocator.connectTransport(TransportAllocator.java:73)
	at org.simplejavamail.smtpconnectionpool.TransportAllocator.allocate(TransportAllocator.java:45)
	at org.simplejavamail.smtpconnectionpool.TransportAllocator.allocate(TransportAllocator.java:28)
	at org.bbottema.genericobjectpool.GenericObjectPool$AutoAllocatorDeallocator.allocatedCorePoolAndDeallocateOneOrPlanDeallocations(GenericObjectPool.java:266)
	at org.bbottema.genericobjectpool.GenericObjectPool$AutoAllocatorDeallocator.run(GenericObjectPool.java:256)
	at java.base/java.lang.Thread.run(Thread.java:840)
Caused by: jakarta.mail.MessagingException: Could not convert socket to TLS;
  nested exception is:
	java.io.IOException: Can't verify identity of server: 00.000.000.000
	at org.eclipse.angus.mail.smtp.SMTPTransport.startTLS(SMTPTransport.java:2173)
	at org.eclipse.angus.mail.smtp.SMTPTransport.protocolConnect(SMTPTransport.java:741)
	at jakarta.mail.Service.connect(Service.java:367)
	at jakarta.mail.Service.connect(Service.java:225)
	at jakarta.mail.Service.connect(Service.java:174)
	at org.simplejavamail.smtpconnectionpool.TransportAllocator.connectTransport(TransportAllocator.java:70)
	... 5 more
Caused by: java.io.IOException: Can't verify identity of server: 00.000.000.000
	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.startTLS(SocketFetcher.java:555)
	at org.eclipse.angus.mail.smtp.SMTPTransport.startTLS(SMTPTransport.java:2168)
	... 10 more

Before this everything worked without errors. I deliberately replaced the IP address with zeros - that’s my real address.

What has changed with the 8.6.0 release, and what configurations need to be changed to continue to benefit from the latest updates and improvements?

Thank you!

@bbottema
Copy link
Owner

This is unexpected. 8.6 is the switch to Angus Mail, which is the successor to Jakarta Mail. Angus splits up the interface from the implementation, effectively providing a reference implementation. However, supposedly, they haven't changed anything.

I'll have to dig through the Angus changes to see what they did.

Just for my sake, can you please, right after it failed, try with a 8.5 version you think should be working and see if that is the case? I just want to rule out a server side issue.

@narpetri
Copy link
Author

Of course, I would not write something without being sure of it. I switch to version 8.5.1 (or 8.4) - everything works right away. I return 8.6.0 or 8.6.3 - everything immediately crashes with the error that I published.

If you need any more information, I will be happy to help (as far as my competence is sufficient).

@bbottema
Copy link
Owner

bbottema commented Feb 15, 2024

What happens if you ditch the line .withProperty("mail.smtp.ssl.enable", false) and replace if with .withTransportStrategy() and try with each of the strategies? Also try with TransportStrategy.SMTP, first disabling opportunistic TLS like so:

TransportStrategy.SMTP.setOpportunisticTLS(false);

@narpetri
Copy link
Author

I tried dropping the .withProperty("mail.smtp.ssl.enable", false) line and ran 5 tests:

  1. Without indicating anything at all;
  2. .withTransportStrategy(TransportStrategy.SMTP_OAUTH2)
  3. .withTransportStrategy(TransportStrategy.SMTP_TLS)
  4. .withTransportStrategy(TransportStrategy.SMTPS)
  5. .withTransportStrategy(TransportStrategy.SMTP)

None of the options gave a positive result.

Then I tried disabling opportunistic TLS and setting the SMTP strategy - it worked. Working option:

    private fun connectMailBuilder() {
        TransportStrategy.SMTP.setOpportunisticTLS(false)

        mailBuilder = MailerBuilder
            .withSMTPServer(
                System.getenv("POSTFIX_HOST"),
                25,
                System.getenv("POSTFIX_USER"),
                System.getenv("POSTFIX_PASSWORD")
            )
            .withTransportStrategy(TransportStrategy.SMTP)
            .withDebugLogging(false)
            .async()

        pooledMailer = mailBuilder
            .withConnectionPoolCoreSize(1)
            .withConnectionPoolMaxSize(2)
            .withConnectionPoolClaimTimeoutMillis(1)
            .withConnectionPoolExpireAfterMillis(59000)
            .withTransportModeLoggingOnly(false)
            .buildMailer()

        connect.set(true)
    }

@bbottema
Copy link
Owner

It seems this is the appropriate way of connecting to the server, then. Classically secured, I would say. Probably privately managed? Or within demilitarized zone? Or do you disagree with this connection configuration?

I generally discourage using custom properties, since that is what causes these kind of issues when a library version changes. Better depend on one of the transport strategies instead.

@narpetri
Copy link
Author

This solved my problem and this option suits me. Thanks for your work!

@ztyzbb
Copy link

ztyzbb commented Apr 3, 2024

@bbottema Please check this: Angus Mail changelog.
Angus Mail check server identity by default after 1.1.0 version, Jakarta Mail never.
Code difference in there source:

// com.sun.mail.util.SocketFetcher#configureSSLSocket
boolean idCheck = PropUtil.getBooleanProperty(props, prefix + ".ssl.checkserveridentity", false);
// org.eclipse.angus.mail.util.SocketFetcher#configureSSLSocket
boolean idCheck = PropUtil.getBooleanProperty(props, prefix + ".ssl.checkserveridentity", true);

The difference cause Simple Java Mail can not connect to server with invalid certificates even with TransportStrategy.SMTP set.

@bbottema bbottema reopened this Apr 4, 2024
@bbottema bbottema added Priority-High maintenance dependencies Pull requests that update a dependency file and removed invalid labels Apr 4, 2024
@bbottema
Copy link
Owner

bbottema commented Apr 5, 2024

Angus 1.1.0 was released in December 30, 2022. We switched to Angus on Jan 17, 2024 with the release of 8.6.0.

The difference cause Simple Java Mail can not connect to server with invalid certificates even with TransportStrategy.SMTP set.

Correction: only when TransportStrategy.SMTP is set. I just checked the code and we always explicitly configure *.ssl.checkserveridentity for all transport strategies, except SMTP.

// MailerImpl.java

	static private void configureServerIdentityVerification(@NotNull final Session session, @NotNull final OperationalConfig operationalConfig, @Nullable final TransportStrategy transportStrategy) {
		if (transportStrategy != null && transportStrategy != TransportStrategy.SMTP) {
			session.getProperties().setProperty(transportStrategy.propertyNameCheckServerIdentity(),
					Boolean.toString(operationalConfig.isVerifyingServerIdentity()));
		}
	}

As for why we don't set this for SMTP, I'm not sure:

// TransportStrategy.SMTP

		/**
		 * Always throws an exception, as this property is not relevant for plain SMTP.
		 */
		@Override
		public String propertyNameCheckServerIdentity() {
			throw new IllegalStateException("This property is not relevant for plain SMTP");
		}

I'm not entirely sure yet, but I think it's because of the opportunisticTLS which tries to upgrade the connection, in which case "*.ssl.checkserveridentity" becomes relevant again and that's why it fails for @narpetri unless opportunisticTLS is disabled manyally... I think. @ztyzbb

bbottema added a commit that referenced this issue Apr 5, 2024
…s as well, as is it still relevant for opportunistic TLS upgrades.
@bbottema bbottema changed the title Stopped connecting to the mail server Add config support for 'verifyingServerIdentity' with SMTP, also: since Angus 1.1.0 server identity checks are on by default and can be countered by mailerBuilder.verifyingServerIdentity(false) Apr 5, 2024
@bbottema bbottema added this to the 8.8.2 milestone Apr 5, 2024
@bbottema bbottema changed the title Add config support for 'verifyingServerIdentity' with SMTP, also: since Angus 1.1.0 server identity checks are on by default and can be countered by mailerBuilder.verifyingServerIdentity(false) Add config support for 'verifyingServerIdentity' with SMTP, also: since Angus 1.1.0 (8.6.0) server identity checks are on by default and can be countered by mailerBuilder.verifyingServerIdentity(false) Apr 5, 2024
@bbottema
Copy link
Owner

bbottema commented Apr 5, 2024

So, @narpetri, you can already revert the workaround of TransportStrategy.SMTP.setOpportunisticTLS(false) and use one of the other transport strategies as long as you also set mailerBuilder.verifyingServerIdentity(false). I've now release a fix in 8.8.2, so that doing this also works for the SMTP transport strategy (previously you would get an error saying it makes no sense).

And @ztyzbb, thanks you so much for bringing this to my attention!

@bbottema bbottema closed this as completed Apr 5, 2024
@ztyzbb
Copy link

ztyzbb commented Apr 10, 2024

@bbottema From web site and javadoc, when use TransportStrategy.SMTP, server certificates are not validated in any way. But after Angus 1.1.0's change, server identity checks are on by default, so we can't connect to server with invalid certificates unless set mailerBuilder.verifyingServerIdentity(false) explicitly. We should set it internally when use TransportStrategy.SMTP to match the doc.

@bbottema
Copy link
Owner

bbottema commented Apr 10, 2024

To be clear: Simple Java Mail already has had this feature turned on for all transport strategies except SMTP. Due to historical reasons, for SMTP this feature was not enabled because it was not relevant until opportunistic TLS was implemented for #105. Now with the switch to Angus, for SMTP too, this feature is now enabled by default.

@ztyzbb, I understand this breaks backwards compatibility, but I think it's the right choice. If you want degraded security on the server, that's totally up to you and you can do so with mailerBuilder.verifyingServerIdentity(false), but the default mode for the client should be to want to verify the server identity.

I already documented this in the RELEASE notes, but I realize I didn't update the documentation on the website. I will also update the JavaDoc. Thank you for reminding me. (edit: I have now updated documentation on these issues).

@ztyzbb
Copy link

ztyzbb commented Apr 10, 2024

Yeah, you are right, verify the server identity by default is better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file maintenance Priority-High
Projects
None yet
Development

No branches or pull requests

3 participants