web_connectivity marks DNS 127.0.0.1 answers as ok #1517

fortuna · 2020-11-20T08:46:41Z

Describe the bug
In web_connectivity, when the DNS answer is 127.0.0.1, it's still marked as ok, even though it's clearly a bad answer.

To Reproduce
Examples:

Expected behavior
We should mark as anomaly if it's a non-global IP.

Context
I'm seeing that a lot in Venezuela when you use a Google resolver (AS15169) on AS21826. Though it's not consistent. Sometimes I get a valid answer: https://explorer.ooni.org/measurement/20201120T010447Z_webconnectivity_VE_21826_n1_hqwmWVMRkojy7iCn?input=https%3A%2F%2Fwww.tunnelbear.com%2F

I wonder if it's a probe bug.

This diff adds extra tests and logging showing that we're not connecting to 127.0.0.1 in endpoints code. We still need to deal with making sure we don't connect to 127.0.0.1 in HTTP code. In principle, this is already true, but the QA framework constructs the oohelperd differently than with the default constructor, and we'll need to improve in this department to make sure the results obtained in the QA suite also give us confidence on the general oohelperd behavior. For now, QA tests attempting to use 127.0.0.1 do not produce the correct result because oohelperd misbehaves. Extracted from #1462. Part of ooni/probe#2652. Part of ooni/probe#1517.

This diff modifies oohelperd and netemx to ensure we construct equivalent HTTPTransports. Let's show that this is actually the case. Let's start from the HTTP/1.1 and HTTP2 transport. This is what `oohelperd` does after this diff: ```Go NewHTTPClient: func(logger model.Logger) model.HTTPClient { // TODO(ooni/probe#2534): the NewHTTPTransportWithResolver has QUIRKS and // we should evaluate whether we can avoid using it here return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTPTransportWithResolver, ) }, ``` This is what `netemx` does after this diff: ```Go handler.NewHTTPClient = func(logger model.Logger) model.HTTPClient { return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) }, ) } ``` We're using the same (now public) `NewHTTPClientWithTransportFactory` function. So, what remains to be done to reach a QED is to show that this code called by `oohelperd`: ```Go return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTPTransportWithResolver, ) ``` is equivalent to this code called by `netemx`: ```Go func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) }, ``` This is evident if we expand `netxlite.NewHTTPTransportWithResolver`, whose implementation is: ```Go func NewHTTPTransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport { dialer := NewDialerWithResolver(logger, reso) thx := NewTLSHandshakerStdlib(logger) tlsDialer := NewTLSDialer(dialer, thx) return NewHTTPTransport(logger, dialer, tlsDialer) } ``` in fact, the following lines of code called from `oohelperd`: ```Go dialer := NewDialerWithResolver(logger, reso) thx := NewTLSHandshakerStdlib(logger) tlsDialer := NewTLSDialer(dialer, thx) return NewHTTPTransport(logger, dialer, tlsDialer) ``` are equivalent to this `netemx` code: ```Go dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) ``` Modulo the fact that `netemx` code is using methods of the `*netxlite.Netx` structure rather than bare functions. Let's now inspect how we construct HTTP3. This is what `oohelperd` does: ```Go NewHTTP3Client: func(logger model.Logger) model.HTTPClient { return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTP3TransportWithResolver, ) }, ``` This is what `netemx` does: ```Go handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient { return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) }, ) } ``` Because we're using the same `NewHTTPClientWithTransportFactory` factory, we need to show that `oohelperd`'s ```Go return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTP3TransportWithResolver, ) ``` is equivalent to `netemx`'s ```Go return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) }, ) ``` To show that we need to expand `NewHTTP3TransportWithResolver`, which reads: ```Go func NewHTTP3TransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport { qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso) return NewHTTP3Transport(logger, qd, nil) } ``` And then we can conclude that we're good because the code invoked by `oohelperd`: ```Go qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso) return NewHTTP3Transport(logger, qd, nil) ``` is equivalent to the code invoked by `netemx`: ```Go qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) ``` modulo the fact that `netemx` is using methods defined by the `netx` object. Extracted from #1462. Part of ooni/probe#2652. Part of ooni/probe#1517.

