diff --git a/client/internal/peer/conn.go b/client/internal/peer/conn.go index 43a8814cd..1d0e95f59 100644 --- a/client/internal/peer/conn.go +++ b/client/internal/peer/conn.go @@ -310,38 +310,44 @@ func (conn *Conn) Open() error { } // useProxy determines whether a direct connection (without a go proxy) is possible -// There are 3 cases: one of the peers has a public IP or both peers are in the same private network +// +// There are 2 cases: +// +// * When neither candidate is from hard nat and one of the peers has a public IP +// +// * both peers are in the same private network +// // Please note, that this check happens when peers were already able to ping each other using ICE layer. func shouldUseProxy(pair *ice.CandidatePair) bool { - remoteIP := net.ParseIP(pair.Remote.Address()) - myIp := net.ParseIP(pair.Local.Address()) - remoteIsPublic := IsPublicIP(remoteIP) - myIsPublic := IsPublicIP(myIp) - - if pair.Local.Type() == ice.CandidateTypeRelay || pair.Remote.Type() == ice.CandidateTypeRelay { - return true - } - - //one of the hosts has a public IP - if remoteIsPublic && pair.Remote.Type() == ice.CandidateTypeHost { - return false - } - if myIsPublic && pair.Local.Type() == ice.CandidateTypeHost { + if !isHardNATCandidate(pair.Local) && isHostCandidateWithPublicIP(pair.Remote) { return false } - if pair.Local.Type() == ice.CandidateTypeHost && pair.Remote.Type() == ice.CandidateTypeHost { - if !remoteIsPublic && !myIsPublic { - //both hosts are in the same private network - return false - } + if !isHardNATCandidate(pair.Remote) && isHostCandidateWithPublicIP(pair.Local) { + return false + } + + if isHostCandidateWithPrivateIP(pair.Local) && isHostCandidateWithPrivateIP(pair.Remote) { + return false } return true } -// IsPublicIP indicates whether IP is public or not. -func IsPublicIP(ip net.IP) bool { +func isHardNATCandidate(candidate ice.Candidate) bool { + return candidate.Type() == ice.CandidateTypeRelay || candidate.Type() == ice.CandidateTypePeerReflexive +} + +func isHostCandidateWithPublicIP(candidate ice.Candidate) bool { + return candidate.Type() == ice.CandidateTypeHost && isPublicIP(candidate.Address()) +} + +func isHostCandidateWithPrivateIP(candidate ice.Candidate) bool { + return candidate.Type() == ice.CandidateTypeHost && !isPublicIP(candidate.Address()) +} + +func isPublicIP(address string) bool { + ip := net.ParseIP(address) if ip.IsLoopback() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() || ip.IsPrivate() { return false } diff --git a/client/internal/peer/conn_test.go b/client/internal/peer/conn_test.go index 631e7fe79..452915112 100644 --- a/client/internal/peer/conn_test.go +++ b/client/internal/peer/conn_test.go @@ -166,3 +166,166 @@ func TestConn_Close(t *testing.T) { wg.Wait() } + +type mockICECandidate struct { + ice.Candidate + AddressFunc func() string + TypeFunc func() ice.CandidateType +} + +// Address mocks and overwrite ice.Candidate Address method +func (m *mockICECandidate) Address() string { + if m.AddressFunc != nil { + return m.AddressFunc() + } + return "" +} + +// Type mocks and overwrite ice.Candidate Type method +func (m *mockICECandidate) Type() ice.CandidateType { + if m.TypeFunc != nil { + return m.TypeFunc() + } + return ice.CandidateTypeUnspecified +} + +func TestConn_ShouldUseProxy(t *testing.T) { + publicHostCandidate := &mockICECandidate{ + AddressFunc: func() string { + return "8.8.8.8" + }, + TypeFunc: func() ice.CandidateType { + return ice.CandidateTypeHost + }, + } + privateHostCandidate := &mockICECandidate{ + AddressFunc: func() string { + return "10.0.0.1" + }, + TypeFunc: func() ice.CandidateType { + return ice.CandidateTypeHost + }, + } + srflxCandidate := &mockICECandidate{ + AddressFunc: func() string { + return "1.1.1.1" + }, + TypeFunc: func() ice.CandidateType { + return ice.CandidateTypeServerReflexive + }, + } + + prflxCandidate := &mockICECandidate{ + AddressFunc: func() string { + return "1.1.1.1" + }, + TypeFunc: func() ice.CandidateType { + return ice.CandidateTypePeerReflexive + }, + } + + relayCandidate := &mockICECandidate{ + AddressFunc: func() string { + return "1.1.1.1" + }, + TypeFunc: func() ice.CandidateType { + return ice.CandidateTypeRelay + }, + } + + testCases := []struct { + name string + candatePair *ice.CandidatePair + expected bool + }{ + { + name: "Use Proxy When Local Candidate Is Relay", + candatePair: &ice.CandidatePair{ + Local: relayCandidate, + Remote: privateHostCandidate, + }, + expected: true, + }, + { + name: "Use Proxy When Remote Candidate Is Relay", + candatePair: &ice.CandidatePair{ + Local: privateHostCandidate, + Remote: relayCandidate, + }, + expected: true, + }, + { + name: "Use Proxy When Local Candidate Is Peer Reflexive", + candatePair: &ice.CandidatePair{ + Local: prflxCandidate, + Remote: privateHostCandidate, + }, + expected: true, + }, + { + name: "Use Proxy When Remote Candidate Is Peer Reflexive", + candatePair: &ice.CandidatePair{ + Local: privateHostCandidate, + Remote: prflxCandidate, + }, + expected: true, + }, + { + name: "Don't Use Proxy When Local Candidate Is Public And Remote Is Private", + candatePair: &ice.CandidatePair{ + Local: publicHostCandidate, + Remote: privateHostCandidate, + }, + expected: false, + }, + { + name: "Don't Use Proxy When Remote Candidate Is Public And Local Is Private", + candatePair: &ice.CandidatePair{ + Local: privateHostCandidate, + Remote: publicHostCandidate, + }, + expected: false, + }, + { + name: "Don't Use Proxy When Local Candidate is Public And Remote Is Server Reflexive", + candatePair: &ice.CandidatePair{ + Local: publicHostCandidate, + Remote: srflxCandidate, + }, + expected: false, + }, + { + name: "Don't Use Proxy When Remote Candidate is Public And Local Is Server Reflexive", + candatePair: &ice.CandidatePair{ + Local: srflxCandidate, + Remote: publicHostCandidate, + }, + expected: false, + }, + { + name: "Don't Use Proxy When Both Candidates Are Public", + candatePair: &ice.CandidatePair{ + Local: publicHostCandidate, + Remote: publicHostCandidate, + }, + expected: false, + }, + { + name: "Don't Use Proxy When Both Candidates Are Private", + candatePair: &ice.CandidatePair{ + Local: privateHostCandidate, + Remote: privateHostCandidate, + }, + expected: false, + }, + } + + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + result := shouldUseProxy(testCase.candatePair) + if result != testCase.expected { + t.Errorf("got a different result. Expected %t Got %t", testCase.expected, result) + } + }) + } +}