Testing mail.smtp.ssl.trust seems to be broken #187#188
Conversation
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
|
Another thought was I wrote some notes in the COMPAT.TXT
I wonder if the TrustManager in MailSSLSocketFactory should extend/implement X509ExtendedTrustManager. Then disable EIA as needed. |
I think that is right fix. However, it should only work that way when |
I will insist more. I have a new laptop and I am configuring everything, but I hope I will be able to comeback to this during the week. |
|
Additionally we might have to implement X509ExtendedTrustManager and conditionally set EIA when not a trusted host and |
4626c42 to
942f6bd
Compare
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
| fail("Expects exception"); | ||
| } catch (MessagingException e) { | ||
| e.printStackTrace(); | ||
| assertEquals("Server is not trusted: mailtest.local", e.getCause().getMessage()); |
There was a problem hiding this comment.
Do we need another test that is a clone of this test but
- mail.smtps.ssl.checkserveridentity=false
- Expected result is pass because other domain is trusted and not subject to EIA.
There was a problem hiding this comment.
Currently this test would fail, because this line does not check the value of ssl.checkserveridentity.
| Prior to this change, MailTrustManager only received the certificate chain | ||
| and authentication type. As a result, it could not participate in hostname | ||
| verification triggered by the SSLParameters endpoint identification | ||
| algorithm. |
There was a problem hiding this comment.
I believe EIA worked as long as MailSSLSocketFactory wasn't installed explicitly or implicitly as it would have used default SSL classes which extend X509ExtendedTrustManager
| throw new AssertionError(t); | ||
| } | ||
| public void testSSLCheckServerIdentityNull() throws Throwable { | ||
| // localhost should fail? See TestSSLSocketFactory#84 -> ((MailSSLSocketFactory) defaultFactory).setTrustedHosts("localhost"); |
There was a problem hiding this comment.
The point of the test was assert that the EIA was being enforced by the JDK. I think only change needed for original code is to switch paramter from "localhost" to "mailtest.local" in each test.
There was a problem hiding this comment.
I am trying this and it is not working as expected because we are missing to implement the validation of real values of trusted hosts. Here we only check that trusted hosts are not null and we should check values of it with the real host of the request.
Then, what is happening is that TestSSLSocketFactory sets localhost as trusted hosts. This has the same effect than trusting all hosts.
I am not sure why this was not implemented and why nobody reported this before.
There was a problem hiding this comment.
Now I read that you were talking about this in the next comment.
There was a problem hiding this comment.
Yep. I think it broke when I implemented EIA in SocketFetcher. I think the checks have to moved to trustmanager. All hostname checks happened in SocketFetcher and I only moved the ones covered by EIA which is incorrect.
There was a problem hiding this comment.
I am not sure why this was not implemented and why nobody reported this before.
I would assume it is because checkserveridentity defaulted to false as you pointed out.
The old flow, prior to EIA validation was to suppress forwarding in trust manager and optional perform hostname validation and isServerTrusted in SocketFetcher.
This legacy behavior can be simulated by setting
mail.<protocol>.ssl.hostnameverifier.class=legacy
mail.<protocol>.ssl.checkserveridentity=false
I think even in older versions of JakartaMail/JavaMail there wouldn't have been a way to trust a host and check not trusted hosts. Which is why hostnameverifier is invoked unconditionally to MailSSLSocketFactory.
| } | ||
|
|
||
| private boolean shouldDelegate() { | ||
| return !(isTrustAllHosts() || getTrustedHosts() != null); |
There was a problem hiding this comment.
Double check me on this, but i think for X509ExtendedTrustManager methods this is not correct when EIA is enabled and we are trusting a list of hosts. In that case we should short circuit if matching, otherwise delegate.
Essentially we should call MailSSLSocketFactory::isServerTrusted to determine if we delegate. Or rather update this private method to take a host name.
There was a problem hiding this comment.
We might have to remove MailSSLSocketFactory::isServerTrusted call from SocketFetcher and just have the call happen in the trustmanager. The reason I think we have to do that is because we don't reflective invoke EIA from SocketFetcher.
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
| if (sf instanceof MailSSLSocketFactory) { | ||
| MailSSLSocketFactory msf = (MailSSLSocketFactory) sf; | ||
| if (!msf.isServerTrusted(host, sslsocket)) { | ||
| if (checkServerIdentity && !msf.isServerTrusted(host, sslsocket)) { |
There was a problem hiding this comment.
I believe all of this code for MailSSLSocketFactory should be removed from SocketFetcher. The isServerTrusted should be invoked in all TrustManager methods and the result should be the decision to forward the call.
There was a problem hiding this comment.
Yes, I get your point and I am working on it.
|
I was implementing it with the suggested changes, and I found out that we don't support to specify custom trust managers: So I am getting the next error in one test that is supposed to fail with an error Do we create new properties to specify custom trust stores?. Or at least being able to inject it for testing purposes. A part of this, to be honest, I am getting puzzled and confused with the checkServerIdentity property. Maybe it is better to document that: |
@jbescos https://eclipse-ee4j.github.io/angus-mail/docs/api/org.eclipse.angus.mail/org/eclipse/angus/mail/util/MailSSLSocketFactory.html#setTrustManagers(javax.net.ssl.TrustManager...) with mock trust manager. |
2 participants
It does not solve #187 but it adds some tests nice to have, a reproducer for that issue (test is disabled to avoid failure), and it has a comment where the issue happens.
I tried to fix it in this way:
But there are other tests failing.
The root issue is that when
setEndpointIdentificationAlgorithmis invoked with a not null argument,MailTrustManageris replaced, and this method we have there is not invoked:I am not brave enough to change this behavior because there are tests failing, and it has been working in this way forever. It just started to be more visible since this commit.