Repository: abhinavsingh/proxy.py
Pull request #691 (2021-11-06 18:35:43)

DEFAULT_CA_FILE is now certifi/cacert.pem
Additionally added:

1. `OSError when wrapping client for TLS Interception` section under FAQ
2. Refactor interception logic into modular chunks
3. Handle several exceptions which are expected during TLS interception (and documented under FAQ).  Log warnings instead of flooding with exception stacktrace.

codecov[bot]: # [Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) Report
> Merging [#691](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) (8566d93) into [develop](https://codecov.io/gh/abhinavsingh/proxy.py/commit/8a463372252d96af5b1617369f7c9ea19a757ac6?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) (8a46337) will **decrease** coverage by `0.17%`.
> The diff coverage is `n/a`.

[![Impacted file tree graph](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/graphs/tree.svg?width=650&height=150&src=pr&token=Zh9J7b4la2&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)

```diff
@@             Coverage Diff             @@
##           develop     #691      +/-   ##
===========================================
- Coverage    86.13%   85.95%   -0.18%     
===========================================
  Files          106      106              
  Lines         4773     4772       -1     
  Branches       390      390              
===========================================
- Hits          4111     4102       -9     
- Misses         591      593       +2     
- Partials        71       77       +6     
```

| Flag | Coverage Δ | |
|---|---|---|
| pytest | `85.95% <ø> (-0.05%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh) | Coverage Δ | |
|---|---|---|
| [proxy/common/types.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvY29tbW9uL3R5cGVzLnB5) | `86.66% <0.00%> (-13.34%)` | :arrow_down: |
| [tests/test\_set\_open\_file\_limit.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdF9zZXRfb3Blbl9maWxlX2xpbWl0LnB5) | `96.29% <0.00%> (-3.71%)` | :arrow_down: |
| [tests/testing/test\_embed.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdGluZy90ZXN0X2VtYmVkLnB5) | `91.17% <0.00%> (-2.95%)` | :arrow_down: |
| [proxy/testing/test\_case.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvdGVzdGluZy90ZXN0X2Nhc2UucHk=) | `98.00% <0.00%> (-2.00%)` | :arrow_down: |
| [proxy/proxy.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvcHJveHkucHk=) | `95.21% <0.00%> (-0.96%)` | :arrow_down: |
| [tests/test\_main.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-dGVzdHMvdGVzdF9tYWluLnB5) | `99.47% <0.00%> (-0.53%)` | :arrow_down: |
| [proxy/http/handler.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvaHR0cC9oYW5kbGVyLnB5) | `74.43% <0.00%> (-0.45%)` | :arrow_down: |
| [proxy/plugin/filter\_by\_url\_regex.py](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh#diff-cHJveHkvcGx1Z2luL2ZpbHRlcl9ieV91cmxfcmVnZXgucHk=) | `74.41% <0.00%> (+1.69%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh). Last update [8a46337...8566d93](https://codecov.io/gh/abhinavsingh/proxy.py/pull/691?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Abhinav+Singh).


Patch:
diff --git a/README.md b/README.md
index b906faf..048741e 100644
--- a/README.md
+++ b/README.md
@@ -100,6 +100,7 @@
   - [Docker image not working on MacOS](#docker-image-not-working-on-macos)
   - [ValueError: filedescriptor out of range in select](#valueerror-filedescriptor-out-of-range-in-select)
   - [None:None in access logs](#nonenone-in-access-logs)
+  - [OSError when wrapping client for TLS Interception](#oserror-when-wrapping-client-for-tls-interception)
 - [Plugin Developer and Contributor Guide](#plugin-developer-and-contributor-guide)
   - [High level architecture](#high-level-architecture)
   - [Everything is a plugin](#everything-is-a-plugin)
@@ -1721,6 +1722,52 @@ few obvious ones include:
 1. Client established a connection but never completed the request.
 2. A plugin returned a response prematurely, avoiding connection to upstream server.
 
+## OSError when wrapping client for TLS Interception
+
+With `TLS Interception` on, you might occasionally see following exceptions:
+
+```console
+2021-11-06 23:33:34,540 - pid:91032 [E] server.intercept:678 - OSError when wrapping client
+Traceback (most recent call last):
+  ...[redacted]...
+  ...[redacted]...
+  ...[redacted]...
+ssl.SSLError: [SSL: TLSV1_ALERT_UNKNOWN_CA] tlsv1 alert unknown ca (_ssl.c:997)
+...[redacted]... - CONNECT oauth2.googleapis.com:443 - 0 bytes - 272.08 ms
+```
+
+Some clients can throw `TLSV1_ALERT_UNKNOWN_CA` if they cannot verify the certificate of the server
+because it is signed by an unknown issuer CA.  Which is the case when we are doing TLS interception.
+This can be for a variety of reasons e.g. certificate pinning etc.
+
+Another exception you might see is `CERTIFICATE_VERIFY_FAILED`:
+
+```console
+2021-11-06 23:36:02,002 - pid:91033 [E] handler.handle_readables:293 - Exception while receiving from client connection <socket.socket fd=28, family=AddressFamily.AF_INET, type=SocketKind.SOCK_STREAM, proto=0, laddr=('127.0.0.1', 8899), raddr=('127.0.0.1', 51961)> with reason SSLCertVerificationError(1, '[SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:997)')
+Traceback (most recent call last):
+  ...[redacted]...
+  ...[redacted]...
+  ...[redacted]...
+ssl.SSLCertVerificationError: [SSL: CERTIFICATE_VERIFY_FAILED] certificate verify failed: self signed certificate in certificate chain (_ssl.c:997)
+...[redacted]... - CONNECT init.push.apple.com:443 - 0 bytes - 892.99 ms
+```
+
+In future, we might support serving original HTTPS content for such clients while still
+performing TLS interception in the background.  This will keep the clients happy without
+impacting our ability to TLS intercept.  Unfortunately, this feature is currently not available.
+
+Another example with `SSLEOFError` exception:
+
+```console
+2021-11-06 23:46:40,446 - pid:91034 [E] server.intercept:678 - OSError when wrapping client
+Traceback (most recent call last):
+  ...[redacted]...
+  ...[redacted]...
+  ...[redacted]...
+ssl.SSLEOFError: EOF occurred in violation of protocol (_ssl.c:997)
+...[redacted]... - CONNECT stock.adobe.io:443 - 0 bytes - 685.32 ms
+```
+
 # Plugin Developer and Contributor Guide
 
 ## High level architecture
diff --git a/proxy/common/constants.py b/proxy/common/constants.py
index 25c6ee5..43c449b 100644
--- a/proxy/common/constants.py
+++ b/proxy/common/constants.py
@@ -12,6 +12,7 @@ import os
 import time
 import secrets
 import pathlib
+import sysconfig
 import ipaddress
 
 from typing import List
@@ -23,6 +24,10 @@ PROXY_PY_START_TIME = time.time()
 # /path/to/proxy.py/proxy folder
 PROXY_PY_DIR = os.path.dirname(os.path.dirname(os.path.realpath(__file__)))
 
+# Path to virtualenv/lib/python3.X/site-packages
+PROXY_PY_SITE_PACKAGES = sysconfig.get_path('purelib')
+assert PROXY_PY_SITE_PACKAGES
+
 CRLF = b'\r\n'
 COLON = b':'
 WHITESPACE = b' '
@@ -46,7 +51,9 @@ DEFAULT_CA_CERT_FILE = None
 DEFAULT_CA_KEY_FILE = None
 DEFAULT_CA_SIGNING_KEY_FILE = None
 DEFAULT_CERT_FILE = None
-DEFAULT_CA_FILE = None
+DEFAULT_CA_FILE = pathlib.Path(
+    PROXY_PY_SITE_PACKAGES,
+) / 'certifi' / 'cacert.pem'
 DEFAULT_CLIENT_RECVBUF_SIZE = DEFAULT_BUFFER_SIZE
 DEFAULT_DEVTOOLS_WS_PATH = b'/devtools'
 DEFAULT_DISABLE_HEADERS: List[bytes] = []
diff --git a/proxy/core/acceptor/threadless.py b/proxy/core/acceptor/threadless.py
index 2e25673..d50d311 100644
--- a/proxy/core/acceptor/threadless.py
+++ b/proxy/core/acceptor/threadless.py
@@ -74,12 +74,22 @@ class Threadless(multiprocessing.Process):
         Tuple[Readables, Writables],
         None, None,
     ]:
+        assert self.selector is not None
         events: Dict[socket.socket, int] = {}
         for work in self.works.values():
-            events.update(work.get_events())
-        assert self.selector is not None
-        for fd in events:
-            self.selector.register(fd, events[fd])
+            worker_events = work.get_events()
+            events.update(worker_events)
+            for fd in worker_events:
+                # Can throw ValueError: Invalid file descriptor: -1
+                #
+                # Work classes must handle the exception and shutdown
+                # gracefully otherwise this will result in bringing down the
+                # entire threadless process
+                #
+                # This is only possible when work.get_events pass
+                # an invalid file descriptor.  Example, because of bad
+                # exception handling within the work implementation class.
+                self.selector.register(fd, worker_events[fd])
         ev = self.selector.select(timeout=1)
         readables = []
         writables = []
diff --git a/proxy/http/proxy/server.py b/proxy/http/proxy/server.py
index 67f7989..113fe65 100644
--- a/proxy/http/proxy/server.py
+++ b/proxy/http/proxy/server.py
@@ -66,9 +66,9 @@ flags.add_argument(
 flags.add_argument(
     '--ca-file',
     type=str,
-    default=DEFAULT_CA_FILE,
-    help='Default: None. Provide path to custom CA file for peer certificate validation. '
-    'Specially useful on MacOS.',
+    default=str(DEFAULT_CA_FILE),
+    help='Default: ' + str(DEFAULT_CA_FILE) +
+    '. Provide path to custom CA bundle for peer certificate verification',
 )
 flags.add_argument(
     '--ca-signing-key-file',
@@ -658,49 +658,122 @@ class HttpProxyPlugin(HttpProtocolHandlerPlugin):
 
     def intercept(self) -> Union[socket.socket, bool]:
         # Perform SSL/TLS handshake with upstream
-        self.wrap_server()
+        teardown = self.wrap_server()
+        if teardown:
+            raise HttpProtocolException(
+                'Exception when wrapping server for interception',
+            )
         # Generate certificate and perform handshake with client
+        # wrap_client also flushes client data before wrapping
+        # sending to client can raise, handle expected exceptions
+        teardown = self.wrap_client()
+        if teardown:
+            raise HttpProtocolException(
+                'Exception when wrapping client for interception',
+            )
+        # Update all plugin connection reference
+        # TODO(abhinavsingh): Is this required?
+        for plugin in self.plugins.values():
+            plugin.client._conn = self.client.connection
+        return self.client.connection
+
+    def wrap_server(self) -> bool:
+        assert self.upstream is not None
+        assert isinstance(self.upstream.connection, socket.socket)
+        try:
+            self.upstream.wrap(text_(self.request.host), self.flags.ca_file)
+        except ssl.SSLCertVerificationError:    # Server raised certificate verification error
+            # When --disable-interception-on-ssl-cert-verification-error flag is on,
+            # we will cache such upstream hosts and avoid intercepting them for future
+            # requests.
+            logger.warning(
+                'ssl.SSLCertVerificationError: ' +
+                'Server raised cert verification error for upstream: {0}'.format(
+                    self.upstream.addr[0],
+                ),
+            )
+            return True
+        except ssl.SSLError as e:
+            if e.reason == 'SSLV3_ALERT_HANDSHAKE_FAILURE':
+                logger.warning(
+                    '{0}: '.format(e.reason) +
+                    'Server raised handshake alert failure for upstream: {0}'.format(
+                        self.upstream.addr[0],
+                    ),
+                )
+            else:
+                logger.exception(
+                    'SSLError when wrapping client for upstream: {0}'.format(
+                        self.upstream.addr[0],
+                    ), exc_info=e,
+                )
+            return True
+        assert isinstance(self.upstream.connection, ssl.SSLSocket)
+        return False
+
+    def wrap_client(self) -> bool:
+        assert self.upstream is not None and self.flags.ca_signing_key_file is not None
+        assert isinstance(self.upstream.connection, ssl.SSLSocket)
         try:
-            # wrap_client also flushes client data before wrapping
-            # sending to client can raise, handle expected exceptions
-            self.wrap_client()
+            # TODO: Perform async certificate generation
+            generated_cert = self.generate_upstream_certificate(
+                cast(Dict[str, Any], self.upstream.connection.getpeercert()),
+            )
+            self.client.wrap(self.flags.ca_signing_key_file, generated_cert)
         except subprocess.TimeoutExpired as e:  # Popen communicate timeout
             logger.exception(
                 'TimeoutExpired during certificate generation', exc_info=e,
             )
             return True
+        except ssl.SSLCertVerificationError:    # Client raised certificate verification error
+            # When --disable-interception-on-ssl-cert-verification-error flag is on,
+            # we will cache such upstream hosts and avoid intercepting them for future
+            # requests.
+            logger.warning(
+                'ssl.SSLCertVerificationError: ' +
+                'Client raised cert verification error for upstream: {0}'.format(
+                    self.upstream.addr[0],
+                ),
+            )
+            return True
+        except ssl.SSLEOFError as e:
+            logger.warning(
+                'ssl.SSLEOFError {0} when wrapping client for upstream: {1}'.format(
+                    str(e), self.upstream.addr[0],
+                ),
+            )
+            return True
+        except ssl.SSLError as e:
+            if e.reason in ('TLSV1_ALERT_UNKNOWN_CA', 'UNSUPPORTED_PROTOCOL'):
+                logger.warning(
+                    '{0}: '.format(e.reason) +
+                    'Client raised cert verification error for upstream: {0}'.format(
+                        self.upstream.addr[0],
+                    ),
+                )
+            else:
+                logger.exception(
+                    'OSError when wrapping client for upstream: {0}'.format(
+                        self.upstream.addr[0],
+                    ), exc_info=e,
+                )
+            return True
         except BrokenPipeError:
             logger.error(
-                'BrokenPipeError when wrapping client',
+                'BrokenPipeError when wrapping client for upstream: {0}'.format(
+                    self.upstream.addr[0],
+                ),
             )
             return True
         except OSError as e:
             logger.exception(
-                'OSError when wrapping client', exc_info=e,
+                'OSError when wrapping client for upstream: {0}'.format(
+                    self.upstream.addr[0],
+                ), exc_info=e,
             )
             return True
-        # Update all plugin connection reference
-        # TODO(abhinavsingh): Is this required?
-        for plugin in self.plugins.values():
-            plugin.client._conn = self.client.connection
-        return self.client.connection
-
-    def wrap_server(self) -> None:
-        assert self.upstream is not None
-        assert isinstance(self.upstream.connection, socket.socket)
-        self.upstream.wrap(text_(self.request.host), self.flags.ca_file)
-        assert isinstance(self.upstream.connection, ssl.SSLSocket)
-
-    def wrap_client(self) -> None:
-        assert self.upstream is not None and self.flags.ca_signing_key_file is not None
-        assert isinstance(self.upstream.connection, ssl.SSLSocket)
-        generated_cert = self.generate_upstream_certificate(
-            cast(Dict[str, Any], self.upstream.connection.getpeercert()),
-        )
-        self.client.wrap(self.flags.ca_signing_key_file, generated_cert)
-        logger.debug(
-            'TLS interception using %s', generated_cert,
-        )
+        logger.debug('TLS intercepting using %s', generated_cert)
+        return False
 
     #
     # Event emitter callbacks
diff --git a/proxy/plugin/cache/store/disk.py b/proxy/plugin/cache/store/disk.py
index f50565f..4142980 100644
--- a/proxy/plugin/cache/store/disk.py
+++ b/proxy/plugin/cache/store/disk.py
@@ -27,7 +27,8 @@ flags.add_argument(
     '--cache-dir',
     type=str,
     default=tempfile.gettempdir(),
-    help='Default: A temporary directory.  Flag only applicable when cache plugin is used with on-disk storage.',
+    help='Default: A temporary directory.  ' +
+    'Flag only applicable when cache plugin is used with on-disk storage.',
 )
 
 
diff --git a/proxy/plugin/reverse_proxy.py b/proxy/plugin/reverse_proxy.py
index 19cd13e..13f5b80 100644
--- a/proxy/plugin/reverse_proxy.py
+++ b/proxy/plugin/reverse_proxy.py
@@ -12,9 +12,7 @@ import ssl
 import random
 import socket
 import logging
-import sysconfig
 
-from pathlib import Path
 from typing import List, Optional, Tuple, Any
 from urllib import parse as urlparse
 
@@ -29,11 +27,6 @@ from ..http.server import HttpWebServerBasePlugin, httpProtocolTypes
 
 logger = logging.getLogger(__name__)
 
-# We need CA bundle to verify TLS connection to upstream servers
-PURE_LIB = sysconfig.get_path('purelib')
-assert PURE_LIB
-CACERT_PEM_PATH = Path(PURE_LIB) / 'certifi' / 'cacert.pem'
-
 
 # TODO: ReverseProxyPlugin and ProxyPoolPlugin are implementing
 # a similar behavior.  Abstract that particular logic out into its
@@ -135,7 +128,7 @@ class ReverseProxyPlugin(HttpWebServerBasePlugin):
                 self.upstream.wrap(
                     text_(
                         url.hostname,
-                    ), ca_file=str(CACERT_PEM_PATH),
+                    ), ca_file=str(self.flags.ca_file),
                 )
             self.upstream.queue(memoryview(request.build()))
         except ConnectionRefusedError:
diff --git a/proxy/proxy.py b/proxy/proxy.py
index 988dec4..6f9d207 100644
--- a/proxy/proxy.py
+++ b/proxy/proxy.py
@@ -514,6 +514,10 @@ def main(
             # configuration etc.
             #
             # TODO: Python shell within running proxy.py environment?
+            #
+            # TODO: Pid watcher which watches for processes started
+            # by proxy.py core.  May be alert or restart those processes
+            # on failure.
             while True:
                 time.sleep(1)
     except KeyboardInterrupt:


Test patch:
diff --git a/tests/http/test_http_proxy_tls_interception.py b/tests/http/test_http_proxy_tls_interception.py
index a413a3f..1c03068 100644
--- a/tests/http/test_http_proxy_tls_interception.py
+++ b/tests/http/test_http_proxy_tls_interception.py
@@ -16,6 +16,7 @@ import selectors
 
 from typing import Any
 from unittest import mock
+from proxy.common.constants import DEFAULT_CA_FILE
 
 from proxy.core.connection import TcpClientConnection, TcpServerConnection
 from proxy.http.handler import HttpProtocolHandler
@@ -168,7 +169,7 @@ class TestHttpProxyTlsInterception(unittest.TestCase):
         )
 
         self.mock_ssl_context.assert_called_with(
-            ssl.Purpose.SERVER_AUTH, cafile=None,
+            ssl.Purpose.SERVER_AUTH, cafile=str(DEFAULT_CA_FILE),
         )
         # self.assertEqual(self.mock_ssl_context.return_value.options,
         # ssl.OP_NO_SSLv2 | ssl.OP_NO_SSLv3 | ssl.OP_NO_TLSv1 |