Skip to content

Commit

Permalink
fix(oohelperd): use cookiejar for HTTP measurements (#1149)
Browse files Browse the repository at this point in the history
See ooni/probe#2488.

This patch needs to be backported to the release/3.17 branch.
  • Loading branch information
bassosimone committed Jun 5, 2023
1 parent e93dd1e commit 6aceca1
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 23 deletions.
79 changes: 57 additions & 22 deletions internal/cmd/oohelperd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"fmt"
"net"
"net/http"
"net/http/cookiejar"
"net/http/pprof"
"os"
"os/signal"
Expand All @@ -21,6 +22,7 @@ import (
"github.com/ooni/probe-cli/v3/internal/runtimex"
"github.com/ooni/probe-cli/v3/internal/version"
"github.com/prometheus/client_golang/prometheus/promhttp"
"golang.org/x/net/publicsuffix"
)

// maxAcceptableBodySize is the maximum acceptable body size for incoming
Expand Down Expand Up @@ -79,41 +81,74 @@ func shutdown(srv *http.Server, wg *sync.WaitGroup) {
srv.Shutdown(ctx)
}

// newCookieJar is the factory for constructing a new cookier jar.
func newCookieJar() *cookiejar.Jar {
// Implementation note: the [cookiejar.New] function always returns a
// nil error; hence, it's safe here to use [runtimex.Try1].
return runtimex.Try1(cookiejar.New(&cookiejar.Options{
PublicSuffixList: publicsuffix.List,
}))
}

// newHTTPClientWithTransportFactory creates a new HTTP client.
func newHTTPClientWithTransportFactory(
logger model.Logger,
txpFactory func(model.DebugLogger, model.Resolver) model.HTTPTransport,
) model.HTTPClient {
// If the DoH resolver we're using insists that a given domain maps to
// bogons, make sure we're going to fail the HTTP measurement.
//
// The TCP measurements scheduler in ipinfo.go will also refuse to
// schedule TCP measurements for bogons.
//
// While this seems theoretical, as of 2022-08-28, I see:
//
// % host polito.it
// polito.it has address 192.168.59.6
// polito.it has address 192.168.40.1
// polito.it mail is handled by 10 mx.polito.it.
//
// So, it's better to consider this as a possible corner case.
reso := netxlite.MaybeWrapWithBogonResolver(
true, // enabled
newResolver(logger),
)

// fix: We MUST set a cookie jar for measuring HTTP. See
// https://github.com/ooni/probe/issues/2488 for additional
// context and pointers to the relevant measurements.
client := &http.Client{
Transport: txpFactory(logger, reso),
CheckRedirect: nil,
Jar: newCookieJar(),
Timeout: 0,
}

return netxlite.WrapHTTPClient(client)
}

// newHandler constructs the [handler] used by [main].
func newHandler() *handler {
return &handler{
BaseLogger: log.Log,
Indexer: &atomic.Int64{},
MaxAcceptableBody: maxAcceptableBodySize,
Measure: measure,

NewHTTPClient: func(logger model.Logger) model.HTTPClient {
// If the DoH resolver we're using insists that a given domain maps to
// bogons, make sure we're going to fail the HTTP measurement.
//
// The TCP measurements scheduler in ipinfo.go will also refuse to
// schedule TCP measurements for bogons.
//
// While this seems theoretical, as of 2022-08-28, I see:
//
// % host polito.it
// polito.it has address 192.168.59.6
// polito.it has address 192.168.40.1
// polito.it mail is handled by 10 mx.polito.it.
//
// So, it's better to consider this as a possible corner case.
reso := netxlite.MaybeWrapWithBogonResolver(
true, // enabled
newResolver(logger),
return newHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTPTransportWithResolver,
)
return netxlite.NewHTTPClientWithResolver(logger, reso)
},

NewHTTP3Client: func(logger model.Logger) model.HTTPClient {
reso := netxlite.MaybeWrapWithBogonResolver(
true, // enabled
newResolver(logger),
return newHTTPClientWithTransportFactory(
logger,
netxlite.NewHTTP3TransportWithResolver,
)
return netxlite.NewHTTP3ClientWithResolver(logger, reso)
},

NewDialer: func(logger model.Logger) model.Dialer {
return netxlite.NewDialerWithoutResolver(logger)
},
Expand Down
2 changes: 1 addition & 1 deletion internal/netxlite/http3.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ func NewHTTP3TransportStdlib(logger model.DebugLogger) model.HTTPTransport {

// NewHTTPTransportWithResolver creates a new HTTPTransport using http3
// that uses the given logger and the given resolver.
func NewHTTP3TransportWithResolver(logger model.Logger, reso model.Resolver) model.HTTPTransport {
func NewHTTP3TransportWithResolver(logger model.DebugLogger, reso model.Resolver) model.HTTPTransport {
qd := NewQUICDialerWithResolver(NewQUICListener(), logger, reso)
return NewHTTP3Transport(logger, qd, nil)
}
Expand Down

0 comments on commit 6aceca1

Please sign in to comment.