From b70062a73778e33ecfcd41294897cb7e7654b604 Mon Sep 17 00:00:00 2001 From: favonia Date: Mon, 22 May 2023 09:55:38 -0400 Subject: [PATCH 1/5] fix(config): display a message when 1.0.0.1 also doesn't work --- internal/config/network_probe.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/internal/config/network_probe.go b/internal/config/network_probe.go index ba5a54d4..124be8f6 100644 --- a/internal/config/network_probe.go +++ b/internal/config/network_probe.go @@ -44,11 +44,13 @@ func (c *Config) ShouldWeUse1001(ctx context.Context, ppfmt pp.PP) bool { if ProbeURL(ctx, "https://1.1.1.1") { ppfmt.Infof(pp.EmojiGood, "1.1.1.1 appears to be working") } else { - ppfmt.Warningf(pp.EmojiError, "1.1.1.1 appears to be blocked or intercepted by your ISP or your router") + ppfmt.Warningf(pp.EmojiError, "1.1.1.1 appears to be blocked or hijacked by your ISP or your router") if ProbeURL(ctx, "https://1.0.0.1") { ppfmt.Warningf(pp.EmojiGood, "1.0.0.1 appears to be working and will be used instead of 1.1.1.1") c.Use1001 = true + } else { + ppfmt.Warningf(pp.EmojiGood, "1.0.0.1 appears to be blocked or hijacked as well; sticking to 1.1.1.1") } } return true From 3d79b2c74121bbcbf0d6716ecc7b40e1e779b0e6 Mon Sep 17 00:00:00 2001 From: favonia Date: Mon, 22 May 2023 11:50:52 -0400 Subject: [PATCH 2/5] feat(provider): check 1.1.1.1 only when we might actually use 1.1.1.1 --- internal/config/network_probe.go | 2 +- internal/mocks/mock_provider.go | 14 ++++++++++++++ internal/provider/base.go | 12 ++++++++++-- internal/provider/cloudflare_doh.go | 3 ++- internal/provider/cloudflare_trace.go | 3 ++- internal/provider/ipify.go | 3 ++- internal/provider/local_cloudflare.go | 3 ++- internal/provider/protocol/doh.go | 8 ++++++-- internal/provider/protocol/doh_test.go | 8 +++++--- internal/provider/protocol/field.go | 8 ++++++-- internal/provider/protocol/field_test.go | 8 +++++--- internal/provider/protocol/http.go | 8 ++++++-- internal/provider/protocol/http_test.go | 8 +++++--- internal/provider/protocol/local.go | 3 +++ 14 files changed, 69 insertions(+), 22 deletions(-) diff --git a/internal/config/network_probe.go b/internal/config/network_probe.go index 124be8f6..3b9a8926 100644 --- a/internal/config/network_probe.go +++ b/internal/config/network_probe.go @@ -33,7 +33,7 @@ func ProbeURL(ctx context.Context, url string) bool { // ShouldWeUse1001 quickly checks 1.1.1.1 and 1.0.0.1 and notes whether 1.0.0.1 should be used. func (c *Config) ShouldWeUse1001(ctx context.Context, ppfmt pp.PP) bool { c.Use1001 = false - if c.Provider[ipnet.IP4] == nil { + if c.Provider[ipnet.IP4] == nil || !c.Provider[ipnet.IP4].ShouldWeCheck1111() { return true } if ppfmt.IsEnabledFor(pp.Info) { diff --git a/internal/mocks/mock_provider.go b/internal/mocks/mock_provider.go index d0543c2e..ac83e46c 100644 --- a/internal/mocks/mock_provider.go +++ b/internal/mocks/mock_provider.go @@ -65,3 +65,17 @@ func (mr *MockProviderMockRecorder) Name() *gomock.Call { mr.mock.ctrl.T.Helper() return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Name", reflect.TypeOf((*MockProvider)(nil).Name)) } + +// ShouldWeCheck1111 mocks base method. +func (m *MockProvider) ShouldWeCheck1111() bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ShouldWeCheck1111") + ret0, _ := ret[0].(bool) + return ret0 +} + +// ShouldWeCheck1111 indicates an expected call of ShouldWeCheck1111. +func (mr *MockProviderMockRecorder) ShouldWeCheck1111() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ShouldWeCheck1111", reflect.TypeOf((*MockProvider)(nil).ShouldWeCheck1111)) +} diff --git a/internal/provider/base.go b/internal/provider/base.go index 1856ecf8..d2b34c8c 100644 --- a/internal/provider/base.go +++ b/internal/provider/base.go @@ -13,8 +13,16 @@ import ( // Provider is the abstraction of a protocol to detect public IP addresses. type Provider interface { - Name() string // name of the protocol - GetIP(ctx context.Context, ppfmt pp.PP, ipNet ipnet.Type, use1001 bool) (netip.Addr, bool) // get the IP + Name() string + // Name gives the name of the protocol + + ShouldWeCheck1111() bool + // ShouldWeCheck1111() says whether the provider will connect to 1.1.1.1. + // Only when the IPv4 provider returns yes on this operation + // the hijacking detection of 1.1.1.1 will be performed. + + GetIP(ctx context.Context, ppfmt pp.PP, ipNet ipnet.Type, use1001 bool) (netip.Addr, bool) + // Actually get the IP } // Name gets the protocol name. It returns "none" for nil. diff --git a/internal/provider/cloudflare_doh.go b/internal/provider/cloudflare_doh.go index 8b6bedfd..7ed993c6 100644 --- a/internal/provider/cloudflare_doh.go +++ b/internal/provider/cloudflare_doh.go @@ -11,7 +11,8 @@ import ( // If use1001 is true, 1.0.0.1 is used instead of 1.1.1.1. func NewCloudflareDOH() Provider { return &protocol.DNSOverHTTPS{ - ProviderName: "cloudflare.doh", + ProviderName: "cloudflare.doh", + Is1111UsedForIP4: true, Param: map[ipnet.Type]struct { URL protocol.Switch Name string diff --git a/internal/provider/cloudflare_trace.go b/internal/provider/cloudflare_trace.go index 16366eb4..4bb035ad 100644 --- a/internal/provider/cloudflare_trace.go +++ b/internal/provider/cloudflare_trace.go @@ -9,7 +9,8 @@ import ( // If use1001 is true, 1.0.0.1 is used instead of 1.1.1.1. func NewCloudflareTrace() Provider { return &protocol.Field{ - ProviderName: "cloudflare.trace", + ProviderName: "cloudflare.trace", + Is1111UsedforIP4: true, Param: map[ipnet.Type]struct { URL protocol.Switch Field string diff --git a/internal/provider/ipify.go b/internal/provider/ipify.go index 74402d1d..ccde42b0 100644 --- a/internal/provider/ipify.go +++ b/internal/provider/ipify.go @@ -8,7 +8,8 @@ import ( // NewIpify creates a specialized HTTP provider that uses the ipify service. func NewIpify() Provider { return &protocol.HTTP{ - ProviderName: "ipify", + ProviderName: "ipify", + Is1111UsedForIP4: false, URL: map[ipnet.Type]protocol.Switch{ ipnet.IP4: protocol.Constant("https://api4.ipify.org"), ipnet.IP6: protocol.Constant("https://api6.ipify.org"), diff --git a/internal/provider/local_cloudflare.go b/internal/provider/local_cloudflare.go index 90ffc7af..ac7b364b 100644 --- a/internal/provider/local_cloudflare.go +++ b/internal/provider/local_cloudflare.go @@ -6,7 +6,8 @@ import ( ) // NewLocal creates a specialized Local provider that uses Cloudflare as the remote server. -// (No actual UDP packets will be sent out.) +// (No actual UDP packets will be sent out due to this provider. However, packets might +// have already been sent to 1.1.1.1 or 1.0.0.1 when detecting possible hijacking of 1.1.1.1.) func NewLocal() Provider { return &protocol.Local{ ProviderName: "local", diff --git a/internal/provider/protocol/doh.go b/internal/provider/protocol/doh.go index 940fa8cf..7cb7ebe2 100644 --- a/internal/provider/protocol/doh.go +++ b/internal/provider/protocol/doh.go @@ -168,8 +168,9 @@ func getIPFromDNS(ctx context.Context, ppfmt pp.PP, // DNSOverHTTPS represents a generic detection protocol using DNS over HTTPS. type DNSOverHTTPS struct { - ProviderName string // name of the protocol - Param map[ipnet.Type]struct { + ProviderName string // name of the protocol + Is1111UsedForIP4 bool // whether 1.1.1.1 is used + Param map[ipnet.Type]struct { URL Switch // the DoH server Name string // domain name to query Class dnsmessage.Class // DNS class to query @@ -196,3 +197,6 @@ func (p *DNSOverHTTPS) GetIP(ctx context.Context, ppfmt pp.PP, ipNet ipnet.Type, return ipNet.NormalizeDetectedIP(ppfmt, ip) } + +// ShouldWeCheck1111 returns whether we should check 1.1.1.1. +func (p *DNSOverHTTPS) ShouldWeCheck1111() bool { return p.Is1111UsedForIP4 } diff --git a/internal/provider/protocol/doh_test.go b/internal/provider/protocol/doh_test.go index 252728db..fe30d777 100644 --- a/internal/provider/protocol/doh_test.go +++ b/internal/provider/protocol/doh_test.go @@ -22,8 +22,9 @@ func TestDNSOverHTTPSName(t *testing.T) { t.Parallel() p := &protocol.DNSOverHTTPS{ - ProviderName: "very secret name", - Param: nil, + ProviderName: "very secret name", + Is1111UsedForIP4: false, + Param: nil, } require.Equal(t, "very secret name", p.Name()) @@ -570,7 +571,8 @@ func TestDNSOverHTTPSGetIP(t *testing.T) { server := setupServer(t, tc.name, tc.class, tc.response, tc.header, tc.idShift, tc.answers) provider := &protocol.DNSOverHTTPS{ - ProviderName: "", + ProviderName: "", + Is1111UsedForIP4: false, Param: map[ipnet.Type]struct { URL protocol.Switch Name string diff --git a/internal/provider/protocol/field.go b/internal/provider/protocol/field.go index 23e6eb96..15d43e64 100644 --- a/internal/provider/protocol/field.go +++ b/internal/provider/protocol/field.go @@ -40,8 +40,9 @@ func getIPFromField(ctx context.Context, ppfmt pp.PP, url string, field string) // Field represents a generic detection protocol to parse an HTTP response. type Field struct { - ProviderName string // name of the detection protocol - Param map[ipnet.Type]struct { + ProviderName string // name of the detection protocol + Is1111UsedforIP4 bool + Param map[ipnet.Type]struct { URL Switch // URL of the detection page Field string // name of the field holding the IP address } @@ -67,3 +68,6 @@ func (p *Field) GetIP(ctx context.Context, ppfmt pp.PP, ipNet ipnet.Type, use100 return ipNet.NormalizeDetectedIP(ppfmt, ip) } + +// ShouldWeCheck1111 returns whether we should check 1.1.1.1. +func (p *Field) ShouldWeCheck1111() bool { return p.Is1111UsedforIP4 } diff --git a/internal/provider/protocol/field_test.go b/internal/provider/protocol/field_test.go index db72b625..a477da67 100644 --- a/internal/provider/protocol/field_test.go +++ b/internal/provider/protocol/field_test.go @@ -21,8 +21,9 @@ func TestFieldName(t *testing.T) { t.Parallel() p := &protocol.Field{ - ProviderName: "very secret name", - Param: nil, + ProviderName: "very secret name", + Is1111UsedforIP4: false, + Param: nil, } require.Equal(t, "very secret name", p.Name()) @@ -149,7 +150,8 @@ func TestFieldGetIP(t *testing.T) { mockCtrl := gomock.NewController(t) provider := &protocol.Field{ - ProviderName: "secret name", + ProviderName: "secret name", + Is1111UsedforIP4: false, Param: map[ipnet.Type]struct { URL protocol.Switch Field string diff --git a/internal/provider/protocol/http.go b/internal/provider/protocol/http.go index 189d21ec..043cd24c 100644 --- a/internal/provider/protocol/http.go +++ b/internal/provider/protocol/http.go @@ -31,8 +31,9 @@ func getIPFromHTTP(ctx context.Context, ppfmt pp.PP, url string) (netip.Addr, bo // HTTP represents a generic detection protocol to use an HTTP response directly. type HTTP struct { - ProviderName string // name of the protocol - URL map[ipnet.Type]Switch // URL of the detection page + ProviderName string // name of the protocol + Is1111UsedForIP4 bool // whether 1.1.1.1 is used + URL map[ipnet.Type]Switch // URL of the detection page } // Name of the detection protocol. @@ -55,3 +56,6 @@ func (p *HTTP) GetIP(ctx context.Context, ppfmt pp.PP, ipNet ipnet.Type, use1001 return ipNet.NormalizeDetectedIP(ppfmt, ip) } + +// ShouldWeCheck1111 returns whether we should check 1.1.1.1. +func (p *HTTP) ShouldWeCheck1111() bool { return p.Is1111UsedForIP4 } diff --git a/internal/provider/protocol/http_test.go b/internal/provider/protocol/http_test.go index 2181939d..cebcb3bb 100644 --- a/internal/provider/protocol/http_test.go +++ b/internal/provider/protocol/http_test.go @@ -21,8 +21,9 @@ func TestHTTPName(t *testing.T) { t.Parallel() p := &protocol.HTTP{ - ProviderName: "very secret name", - URL: nil, + ProviderName: "very secret name", + Is1111UsedForIP4: false, + URL: nil, } require.Equal(t, "very secret name", p.Name()) @@ -141,7 +142,8 @@ func TestHTTPGetIP(t *testing.T) { mockCtrl := gomock.NewController(t) provider := &protocol.HTTP{ - ProviderName: "secret name", + ProviderName: "secret name", + Is1111UsedForIP4: false, URL: map[ipnet.Type]protocol.Switch{ tc.urlKey: protocol.Constant(tc.url), }, diff --git a/internal/provider/protocol/local.go b/internal/provider/protocol/local.go index dbebd5a3..660b6281 100644 --- a/internal/provider/protocol/local.go +++ b/internal/provider/protocol/local.go @@ -48,3 +48,6 @@ func (p *Local) GetIP(_ctx context.Context, ppfmt pp.PP, ipNet ipnet.Type, use10 return ipNet.NormalizeDetectedIP(ppfmt, ip) } + +// ShouldWeCheck1111 returns whether we should check 1.1.1.1. +func (p *Local) ShouldWeCheck1111() bool { return false } From c023d723e8df9885b83f6432df410f132dd3d7d8 Mon Sep 17 00:00:00 2001 From: favonia Date: Mon, 22 May 2023 11:59:34 -0400 Subject: [PATCH 3/5] fix(provider): fix the local IP provider --- internal/provider/local_cloudflare.go | 12 +++++------- internal/provider/protocol/local.go | 5 ++++- 2 files changed, 9 insertions(+), 8 deletions(-) diff --git a/internal/provider/local_cloudflare.go b/internal/provider/local_cloudflare.go index ac7b364b..c4258bac 100644 --- a/internal/provider/local_cloudflare.go +++ b/internal/provider/local_cloudflare.go @@ -6,16 +6,14 @@ import ( ) // NewLocal creates a specialized Local provider that uses Cloudflare as the remote server. -// (No actual UDP packets will be sent out due to this provider. However, packets might -// have already been sent to 1.1.1.1 or 1.0.0.1 when detecting possible hijacking of 1.1.1.1.) +// (No actual UDP packets will be sent to Cloudflare.) func NewLocal() Provider { return &protocol.Local{ - ProviderName: "local", + ProviderName: "local", + Is1111UsedForIP4: false, // 1.0.0.1 is used in case 1.1.1.1 is hijacked by the router RemoteUDPAddr: map[ipnet.Type]protocol.Switch{ - ipnet.IP4: protocol.Switchable{ - Use1111: "1.1.1.1:443", - Use1001: "1.0.0.1:443", - }, + // 1.0.0.1 is used in case 1.1.1.1 is hijacked by the router + ipnet.IP4: protocol.Constant("1.0.0.1:443"), ipnet.IP6: protocol.Constant("[2606:4700:4700::1111]:443"), }, } diff --git a/internal/provider/protocol/local.go b/internal/provider/protocol/local.go index 660b6281..94687927 100644 --- a/internal/provider/protocol/local.go +++ b/internal/provider/protocol/local.go @@ -17,6 +17,9 @@ type Local struct { // Name of the detection protocol. ProviderName string + // Whether 1.1.1.1 is used for IPv4 + Is1111UsedForIP4 bool + // The target IP address of the UDP packet to be sent. RemoteUDPAddr map[ipnet.Type]Switch } @@ -50,4 +53,4 @@ func (p *Local) GetIP(_ctx context.Context, ppfmt pp.PP, ipNet ipnet.Type, use10 } // ShouldWeCheck1111 returns whether we should check 1.1.1.1. -func (p *Local) ShouldWeCheck1111() bool { return false } +func (p *Local) ShouldWeCheck1111() bool { return p.Is1111UsedForIP4 } From 8bf97a3a5107da3c565e0697d15233d4cf3de59d Mon Sep 17 00:00:00 2001 From: favonia Date: Mon, 22 May 2023 12:10:54 -0400 Subject: [PATCH 4/5] test(provider): improve coverage --- internal/provider/protocol/doh_test.go | 16 ++++++++++++++++ internal/provider/protocol/field_test.go | 16 ++++++++++++++++ internal/provider/protocol/http_test.go | 16 ++++++++++++++++ internal/provider/protocol/local_test.go | 24 +++++++++++++++++++++--- 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/internal/provider/protocol/doh_test.go b/internal/provider/protocol/doh_test.go index fe30d777..49c9688d 100644 --- a/internal/provider/protocol/doh_test.go +++ b/internal/provider/protocol/doh_test.go @@ -592,3 +592,19 @@ func TestDNSOverHTTPSGetIP(t *testing.T) { }) } } + +func TestDOHShouldWeCheck1111(t *testing.T) { + t.Parallel() + + require.True(t, (&protocol.DNSOverHTTPS{ + ProviderName: "", + Is1111UsedForIP4: true, + Param: nil, + }).ShouldWeCheck1111()) + + require.False(t, (&protocol.DNSOverHTTPS{ + ProviderName: "", + Is1111UsedForIP4: false, + Param: nil, + }).ShouldWeCheck1111()) +} diff --git a/internal/provider/protocol/field_test.go b/internal/provider/protocol/field_test.go index a477da67..38fb5103 100644 --- a/internal/provider/protocol/field_test.go +++ b/internal/provider/protocol/field_test.go @@ -174,3 +174,19 @@ func TestFieldGetIP(t *testing.T) { } }) } + +func TestFieldShouldWeCheck1111(t *testing.T) { + t.Parallel() + + require.True(t, (&protocol.Field{ + ProviderName: "", + Is1111UsedforIP4: true, + Param: nil, + }).ShouldWeCheck1111()) + + require.False(t, (&protocol.Field{ + ProviderName: "", + Is1111UsedforIP4: false, + Param: nil, + }).ShouldWeCheck1111()) +} diff --git a/internal/provider/protocol/http_test.go b/internal/provider/protocol/http_test.go index cebcb3bb..c248cff9 100644 --- a/internal/provider/protocol/http_test.go +++ b/internal/provider/protocol/http_test.go @@ -164,3 +164,19 @@ func TestHTTPGetIP(t *testing.T) { } }) } + +func TestHTTPShouldWeCheck1111(t *testing.T) { + t.Parallel() + + require.True(t, (&protocol.HTTP{ + ProviderName: "", + Is1111UsedForIP4: true, + URL: nil, + }).ShouldWeCheck1111()) + + require.False(t, (&protocol.HTTP{ + ProviderName: "", + Is1111UsedForIP4: false, + URL: nil, + }).ShouldWeCheck1111()) +} diff --git a/internal/provider/protocol/local_test.go b/internal/provider/protocol/local_test.go index 1bd25db1..a2bfa881 100644 --- a/internal/provider/protocol/local_test.go +++ b/internal/provider/protocol/local_test.go @@ -18,8 +18,9 @@ func TestLocalName(t *testing.T) { t.Parallel() p := &protocol.Local{ - ProviderName: "very secret name", - RemoteUDPAddr: nil, + ProviderName: "very secret name", + Is1111UsedForIP4: false, + RemoteUDPAddr: nil, } require.Equal(t, "very secret name", p.Name()) @@ -83,7 +84,8 @@ func TestLocalGetIP(t *testing.T) { mockCtrl := gomock.NewController(t) provider := &protocol.Local{ - ProviderName: "", + ProviderName: "", + Is1111UsedForIP4: false, RemoteUDPAddr: map[ipnet.Type]protocol.Switch{ tc.addrKey: protocol.Constant(tc.addr), }, @@ -99,3 +101,19 @@ func TestLocalGetIP(t *testing.T) { }) } } + +func TestLocalShouldWeCheck1111(t *testing.T) { + t.Parallel() + + require.True(t, (&protocol.Local{ + ProviderName: "", + Is1111UsedForIP4: true, + RemoteUDPAddr: nil, + }).ShouldWeCheck1111()) + + require.False(t, (&protocol.Local{ + ProviderName: "", + Is1111UsedForIP4: false, + RemoteUDPAddr: nil, + }).ShouldWeCheck1111()) +} From cc906e1932645dad67ff9d63a9cbcfba02f04536 Mon Sep 17 00:00:00 2001 From: favonia Date: Mon, 22 May 2023 12:16:20 -0400 Subject: [PATCH 5/5] style(config): minor style tweak --- internal/config/network_probe.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/config/network_probe.go b/internal/config/network_probe.go index 3b9a8926..40a9e0d9 100644 --- a/internal/config/network_probe.go +++ b/internal/config/network_probe.go @@ -36,6 +36,7 @@ func (c *Config) ShouldWeUse1001(ctx context.Context, ppfmt pp.PP) bool { if c.Provider[ipnet.IP4] == nil || !c.Provider[ipnet.IP4].ShouldWeCheck1111() { return true } + if ppfmt.IsEnabledFor(pp.Info) { ppfmt.Infof(pp.EmojiEnvVars, "Checking 1.1.1.1 . . .") ppfmt = ppfmt.IncIndent()