Fix role conflict resolution (#71)

Author: mickel8

README.md

@@ -30,7 +30,7 @@ as WebRTC multiplexes traffic on a single socket but PRs are welcomed
 ```elixir
 def deps do
   [
-    {:ex_ice, "~> 0.10.0"}
+    {:ex_ice, "~> 0.10.1"}
   ]
 end
 ```

lib/ex_ice/priv/ice_agent.ex

@@ -1521,7 +1521,10 @@ defmodule ExICE.Priv.ICEAgent do
 
   ## BINDING RESPONSE HANDLING ##
   defp handle_conn_check_response(ice_agent, local_cand, src_ip, src_port, msg) do
-    {%{pair_id: pair_id}, conn_checks} = Map.pop!(ice_agent.conn_checks, msg.transaction_id)
+    {%{pair_id: pair_id, raw_req: raw_req}, conn_checks} =
+      Map.pop!(ice_agent.conn_checks, msg.transaction_id)
+
+    {:ok, req} = Message.decode(raw_req)
     ice_agent = %__MODULE__{ice_agent | conn_checks: conn_checks}
     conn_check_pair = Map.fetch!(ice_agent.checklist, pair_id)
 
@@ -1539,7 +1542,7 @@ defmodule ExICE.Priv.ICEAgent do
     if symmetric?(ice_agent, local_cand.base.socket, {src_ip, src_port}, conn_check_pair) do
       case msg.type.class do
         :success_response -> handle_conn_check_success_response(ice_agent, conn_check_pair, msg)
-        :error_response -> handle_conn_check_error_response(ice_agent, conn_check_pair, msg)
+        :error_response -> handle_conn_check_error_response(ice_agent, conn_check_pair, req, msg)
       end
     else
       cc_local_cand = Map.fetch!(ice_agent.local_cands, conn_check_pair.local_cand_id)
@@ -1609,14 +1612,14 @@ defmodule ExICE.Priv.ICEAgent do
     end
   end
 
-  defp handle_conn_check_error_response(ice_agent, conn_check_pair, msg) do
+  defp handle_conn_check_error_response(ice_agent, conn_check_pair, req, resp) do
     # We only authenticate role conflict as it changes our state.
     # We don't add message-integrity to bad request and unauthenticated errors
     # so we also don't expect to receive it.
     # In the worst case scenario, we won't allow for the connection.
-    case Message.get_attribute(msg, ErrorCode) do
+    case Message.get_attribute(resp, ErrorCode) do
       {:ok, %ErrorCode{code: 487}} ->
-        handle_role_conflict_error_response(ice_agent, conn_check_pair, msg)
+        handle_role_conflict_error_response(ice_agent, conn_check_pair, req, resp)
 
       other ->
         Logger.debug(
@@ -1634,15 +1637,39 @@ defmodule ExICE.Priv.ICEAgent do
     end
   end
 
-  defp handle_role_conflict_error_response(ice_agent, conn_check_pair, msg) do
-    case authenticate_msg(msg, ice_agent.remote_pwd) do
+  defp handle_role_conflict_error_response(ice_agent, conn_check_pair, req, resp) do
+    case authenticate_msg(resp, ice_agent.remote_pwd) do
       :ok ->
-        new_role = if ice_agent.role == :controlling, do: :controlled, else: :controlling
+        {:ok, role} = get_role_attribute(req)
 
-        Logger.debug("""
-        Conn check failed due to role conflict. Changing our role to: #{new_role}, \
-        recomputing pair priorities, regenerating tiebreaker and rescheduling conn check \
-        """)
+        ice_agent =
+          case {role, ice_agent.role} do
+            {%ICEControlled{}, :controlling} ->
+              # seems that we've already switched
+              ice_agent
+
+            {%ICEControlling{}, :controlled} ->
+              # seems that we've already switched
+              ice_agent
+
+            _ ->
+              new_role = if ice_agent.role == :controlling, do: :controlled, else: :controlling
+
+              Logger.debug("""
+              Conn check failed due to role conflict. Changing our role to: #{new_role}, \
+              recomputing pair priorities, regenerating tiebreaker and rescheduling conn check \
+              """)
+
+              tiebreaker = generate_tiebreaker()
+              checklist = recompute_pair_prios(ice_agent)
+
+              %__MODULE__{
+                ice_agent
+                | role: new_role,
+                  checklist: checklist,
+                  tiebreaker: tiebreaker
+              }
+          end
 
         conn_check_pair = %CandidatePair{
           conn_check_pair
@@ -1651,8 +1678,8 @@ defmodule ExICE.Priv.ICEAgent do
         }
 
         checklist = Map.replace!(ice_agent.checklist, conn_check_pair.id, conn_check_pair)
-        tiebreaker = generate_tiebreaker()
-        %__MODULE__{ice_agent | role: new_role, checklist: checklist, tiebreaker: tiebreaker}
+
+        %__MODULE__{ice_agent | checklist: checklist}
 
       {:error, reason} ->
         Logger.debug(

mix.exs

@@ -1,7 +1,7 @@
 defmodule ExICE.MixProject do
   use Mix.Project
 
-  @version "0.10.0"
+  @version "0.10.1"
   @source_url "https://github.com/elixir-webrtc/ex_ice"
 
   def project do

test/priv/ice_agent_test.exs

@@ -63,6 +63,7 @@ defmodule ExICE.Priv.ICEAgentTest do
   end
 
   @remote_cand ExICE.Candidate.new(:host, address: {192, 168, 0, 2}, port: 8445)
+  @remote_cand2 ExICE.Candidate.new(:host, address: {192, 168, 0, 3}, port: 8445)
 
   describe "unmarshal_remote_candidate/1" do
     test "with correct candidate" do
@@ -644,6 +645,62 @@ defmodule ExICE.Priv.ICEAgentTest do
       assert_bad_request_error_response(socket, request)
     end
 
+    test "with role conflict", %{ice_agent: ice_agent, remote_cand: remote_cand} do
+      [socket] = ice_agent.sockets
+
+      binding_request = fn tiebreaker ->
+        Message.new(%Type{class: :request, method: :binding}, [
+          %Username{value: "#{ice_agent.local_ufrag}:someufrag"},
+          %Priority{priority: 1234},
+          %ICEControlling{tiebreaker: tiebreaker},
+          %UseCandidate{}
+        ])
+        |> Message.with_integrity(ice_agent.local_pwd)
+        |> Message.with_fingerprint()
+      end
+
+      # feed binding request with higher tiebreaker
+      request = binding_request.(ice_agent.tiebreaker + 1)
+      raw_request = Message.encode(request)
+
+      new_ice_agent =
+        ICEAgent.handle_udp(
+          ice_agent,
+          socket,
+          remote_cand.address,
+          remote_cand.port,
+          raw_request
+        )
+
+      # agent should switch its role and send success response
+      assert new_ice_agent.role == :controlled
+      assert new_ice_agent.tiebreaker == ice_agent.tiebreaker
+      assert packet = Transport.Mock.recv(socket)
+      assert {:ok, msg} = ExSTUN.Message.decode(packet)
+      assert msg.type == %ExSTUN.Message.Type{class: :success_response, method: :binding}
+
+      # feed binding request with smaller tiebreaker
+      request = binding_request.(ice_agent.tiebreaker - 1)
+      raw_request = Message.encode(request)
+
+      new_ice_agent =
+        ICEAgent.handle_udp(
+          ice_agent,
+          socket,
+          remote_cand.address,
+          remote_cand.port,
+          raw_request
+        )
+
+      # agent shouldn't switch its role and should send 487 error response
+      assert new_ice_agent.role == :controlling
+      assert new_ice_agent.tiebreaker == ice_agent.tiebreaker
+      assert packet = Transport.Mock.recv(socket)
+      assert {:ok, msg} = ExSTUN.Message.decode(packet)
+      assert msg.type == %ExSTUN.Message.Type{class: :error_response, method: :binding}
+      assert {:ok, %ErrorCode{code: 487, reason: ""}} = Message.get_attribute(msg, ErrorCode)
+    end
+
     test "without username", %{ice_agent: ice_agent, remote_cand: remote_cand} do
       [socket] = ice_agent.sockets
 
@@ -1157,6 +1214,80 @@ defmodule ExICE.Priv.ICEAgentTest do
       assert new_pair.responses_received == pair.responses_received + 1
     end
 
+    test "role conflict error response" do
+      ice_agent =
+        ICEAgent.new(
+          controlling_process: self(),
+          role: :controlling,
+          if_discovery_module: IfDiscovery.Mock,
+          transport_module: Transport.Mock
+        )
+        |> ICEAgent.set_remote_credentials("someufrag", "somepwd")
+        |> ICEAgent.gather_candidates()
+        |> ICEAgent.add_remote_candidate(@remote_cand)
+
+      [socket] = ice_agent.sockets
+
+      # trigger check
+      ice_agent = ICEAgent.handle_ta_timeout(ice_agent)
+      [pair1] = Map.values(ice_agent.checklist)
+      req1 = read_binding_request(socket, ice_agent.remote_pwd)
+
+      # Add the second candidate and trigger another check.
+      # We add candidate after generating the first check to be sure
+      # it is related to @remote_cand2.
+      ice_agent = ICEAgent.add_remote_candidate(ice_agent, @remote_cand2)
+      ice_agent = ICEAgent.handle_ta_timeout(ice_agent)
+      [pair2] = Map.values(ice_agent.checklist) |> Enum.reject(&(&1.id == pair1.id))
+      req2 = read_binding_request(socket, ice_agent.remote_pwd)
+
+      # reply to the first check with role conflict error response
+      resp =
+        Message.new(req1.transaction_id, %Type{class: :error_response, method: :binding}, [
+          %ErrorCode{code: 487}
+        ])
+        |> Message.with_integrity(ice_agent.remote_pwd)
+        |> Message.with_fingerprint()
+        |> Message.encode()
+
+      new_ice_agent =
+        ICEAgent.handle_udp(
+          ice_agent,
+          socket,
+          @remote_cand.address,
+          @remote_cand.port,
+          resp
+        )
+
+      # assert that the agent changed its role and tiebreaker
+      assert new_ice_agent.role != ice_agent.role
+      assert new_ice_agent.tiebreaker != ice_agent.tiebreaker
+      assert Map.fetch!(new_ice_agent.checklist, pair1.id).state == :waiting
+
+      # reply to the second check with role conflict error response
+      resp =
+        Message.new(req2.transaction_id, %Type{class: :error_response, method: :binding}, [
+          %ErrorCode{code: 487}
+        ])
+        |> Message.with_integrity(new_ice_agent.remote_pwd)
+        |> Message.with_fingerprint()
+        |> Message.encode()
+
+      new_ice_agent2 =
+        ICEAgent.handle_udp(
+          new_ice_agent,
+          socket,
+          @remote_cand2.address,
+          @remote_cand2.port,
+          resp
+        )
+
+      # assert that agent didn't switch its role and tiebreaker
+      assert new_ice_agent2.role == new_ice_agent.role
+      assert new_ice_agent2.tiebreaker == new_ice_agent.tiebreaker
+      assert Map.fetch!(new_ice_agent2.checklist, pair2.id).state == :waiting
+    end
+
     test "unauthenticated error response", %{ice_agent: ice_agent, remote_cand: remote_cand} do
       [socket] = ice_agent.sockets
       [pair] = Map.values(ice_agent.checklist)