Illegal reflective access by com.sun.mail.util.SocketFetcher #124

[jmehrens]

Co-authored-by: jmehrens [email protected]
Co-authored-by: icu5545 [email protected]
Signed-off-by: jmehrens [email protected]

[jmehrens]

Summary of changes:

1. Modified .ssl.checkserveridentity property to set the SSLParameters.setEndpointIdentificationAlgorithm as this is the recommended replacement sun.security.util.HostnameChecker. The LDAPS algorithm is used because that is compatible with what was done in previous releases of Angus Mail. Reading the OpenJDK source this eventually ends up in a call to the sun.security.util.HostnameChecker via the installed TrustManager during the call to startHandshake.
2. Added ssl.hostnameverifier.class property that is a factory for hostname verifier objects with public default constructors. If not null, this validation is performed regardless of state of the ssl.checkserveridentity property. Normal deny or exceptional failure will close the connection.
3. Added .ssl.hostnameverifier property for verifier objects that don't have a default constructor. If non-null it is used instead of ssl.hostnameverifier.class property and validation is performed regardless of state of the ssl.checkserveridentity property. Normal deny or exceptional failure will close the connection.
4. If .ssl.checkserveridentity is true and ssl.hostnameverifier.class or .ssl.hostnameverifier is non-null the hostname is verified as the logical AND between the two. The values of false and null simply mean that the property is not participating in verifying the connection.
5. Moved the reflective access of the sun.security.util.HostnameChecker to an internal JdkHostnameVerifier class. If users really want to use this class it can be used via alias name the ssl.hostnameverifier.class property. Behavior is just like point 2.
6. Moved the 'crude' hostname checker logic into a HostnameVerifier class. If users really want to use this class it has an alias name that can be used with the ssl.hostnameverifier.class property. Behavior is just like point 2.
7. The ssl.hostnameverifier.class alias name legacy can be used to create a hostname verifier that is the HostnameChecker and the old 'crude' logic (MailHostnameVerifier) chained together as a failover. This allows the user to enabled the old hostname validation behavior prior to this release for compatibility sake. If running on JDK9+ the user can specify just running the 'crude' logic (MailHostnameVerifier) to avoid --add-opens and or illegal access errors.
8. Updated to old hostname validation logic to propagate an exception containing the reason for deny to expose problems with missing add-opens when the user is trying to enable compatibility mode.
9. Fixed what appeared to be socket leaks on exception in the SocketFetcher class during handshake and configuration of SSL.
10. Existing Angus Mail configurations that are using .ssl.checkserveridentity=true will opt-in to the new way of having the SSL API perform the hostname validations and opt-out of doing the Angus Mail hostname validations.
11. Implications of the behavior changes of point 1 and point 10 are explained in the COMPAT.txt. I think this PR makes the correct trade offs by pushing the responsibility of hostname validation to the JDK SSL API. This makes it so Angus Mail is not on a endless cycle of creating a comprehensive HostnameVerifier implementation when the JDK already has one internally and org.apache.hc.client5.http.ssl.DefaultHostnameVerifier could be configured as a ssl.hostnameverifier.class or a ssl.hostnameverifier to unconditionally verify the hostname.
12. I did not modify the pom.xml to include --add-opens since we are running JDK8 tests during build. Adding this switch requires adding an implicit profile as JDK8 doesn't know of --add-opens. This is the reason the test output will have some skipped tests. Even though they are listed as skipped, the test still verify as much common information as they can. Therefore, the coverage is really not that much different.

[jmehrens]

Example of a custom hostname verifier issuing a DENY:

jakarta.mail.MessagingException: Unable to check server identity for: localhost;
  nested exception is:
	java.io.IOException: Unable to check server identity for: localhost
	at org.eclipse.angus.mail.imap.IMAPStore.protocolConnect(IMAPStore.java:734)
	at jakarta.mail.Service.connect(Service.java:345)
	at jakarta.mail.Service.connect(Service.java:225)
	at jakarta.mail.Service.connect(Service.java:246)
	at org.eclipse.angus.mail.util.SocketFetcherTest.lambda$testSSLHostnameVerifier$0(SocketFetcherTest.java:206)
	at org.junit.Assert.assertThrows(Assert.java:1001)
	at org.junit.Assert.assertThrows(Assert.java:981)
	at org.eclipse.angus.mail.util.SocketFetcherTest.testSSLHostnameVerifier(SocketFetcherTest.java:220)
	at org.eclipse.angus.mail.util.SocketFetcherTest.testSSLHostnameVerifierRejectsConnections(SocketFetcherTest.java:166)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.base/java.lang.reflect.Method.invoke(Method.java:566)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.base/java.lang.Thread.run(Thread.java:829)
Caused by: java.io.IOException: Unable to check server identity for: localhost
	at org.eclipse.angus.mail.util.SocketFetcher.configureSSLSocket(SocketFetcher.java:675)
	at org.eclipse.angus.mail.util.SocketFetcher.createSocket(SocketFetcher.java:408)
	at org.eclipse.angus.mail.util.SocketFetcher.getSocket(SocketFetcher.java:221)
	at org.eclipse.angus.mail.iap.Protocol.<init>(Protocol.java:117)
	at org.eclipse.angus.mail.imap.protocol.IMAPProtocol.<init>(IMAPProtocol.java:132)
	at org.eclipse.angus.mail.imap.IMAPStore.newIMAPProtocol(IMAPStore.java:755)
	at org.eclipse.angus.mail.imap.IMAPStore.protocolConnect(IMAPStore.java:690)
	... 20 more
