Merge pull request #2934 from mhils/cleanup-proxyconfig

Cleanup ProxyConfig

7 files changed, 40 insertions, 56 deletions

diff --git a/mitmproxy/addons/core.py b/mitmproxy/addons/core.py
index 6edac6c3..dbbbfd51 100644
--- a/mitmproxy/addons/core.py
+++ b/mitmproxy/addons/core.py
@@ -1,5 +1,7 @@
 import typing
 
+import os
+
 from mitmproxy.utils import human
 from mitmproxy import ctx
 from mitmproxy import exceptions
@@ -42,6 +44,12 @@ class Core:
                 "then the upstream certificate is not retrieved before generating "
                 "the client certificate chain."
             )
+        if opts.add_upstream_certs_to_client_chain and not opts.ssl_insecure:
+            raise exceptions.OptionsError(
+                "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 "body_size_limit" in updated:
             try:
                 human.parse_size(opts.body_size_limit)
@@ -66,6 +74,13 @@ class Core:
                 raise exceptions.OptionsError(
                     "Invalid mode specification: %s" % mode
                 )
+        if "client_certs" in updated:
+            if opts.client_certs:
+                client_certs = os.path.expanduser(opts.client_certs)
+                if not os.path.exists(client_certs):
+                    raise exceptions.OptionsError(
+                        "Client certificate path does not exist: {}".format(opts.client_certs)
+                    )
 
     @command.command("set")
     def set(self, *spec: str) -> None:
diff --git a/mitmproxy/connections.py b/mitmproxy/connections.py
index e8fc4fbf..29ab6ab5 100644
--- a/mitmproxy/connections.py
+++ b/mitmproxy/connections.py
@@ -281,6 +281,7 @@ class ServerConnection(tcp.TCPClient, stateobject.StateObject):
             raise ValueError("sni must be str, not " + type(sni).__name__)
         client_cert = None
         if client_certs:
+            client_certs = os.path.expanduser(client_certs)
             if os.path.isfile(client_certs):
                 client_cert = client_certs
             else:
diff --git a/mitmproxy/proxy/config.py b/mitmproxy/proxy/config.py
index c15640d7..439beb3d 100644
--- a/mitmproxy/proxy/config.py
+++ b/mitmproxy/proxy/config.py
@@ -2,12 +2,11 @@ import os
 import re
 import typing
 
-from OpenSSL import SSL, crypto
+from OpenSSL import crypto
 
 from mitmproxy import exceptions
 from mitmproxy import options as moptions
 from mitmproxy import certs
-from mitmproxy.net import tls
 from mitmproxy.net import server_spec
 
 CONF_BASENAME = "mitmproxy"
@@ -40,35 +39,16 @@ class ProxyConfig:
         self.check_ignore = None  # type: HostMatcher
         self.check_tcp = None  # type: HostMatcher
         self.certstore = None  # type: certs.CertStore
-        self.client_certs = None  # type: str
-        self.openssl_verification_mode_server = None  # type: int
         self.upstream_server = None  # type: typing.Optional[server_spec.ServerSpec]
         self.configure(options, set(options.keys()))
         options.changed.connect(self.configure)
 
     def configure(self, options: moptions.Options, updated: typing.Any) -> None:
-        if options.add_upstream_certs_to_client_chain and not options.ssl_insecure:
-            raise exceptions.OptionsError(
-                "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_insecure:
-            self.openssl_verification_mode_server = SSL.VERIFY_NONE
-        else:
-            self.openssl_verification_mode_server = SSL.VERIFY_PEER
-
         if "ignore_hosts" in updated:
             self.check_ignore = HostMatcher(options.ignore_hosts)
         if "tcp_hosts" in updated:
             self.check_tcp = HostMatcher(options.tcp_hosts)
 
-        self.openssl_method_client, self.openssl_options_client = \
-            tls.VERSION_CHOICES[options.ssl_version_client]
-        self.openssl_method_server, self.openssl_options_server = \
-            tls.VERSION_CHOICES[options.ssl_version_server]
-
         certstore_path = os.path.expanduser(options.cadir)
         if not os.path.exists(os.path.dirname(certstore_path)):
             raise exceptions.OptionsError(
@@ -80,15 +60,6 @@ class ProxyConfig:
             CONF_BASENAME
         )
 
-        if options.client_certs:
-            client_certs = os.path.expanduser(options.client_certs)
-            if not os.path.exists(client_certs):
-                raise exceptions.OptionsError(
-                    "Client certificate path does not exist: %s" %
-                    options.client_certs
-                )
-            self.client_certs = client_certs
-
         for c in options.certs:
             parts = c.split("=", 1)
             if len(parts) == 1:
diff --git a/mitmproxy/proxy/protocol/tls.py b/mitmproxy/proxy/protocol/tls.py
index 09ce87ba..ce3dc662 100644
--- a/mitmproxy/proxy/protocol/tls.py
+++ b/mitmproxy/proxy/protocol/tls.py
@@ -374,10 +374,11 @@ class TlsLayer(base.Layer):
             extra_certs = None
 
         try:
+            tls_method, tls_options = net_tls.VERSION_CHOICES[self.config.options.ssl_version_client]
             self.client_conn.convert_to_tls(
                 cert, key,
-                method=self.config.openssl_method_client,
-                options=self.config.openssl_options_client,
+                method=tls_method,
+                options=tls_options,
                 cipher_list=self.config.options.ciphers_client or DEFAULT_CLIENT_CIPHERS,
                 dhparams=self.config.certstore.dhparams,
                 chain_file=chain_file,
diff --git a/test/mitmproxy/addons/test_core.py b/test/mitmproxy/addons/test_core.py
index b1b105d9..3c674b3f 100644
--- a/test/mitmproxy/addons/test_core.py
+++ b/test/mitmproxy/addons/test_core.py
@@ -3,6 +3,7 @@ from unittest import mock
 from mitmproxy.addons import core
 from mitmproxy.test import taddons
 from mitmproxy.test import tflow
+from mitmproxy.test import tutils
 from mitmproxy import exceptions
 import pytest
 
@@ -167,6 +168,12 @@ def test_validation_simple():
                 add_upstream_certs_to_client_chain = True,
                 upstream_cert = False
             )
+        with pytest.raises(exceptions.OptionsError, match="requires certificate verification to be disabled"):
+            tctx.configure(
+                sa,
+                add_upstream_certs_to_client_chain = True,
+                ssl_insecure = False
+            )
         with pytest.raises(exceptions.OptionsError, match="Invalid mode"):
             tctx.configure(
                 sa,
@@ -188,4 +195,16 @@ def test_validation_modes(m):
     with taddons.context() as tctx:
         tctx.configure(sa, mode = "reverse:http://localhost")
         with pytest.raises(Exception, match="Invalid server specification"):
-            tctx.configure(sa, mode = "reverse:")
\ No newline at end of file
+            tctx.configure(sa, mode = "reverse:")
+
+
+def test_client_certs():
+    sa = core.Core()
+    with taddons.context() as tctx:
+        # Folders should work.
+        tctx.configure(sa, client_certs = tutils.test_data.path("mitmproxy/data/clientcert"))
+        # Files, too.
+        tctx.configure(sa, client_certs = tutils.test_data.path("mitmproxy/data/clientcert/client.pem"))
+
+        with pytest.raises(exceptions.OptionsError, match="certificate path does not exist"):
+            tctx.configure(sa, client_certs = "invalid")
diff --git a/test/mitmproxy/proxy/test_config.py b/test/mitmproxy/proxy/test_config.py
index a7da980b..60a0deb5 100644
--- a/test/mitmproxy/proxy/test_config.py
+++ b/test/mitmproxy/proxy/test_config.py
@@ -7,30 +7,12 @@ from mitmproxy.test import tutils
 
 
 class TestProxyConfig:
-    def test_upstream_cert_insecure(self):
-        opts = options.Options()
-        opts.add_upstream_certs_to_client_chain = True
-        with pytest.raises(exceptions.OptionsError, match="verify-upstream-cert"):
-            ProxyConfig(opts)
-
     def test_invalid_cadir(self):
         opts = options.Options()
         opts.cadir = "foo"
         with pytest.raises(exceptions.OptionsError, match="parent directory does not exist"):
             ProxyConfig(opts)
 
-    def test_invalid_client_certs(self):
-        opts = options.Options()
-        opts.client_certs = "foo"
-        with pytest.raises(exceptions.OptionsError, match="certificate path does not exist"):
-            ProxyConfig(opts)
-
-    def test_valid_client_certs(self):
-        opts = options.Options()
-        opts.client_certs = tutils.test_data.path("mitmproxy/data/clientcert/")
-        p = ProxyConfig(opts)
-        assert p.client_certs
-
     def test_invalid_certificate(self):
         opts = options.Options()
         opts.certs = [tutils.test_data.path("mitmproxy/data/dumpfile-011")]
diff --git a/test/mitmproxy/test_proxy.py b/test/mitmproxy/test_proxy.py
index 299abab3..75d4cdf0 100644
--- a/test/mitmproxy/test_proxy.py
+++ b/test/mitmproxy/test_proxy.py
@@ -1,6 +1,5 @@
 import argparse
 from unittest import mock
-from OpenSSL import SSL
 import pytest
 
 from mitmproxy.tools import cmdline
@@ -50,10 +49,6 @@ class TestProcessProxyOptions:
         with pytest.raises(Exception, match="does not exist"):
             self.p("--cert", "nonexistent")
 
-    def test_insecure(self):
-        p = self.assert_noerr("--ssl-insecure")
-        assert p.openssl_verification_mode_server == SSL.VERIFY_NONE
-
 
 class TestProxyServer: