improve TLS handling with SNI support and cert caching #432
Conversation
Implements improved TLS handling system with Server Name Indication (SNI) support and certificate caching. Changes include: - Implement SNI support to provide better hostname verification - Add certificate expiration checks and automatic cleanup - Cache SSL contexts to reduce overhead on repeated connections - Domain cert caching, with validation - Improved and refactored code layout The new implementation improves performance by caching certificates and contexts while providing proper hostname verification through SNI. This allows the proxy to securely handle multiple GitHub domains with domain-specific certificates.
| self._ca = ca_provider | ||
| # Use strong references for caching | ||
| self._cert_cache: Dict[str, CachedCertificate] = {} | ||
| self._context_cache: Dict[str, ssl.SSLContext] = {} |
There was a problem hiding this comment.
This is not for this PR, but would we ever want to do some cache cleanup like least-recently used or are the certificates small enough that we don't care?
There was a problem hiding this comment.
I think so, I will follow up, just want to think it over a bit more
| return | ||
|
|
||
| # Get current time for expiry checks | ||
| current_time = datetime.now(timezone.utc) |
There was a problem hiding this comment.
I had to look up in the docs if datetime.now is equivalent to datetime.utcnow and based on my understanding it is - just wanted to double check during review if you think it's OK to use both in the code (not a blocker)
| # remove and recreate certs directory | ||
| try: | ||
| logger.debug(f"Removing certs directory: {self.certs_dir}") | ||
| os.rmdir(self.certs_dir) |
There was a problem hiding this comment.
This will only work if the certs_dir is empty, right? Or did you mean to use rmtree here?
There was a problem hiding this comment.
This seems to work great and is an improvement. I left some small comments inline - two are questions and one might be a bug (the confusion between rmdir and rmtree).
But since this is quite a nice improvement, I say let's merge and if there are things to follow up on, let's make a follow-up patch.
Implements improved TLS handling system with Server Name Indication (SNI) support and certificate caching. Changes include:
The new implementation improves performance by caching certificates and contexts
while providing proper hostname verification through SNI. This allows the proxy
to securely handle multiple GitHub domains with domain-specific certificates which in turn fixes: #371