Skip to content

Commit

Permalink
fix(netemx): address issues with quic-go/quic-go (#1228)
Browse files Browse the repository at this point in the history 
This diff fixes ooni/probe#2527.

The comment I originally wrote in the code to explain the issue read:

> I am wondering whether I am not fully understanding how
quic-go/quic-go works.

It turns out this comment was true. I was assuming http3.Server was
closing the listener, however, that was not the case.

I noticed this issue when reading again netemx/qaenv.go.

So, it all boiled down to PEBCAK 😬.

## 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#2527
- [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: N/A
- [x] if you changed code inside an experiment, make sure you bump its
version number: N/A
  • Loading branch information
@bassosimone
bassosimone authored Sep 4, 2023
1 parent 26d03fa commit 18b86e8
Show file tree
Hide file tree
Showing 8 changed files with 40 additions and 75 deletions.
2 changes: 1 addition & 1 deletion internal/netemx/example_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -371,7 +371,7 @@ func Example_oohelperdWithInternetScenario() {
})

// Output:
// {"tcp_connect":{"93.184.216.34:443":{"status":true,"failure":null}},"tls_handshake":{"93.184.216.34:443":{"server_name":"www.example.com","status":true,"failure":null}},"quic_handshake":{},"http_request":{"body_length":194,"discovered_h3_endpoint":"","failure":null,"title":"Default Web Page","headers":{"Content-Length":"194","Content-Type":"text/html; charset=utf-8","Date":"Thu, 24 Aug 2023 14:35:29 GMT"},"status_code":200},"http3_request":null,"dns":{"failure":null,"addrs":["93.184.216.34"]},"ip_info":{"93.184.216.34":{"asn":15133,"flags":11}}}
// {"tcp_connect":{"93.184.216.34:443":{"status":true,"failure":null}},"tls_handshake":{"93.184.216.34:443":{"server_name":"www.example.com","status":true,"failure":null}},"quic_handshake":{},"http_request":{"body_length":194,"discovered_h3_endpoint":"www.example.com:443","failure":null,"title":"Default Web Page","headers":{"Alt-Svc":"h3=\":443\"","Content-Length":"194","Content-Type":"text/html; charset=utf-8","Date":"Thu, 24 Aug 2023 14:35:29 GMT"},"status_code":200},"http3_request":null,"dns":{"failure":null,"addrs":["93.184.216.34"]},"ip_info":{"93.184.216.34":{"asn":15133,"flags":11}}}
}

// This example shows how the [InternetScenario] defines a GeoIP service like Ubuntu's one.
Expand Down
8 changes: 6 additions & 2 deletions internal/netemx/http3.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,11 @@ func (srv *http3Server) mustListenPortLocked(handler http.Handler, ipAddr net.IP
}
go srvr.Serve(listener)

// make sure we track the server (the .Serve method will close the
// listener once we close the server itself)
// make sure we track and close the listener: assuming the server was closing the
// listener seems to be the root cause of https://github.com/ooni/probe/issues/2527
// and closing the listener completely fixes the issue.
srv.closers = append(srv.closers, listener)

