verify upstream certificates by default (#1111)

squashed and merged by @mhils

8 files changed, 52 insertions, 44 deletions

diff --git a/mitmproxy/cmdline.py b/mitmproxy/cmdline.py
index a6844241..4eadce11 100644
--- a/mitmproxy/cmdline.py
+++ b/mitmproxy/cmdline.py
@@ -260,7 +260,7 @@ def get_common_options(args):
         upstream_auth = args.upstream_auth,
         ssl_version_client = args.ssl_version_client,
         ssl_version_server = args.ssl_version_server,
-        ssl_verify_upstream_cert = args.ssl_verify_upstream_cert,
+        ssl_insecure = args.ssl_insecure,
         ssl_verify_upstream_trusted_cadir = args.ssl_verify_upstream_trusted_cadir,
         ssl_verify_upstream_trusted_ca = args.ssl_verify_upstream_trusted_ca,
         tcp_hosts = args.tcp_hosts,
@@ -519,10 +519,9 @@ def proxy_ssl_options(parser):
              "that will be served to the proxy client, as extras."
     )
     group.add_argument(
-        "--verify-upstream-cert", default=False,
-        action="store_true", dest="ssl_verify_upstream_cert",
-        help="Verify upstream server SSL/TLS certificates and fail if invalid "
-             "or not present."
+        "--insecure", default=False,
+        action="store_true", dest="ssl_insecure",
+        help="Do not verify upstream server SSL/TLS certificates."
     )
     group.add_argument(
         "--upstream-trusted-cadir", default=None, action="store",
diff --git a/mitmproxy/console/options.py b/mitmproxy/console/options.py
index f9fc3764..d34672d3 100644
--- a/mitmproxy/console/options.py
+++ b/mitmproxy/console/options.py
@@ -8,6 +8,7 @@ from mitmproxy.console import grideditor
 from mitmproxy.console import palettes
 from mitmproxy.console import select
 from mitmproxy.console import signals
+from OpenSSL import SSL
 
 footer = [
     ('heading_key', "enter/space"), ":toggle ",
@@ -91,6 +92,12 @@ class Options(urwid.WidgetWrap):
                     lambda: master.options.tcp_hosts,
                     self.tcp_hosts
                 ),
+                select.Option(
+                    "Don't Verify SSL/TLS Certificates",
+                    "V",
+                    lambda: master.server.config.ssl_insecure,
+                    master.options.toggler("ssl_insecure")
+                ),
 
                 select.Heading("Utility"),
                 select.Option(
diff --git a/mitmproxy/options.py b/mitmproxy/options.py
index bdc0db4e..75798381 100644
--- a/mitmproxy/options.py
+++ b/mitmproxy/options.py
@@ -73,7 +73,7 @@ class Options(optmanager.OptManager):
             upstream_auth = "",  # type: str
             ssl_version_client="secure",  # type: str
             ssl_version_server="secure",  # type: str
-            ssl_verify_upstream_cert=False,  # type: bool
+            ssl_insecure=False,  # type: bool
             ssl_verify_upstream_trusted_cadir=None,  # type: str
             ssl_verify_upstream_trusted_ca=None,  # type: str
             tcp_hosts = (),  # type: Sequence[str]
@@ -130,7 +130,7 @@ class Options(optmanager.OptManager):
         self.upstream_auth = upstream_auth
         self.ssl_version_client = ssl_version_client
         self.ssl_version_server = ssl_version_server
-        self.ssl_verify_upstream_cert = ssl_verify_upstream_cert
+        self.ssl_insecure = ssl_insecure
         self.ssl_verify_upstream_trusted_cadir = ssl_verify_upstream_trusted_cadir
         self.ssl_verify_upstream_trusted_ca = ssl_verify_upstream_trusted_ca
         self.tcp_hosts = tcp_hosts
diff --git a/mitmproxy/proxy/config.py b/mitmproxy/proxy/config.py
index a74ba7e2..cf75830a 100644
--- a/mitmproxy/proxy/config.py
+++ b/mitmproxy/proxy/config.py
@@ -83,24 +83,18 @@ class ProxyConfig:
         options.changed.connect(self.configure)
 
     def configure(self, options, updated):
-        conflict = all(
-            [
-                options.add_upstream_certs_to_client_chain,
-                options.ssl_verify_upstream_cert
-            ]
-        )
-        if conflict:
+        # type: (mitmproxy.options.Options, Any) -> None
+        if options.add_upstream_certs_to_client_chain and not options.ssl_insecure:
             raise exceptions.OptionsError(
-                "The verify-upstream-cert and add-upstream-certs-to-client-chain "
-                "options are mutually exclusive. If upstream certificates are verified "
-                "then extra upstream certificates are not available for inclusion "
-                "to the client chain."
+                "The verify-upstream-cert requires certificate verification to be disabled. "
+                "If upstream certificates are verified then extra upstream certificates are "
+                "not available for inclusion to the client chain."
             )
 
-        if options.ssl_verify_upstream_cert:
-            self.openssl_verification_mode_server = SSL.VERIFY_PEER
-        else:
+        if options.ssl_insecure:
             self.openssl_verification_mode_server = SSL.VERIFY_NONE
+        else:
+            self.openssl_verification_mode_server = SSL.VERIFY_PEER
 
         self.check_ignore = HostMatcher(options.ignore_hosts)
         self.check_tcp = HostMatcher(options.tcp_hosts)
diff --git a/test/mitmproxy/test_protocol_http2.py b/test/mitmproxy/test_protocol_http2.py
index aa096a72..f0fa9a40 100644
--- a/test/mitmproxy/test_protocol_http2.py
+++ b/test/mitmproxy/test_protocol_http2.py
@@ -102,7 +102,11 @@ class _Http2TestBase(object):
 
     @classmethod
     def get_options(cls):
-        opts = options.Options(listen_port=0, no_upstream_cert=False)
+        opts = options.Options(
+            listen_port=0,
+            no_upstream_cert=False,
+            ssl_insecure=True
+        )
         opts.cadir = os.path.join(tempfile.gettempdir(), "mitmproxy")
         return opts
 
diff --git a/test/mitmproxy/test_proxy.py b/test/mitmproxy/test_proxy.py
index 6e790e28..84838018 100644
--- a/test/mitmproxy/test_proxy.py
+++ b/test/mitmproxy/test_proxy.py
@@ -146,9 +146,9 @@ class TestProcessProxyOptions:
             "--singleuser",
             "test")
 
-    def test_verify_upstream_cert(self):
-        p = self.assert_noerr("--verify-upstream-cert")
-        assert p.openssl_verification_mode_server == SSL.VERIFY_PEER
+    def test_insecure(self):
+        p = self.assert_noerr("--insecure")
+        assert p.openssl_verification_mode_server == SSL.VERIFY_NONE
 
     def test_upstream_trusted_cadir(self):
         expected_dir = "/path/to/a/ca/dir"
diff --git a/test/mitmproxy/test_server.py b/test/mitmproxy/test_server.py
index 6230fc1f..a6dffb69 100644
--- a/test/mitmproxy/test_server.py
+++ b/test/mitmproxy/test_server.py
@@ -2,7 +2,6 @@ import os
 import socket
 import time
 import types
-from OpenSSL import SSL
 from netlib.exceptions import HttpReadDisconnect, HttpException
 from netlib.tcp import Address
 
@@ -15,6 +14,7 @@ from pathod import pathoc, pathod
 
 from mitmproxy.builtins import script
 from mitmproxy import controller
+from mitmproxy import options
 from mitmproxy.proxy.config import HostMatcher, parse_server_spec
 from mitmproxy.models import Error, HTTPResponse, HTTPFlow
 
@@ -350,6 +350,15 @@ class TestHTTPSCertfile(tservers.HTTPProxyTest, CommonMixin):
         assert self.pathod("304")
 
 
+class TestHTTPSSecureByDefault:
+    def test_secure_by_default(self):
+        """
+        Certificate verification should be turned on by default.
+        """
+        default_opts = options.Options()
+        assert not default_opts.ssl_insecure
+
+
 class TestHTTPSUpstreamServerVerificationWTrustedCert(tservers.HTTPProxyTest):
 
     """
@@ -360,11 +369,12 @@ class TestHTTPSUpstreamServerVerificationWTrustedCert(tservers.HTTPProxyTest):
         cn=b"trusted-cert",
         certs=[
             ("trusted-cert", tutils.test_data.path("data/trusted-server.crt"))
-        ])
+        ]
+    )
 
     def test_verification_w_cadir(self):
         self.config.options.update(
-            ssl_verify_upstream_cert = True,
+            ssl_insecure = False,
             ssl_verify_upstream_trusted_cadir = tutils.test_data.path(
                 "data/trusted-cadir/"
             )
@@ -372,10 +382,12 @@ class TestHTTPSUpstreamServerVerificationWTrustedCert(tservers.HTTPProxyTest):
         self.pathoc()
 
     def test_verification_w_pemfile(self):
-        self.config.openssl_verification_mode_server = SSL.VERIFY_PEER
-        self.config.options.ssl_verify_upstream_trusted_ca = tutils.test_data.path(
-            "data/trusted-cadir/trusted-ca.pem")
-
+        self.config.options.update(
+            ssl_insecure = False,
+            ssl_verify_upstream_trusted_ca = tutils.test_data.path(
+                "data/trusted-cadir/trusted-ca.pem"
+            ),
+        )
         self.pathoc()
 
 
@@ -396,18 +408,9 @@ class TestHTTPSUpstreamServerVerificationWBadCert(tservers.HTTPProxyTest):
         # We need to make an actual request because the upstream connection is lazy-loaded.
         return p.request("get:/p/242")
 
-    def test_default_verification_w_bad_cert(self):
-        """Should use no verification."""
-        self.config.options.update(
-            ssl_verify_upstream_trusted_ca = tutils.test_data.path(
-                "data/trusted-cadir/trusted-ca.pem"
-            )
-        )
-        assert self._request().status_code == 242
-
     def test_no_verification_w_bad_cert(self):
         self.config.options.update(
-            ssl_verify_upstream_cert = False,
+            ssl_insecure = True,
             ssl_verify_upstream_trusted_ca = tutils.test_data.path(
                 "data/trusted-cadir/trusted-ca.pem"
             )
@@ -416,7 +419,7 @@ class TestHTTPSUpstreamServerVerificationWBadCert(tservers.HTTPProxyTest):
 
     def test_verification_w_bad_cert(self):
         self.config.options.update(
-            ssl_verify_upstream_cert = True,
+            ssl_insecure = False,
             ssl_verify_upstream_trusted_ca = tutils.test_data.path(
                 "data/trusted-cadir/trusted-ca.pem"
             )
diff --git a/test/mitmproxy/tservers.py b/test/mitmproxy/tservers.py
index d364162c..1597f59c 100644
--- a/test/mitmproxy/tservers.py
+++ b/test/mitmproxy/tservers.py
@@ -120,7 +120,8 @@ class ProxyTestBase(object):
         return options.Options(
             listen_port=0,
             cadir=cls.cadir,
-            add_upstream_certs_to_client_chain=cls.add_upstream_certs_to_client_chain
+            add_upstream_certs_to_client_chain=cls.add_upstream_certs_to_client_chain,
+            ssl_insecure=True,
         )