From 6efffc5a961e14c05f160af012ae3383f3cdc49b Mon Sep 17 00:00:00 2001 From: Simone Basso Date: Wed, 17 Apr 2024 10:28:52 +0200 Subject: [PATCH] feat(enginenetx): test `bridgesPolicy` with DNS success (#1554) This diff introduces a test case for `bridgesPolicy` where we count after how many policies we observe a DNS-generated policy. This test has been crucial to investigate https://github.com/ooni/probe/issues/2704. Based on this test we can conclude the following: 1. if the bridge IP address gets blocked or stops working, we're still falling back to using the DNS; 2. however, the current algorithm does that in a too-slow fashion. Additionally, I manually verified that we're actually falling back to the DNS and that it really takes a long time by changing the implementation to use `10.0.0.1` as the bridge address and verifying that the code behaves as expected (though the "expected" behavior here is not nice at all and we should improve upon that). While there, fix naming and comments. --- internal/enginenetx/bridgespolicy_test.go | 76 ++++++++++++++++++++++- internal/enginenetx/httpsdialer.go | 6 +- internal/enginenetx/network.go | 14 ++--- internal/enginenetx/statsmanager.go | 6 +- internal/enginenetx/statspolicy.go | 4 +- internal/enginenetx/statspolicy_test.go | 6 +- internal/enginenetx/userpolicy.go | 2 +- 7 files changed, 92 insertions(+), 22 deletions(-) diff --git a/internal/enginenetx/bridgespolicy_test.go b/internal/enginenetx/bridgespolicy_test.go index 21521a1387..0b319b3835 100644 --- a/internal/enginenetx/bridgespolicy_test.go +++ b/internal/enginenetx/bridgespolicy_test.go @@ -9,7 +9,7 @@ import ( "github.com/ooni/probe-cli/v3/internal/model" ) -func TestBeaconsPolicy(t *testing.T) { +func TestBridgesPolicy(t *testing.T) { t.Run("for domains for which we don't have bridges and DNS failure", func(t *testing.T) { expected := errors.New("mocked error") p := &bridgesPolicy{ @@ -76,7 +76,7 @@ func TestBeaconsPolicy(t *testing.T) { } }) - t.Run("for the api.ooni.io domain", func(t *testing.T) { + t.Run("for the api.ooni.io domain with DNS failure", func(t *testing.T) { expected := errors.New("mocked error") p := &bridgesPolicy{ Fallback: &dnsPolicy{ @@ -92,6 +92,7 @@ func TestBeaconsPolicy(t *testing.T) { ctx := context.Background() tactics := p.LookupTactics(ctx, "api.ooni.io", "443") + // since the DNS fails, we should only see tactics generated by bridges var count int for tactic := range tactics { count++ @@ -117,6 +118,77 @@ func TestBeaconsPolicy(t *testing.T) { } }) + t.Run("for the api.ooni.io domain with DNS success", func(t *testing.T) { + p := &bridgesPolicy{ + Fallback: &dnsPolicy{ + Logger: model.DiscardLogger, + Resolver: &mocks.Resolver{ + MockLookupHost: func(ctx context.Context, domain string) ([]string, error) { + return []string{"130.192.91.211"}, nil + }, + }, + }, + } + + ctx := context.Background() + tactics := p.LookupTactics(ctx, "api.ooni.io", "443") + + // since the DNS succeeds we should see bridge tactics mixed with DNS tactics + var ( + bridgesCount int + dnsCount int + overallCount int + ) + const expectedDNSEntryCount = 153 // yikes! + for tactic := range tactics { + overallCount++ + + t.Log(overallCount, tactic) + + if tactic.Port != "443" { + t.Fatal("the port should always be 443") + } + + switch { + case overallCount == expectedDNSEntryCount: + if tactic.Address != "130.192.91.211" { + t.Fatal("the host should be 130.192.91.211 for count ==", expectedDNSEntryCount) + } + + if tactic.SNI != "api.ooni.io" { + t.Fatal("we should see the `api.ooni.io` SNI on the wire for count ==", expectedDNSEntryCount) + } + + dnsCount++ + + default: + if tactic.Address != "162.55.247.208" { + t.Fatal("the host should be 162.55.247.208 for count !=", expectedDNSEntryCount) + } + + if tactic.SNI == "api.ooni.io" { + t.Fatal("we should not see the `api.ooni.io` SNI on the wire for count !=", expectedDNSEntryCount) + } + + bridgesCount++ + } + + if tactic.VerifyHostname != "api.ooni.io" { + t.Fatal("the VerifyHostname field should always be like `api.ooni.io`") + } + } + + if overallCount <= 0 { + t.Fatal("expected to see at least one tactic") + } + if dnsCount != 1 { + t.Fatal("expected to see exactly one DNS based tactic") + } + if bridgesCount <= 0 { + t.Fatal("expected to see at least one bridge tactic") + } + }) + t.Run("for test helper domains", func(t *testing.T) { for _, domain := range bridgesPolicyTestHelpersDomains { t.Run(domain, func(t *testing.T) { diff --git a/internal/enginenetx/httpsdialer.go b/internal/enginenetx/httpsdialer.go index 12c46e314f..038569fac8 100644 --- a/internal/enginenetx/httpsdialer.go +++ b/internal/enginenetx/httpsdialer.go @@ -97,7 +97,7 @@ type httpsDialerPolicy interface { // httpsDialerEventsHandler handles events occurring while we try dialing TLS. type httpsDialerEventsHandler interface { - // These callbacks are invoked during the TLS handshake to inform this + // These callbacks are invoked during the TLS dialing to inform this // interface about events that occurred. A policy SHOULD keep track of which // addresses, SNIs, etc. work and return them more frequently. // @@ -236,8 +236,10 @@ func (hd *httpsDialer) DialTLSContext(ctx context.Context, network string, endpo continue } - // Save the conn and tell goroutines to stop ASAP + // Save the conn connv = append(connv, result.Conn) + + // Interrupt other concurrent dialing attempts cancel() } } diff --git a/internal/enginenetx/network.go b/internal/enginenetx/network.go index fce1d53c7c..e681de1f1e 100644 --- a/internal/enginenetx/network.go +++ b/internal/enginenetx/network.go @@ -93,7 +93,8 @@ func NewNetwork( netx := &netxlite.Netx{} dialer := netx.NewDialerWithResolver(logger, resolver) - // Create manager for keeping track of statistics + // Create manager for keeping track of statistics. This implies creating a background + // goroutine that we'll need to close when we're done. const trimInterval = 30 * time.Second stats := newStatsManager(kvStore, logger, trimInterval) @@ -118,15 +119,8 @@ func NewNetwork( // the proxy, otherwise it means that we're using the ooni/oohttp library // to dial for proxies, which has some restrictions. // - // In particular, the returned transport uses dialer for dialing with - // cleartext proxies (e.g., socks5 and http) and httpsDialer for dialing - // with encrypted proxies (e.g., https). After this has happened, - // the code currently falls back to using the standard library's tls - // client code for establishing TLS connections over the proxy. The main - // implication here is that we're not using our custom mozilla CA for - // validating TLS certificates, rather we're using the system's cert store. - // - // Fixing this issue is TODO(https://github.com/ooni/probe/issues/2536). + // - this code does not work as intended when using netem and proxies + // as documented by TODO(https://github.com/ooni/probe/issues/2536). txp := netxlite.NewHTTPTransportWithOptions( logger, dialer, httpsDialer, netxlite.HTTPTransportOptionDisableCompression(false), diff --git a/internal/enginenetx/statsmanager.go b/internal/enginenetx/statsmanager.go index a95c9aa9e0..e82bcd0d0b 100644 --- a/internal/enginenetx/statsmanager.go +++ b/internal/enginenetx/statsmanager.go @@ -137,6 +137,8 @@ func statsDefensivelySortTacticsByDescendingSuccessRateWithAcceptPredicate( input []*statsTactic, acceptfunc func(*statsTactic) bool) []*statsTactic { // first let's create a working list such that we don't modify // the input in place thus avoiding any data race + // + // make sure we explicitly filter out malformed entries work := []*statsTactic{} for _, t := range input { if t != nil && t.Tactic != nil { @@ -193,8 +195,8 @@ func (st *statsTactic) Clone() *statsTactic { // a pointer to a location which is typically immutable, so it's perfectly // fine to copy the LastUpdate field by assignment. // - // here we're using a bunch of robustness aware mechanisms to clone - // considering that the struct may be edited by the user + // here we're using safe functions to clone the original struct considering + // that a user can edit the content on disk freely introducing nulls. return &statsTactic{ CountStarted: st.CountStarted, CountTCPConnectError: st.CountTCPConnectError, diff --git a/internal/enginenetx/statspolicy.go b/internal/enginenetx/statspolicy.go index 8ff144f6a8..75acf397eb 100644 --- a/internal/enginenetx/statspolicy.go +++ b/internal/enginenetx/statspolicy.go @@ -61,7 +61,7 @@ func (p *statsPolicy) LookupTactics(ctx context.Context, domain string, port str } // give priority to what we know from stats - for _, t := range statsPolicyPostProcessTactics(p.Stats.LookupTactics(domain, port)) { + for _, t := range statsPolicyFilterStatsTactics(p.Stats.LookupTactics(domain, port)) { maybeEmitTactic(t) } @@ -74,7 +74,7 @@ func (p *statsPolicy) LookupTactics(ctx context.Context, domain string, port str return out } -func statsPolicyPostProcessTactics(tactics []*statsTactic, good bool) (out []*httpsDialerTactic) { +func statsPolicyFilterStatsTactics(tactics []*statsTactic, good bool) (out []*httpsDialerTactic) { // when good is false, it means p.Stats.LookupTactics failed if !good { return diff --git a/internal/enginenetx/statspolicy_test.go b/internal/enginenetx/statspolicy_test.go index e7c66514e9..31880dccf6 100644 --- a/internal/enginenetx/statspolicy_test.go +++ b/internal/enginenetx/statspolicy_test.go @@ -319,9 +319,9 @@ func (p *mocksPolicy) LookupTactics(ctx context.Context, domain string, port str return p.MockLookupTactics(ctx, domain, port) } -func TestStatsPolicyPostProcessTactics(t *testing.T) { +func TestStatsPolicyFilterStatsTactics(t *testing.T) { t.Run("we do nothing when good is false", func(t *testing.T) { - tactics := statsPolicyPostProcessTactics(nil, false) + tactics := statsPolicyFilterStatsTactics(nil, false) if len(tactics) != 0 { t.Fatal("expected zero-lenght return value") } @@ -390,7 +390,7 @@ func TestStatsPolicyPostProcessTactics(t *testing.T) { }, } - got := statsPolicyPostProcessTactics(input, true) + got := statsPolicyFilterStatsTactics(input, true) if len(got) != 1 { t.Fatal("expected just one element") diff --git a/internal/enginenetx/userpolicy.go b/internal/enginenetx/userpolicy.go index 9409570b43..778c1393f2 100644 --- a/internal/enginenetx/userpolicy.go +++ b/internal/enginenetx/userpolicy.go @@ -104,7 +104,7 @@ func (ldp *userPolicy) LookupTactics( return ldp.Fallback.LookupTactics(ctx, domain, port) } - // emit the resuults, which may possibly be empty + // emit the results, which may possibly be empty out := make(chan *httpsDialerTactic) go func() { defer close(out) // let the caller know we're done