QA: LTE handles more corner cases #2652

bassosimone · 2024-01-22T12:40:45Z

The purpose of this issue is to write integration tests and modify LTE such that it handles more corner case.

We also don't aim for strict v0.4 compatibility anymore and we'll try to do better whenever possible.

This commit modifies webconnectivitylte to handle ghost DNS censorship. We define ghost DNS censorship the case where the original domain does not exist anymore but the censor continues to return an IP address for the original domain nonetheless. We used to have null-null handling for this case in the "orig" engine and a reference issue as ooni/probe#2307. With this commit, we modify the "classic" engine to correctly handle this case. In doing this, we notice that we should probably start departing from strict v0.4 orthodoxy and instead produce better results in the following cases: 1. mark DNS inconsistency when we have successful probe lookups and no IP address has been resolved by the test helper; 2. specific handling on the case in which the website seems down where we also ask ourselves the question of whether the culprit could be the DNS. These two changes are required to handle ghost DNS censorship. Additionally, let's recognize that always setting HTTPExprimentFailure is probably for the greater good anyway, given that we're stopping to be very compatible with v0.4 anyway. Part of ooni/probe#2652.

## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: ooni/probe#2652 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: no need - [x] if you changed code inside an experiment, make sure you bump its version number: already bumped for 3.21.x ## Description This commit modifies webconnectivitylte to handle ghost DNS censorship. We define ghost DNS censorship the case where the original domain does not exist anymore but the censor continues to return an IP address for the original domain nonetheless. We used to have null-null handling for this case in the "orig" engine and a reference issue as ooni/probe#2307. With this commit, we modify the "classic" engine to correctly handle this case. To this end, we need to: 1. mark DNS inconsistency when we have successful probe lookups and no IP address has been resolved by the test helper, which is indeed the case of ghost DNS censorship. 2. specific handling on the case in which the website seems down where we also ask ourselves the question of whether the culprit could be the DNS and otherwise set accessible = false and blocking = false. Note that, with those two changes, we depart from strict v0.4-is-always-right orthodoxy. So, while there, let's recognize that always setting HTTPExprimentFailure is probably for the greater good.

This diff ensures that we do not lose any comment or test that we removed in #1455 and rather aims to ensure we have equivalent tests and comments. Part of ooni/probe#2652.

This diff corrects an embarassing bug in the logic we were using for expected TCP connect and TLS handshakes failures. We spotted this issue with the new testcase we added called websiteDownTCPConnect, which is also included. Additionally, we needed to fix some tests because the logic bugs were preventing us from seeing some events. Part of ooni/probe#2652. Closes ooni/probe#2299.

This diff corrects an embarrassing bugs in the logic we were using for expected TCP and TLS failures. We spotted this issue with the new testcase we added called websiteDownTCPConnect, which is also included. Additionally, we updated some tests because of the embarrassing bugs we fixed. Additionally, we fixed some comments that were outdated. Part of ooni/probe#2652. Closes ooni/probe#2299.

This diff modifies webconnectivitylte to have full IDNA support. It turns out existing code for supporting IDNA was not WAI for yandex.ru when using https://яндекс.рф/. I have fixed IDNA code, I ensured we always use the same IDNA code, manually tested (see below), and then generated syntethic test cases for continuous integration. See this measurement: https://explorer.ooni.org/m/20240123110911.104106_IT_webconnectivity_f0a3b891c176125b While there, I started adding initial support for printing the measurement URL based on the measurement UID. Closes ooni/probe#1925. Part of ooni/probe#2652.

## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: ooni/probe#2652, ooni/probe#1925 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: not needed - [x] if you changed code inside an experiment, make sure you bump its version number: already bumped for 3.21.x ## Description This diff fixes webconnectivitylte to make IDNA support working as intended. It turns out existing code for supporting IDNA was not WAI for yandex.ru when using https://яндекс.рф/. I have fixed IDNA code, ensured we always use the same correct IDNA code, manually tested (see below), and then generated syntethic test cases for continuous integration. See this measurement: https://explorer.ooni.org/m/20240123110911.104106_IT_webconnectivity_f0a3b891c176125b. While there, I started adding initial support for printing the measurement URL based on the measurement UID. Closes ooni/probe#1925. Part of ooni/probe#2652.

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 improves webconnectivitylte and minipipeline to correctly handle the case where a website has no configured IP addresses but the domain for the website exists. Let's also add QA test cases to make sure we don't regress. Part of ooni/probe#2652.

This diff improves webconnectivitylte and minipipeline to correctly handle the case where a website has no configured IP addresses but the domain for the website exists. When this happens, the TH returns no error but an empty list of resolved addresses for historical reasons, while the probe fails with the `"dns_no_answer"` error. We need to intercept and correctly handle this scenario both for the ordinary analysis and the classic analysis. Let's also add QA test cases to make sure we don't regress. Part of ooni/probe#2652.