// make sure we track the server
srv.closers = append(srv.closers, srvr)
}
46 changes: 4 additions & 42 deletions internal/netemx/http3_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,27 +13,8 @@ import (

func TestHTTP3ServerFactory(t *testing.T) {
t.Run("when using the TLSConfig provided by netem", func(t *testing.T) {
/*
__ ________________________
/ \ / \__ ___/\_ _____/
\ \/\/ / | | | __)
\ / | | | \
\__/\ / |____| \___ /
\/ \/
I originally wrote this test to use AddressWwwExampleCom and the test
failed with generic_timeout_error. Now, instead, if I change it to use
10.55.56.57, the test is working as intended. I am wondering whether
I am not fully understanding how quic-go/quic-go works.
My (limited?) understanding: just a single test can use AddressWwwExampleCom
and, if I use it in other tests, there are issues leading to timeouts.
See https://github.com/ooni/probe/issues/2527.
*/

env := MustNewQAEnv(
QAEnvOptionNetStack("10.55.56.57", &HTTP3ServerFactory{
QAEnvOptionNetStack(AddressWwwExampleCom, &HTTP3ServerFactory{
Factory: HTTPHandlerFactoryFunc(func(_ *netem.UNetStack) http.Handler {
return ExampleWebPageHandler()
}),
Expand All @@ -43,7 +24,7 @@ func TestHTTP3ServerFactory(t *testing.T) {
)
defer env.Close()

env.AddRecordToAllResolvers("www.example.com", "", "10.55.56.57")
env.AddRecordToAllResolvers("www.example.com", "", AddressWwwExampleCom)

env.Do(func() {
client := netxlite.NewHTTP3ClientWithResolver(log.Log, netxlite.NewStdlibResolver(log.Log))
Expand All @@ -67,31 +48,12 @@ func TestHTTP3ServerFactory(t *testing.T) {
})

t.Run("when using an incompatible TLS config", func(t *testing.T) {
/*
__ ________________________
/ \ / \__ ___/\_ _____/
\ \/\/ / | | | __)
\ / | | | \
\__/\ / |____| \___ /
\/ \/
I originally wrote this test to use AddressWwwExampleCom and the test
failed with generic_timeout_error. Now, instead, if I change it to use
10.55.56.100, the test is working as intended. I am wondering whether
I am not fully understanding how quic-go/quic-go works.
My (limited?) understanding: just a single test can use AddressWwwExampleCom
and, if I use it in other tests, there are issues leading to timeouts.
See https://github.com/ooni/probe/issues/2527.
*/

// we're creating a distinct MITM TLS config and we're using it, so we expect
// that we're not able to verify certificates in client code
mitmConfig := runtimex.Try1(netem.NewTLSMITMConfig())

env := MustNewQAEnv(
QAEnvOptionNetStack("10.55.56.100", &HTTP3ServerFactory{
QAEnvOptionNetStack(AddressWwwExampleCom, &HTTP3ServerFactory{
Factory: HTTPHandlerFactoryFunc(func(_ *netem.UNetStack) http.Handler {
return ExampleWebPageHandler()
}),
Expand All @@ -101,7 +63,7 @@ func TestHTTP3ServerFactory(t *testing.T) {
)
defer env.Close()

env.AddRecordToAllResolvers("www.example.com", "", "10.55.56.100")
env.AddRecordToAllResolvers("www.example.com", "", AddressWwwExampleCom)

env.Do(func() {
client := netxlite.NewHTTP3ClientWithResolver(log.Log, netxlite.NewStdlibResolver(log.Log))
Expand Down
4 changes: 0 additions & 4 deletions internal/netemx/oohelperd.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,6 @@ func (f *OOHelperDFactory) NewHandler(unet *netem.UNetStack) http.Handler {
return netx.NewDialerWithResolver(logger, netx.NewStdlibResolver(logger))
}

// hard to test because of https://github.com/ooni/probe/issues/2527, which
// makes tests become flaky and fragile in an instant
handler.NewQUICDialer = func(logger model.Logger) model.QUICDialer {
return netx.NewQUICDialerWithResolver(
netx.NewQUICListener(),
Expand All @@ -59,8 +57,6 @@ func (f *OOHelperDFactory) NewHandler(unet *netem.UNetStack) http.Handler {
}
}

// hard to test because of https://github.com/ooni/probe/issues/2527, which
// makes tests become flaky and fragile in an instant
handler.NewHTTP3Client = func(logger model.Logger) model.HTTPClient {
cookieJar, _ := cookiejar.New(&cookiejar.Options{
PublicSuffixList: publicsuffix.List,
Expand Down
24 changes: 21 additions & 3 deletions internal/netemx/oohelperd_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,20 +74,38 @@ func TestOOHelperDHandler(t *testing.T) {
Failure: nil,
},
},
QUICHandshake: map[string]model.THTLSHandshakeResult{},
QUICHandshake: map[string]model.THTLSHandshakeResult{
"93.184.216.34:443": {
ServerName: "www.example.com",
Status: true,
Failure: nil,
},
},
HTTPRequest: model.THHTTPRequestResult{
BodyLength: 194,
DiscoveredH3Endpoint: "",
DiscoveredH3Endpoint: "www.example.com:443",
Failure: nil,
Title: "Default Web Page",
Headers: map[string]string{
"Alt-Svc": `h3=":443"`,
"Content-Length": "194",
"Content-Type": "text/html; charset=utf-8",
"Date": "Thu, 24 Aug 2023 14:35:29 GMT",
},
StatusCode: 200,
},
HTTP3Request: nil,
HTTP3Request: &model.THHTTPRequestResult{
BodyLength: 194,
DiscoveredH3Endpoint: "",
Failure: nil,
Title: "Default Web Page",
Headers: map[string]string{
"Alt-Svc": `h3=":443"`,
"Content-Type": "text/html; charset=utf-8",
"Date": "Thu, 24 Aug 2023 14:35:29 GMT",
},
StatusCode: 200,
},
DNS: model.THDNSResult{
Failure: nil,
Addrs: []string{"93.184.216.34"},
Expand Down
23 changes: 2 additions & 21 deletions internal/netemx/qaenv_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -136,29 +136,10 @@ func TestQAEnv(t *testing.T) {
// If all of this works, it means we're using the userspace TCP/IP
// stack exported by the [Environment] struct.
t.Run("we can hijack HTTP3 requests", func(t *testing.T) {
/*
__ ________________________
/ \ / \__ ___/\_ _____/
\ \/\/ / | | | __)
\ / | | | \
\__/\ / |____| \___ /
\/ \/
I originally wrote this test to use AddressWwwExampleCom and the test
failed with generic_timeout_error. Now, instead, if I change it to use
10.55.56.101, the test is working as intended. I am wondering whether
I am not fully understanding how quic-go/quic-go works.
My (limited?) understanding: just a single test can use AddressWwwExampleCom
and, if I use it in other tests, there are issues leading to timeouts.
See https://github.com/ooni/probe/issues/2527.
*/

// create QA env
env := netemx.MustNewQAEnv(
netemx.QAEnvOptionHTTPServer(
"10.55.56.101",
netemx.AddressWwwExampleCom,
netemx.ExampleWebPageHandlerFactory(),
),
)
Expand All @@ -168,7 +149,7 @@ func TestQAEnv(t *testing.T) {
env.AddRecordToAllResolvers(
"www.example.com",
"", // CNAME
"10.55.56.101",
netemx.AddressWwwExampleCom,
)

env.Do(func() {
Expand Down
4 changes: 4 additions & 0 deletions internal/netemx/scenario.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,10 @@ func MustNewScenario(config []*ScenarioDomainAddresses) *QAEnv {
Factory: sad.WebServerFactory,
Ports: []int{443},
TLSConfig: nil, // use netem's default
}, &HTTP3ServerFactory{
Factory: sad.WebServerFactory,
Ports: []int{443},
TLSConfig: nil, // use netem's default
}))
}

Expand Down
4 changes: 2 additions & 2 deletions internal/netemx/web.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ const ExampleWebPage = `<!doctype html>
// is www.example.{com,org} and redirecting to www. when the domain is example.{com,org}.
func ExampleWebPageHandler() http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
//w.Header().Add("Alt-Svc", `h3=":443"`) // see https://github.com/ooni/probe/issues/2527
w.Header().Add("Alt-Svc", `h3=":443"`)
w.Header().Add("Date", "Thu, 24 Aug 2023 14:35:29 GMT")

// According to Go documentation, the host header is removed from the
Expand Down Expand Up @@ -87,7 +87,7 @@ const Blockpage = `<!doctype html>
func BlockpageHandlerFactory() HTTPHandlerFactory {
return HTTPHandlerFactoryFunc(func(_ *netem.UNetStack) http.Handler {
return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
//w.Header().Add("Alt-Svc", `h3=":443"`) // see https://github.com/ooni/probe/issues/2527
w.Header().Add("Alt-Svc", `h3=":443"`)
w.Header().Add("Date", "Thu, 24 Aug 2023 14:35:29 GMT")
w.Write([]byte(Blockpage))
})
Expand Down

0 comments on commit 18b86e8

Please sign in to comment.