[ConnectionPool] Add support for HTTPS request pooling

[JerryKwan]

Here is how to try HTTPS connection pooling. Currently only available when TLS Interception is enabled and within a single worker (no multiprocessing support yet)

You can use the following commands to do some tests:

Generate required certificates

make ca-certificates

Start proxy with TLS Interception enabled & with connection pool

python3.7 -m proxy \
--ca-key-file ca-key.pem \
--ca-cert-file ca-cert.pem \
--ca-signing-key-file ca-signing-key.pem \
--num-workers 1 \
--log-level DEBUG \
--enable-conn-pool \
--log-format "%(asctime)s - pid:%(process)d [%(levelname)-.1s] %(pathname)s.%(funcName)s:%(lineno)d - %(message)s"

Client

curl -v -x localhost:8899 --cacert ca-cert.pem https://httpbin.org/get

[abhinavsingh]

Thank you for this PR and work on https connection pool. This not only help with within TLS interception mode but also at every place where proxy.py or plugins needs to establish a HTTPS connection upstream.

I'll run it locally and also try to dig into multiprocessing aspects. But we should be able to merge this in as-in (with green CI :)) and need not wait for multiprocessing support to land within this PR.

I'll update back with my findings. Meanwhile try to address these lint warnings. I know they might seem annoying but are necessary and sometimes might even help you find bugs in the code. PTAL at https://github.com/abhinavsingh/proxy.py/runs/4504222230?check_suite_focus=true

[abhinavsingh]

@JerryKwan Can you check in the tls_server_hello.data file too. Thank you

[abhinavsingh]

PTAL. Also add the missing tls_server_hello.data file. Thank you

[abhinavsingh]

I made several changes related to list here JerryKwan#4. Please take a look and merge into your merge, which in-turn will update this PR.

You will have to additionally address the comments I left before PR will become merge ready (green CI). Let me know if you run into any issues. Thank you.

[codecov]

Codecov Report

Merging #865 (fe8fec6) into develop (32fb729) will increase coverage by 0.05%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop     #865      +/-   ##
===========================================
+ Coverage    86.90%   86.96%   +0.05%     
===========================================
  Files          130      130              
  Lines         5843     5845       +2     
  Branches       586      586              
===========================================
+ Hits          5078     5083       +5     
+ Misses         656      652       -4     
- Partials       109      110       +1     

Flag	Coverage Δ
pytest	86.80% <ø> (+0.05%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
proxy/core/connection/pool.py	96.77% <ø> (ø)
proxy/http/handler.py	71.49% <0.00%> (-0.91%)	⬇️
proxy/plugin/proxy_pool.py	32.60% <0.00%> (+0.69%)	⬆️
proxy/http/exception/proxy_conn_failed.py	88.23% <0.00%> (+0.73%)	⬆️
proxy/http/server/web.py	64.15% <0.00%> (+0.79%)	⬆️
proxy/plugin/reverse_proxy.py	46.66% <0.00%> (+1.01%)	⬆️
proxy/http/exception/http_request_rejected.py	88.88% <0.00%> (+1.38%)	⬆️
proxy/http/exception/proxy_auth_failed.py	85.71% <0.00%> (+2.38%)	⬆️
proxy/http/exception/base.py	75.00% <0.00%> (+8.33%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 32fb729...fe8fec6. Read the comment docs.

[abhinavsingh]

@JerryKwan Awesome looks like all has been resolved. Docker problems are due to permission issues. I'll take another look later today and merge if all looks good. Thank you!!!

[JerryKwan]

Agree. These lines should be commented out

…

On Tue, Dec 21, 2021 at 11:57 PM Abhinav Singh ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In proxy/core/connection/connection.py <#865 (comment)>: > @@ -52,6 +52,7 @@ def connection(self) -> Union[ssl.SSLSocket, socket.socket]: def send(self, data: bytes) -> int: """Users must handle BrokenPipeError exceptions""" # logger.info(data) + logger.info('send to %s, data = |%s|', self.tag, data) I didn't get you. By having a logger.info in send, instance will keep printing all data sent to client and servers (even encrypted data). So we should not have this enabled. At-best we must have .debug, but even in debug mode, printing all raw data gets too verbose. There is a reason I have a commented out line above. I uncomment it when necessary for local debugging. ------------------------------ In proxy/core/connection/connection.py <#865 (comment)>: > @@ -66,6 +67,7 @@ def recv( (len(data), self.tag), ) # logger.info(data) + logger.info('recv from %s, data = |%s|', self.tag, data) Same here, we should not keep this line. Comment this out. — Reply to this email directly, view it on GitHub <#865 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAHRXINI5VOUEERKWWTHGGLUSCPXZANCNFSM5J5SWUTQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[abhinavsingh]

Closing this for now as we separately merged only the base TlsParser skeleton in #922 . Next we still need to cache the SSL session to successfully implement HTTPS connection resumptions. See discussion in #748 for more details.