From a7bcd4072508d87380df10b894f89f02f1eb022a Mon Sep 17 00:00:00 2001 From: Mikhail Golubev Date: Thu, 29 Sep 2016 14:22:14 +0300 Subject: [PATCH] Remove ConfirmingHostnameVerifier and unused settings from CertificateManager It turned out to be non-obvious how to properly save hostname and expiration date exceptions in settings, so these checks were disabled in the first place and the corresponding settings were hidden in the configurable. CertificateManager.HOSTNAME_VERIFIER was left in CertificateManager my mistake, and, unfortunately, has acquired some users since then. --- .../util/net/ssl/CertificateConfigurable.form | 22 +--- .../util/net/ssl/CertificateConfigurable.java | 14 --- .../util/net/ssl/CertificateManager.java | 29 ++--- .../net/ssl/CertificateWarningDialog.java | 8 -- .../net/ssl/ConfirmingHostnameVerifier.java | 109 ------------------ .../http/DefaultRemoteContentProvider.java | 2 - .../github/api/GithubConnectionBuilder.java | 4 +- .../httpclient/NewBaseRepositoryImpl.java | 4 - 8 files changed, 12 insertions(+), 180 deletions(-) delete mode 100644 platform/platform-api/src/com/intellij/util/net/ssl/ConfirmingHostnameVerifier.java diff --git a/platform/platform-api/src/com/intellij/util/net/ssl/CertificateConfigurable.form b/platform/platform-api/src/com/intellij/util/net/ssl/CertificateConfigurable.form index fc148671aeab..391be4a302a0 100644 --- a/platform/platform-api/src/com/intellij/util/net/ssl/CertificateConfigurable.form +++ b/platform/platform-api/src/com/intellij/util/net/ssl/CertificateConfigurable.form @@ -3,7 +3,7 @@ - + @@ -19,7 +19,7 @@ - + @@ -30,25 +30,9 @@ - - - - - - - - - - - - - - - - - + diff --git a/platform/platform-api/src/com/intellij/util/net/ssl/CertificateConfigurable.java b/platform/platform-api/src/com/intellij/util/net/ssl/CertificateConfigurable.java index fa8b373a580d..728f2955ff5b 100644 --- a/platform/platform-api/src/com/intellij/util/net/ssl/CertificateConfigurable.java +++ b/platform/platform-api/src/com/intellij/util/net/ssl/CertificateConfigurable.java @@ -22,14 +22,12 @@ import com.intellij.openapi.options.ConfigurationException; import com.intellij.openapi.options.SearchableConfigurable; import com.intellij.openapi.ui.Messages; import com.intellij.openapi.util.Disposer; -import com.intellij.openapi.vfs.VirtualFile; import com.intellij.ui.AnActionButton; import com.intellij.ui.AnActionButtonRunnable; import com.intellij.ui.ToolbarDecorator; import com.intellij.ui.components.JBCheckBox; import com.intellij.ui.components.JBScrollPane; import com.intellij.ui.treeStructure.Tree; -import com.intellij.util.Consumer; import com.intellij.util.containers.HashSet; import com.intellij.util.ui.UIUtil; import org.jetbrains.annotations.Nls; @@ -59,8 +57,6 @@ public class CertificateConfigurable implements SearchableConfigurable, Configur private JPanel myRootPanel; private JBCheckBox myAcceptAutomatically; - private JBCheckBox myCheckHostname; - private JBCheckBox myCheckValidityPeriod; private JPanel myCertificatesListPanel; private JPanel myDetailsPanel; @@ -75,10 +71,6 @@ public class CertificateConfigurable implements SearchableConfigurable, Configur myTree = new Tree(); myTreeBuilder = new CertificateTreeBuilder(myTree); - // are not fully functional by now - myCheckHostname.setVisible(false); - myCheckValidityPeriod.setVisible(false); - myTrustManager = CertificateManager.getInstance().getCustomTrustManager(); // show newly added certificates myTrustManager.addListener(this); @@ -187,8 +179,6 @@ public class CertificateConfigurable implements SearchableConfigurable, Configur public boolean isModified() { CertificateManager.Config state = CertificateManager.getInstance().getState(); return myAcceptAutomatically.isSelected() != state.ACCEPT_AUTOMATICALLY || - myCheckHostname.isSelected() != state.CHECK_HOSTNAME || - myCheckValidityPeriod.isSelected() != state.CHECK_VALIDITY || !myCertificates.equals(new HashSet<>(myTrustManager.getCertificates())); } @@ -216,8 +206,6 @@ public class CertificateConfigurable implements SearchableConfigurable, Configur CertificateManager.Config state = CertificateManager.getInstance().getState(); state.ACCEPT_AUTOMATICALLY = myAcceptAutomatically.isSelected(); - state.CHECK_HOSTNAME = myCheckHostname.isSelected(); - state.CHECK_VALIDITY = myCheckValidityPeriod.isSelected(); } @Override @@ -242,8 +230,6 @@ public class CertificateConfigurable implements SearchableConfigurable, Configur CertificateManager.Config state = CertificateManager.getInstance().getState(); myAcceptAutomatically.setSelected(state.ACCEPT_AUTOMATICALLY); - myCheckHostname.setSelected(state.CHECK_HOSTNAME); - myCheckValidityPeriod.setSelected(state.CHECK_VALIDITY); } @Override diff --git a/platform/platform-api/src/com/intellij/util/net/ssl/CertificateManager.java b/platform/platform-api/src/com/intellij/util/net/ssl/CertificateManager.java index 82896b25356a..2ca70e681f9e 100644 --- a/platform/platform-api/src/com/intellij/util/net/ssl/CertificateManager.java +++ b/platform/platform-api/src/com/intellij/util/net/ssl/CertificateManager.java @@ -28,9 +28,6 @@ import com.intellij.openapi.util.io.FileUtil; import com.intellij.openapi.util.io.StreamUtil; import com.intellij.openapi.util.registry.Registry; import com.intellij.util.xmlb.XmlSerializerUtil; -import com.intellij.util.xmlb.annotations.AbstractCollection; -import com.intellij.util.xmlb.annotations.Property; -import com.intellij.util.xmlb.annotations.Tag; import org.apache.http.conn.ssl.SSLConnectionSocketFactory; import org.jetbrains.annotations.NonNls; import org.jetbrains.annotations.NotNull; @@ -45,7 +42,6 @@ import java.security.KeyManagementException; import java.security.KeyStore; import java.security.KeyStoreException; import java.security.NoSuchAlgorithmException; -import java.util.LinkedHashSet; import java.util.concurrent.Callable; import java.util.concurrent.CountDownLatch; import java.util.concurrent.TimeUnit; @@ -93,10 +89,15 @@ public class CertificateManager implements PersistentStateComponent BROKEN_CERTIFICATES = new LinkedHashSet<>(); - /** * Do not show the dialog and accept untrusted certificates automatically. */ diff --git a/platform/platform-api/src/com/intellij/util/net/ssl/CertificateWarningDialog.java b/platform/platform-api/src/com/intellij/util/net/ssl/CertificateWarningDialog.java index 2474b071869c..d3d55de393c6 100644 --- a/platform/platform-api/src/com/intellij/util/net/ssl/CertificateWarningDialog.java +++ b/platform/platform-api/src/com/intellij/util/net/ssl/CertificateWarningDialog.java @@ -30,14 +30,6 @@ public class CertificateWarningDialog extends DialogWrapper { throw new UnsupportedOperationException("Not supported"); } - public static CertificateWarningDialog createHostnameMismatchWarning(@NotNull X509Certificate certificate, - @NotNull String hostname) { - String message = String.format("Server's certificate common name doesn't match hostname in URL: '%s' != '%s'", - new CertificateWrapper(certificate).getSubjectField(CertificateWrapper.CommonField.COMMON_NAME), - hostname); - return new CertificateWarningDialog(certificate, "Invalid hostname", message); - } - private JPanel myRootPanel; private JLabel myWarningSign; private JPanel myCertificateInfoPanel; diff --git a/platform/platform-api/src/com/intellij/util/net/ssl/ConfirmingHostnameVerifier.java b/platform/platform-api/src/com/intellij/util/net/ssl/ConfirmingHostnameVerifier.java deleted file mode 100644 index 72d18fc5c88d..000000000000 --- a/platform/platform-api/src/com/intellij/util/net/ssl/ConfirmingHostnameVerifier.java +++ /dev/null @@ -1,109 +0,0 @@ -package com.intellij.util.net.ssl; - -import com.intellij.openapi.ui.DialogWrapper; -import org.apache.http.conn.ssl.X509HostnameVerifier; -import org.jetbrains.annotations.NotNull; - -import javax.net.ssl.SSLException; -import javax.net.ssl.SSLSession; -import javax.net.ssl.SSLSocket; -import java.io.IOException; -import java.io.InputStream; -import java.security.cert.Certificate; -import java.security.cert.X509Certificate; -import java.util.concurrent.Callable; - -/** -* @author Mikhail Golubev -*/ /* - * BrowserCompatibleHostnameVerifier has all final/package-private methods, so direct - * inheritance from it makes no sense. Inheriting from AbstractVerifier also makes no sense, because - * the only method I can override verify(String, String[], String[]), and I have no access to certificate for the dialog - * in this case. Why the heck verify(String, X509Certificate) is final? It basically means, that I should copy half of the - * AbstractVerifier here, which gives me a strong feel of stupidity. - * - * I submit a bug about this problem https://issues.apache.org/jira/browse/HTTPCLIENT-1449 and it seems to be - * resolved in httpclient4.4. - */ -class ConfirmingHostnameVerifier implements X509HostnameVerifier { - private final X509HostnameVerifier myVerifier; - - public ConfirmingHostnameVerifier(@NotNull X509HostnameVerifier verifier) { - myVerifier = verifier; - } - - // Copied from httpclient 4.2 sources, read class level commentary for explanation. - @Override - public void verify(String host, SSLSocket ssl) throws IOException { - if (host == null) { - throw new NullPointerException("host to verify is null"); - } - - SSLSession session = ssl.getSession(); - if (session == null) { - // In our experience this only happens under IBM 1.4.x when - // spurious (unrelated) certificates show up in the server' - // chain. Hopefully this will unearth the real problem: - final InputStream in = ssl.getInputStream(); - in.available(); - // If ssl.getInputStream().available() didn't cause an - // exception, maybe at least now the session is available? - session = ssl.getSession(); - if (session == null) { - // If it's still null, probably a startHandshake() will - // unearth the real problem. - ssl.startHandshake(); - - // Okay, if we still haven't managed to cause an exception, - // might as well go for the NPE. Or maybe we're okay now? - session = ssl.getSession(); - } - } - - final Certificate[] certs = session.getPeerCertificates(); - final X509Certificate x509 = (X509Certificate)certs[0]; - verify(host, x509); - } - - @Override - public void verify(final String host, final X509Certificate cert) throws SSLException { - if (!CertificateManager.getInstance().getState().CHECK_HOSTNAME) { - return; - } - try { - myVerifier.verify(host, cert); - } - catch (SSLException e) { - //noinspection ConstantConditions - if (!accepted(host, cert)) { - throw e; - } - // TODO: inclusion in some kind of persistent settings - // Read/Write lock to protect storage? - } - } - - private static boolean accepted(final String host, final X509Certificate cert) { - return CertificateManager.showAcceptDialog(() -> CertificateWarningDialog.createHostnameMismatchWarning(cert, host)); - } - - // Copied from httpclient 4.2 sources, read class level commentary for explanation. - @Override - public boolean verify(String host, SSLSession session) { - try { - final Certificate[] certs = session.getPeerCertificates(); - final X509Certificate x509 = (X509Certificate)certs[0]; - verify(host, x509); - return true; - } - catch (final SSLException e) { - return false; - } - } - - @Override - public void verify(String host, String[] cns, String[] subjectAlts) throws SSLException { - // actually never used, because it's only used in verify(final String host, final X509Certificate cert) - myVerifier.verify(host, cns, subjectAlts); - } -} diff --git a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/http/DefaultRemoteContentProvider.java b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/http/DefaultRemoteContentProvider.java index c0912904d376..6b5cd5eddbf8 100644 --- a/platform/platform-impl/src/com/intellij/openapi/vfs/impl/http/DefaultRemoteContentProvider.java +++ b/platform/platform-impl/src/com/intellij/openapi/vfs/impl/http/DefaultRemoteContentProvider.java @@ -28,7 +28,6 @@ import com.intellij.util.PathUtilRt; import com.intellij.util.Url; import com.intellij.util.io.HttpRequests; import com.intellij.util.io.RequestBuilder; -import com.intellij.util.net.ssl.CertificateManager; import org.jetbrains.annotations.NotNull; import org.jetbrains.ide.BuiltInServerManager; @@ -65,7 +64,6 @@ public class DefaultRemoteContentProvider extends RemoteContentProvider { addRequestTuner(url, HttpRequests.request(url.toExternalForm())) .connectTimeout(60 * 1000) .productNameAsUserAgent() - .hostNameVerifier(CertificateManager.HOSTNAME_VERIFIER) .connect(new HttpRequests.RequestProcessor() { @Override public Object process(@NotNull HttpRequests.Request request) throws IOException { diff --git a/plugins/github/src/org/jetbrains/plugins/github/api/GithubConnectionBuilder.java b/plugins/github/src/org/jetbrains/plugins/github/api/GithubConnectionBuilder.java index 660f69d70cb0..8be1b5b4e248 100644 --- a/plugins/github/src/org/jetbrains/plugins/github/api/GithubConnectionBuilder.java +++ b/plugins/github/src/org/jetbrains/plugins/github/api/GithubConnectionBuilder.java @@ -27,7 +27,6 @@ import org.apache.http.client.config.RequestConfig; import org.apache.http.client.protocol.HttpClientContext; import org.apache.http.client.utils.URIBuilder; import org.apache.http.config.ConnectionConfig; -import org.apache.http.conn.ssl.X509HostnameVerifier; import org.apache.http.impl.auth.BasicScheme; import org.apache.http.impl.client.BasicCredentialsProvider; import org.apache.http.impl.client.CloseableHttpClient; @@ -62,8 +61,7 @@ class GithubConnectionBuilder { .setDefaultRequestConfig(createRequestConfig()) .setDefaultConnectionConfig(createConnectionConfig()) .setDefaultHeaders(createHeaders()) - .setSslcontext(CertificateManager.getInstance().getSslContext()) - .setHostnameVerifier((X509HostnameVerifier)CertificateManager.HOSTNAME_VERIFIER); + .setSslcontext(CertificateManager.getInstance().getSslContext()); setupCredentialsProvider(builder); diff --git a/plugins/tasks/tasks-core/src/com/intellij/tasks/impl/httpclient/NewBaseRepositoryImpl.java b/plugins/tasks/tasks-core/src/com/intellij/tasks/impl/httpclient/NewBaseRepositoryImpl.java index ff2fbe2465aa..f5db68d29ae8 100644 --- a/plugins/tasks/tasks-core/src/com/intellij/tasks/impl/httpclient/NewBaseRepositoryImpl.java +++ b/plugins/tasks/tasks-core/src/com/intellij/tasks/impl/httpclient/NewBaseRepositoryImpl.java @@ -19,7 +19,6 @@ import org.apache.http.client.config.RequestConfig; import org.apache.http.client.methods.HttpRequestBase; import org.apache.http.client.protocol.HttpClientContext; import org.apache.http.conn.routing.HttpRoute; -import org.apache.http.conn.ssl.X509HostnameVerifier; import org.apache.http.impl.auth.BasicScheme; import org.apache.http.impl.client.BasicCredentialsProvider; import org.apache.http.impl.client.HttpClientBuilder; @@ -63,9 +62,6 @@ public abstract class NewBaseRepositoryImpl extends BaseRepository { HttpClientBuilder builder = HttpClients.custom() .setDefaultRequestConfig(createRequestConfig()) .setSslcontext(CertificateManager.getInstance().getSslContext()) - // TODO: use custom one for additional certificate check - //.setHostnameVerifier(SSLConnectionSocketFactory.ALLOW_ALL_HOSTNAME_VERIFIER) - .setHostnameVerifier((X509HostnameVerifier)CertificateManager.HOSTNAME_VERIFIER) .setDefaultCredentialsProvider(createCredentialsProvider()) .addInterceptorFirst(PREEMPTIVE_BASIC_AUTH) .addInterceptorLast(createRequestInterceptor());