-
Notifications
You must be signed in to change notification settings - Fork 22
Testing mail.smtp.ssl.trust seems to be broken #187 #188
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,5 @@ | ||
| /* | ||
| * Copyright (c) 1997, 2024 Oracle and/or its affiliates. All rights reserved. | ||
| * Copyright (c) 1997, 2025 Oracle and/or its affiliates. All rights reserved. | ||
| * | ||
| * This program and the accompanying materials are made available under the | ||
| * terms of the Eclipse Public License v. 2.0, which is available at | ||
|
|
@@ -628,15 +628,15 @@ private static void configureSSLSocket(Socket socket, String host, | |
| Arrays.asList(sslsocket.getEnabledCipherSuites())); | ||
| } | ||
|
|
||
| boolean checkServerIdentity = | ||
| PropUtil.getBooleanProperty(props, prefix + ".ssl.checkserveridentity", true); | ||
| try { | ||
| /* | ||
| * Check server identity and trust. | ||
| * See: JDK-8062515 and JDK-7192189 | ||
| * LDAPS requires the same regex handling as we need | ||
| */ | ||
| String eia = PropUtil.getBooleanProperty(props, | ||
| prefix + ".ssl.checkserveridentity", true) | ||
| ? "LDAPS" : (String) null; | ||
| String eia = checkServerIdentity ? "LDAPS" : (String) null; | ||
| SSLParameters params = sslsocket.getSSLParameters(); | ||
| params.setEndpointIdentificationAlgorithm(eia); | ||
| sslsocket.setSSLParameters(params); | ||
|
|
@@ -679,7 +679,7 @@ private static void configureSSLSocket(Socket socket, String host, | |
|
|
||
| if (sf instanceof MailSSLSocketFactory) { | ||
| MailSSLSocketFactory msf = (MailSSLSocketFactory) sf; | ||
| if (!msf.isServerTrusted(host, sslsocket)) { | ||
| if (checkServerIdentity && !msf.isServerTrusted(host, sslsocket)) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I get your point and I am working on it. |
||
| throw cleanupAndThrow(sslsocket, | ||
| new IOException("Server is not trusted: " + host)); | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.