bassosimone · 2024-01-24T18:02:58Z

I spent some time writing tests for corner cases. This amount of work seems enough to close this issue.

## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: ooni/probe#2652 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: no need - [x] if you changed code inside an experiment, make sure you bump its version number: already bumped for 3.21.x ## Description This commit modifies webconnectivitylte to handle ghost DNS censorship. We define ghost DNS censorship the case where the original domain does not exist anymore but the censor continues to return an IP address for the original domain nonetheless. We used to have null-null handling for this case in the "orig" engine and a reference issue as ooni/probe#2307. With this commit, we modify the "classic" engine to correctly handle this case. To this end, we need to: 1. mark DNS inconsistency when we have successful probe lookups and no IP address has been resolved by the test helper, which is indeed the case of ghost DNS censorship. 2. specific handling on the case in which the website seems down where we also ask ourselves the question of whether the culprit could be the DNS and otherwise set accessible = false and blocking = false. Note that, with those two changes, we depart from strict v0.4-is-always-right orthodoxy. So, while there, let's recognize that always setting HTTPExprimentFailure is probably for the greater good.

This diff ensures that we do not lose any comment or test that we removed in ooni#1455 and rather aims to ensure we have equivalent tests and comments. Part of ooni/probe#2652.

This diff corrects an embarrassing bugs in the logic we were using for expected TCP and TLS failures. We spotted this issue with the new testcase we added called websiteDownTCPConnect, which is also included. Additionally, we updated some tests because of the embarrassing bugs we fixed. Additionally, we fixed some comments that were outdated. Part of ooni/probe#2652. Closes ooni/probe#2299.

## Checklist - [x] I have read the [contribution guidelines](https://github.com/ooni/probe-cli/blob/master/CONTRIBUTING.md) - [x] reference issue for this pull request: ooni/probe#2652, ooni/probe#1925 - [x] if you changed anything related to how experiments work and you need to reflect these changes in the ooni/spec repository, please link to the related ooni/spec pull request: not needed - [x] if you changed code inside an experiment, make sure you bump its version number: already bumped for 3.21.x ## Description This diff fixes webconnectivitylte to make IDNA support working as intended. It turns out existing code for supporting IDNA was not WAI for yandex.ru when using https://яндекс.рф/. I have fixed IDNA code, ensured we always use the same correct IDNA code, manually tested (see below), and then generated syntethic test cases for continuous integration. See this measurement: https://explorer.ooni.org/m/20240123110911.104106_IT_webconnectivity_f0a3b891c176125b. While there, I started adding initial support for printing the measurement URL based on the measurement UID. Closes ooni/probe#1925. Part of ooni/probe#2652.

…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 improves webconnectivitylte and minipipeline to correctly handle the case where a website has no configured IP addresses but the domain for the website exists. When this happens, the TH returns no error but an empty list of resolved addresses for historical reasons, while the probe fails with the `"dns_no_answer"` error. We need to intercept and correctly handle this scenario both for the ordinary analysis and the classic analysis. Let's also add QA test cases to make sure we don't regress. Part of ooni/probe#2652.

bassosimone self-assigned this Jan 22, 2024

bassosimone added the data quality label Jan 22, 2024

bassosimone mentioned this issue Jan 22, 2024

feat(webconnectivitylte): handle ghost DNS censorship ooni/probe-cli#1457

Merged

4 tasks

jbonisteel added the funder/drl2022-2024 label Jan 22, 2024

bassosimone mentioned this issue Jan 22, 2024

feat(webconnectivitylte): add more tests and comments ooni/probe-cli#1458

Merged

bassosimone mentioned this issue Jan 22, 2024

minipipeline: fix expected TCP & TLS failures ooni/probe-cli#1459

Merged

bassosimone mentioned this issue Jan 23, 2024

fix(webconnectivitylte): make IDNA WAI ooni/probe-cli#1460

Merged

4 tasks

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

fix(webconnectivitylte): handle domains w/o A/AAAA records ooni/probe-cli#1473

Merged

bassosimone closed this as completed 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

QA: LTE handles more corner cases #2652

QA: LTE handles more corner cases #2652

bassosimone commented Jan 22, 2024 •

edited

Loading

bassosimone commented Jan 24, 2024

QA: LTE handles more corner cases #2652

QA: LTE handles more corner cases #2652

Comments

bassosimone commented Jan 22, 2024 • edited Loading

bassosimone commented Jan 24, 2024

bassosimone commented Jan 22, 2024 •

edited

Loading