This diff removes the first-order implicit-Netx wrappers. We define first-order implicit-Netx wrappers the top-level functions that have the same name of netxlite.Netx methods, allocate an empty Netx, and call the corresponding method. The reason why we're doing this now is that it has been relatively hard to implement #1464 because of the ambiguity between those first-order wrappers and the methods. Getting this wrong means that QA tests would behave in a funny way. The overall goal here is to refactor `oohelperd` to only depend on `netxlite.Netx` such that we can remove the code duplication between how we instantiate `oohelperd.Handler` in `oohelperd` and how we instantiate it inside `netemx`. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

This diff modifies netxlite such that the oohelperd only depends on the netxlite.Netx instance. The overall goal here is to refactor `oohelperd` to only depend on `netxlite.Netx` such that we can remove the code duplication between how we instantiate `oohelperd.Handler` in `oohelperd` and how we instantiate it inside `netemx`. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

This diff modifies netxlite such that the oohelperd only depends on a netxlite.Netx instance. The overall goal here is to refactor `oohelperd` to only depend on `netxlite.Netx` such that we can remove the code duplication between how we instantiate `oohelperd.Handler` in `oohelperd` and how we instantiate it inside `netemx`. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

This diff modifies how we construct oohelperd.Handler inside the oohelperd package and inside the netemx package such that we're now constructing using equivalent code. We know that construction is equivalent for HTTP clients because previosuly we made sure it was the case in #1464. So, the next step would be removing the custom construction code inside of netemx and always use oohelperd.NewHandler. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

In #1467 we made the netemx constructor for oohelperd.Handler equivalent to oohelperd.NewHandler. So, now it becomes possible to always use oohelperd.NewHandler. While there, notice that we can make all the Handler fields private because there's no need to share them anymore, so do that. Having done this, we are now sure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

This diff extracts the part of #1462 that only consists in regenerating test cases for minipipeline. Because the measurements are not deterministic, we have some churn every time we rerun the script that regenerates such test cases. Reference issue: ooni/probe#1517.

This diff modifies webconnectivitylte and related packages to correctly handle measurements containing loopback addresses. There are two cases: (a) when both the probe and the TH only see loopback addresses at least for the getaddrinfo lookups; (b) when only the probe sees loopback addresses at least for the getaddrinfo lookups. In the former case, we should mark the website as "down" (maybe a little bit of a stretch, but if we mark down a website where the TLS is misconfigured, arguably we can do the same when DNS is misconfigured). In the latter case, we mark the measurement as censorship, since seeing a loopback address is not what we expect. Closes ooni/probe#1517.

…1463) This diff adds extra tests and logging showing that we're not connecting to 127.0.0.1 in endpoints code. We still need to deal with making sure we don't connect to 127.0.0.1 in HTTP code. In principle, this is already true, but the QA framework constructs the oohelperd differently than with the default constructor, and we'll need to improve in this department to make sure the results obtained in the QA suite also give us confidence on the general oohelperd behavior. For now, QA tests attempting to use 127.0.0.1 do not produce the correct result because oohelperd misbehaves. Extracted from ooni#1462. Part of ooni/probe#2652. Part of ooni/probe#1517.