Caused by: javax.net.ssl.SSLException: Server identity does not match authentication scheme: org.eclipse.angus.mail.util.SocketFetcherTest$TestHostnameVerifier@54d4de90 DENY
	at org.eclipse.angus.mail.util.SocketFetcher.checkServerIdentity(SocketFetcher.java:743)
	at org.eclipse.angus.mail.util.SocketFetcher.configureSSLSocket(SocketFetcher.java:671)
	... 26 more

[jmehrens]

I'll wait for concensus before merging.

[jmehrens]

Here is an example exception showing the new behavior of .ssl.checkserveridentity with implicit true:

jakarta.mail.MessagingException: No name matching localhost found;
  nested exception is:
	javax.net.ssl.SSLHandshakeException: No name matching localhost found
	at org.eclipse.angus.mail.imap.IMAPStore.protocolConnect(IMAPStore.java:734)
	at jakarta.mail.Service.connect(Service.java:345)
	at jakarta.mail.Service.connect(Service.java:225)
	at jakarta.mail.Service.connect(Service.java:246)
	at org.eclipse.angus.mail.util.SocketFetcherTest.testSSLCheckServerIdentity(SocketFetcherTest.java:669)
	at org.eclipse.angus.mail.util.SocketFetcherTest.testSSLCheckServerIdentityNull(SocketFetcherTest.java:539)
	at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)
	at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
	at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
	at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
	at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:299)
	at org.junit.internal.runners.statements.FailOnTimeout$CallableStatement.call(FailOnTimeout.java:293)
	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:317)
	at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: javax.net.ssl.SSLHandshakeException: No name matching localhost found
	at java.base/sun.security.ssl.Alert.createSSLException(Alert.java:130)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:378)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:321)
	at java.base/sun.security.ssl.TransportContext.fatal(TransportContext.java:316)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.checkServerCerts(CertificateMessage.java:1318)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.onConsumeCertificate(CertificateMessage.java:1195)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.consume(CertificateMessage.java:1138)
	at java.base/sun.security.ssl.SSLHandshake.consume(SSLHandshake.java:393)
	at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:476)
	at java.base/sun.security.ssl.HandshakeContext.dispatch(HandshakeContext.java:447)
	at java.base/sun.security.ssl.TransportContext.dispatch(TransportContext.java:201)
	at java.base/sun.security.ssl.SSLTransport.decode(SSLTransport.java:172)
	at java.base/sun.security.ssl.SSLSocketImpl.decode(SSLSocketImpl.java:1506)
	at java.base/sun.security.ssl.SSLSocketImpl.readHandshakeRecord(SSLSocketImpl.java:1421)
	at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:455)
	at java.base/sun.security.ssl.SSLSocketImpl.startHandshake(SSLSocketImpl.java:426)
	at org.eclipse.angus.mail.util.SocketFetcher.configureSSLSocket(SocketFetcher.java:662)
	at org.eclipse.angus.mail.util.SocketFetcher.createSocket(SocketFetcher.java:408)
	at org.eclipse.angus.mail.util.SocketFetcher.getSocket(SocketFetcher.java:221)
	at org.eclipse.angus.mail.iap.Protocol.<init>(Protocol.java:117)
	at org.eclipse.angus.mail.imap.protocol.IMAPProtocol.<init>(IMAPProtocol.java:132)
	at org.eclipse.angus.mail.imap.IMAPStore.newIMAPProtocol(IMAPStore.java:755)
	at org.eclipse.angus.mail.imap.IMAPStore.protocolConnect(IMAPStore.java:690)
	... 15 more
Caused by: java.security.cert.CertificateException: No name matching localhost found
	at java.base/sun.security.util.HostnameChecker.matchDNS(HostnameChecker.java:229)
	at java.base/sun.security.util.HostnameChecker.match(HostnameChecker.java:103)
	at java.base/sun.security.ssl.X509TrustManagerImpl.checkIdentity(X509TrustManagerImpl.java:461)
	at java.base/sun.security.ssl.X509TrustManagerImpl.checkIdentity(X509TrustManagerImpl.java:431)
	at java.base/sun.security.ssl.AbstractTrustManagerWrapper.checkAdditionalTrust(SSLContextImpl.java:1463)
	at java.base/sun.security.ssl.AbstractTrustManagerWrapper.checkServerTrusted(SSLContextImpl.java:1431)
	at java.base/sun.security.ssl.CertificateMessage$T13CertificateConsumer.checkServerCerts(CertificateMessage.java:1302)
	... 33 more

The CertificateException stacktrace shows what I'm explaining in the COMPAT.txt.

[lukasj]

LGTM

[lukasj]

12. I did not modify the pom.xml to include --add-opens since we are running JDK8 tests during build. Adding this switch requires adding an implicit profile as JDK8 doesn't know of --add-opens. This is the reason the test output will have some skipped tests. Even though they are listed as skipped, the test still verify as much comment information as they can. Therefore, the coverage is really not that much different.

let's move to 11+ (in jaf,mail apis too) once we get angus-mail 2.1.3 out (in a week or so)

[jmehrens]

@lukasj ok. I'll merge this and I'll clean up merge commit message so no one has to do that 😀.

I'll create a new ticket for jdk11+