fix(webconnectivitylte): handle measurements with loopback addrs #1462
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bassosimone
added a commit
that referenced
this pull request
Jan 24, 2024
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.
bassosimone
added a commit
that referenced
this pull request
Jan 24, 2024
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.
Conflicts: internal/netemx/oohelperd.go internal/oohelperd/handler.go
bassosimone
added a commit
that referenced
this pull request
Jan 24, 2024
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.
bassosimone
commented
Jan 24, 2024
bassosimone
commented
Jan 24, 2024
bassosimone
changed the title
bassosimone
commented
Jan 24, 2024
bassosimone
commented
Jan 24, 2024
bassosimone
commented
Jan 24, 2024
bassosimone
commented
Jan 24, 2024
bassosimone
commented
Jan 24, 2024
bassosimone
commented
Jan 24, 2024
bassosimone
commented
Jan 24, 2024
| @@ -74,12 +74,12 @@ func (sx *Set[T]) UnmarshalJSON(data []byte) error { | |||
| } | |||
|
|
|||
| // Contains returns whether the set contains a key. | |||
| func (sx *Set[T]) Contains(key T) bool { | |||
| func (sx Set[T]) Contains(key T) bool { | |||
There was a problem hiding this comment.
No need to use pointer receiver here
bassosimone
commented
Jan 24, 2024
| _, found := sx.state[key] | ||
| return found | ||
| } | ||
|
|
||
| // String implements fmt.Stringer | ||
| func (sx *Set[T]) String() string { | ||
| func (sx Set[T]) String() string { |
There was a problem hiding this comment.
No need to use pointer receiver here
This was referenced Jan 24, 2024
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
…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.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
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.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
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.
Murphy-OrangeMud
pushed a commit
to Murphy-OrangeMud/probe-cli
that referenced
this pull request
Feb 13, 2024
…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.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.