Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: utls for TLS parroting #414

Closed
wants to merge 31 commits into from
Closed

cli: utls for TLS parroting #414

wants to merge 31 commits into from

Conversation

kelmenhorst
Copy link
Contributor

This is the ground work for issue #1424.
This PR introduces the use of utls for mimicking the TLS ClientHellos of popular browser implementations in order to decrease the risk of OONI clients being blocked based on their characteristic TLS fingerprint. The functionality is implemented by a UTLSHandshaker in netxlite.

When using utls, net/http fails to derive the right HTTP version from the ALPN (the issue is described here). To solve this, the PR adds a roundTripper wrapper (package httptransport) to take care of this decision:

roundtripper

Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to proceed to merge this PR piecemeal. The first step I'd like to achieve is to add support for handshaking using utls. I would like you to focus just on changes in that part of the codebase for now, by trying to implement using utls via the NewConn factory rather than adding a new UTLSHandshaker type.

Once we have refactored this part of the PR, we'll work to only merge that part and then we'll continue the discussion regarding the rest of your PR.

My general sense is that the code you've written is consistent with the way in which we do things currently in probe-cli, but it may be that there is a better way that could allow us to perform the same measurements with less code. Because this PR addresses a crucial and fundamental issue in probe-cli, it may be worth it spending some extra brain cycles to figure out the extent to which we can simplify and improve the code design.

Thank you!

(To further clarify what I mean: I suggest working on this branch and applying the required NewConn change. Once that is okay, I will take care of merging this part of your contribution to master. Once that is done, I'd like to have a design conversation regarding a few design options that I see here, so that we can discuss how to finish the rest of the work.)

internal/netxlite/tlshandshaker.go Outdated Show resolved Hide resolved
Copy link
Contributor

@bassosimone bassosimone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm requesting some small changes before we can merge the part in which extend netxlite so that later we can focus on the rest of the diff. Thank you!

go.mod Outdated Show resolved Hide resolved
internal/netxlite/tlshandshaker.go Outdated Show resolved Hide resolved
internal/netxlite/tlshandshaker.go Outdated Show resolved Hide resolved
internal/netxlite/tlshandshaker.go Outdated Show resolved Hide resolved
This branch is currently using a connection to probe for whether the
server uses http/1.1 or h2 and chooses a transport accordingly.

The problem is that this extra connection is breaking the proxy because
it uses net.Dial directly, so OONI doesn't work where a proxy is required.

As a fix, stop using the whole of netx inside session but rather let
us construct a netxlite transport.

I was already planning on doing that and this issue here is a good
excuse to see whether doing that makes sense.
@bassosimone
Copy link
Contributor

Superseded by #432

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants