Repository: pion/ice (go)

Issue:
different ice candidate behavior as chrome
<!--
  Come join our community on Slack https://pion.ly/slack for quicker support and more resources

  We also provide a Knowledge Base https://pion.ly/knowledge-base that covers Pion and general WebRTC
  
  We maintain a FAQ of common issues and questions https://pion.ly/knowledge-base/pion-basics/faq/

 Please answer these questions before submitting your issue. Thanks!
-->

### Your environment.
  - Version: *v3.1.27*
  - Browser: *chrome 100.0.4896.75*
  - Other Information - *stacktraces, related issues, suggestions how to fix, links for us to have context*

### What did you do?
<!--
If possible, provide an example for reproducing the error.
A complete runnable program is good.
-->

I have more than one interfaces, 1 wireless(172.19.4.x), 1 wire(172.19.20.x), 2 vpn. And  enable udp mux at 7884 and network interface filter only include the en0 (wireless).

That will cause udp mux listen on(0.0.0.0), but only send candidate 172.19.4.x, chrome first nominate 172.19.4.x candidate and accept by pion, then it discover the new reachable address not announced (172.19.20.x), and try nominate this new candidate, pion response to the ping request with ‘use-candidate’, but reject switch to the new candidate because it already have one. But chrome switch to the new candidate(because it received the ping response) and stop ping the old one, so after some seconds, timeout and disconnected.

Pion:

```
ice INFO: 2022/04/11 14:36:34 Setting new connection state: Connected
ice TRACE: 14:36:34.088768 agent.go:964: ping STUN from udp4 host 172.19.4.80:7884 to udp4 host 172.19.4.80:63531
pc INFO: 2022/04/11 14:36:34 ICE connection state changed: connected
ice TRACE: 14:36:34.090198 selection.go:221: inbound STUN (SuccessResponse) from udp4 host 172.19.4.80:63531 to udp4 host 172.19.4.80:7884
ice TRACE: 14:36:34.090215 selection.go:231: Found valid candidate pair: prio 9114756778549174527 (local, prio 2130706431) udp4 host 172.19.4.80:7884 <-> udp4 host 172.19.4.80:63531 (remote, prio 2122194687)
```


chrome:
```
[3639:83971:0411/143634.040308:INFO:p2p_transport_channel.cc(306)] Switching selected connection due to: candidate pair state changed
[3639:83971:0411/143634.040318:INFO:p2p_transport_channel.cc(1821)] Channel[0|1|__]: New selected connection: Conn[44a800:0:Net[en0:172.19.4.x/22:Wifi:id=1]:rzwnBbZR:1:0:local:udp:172.19.4.x:63531->S5o7kJ      mC:1:2130706431:local:udp:172.19.4.x:7884|CRWS|S|0|0|9114756780671369214|18]

................


[3639:83971:0411/143634.187853:INFO:p2p_transport_channel.cc(1818)] Channel[0|1|RW]: Previous selected connection: Conn[44a800:0:Net[en0:172.19.4.x/22:Wifi:id=1]:rzwnBbZR:1:0:local:udp:172.19.4.x:63531->S      5o7kJmC:1:2130706431:local:udp:172.19.4.x:7884|CRWS|-|0|0|9114756780671369214|10]
[3639:83971:0411/143634.187873:INFO:p2p_transport_channel.cc(1821)] Channel[0|1|RW]: New selected connection: Conn[44f800:0:Net[en7:172.19.20.x/22:Ethernet:id=2]:aJepAjnq:1:0:local:udp:172.19.20.x:58485->      LqEBX0Qm:1:2130706431:prflx:udp:172.19.20.x:7884|CRWS|S|0|0|9115038255648079870|1]
```

chrome first nominate 172.19.4.x, then nominate 172.19.20.x. Pion response to both nominate, but only select the first one and expect ping request from first one.

After a few seconds, pion disconnect because no check alive ping request received.

```
ice TRACE: 14:36:56.935129 selection.go:221: inbound STUN (SuccessResponse) from udp4 host 172.19.20.119:58485 to udp4 host 172.19.4.80:7884
ice TRACE: 14:36:56.935182 selection.go:231: Found valid candidate pair: prio 9115038253525819647 (local, prio 2130706431) udp4 host 172.19.4.80:7884 <-> udp4 host 172.19.20.119:58485 (remote, prio 2122260223)
mdns WARNING: 2022/04/11 14:36:57 Failed to parse mDNS packet parsing/packing of this type isn't available yet
mdns WARNING: 2022/04/11 14:36:57 Failed to parse mDNS packet parsing/packing of this type isn't available yet
ice INFO: 2022/04/11 14:36:58 Setting new connection state: Disconnected
pc INFO: 2022/04/11 14:36:58 ICE connection state changed: disconnected
pc INFO: 2022/04/11 14:36:58 peer connection state changed: disconnected
```

From log we can see chrome send ping reqeust from the latter candidate, but pion expect the first one.

### What did you expect?

Pion select the second nominated candidate if it has higher priority than the previous one, or don't response to the stun bind request with 'Use-Candidate' attribute if it don't want to switch to.

### What happened?

Pion response to the second request, but not switch to it, state inconsistency with client

In https://datatracker.ietf.org/doc/html/rfc8445#section-8

> Once the controlling agent has successfully nominated a candidate
>    pair ([Section 7.2.5.3.4](https://datatracker.ietf.org/doc/html/rfc8445#section-7.2.5.3.4)), the agent MUST NOT nominate another pair
>    for same component of the data stream within the ICE session.  Doing
>    so requires an ICE restart.

Looks chrome don't compliant this.

And Pion seems not compliant below behavior

> A controlling agent that does not support this specification (i.e.,
>    it is implemented according to [RFC 5245](https://datatracker.ietf.org/doc/html/rfc5245)) might nominate more than one
>    candidate pair.  This was referred to as "aggressive nomination" in
>    [RFC 5245](https://datatracker.ietf.org/doc/html/rfc5245).  If more than one candidate pair is nominated by the
>    controlling agent, and if the controlled agent accepts multiple
>    nominations requests, the agents MUST produce the selected pairs and
>    use the pairs with the highest priority.

Pull request description:
Add option for accept aggressive nominate
#### Description
 Add config for accept aggressive nomination, it is useful for compatible with chrome or other clients use aggressive nominate to update nominate candidate pairs.

#### Reference issue
Fixes #440


Patch:
diff --git a/AUTHORS.txt b/AUTHORS.txt
index 4b96858..301f3f4 100644
--- a/AUTHORS.txt
+++ b/AUTHORS.txt
@@ -14,6 +14,7 @@ backkem <mail@backkem.me>
 buptczq <czq@qq.com>
 cgojin <gongjin21@hotmail.com>
 Chao Yuan <yuanchao0310@163.com>
+cnderrauber <zengjie9004@gmail.com>
 David Hamilton <davidhamiltron@gmail.com>
 David Zhao <david@davidzhao.com>
 Henry <cryptix@riseup.net>
diff --git a/agent.go b/agent.go
index 951103b..ebdd1d8 100644
--- a/agent.go
+++ b/agent.go
@@ -133,6 +133,8 @@ type Agent struct {
 	insecureSkipVerify bool
 
 	proxyDialer proxy.Dialer
+
+	acceptAggressiveNomination bool
 }
 
 type task struct {
@@ -314,6 +316,8 @@ func NewAgent(config *AgentConfig) (*Agent, error) { //nolint:gocognit
 		interfaceFilter: config.InterfaceFilter,
 
 		insecureSkipVerify: config.InsecureSkipVerify,
+
+		acceptAggressiveNomination: config.AcceptAggressiveNomination,
 	}
 
 	a.tcpMux = config.TCPMux
diff --git a/agent_config.go b/agent_config.go
index b373939..08aa158 100644
--- a/agent_config.go
+++ b/agent_config.go
@@ -159,6 +159,9 @@ type AgentConfig struct {
 	// Proxy Dialer is a dialer that should be implemented by the user based on golang.org/x/net/proxy
 	// dial interface in order to support corporate proxies
 	ProxyDialer proxy.Dialer
+
+	// Accept aggressive nomination in RFC 5245 for compatible with chrome and other browsers
+	AcceptAggressiveNomination bool
 }
 
 // initWithDefaults populates an agent and falls back to defaults if fields are unset
diff --git a/selection.go b/selection.go
index df43f03..459d3ed 100644
--- a/selection.go
+++ b/selection.go
@@ -252,8 +252,11 @@ func (s *controlledSelector) HandleBindingRequest(m *stun.Message, local, remote
 			// previously sent by this pair produced a successful response and
 			// generated a valid pair (Section 7.2.5.3.2).  The agent sets the
 			// nominated flag value of the valid pair to true.
-			if selectedPair := s.agent.getSelectedPair(); selectedPair == nil {
+			if selectedPair := s.agent.getSelectedPair(); selectedPair == nil ||
+				(s.agent.acceptAggressiveNomination && selectedPair.priority() < p.priority()) {
 				s.agent.setSelectedPair(p)
+			} else if selectedPair != p {
+				s.log.Tracef("ignore nominate new pair %s, already nominated pair %s", p, selectedPair)
 			}
 		} else {
 			// If the received Binding request triggered a new check to be


Test patch:
diff --git a/agent_test.go b/agent_test.go
index ff06dca..24537f3 100644
--- a/agent_test.go
+++ b/agent_test.go
@@ -1,3 +1,4 @@
+//go:build !js
 // +build !js
 
 package ice
@@ -1763,3 +1764,126 @@ func TestGetSelectedCandidatePair(t *testing.T) {
 	assert.NoError(t, aAgent.Close())
 	assert.NoError(t, bAgent.Close())
 }
+
+func TestAcceptAggressiveNomination(t *testing.T) {
+	report := test.CheckRoutines(t)
+	defer report()
+
+	lim := test.TimeOut(time.Second * 30)
+	defer lim.Stop()
+
+	// Create a network with two interfaces
+	wan, err := vnet.NewRouter(&vnet.RouterConfig{
+		CIDR:          "0.0.0.0/0",
+		LoggerFactory: logging.NewDefaultLoggerFactory(),
+	})
+	assert.NoError(t, err)
+
+	net0 := vnet.NewNet(&vnet.NetConfig{
+		StaticIPs: []string{"192.168.0.1"},
+	})
+	assert.NoError(t, wan.AddNet(net0))
+
+	net1 := vnet.NewNet(&vnet.NetConfig{
+		StaticIPs: []string{"192.168.0.2", "192.168.0.3", "192.168.0.4"},
+	})
+	assert.NoError(t, wan.AddNet(net1))
+
+	assert.NoError(t, wan.Start())
+
+	aNotifier, aConnected := onConnected()
+	bNotifier, bConnected := onConnected()
+
+	KeepaliveInterval := time.Hour
+	cfg0 := &AgentConfig{
+		NetworkTypes:     supportedNetworkTypes(),
+		MulticastDNSMode: MulticastDNSModeDisabled,
+		Net:              net0,
+
+		KeepaliveInterval:          &KeepaliveInterval,
+		CheckInterval:              &KeepaliveInterval,
+		AcceptAggressiveNomination: true,
+	}
+
+	var aAgent, bAgent *Agent
+	aAgent, err = NewAgent(cfg0)
+	require.NoError(t, err)
+	require.NoError(t, aAgent.OnConnectionStateChange(aNotifier))
+
+	cfg1 := &AgentConfig{
+		NetworkTypes:      supportedNetworkTypes(),
+		MulticastDNSMode:  MulticastDNSModeDisabled,
+		Net:               net1,
+		KeepaliveInterval: &KeepaliveInterval,
+		CheckInterval:     &KeepaliveInterval,
+	}
+
+	bAgent, err = NewAgent(cfg1)
+	require.NoError(t, err)
+	require.NoError(t, bAgent.OnConnectionStateChange(bNotifier))
+
+	aConn, bConn := connect(aAgent, bAgent)
+
+	// Ensure pair selected
+	// Note: this assumes ConnectionStateConnected is thrown after selecting the final pair
+	<-aConnected
+	<-bConnected
+
+	// Send new USE-CANDIDATE message with higher priority to update the selected pair
+	buildMsg := func(class stun.MessageClass, username, key string, priority uint32) *stun.Message {
+		msg, err1 := stun.Build(stun.NewType(stun.MethodBinding, class), stun.TransactionID,
+			stun.NewUsername(username),
+			stun.NewShortTermIntegrity(key),
+			UseCandidate(),
+			PriorityAttr(priority),
+			stun.Fingerprint,
+		)
+		if err1 != nil {
+			t.Fatal(err1)
+		}
+
+		return msg
+	}
+
+	selectedCh := make(chan Candidate, 1)
+	var expectNewSelectedCandidate Candidate
+	err = aAgent.OnSelectedCandidatePairChange(func(_, remote Candidate) {
+		selectedCh <- remote
+	})
+	require.NoError(t, err)
+	var bcandidates []Candidate
+	bcandidates, err = bAgent.GetLocalCandidates()
+	require.NoError(t, err)
+
+	for _, c := range bcandidates {
+		if c != bAgent.getSelectedPair().Local {
+			if expectNewSelectedCandidate == nil {
+			incr_priority:
+				for _, candidates := range aAgent.remoteCandidates {
+					for _, candidate := range candidates {
+						if candidate.Equal(c) {
+							candidate.(*CandidateHost).priorityOverride += 1000
+							break incr_priority
+						}
+					}
+				}
+				expectNewSelectedCandidate = c
+			}
+			_, err = c.writeTo(buildMsg(stun.ClassRequest, aAgent.localUfrag+":"+aAgent.remoteUfrag, aAgent.localPwd, c.Priority()).Raw, bAgent.getSelectedPair().Remote)
+			require.NoError(t, err)
+		}
+	}
+
+	time.Sleep(1 * time.Second)
+	select {
+	case selected := <-selectedCh:
+		assert.True(t, selected.Equal(expectNewSelectedCandidate))
+	default:
+		t.Fatal("No selected candidate pair")
+	}
+
+	assert.NoError(t, wan.Stop())
+	if !closePipe(t, aConn, bConn) {
+		return
+	}
+}