search

getlantern / tlsmasq

A library for servers which masquerade as other TLS servers
Apache License 2.0
2 stars 1 forks source link

Harry/20 #27

Closed hwh33 closed 3 years ago

hwh33 commented 3 years ago

Resolves https://github.com/getlantern/tlsmasq/issues/20 by adding a test utilizing nettest.TestConn. Unfortunately, we cannot test the full tlsmasq connection with TestConn. The connection wraps a tls.Conn which, surprisingly, does not itself pass the tests. However, we are able to test the inner ptlshs.Conn. Most of the complex logic is in this inner package (ptlshs) anyway, so this is still a win IMO. A few bugs were caught as a result of adding this test - they have been fixed in this PR.

hwh33 commented 3 years ago

Some of the bug fixes were fairly subtle. The logic is already a bit complex and I think the bug fixes unfortunately added to this complexity, as have bug fixes merged in a number of recent PRs. It's tough because there are a lot of moving parts and tricky edge cases to handle. If anyone reviewing this PR sees potential simplifications I'd be quite appreciative. I think I've been looking at this code too long!

hwh33 commented 3 years ago

@reflog and @soltzen I've requested reviews from the two of you as I think it'd be beneficial to get more eyes on this codebase. Currently @oxtoacart is the only other member of the team with significant familiarity.

Let me know if you have any questions! This PR is fairly low-impact as it is really just adding tests and fixing a few minor bugs. No big rush here either.

soltzen commented 3 years ago

Taking this up. Thanks @hwh33!

Just to manage expectations: I'll post my review before Tuesday the 24th

hwh33 commented 3 years ago

Thanks @soltzen!