This diff modifies oohelperd and netemx to ensure we construct equivalent HTTPTransports. Let's show that this is actually the case. Let's start from the HTTP/1.1 and HTTP2 transport. This is what `oohelperd` does after this diff: ```Go NewHTTPClient: func(logger model.Logger) model.HTTPClient { // TODO(ooni/probe#2534): the NewHTTPTransportWithResolver has QUIRKS and // we should evaluate whether we can avoid using it here return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTPTransportWithResolver, ) }, ``` This is what `netemx` does after this diff: ```Go handler.NewHTTPClient = func(logger model.Logger) model.HTTPClient { return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) }, ) } ``` We're using the same (now public) `NewHTTPClientWithTransportFactory` function. So, what remains to be done to reach a QED is to show that this code called by `oohelperd`: ```Go return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTPTransportWithResolver, ) ``` is equivalent to this code called by `netemx`: ```Go func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) }, ``` This is evident if we expand `netxlite.NewHTTPTransportWithResolver`, whose implementation is: ```Go func NewHTTPTransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport { dialer := NewDialerWithResolver(logger, reso) thx := NewTLSHandshakerStdlib(logger) tlsDialer := NewTLSDialer(dialer, thx) return NewHTTPTransport(logger, dialer, tlsDialer) } ``` in fact, the following lines of code called from `oohelperd`: ```Go dialer := NewDialerWithResolver(logger, reso) thx := NewTLSHandshakerStdlib(logger) tlsDialer := NewTLSDialer(dialer, thx) return NewHTTPTransport(logger, dialer, tlsDialer) ``` are equivalent to this `netemx` code: ```Go dialer := netx.NewDialerWithResolver(dl, r) tlsDialer := netxlite.NewTLSDialer(dialer, netx.NewTLSHandshakerStdlib(dl)) // TODO(ooni/probe#2534): NewHTTPTransport is QUIRKY but // we probably don't care about using a QUIRKY function here return netxlite.NewHTTPTransport(dl, dialer, tlsDialer) ``` Modulo the fact that `netemx` code is using methods of the `*netxlite.Netx` structure rather than bare functions. Let's now inspect how we construct HTTP3. This is what `oohelperd` does: ```Go NewHTTP3Client: func(logger model.Logger) model.HTTPClient { return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTP3TransportWithResolver, ) }, ``` This is what `netemx` does: ```Go handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient { return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) }, ) } ``` Because we're using the same `NewHTTPClientWithTransportFactory` factory, we need to show that `oohelperd`'s ```Go return NewHTTPClientWithTransportFactory( logger, netxlite.NewHTTP3TransportWithResolver, ) ``` is equivalent to `netemx`'s ```Go return oohelperd.NewHTTPClientWithTransportFactory( logger, func(dl model.DebugLogger, r model.Resolver) model.HTTPTransport { qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) }, ) ``` To show that we need to expand `NewHTTP3TransportWithResolver`, which reads: ```Go func NewHTTP3TransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport { qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso) return NewHTTP3Transport(logger, qd, nil) } ``` And then we can conclude that we're good because the code invoked by `oohelperd`: ```Go qd := NewQUICDialerWithResolver(NewUDPListener(), logger, reso) return NewHTTP3Transport(logger, qd, nil) ``` is equivalent to the code invoked by `netemx`: ```Go qd := netx.NewQUICDialerWithResolver(netx.NewUDPListener(), dl, r) return netxlite.NewHTTP3Transport(dl, qd, nil) ``` modulo the fact that `netemx` is using methods defined by the `netx` object. Extracted from ooni#1462. Part of ooni/probe#2652. Part of ooni/probe#1517.

This diff removes the first-order implicit-Netx wrappers. We define first-order implicit-Netx wrappers the top-level functions that have the same name of netxlite.Netx methods, allocate an empty Netx, and call the corresponding method. The reason why we're doing this now is that it has been relatively hard to implement ooni#1464 because of the ambiguity between those first-order wrappers and the methods. Getting this wrong means that QA tests would behave in a funny way. The overall goal here is to refactor `oohelperd` to only depend on `netxlite.Netx` such that we can remove the code duplication between how we instantiate `oohelperd.Handler` in `oohelperd` and how we instantiate it inside `netemx`. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

This diff modifies netxlite such that the oohelperd only depends on a netxlite.Netx instance. The overall goal here is to refactor `oohelperd` to only depend on `netxlite.Netx` such that we can remove the code duplication between how we instantiate `oohelperd.Handler` in `oohelperd` and how we instantiate it inside `netemx`. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

This diff modifies how we construct oohelperd.Handler inside the oohelperd package and inside the netemx package such that we're now constructing using equivalent code. We know that construction is equivalent for HTTP clients because previosuly we made sure it was the case in ooni#1464. So, the next step would be removing the custom construction code inside of netemx and always use oohelperd.NewHandler. In turn, by doing this, we would ensure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

…i#1468) In ooni#1467 we made the netemx constructor for oohelperd.Handler equivalent to oohelperd.NewHandler. So, now it becomes possible to always use oohelperd.NewHandler. While there, notice that we can make all the Handler fields private because there's no need to share them anymore, so do that. Having done this, we are now sure we have the same `oohelperd` behavior for QA and production. In turn, with this guarantee, we can write QA tests that ensure we're correctly dealing with 127.0.0.1. The reference issue is ooni/probe#1517.

This diff extracts the part of ooni#1462 that only consists in regenerating test cases for minipipeline. Because the measurements are not deterministic, we have some churn every time we rerun the script that regenerates such test cases. Reference issue: ooni/probe#1517.

…i#1462) This diff modifies webconnectivitylte and related packages to correctly handle measurements containing loopback addresses. There are two cases: (a) when both the probe and the TH only see loopback addresses at least for the getaddrinfo lookups; (b) when only the probe sees loopback addresses at least for the getaddrinfo lookups. In the former case, we should mark the website as "down" (maybe a little bit of a stretch, but if we mark down a website where the TLS is misconfigured, arguably we can do the same when DNS is misconfigured). In the latter case, we mark the measurement as censorship, since seeing a loopback address is not what we expect. Closes ooni/probe#1517.

fortuna assigned bassosimone Nov 20, 2020

bassosimone transferred this issue from ooni/probe-engine Jun 7, 2021

hellais added data quality priority/medium ooni/probe-engine labels Jun 10, 2021

bassosimone added the bug Something isn't working label Jul 6, 2021

bassosimone added priority/low and removed priority/medium labels Nov 11, 2021

bassosimone mentioned this issue Jan 24, 2024

fix(oohelperd): make sure endpoints don't connect to 127.0.0.1 ooni/probe-cli#1463

Merged

bassosimone mentioned this issue Jan 24, 2024

fix(oohelperd,netemx): construct equivalent HTTPTransports ooni/probe-cli#1464

Merged

bassosimone mentioned this issue Jan 24, 2024

cleanup(netxlite): remove first-order implicit-Netx wrappers ooni/probe-cli#1465

Merged

bassosimone mentioned this issue Jan 24, 2024

refactor(oohelperd): depend on netxlite.Netx only ooni/probe-cli#1466

Merged

bassosimone mentioned this issue Jan 24, 2024

refactor(oohelperd,netmx): reduce construction diff to zero ooni/probe-cli#1467

Merged

bassosimone mentioned this issue Jan 24, 2024

refactor(netemx,oohelperd): use oohelperd.NewHandler constructor ooni/probe-cli#1468

Merged

bassosimone mentioned this issue Jan 24, 2024

chore(minipipeline): regenerate test cases ooni/probe-cli#1469

Merged

bassosimone mentioned this issue Jan 24, 2024

fix(webconnectivitylte): handle measurements with loopback addrs ooni/probe-cli#1462

Merged

bassosimone closed this as completed in ooni/probe-cli#1462 Jan 24, 2024

bassosimone added the 2024-01-data-quality-cleanup Data quality issues addressed on 2024-01 label Jan 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

web_connectivity marks DNS 127.0.0.1 answers as ok #1517

web_connectivity marks DNS 127.0.0.1 answers as ok #1517

fortuna commented Nov 20, 2020

web_connectivity marks DNS 127.0.0.1 answers as ok #1517

web_connectivity marks DNS 127.0.0.1 answers as ok #1517

Comments

fortuna commented Nov 20, 2020