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

NewHTTPTransportWithOptions with proxy uses the wrong TLS stack #2536

Open
bassosimone opened this issue Sep 15, 2023 · 0 comments
Open

NewHTTPTransportWithOptions with proxy uses the wrong TLS stack #2536

bassosimone opened this issue Sep 15, 2023 · 0 comments
Assignees
Labels
bug Something isn't working ooni/probe-engine priority/low techdebt This issue describes technical debt

Comments

@bassosimone
Copy link
Member

bassosimone commented Sep 15, 2023

Consider this scenario:

  1. You use netxlite.NewHTTPTransportWithOptions passing it a model.TLSDialer and you also use the netxlite.HTTPTransportOptionProxyURL to configure an HTTPS proxy.

  2. The *Transport you are using, which is implemented by github.com/ooni/oohttp, uses the model.TLSDialer to establish the connection with the proxy.

  3. Assuming the URL to fetch is https://www.example.com, the proxy will dial with www.example.com:443 and then the *Transport will have this nice TLS tunnel over which to TLS handshake again.

  4. However, the semantics available to the *Transport is .DialTLSContext, which cannot be used over an already established connection with the proxy, so the oohttp stack falls back on the second best, which is using the .TLSClientFactory to make a TLS connection over the proxy connection.

  5. The default of .TLSClientFactory is to use the crypto/tls stack.

This means that you're using a crypto/tls instead of using the TLSDialer in this scenario.

Normally, you would not notice this issue. But, if you're using ./internal/netemx the second TLS handshake would break, because it would not have access to the same certificates configured inside the model.UnderlyingNetwork.

This is technical debt for the engine that I am documenting so that I can reference it. Because currently this issue only impacts netem based testing, I am going to commit a simple workaround referencing this issue for the details.

A more proper fix is to modify the .TLSClientFactory to use the TLS stack also used for .DialTLSContext.

@bassosimone bassosimone added bug Something isn't working priority/low ooni/probe-engine techdebt This issue describes technical debt labels Sep 15, 2023
@bassosimone bassosimone self-assigned this Sep 15, 2023
bassosimone added a commit to ooni/probe-cli that referenced this issue Sep 15, 2023
I'm glad I did this, because it allowed me to discover
ooni/probe#2536.

Apart from that, business as usual: adapt existing test cases for the
previous simpler HTTP proxy to use netem.

Reference issue: ooni/probe#2531

Overall objective: have better testing for the boostrap, which is
important to validate new beacons code.
@bassosimone bassosimone changed the title NewHTTPTransportWithOptions uses the wrong TLS stack NewHTTPTransportWithOptions with proxy uses the wrong TLS stack Oct 4, 2023
Murphy-OrangeMud pushed a commit to Murphy-OrangeMud/probe-cli that referenced this issue Feb 13, 2024
I'm glad I did this, because it allowed me to discover
ooni/probe#2536.

Apart from that, business as usual: adapt existing test cases for the
previous simpler HTTP proxy to use netem.

Reference issue: ooni/probe#2531

Overall objective: have better testing for the boostrap, which is
important to validate new beacons code.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ooni/probe-engine priority/low techdebt This issue describes technical debt
Projects
None yet
Development

No branches or pull requests

1 participant