Requested changes

Author: Karolk99

lib/ex_ice/ice_agent.ex

@@ -48,17 +48,19 @@ defmodule ExICE.ICEAgent do
   @type ip_filter() :: (:inet.ip_address() -> boolean)
 
   @typedoc """
-  Mapping function used instead of STUN server. Maps local ip addresses into public ones.
-  These public addresses are then used to create server reflexive candidates.
+  Function called for each host candidate to derive a new "fabricated" srflx candidate from it.
+  This function takes host's ip as an argument and should return srflx's ip as a result or nil if for given host candidate
+  there should be no srflx one.
 
   Note that each returned IP address must be unique.
   If the mapping function repeatedly returns the same address,
   it will be ignored, and only one server reflexive candidate will be created.
 
-  This function is meant to be used for server implementations where the public addresses are well known
-  and NAT use 1 to 1 port mapping.
+  This function is meant to be used for server implementations where the public addresses are well known.
+  NAT uses 1 to 1 port mapping and using STUN server for discovering public IP is undesirable
+  (e.g. because of unknown response time).
   """
-  @type map_to_nat_ip() :: (:inet.ip_address() -> :inet.ip_address() | nil)
+  @type host_to_srflx_ip_mapper() :: (:inet.ip_address() -> :inet.ip_address() | nil)
 
   @typedoc """
   ICE Agent configuration options.
@@ -84,16 +86,9 @@ defmodule ExICE.ICEAgent do
   * `on_connection_state_change` - where to send connection state change notifications. Defaults to a process that spawns `ExICE`.
   * `on_data` - where to send data. Defaults to a process that spawns `ExICE`.
   * `on_new_candidate` - where to send new candidates. Defaults to a process that spawns `ExICE`.
-  * `map_to_nat_ip` - Mapping function used instead of STUN server. Maps
-  local ip addresses into public ones.
-  These public addresses are then used to create server reflexive candidates.
-
-  Note that each returned IP address must be unique.
-  If the mapping function repeatedly returns the same address,
-  it will be ignored, and only one server reflexive candidate will be created.
-
-  This function is meant to be used for server implementations where the public addresses are well known
-  and NAT use 1 to 1 port mapping.
+  * `host_to_srflx_ip_mapper` - function called for each host candidate to derive a new "fabricated" srflx candidate from it.
+  This function takes host's ip as an argument and should return srflx's ip as a result or nil if for given host candidate
+  there should be no srflx one.
   """
   @type opts() :: [
           role: role() | nil,
@@ -112,7 +107,7 @@ defmodule ExICE.ICEAgent do
           on_connection_state_change: pid() | nil,
           on_data: pid() | nil,
           on_new_candidate: pid() | nil,
-          map_to_nat_ip: map_to_nat_ip() | nil
+          host_to_srflx_ip_mapper: host_to_srflx_ip_mapper() | nil
         ]
 
   @doc """

lib/ex_ice/priv/gatherer.ex

@@ -1,6 +1,7 @@
 defmodule ExICE.Priv.Gatherer do
   @moduledoc false
 
+  alias ExICE.ICEAgent
   alias ExICE.Priv.{Candidate, Transport, Utils}
   alias ExSTUN.Message
   alias ExSTUN.Message.Type
@@ -139,6 +140,104 @@ defmodule ExICE.Priv.Gatherer do
     end
   end
 
+  @spec fabricate_srflx_candidates([Candidate.Host.t()], ICEAgent.host_to_srflx_ip_mapper(), %{
+          :inet.ip_address() => non_neg_integer()
+        }) :: [Candidate.Srflx.t()]
+  def fabricate_srflx_candidates(_host_cands, nil, _local_preferences) do
+    []
+  end
+
+  def fabricate_srflx_candidates(host_cands, host_to_srflx_ip_mapper, local_preferences) do
+    do_fabricate_srflx_candidates(
+      host_cands,
+      host_to_srflx_ip_mapper,
+      local_preferences,
+      [],
+      []
+    )
+  end
+
+  defp do_fabricate_srflx_candidates(
+         [],
+         _host_to_srflx_ip_mapper,
+         _local_preferences,
+         srflx_cands,
+         _external_ips
+       ) do
+    srflx_cands
+  end
+
+  defp do_fabricate_srflx_candidates(
+         [host_cand | rest],
+         host_to_srflx_ip_mapper,
+         local_preferences,
+         srflx_cands,
+         external_ips
+       ) do
+    external_ip = host_to_srflx_ip_mapper.(host_cand.base.address)
+
+    if valid_external_ip?(external_ip, host_cand.base.address, external_ips) do
+      priority =
+        Candidate.priority!(local_preferences, host_cand.base.address, :srflx)
+
+      cand =
+        Candidate.Srflx.new(
+          address: external_ip,
+          port: host_cand.base.port,
+          base_address: host_cand.base.address,
+          base_port: host_cand.base.port,
+          priority: priority,
+          transport_module: host_cand.base.transport_module,
+          socket: host_cand.base.socket
+        )
+
+      Logger.debug("New srflx candidate from NAT mapping: #{inspect(cand)}")
+
+      do_fabricate_srflx_candidates(
+        rest,
+        host_to_srflx_ip_mapper,
+        local_preferences,
+        [cand | srflx_cands],
+        [external_ip | external_ips]
+      )
+    else
+      do_fabricate_srflx_candidates(
+        rest,
+        host_to_srflx_ip_mapper,
+        local_preferences,
+        srflx_cands,
+        external_ips
+      )
+    end
+  end
+
+  defp valid_external_ip?(external_ip, host_ip, external_ips) do
+    same_type? = :inet.is_ipv4_address(external_ip) == :inet.is_ipv4_address(host_ip)
+
+    cond do
+      host_ip == external_ip ->
+        log_warning(host_ip, external_ip, "external IP is the same as local IP")
+        false
+
+      not :inet.is_ip_address(external_ip) or not same_type? ->
+        log_warning(host_ip, external_ip, "not valid IP address")
+        false
+
+      external_ip in external_ips ->
+        log_warning(host_ip, external_ip, "address already in use")
+        false
+
+      true ->
+        true
+    end
+  end
+
+  defp log_warning(host_ip, external_ip, reason),
+    do:
+      Logger.warning(
+        "Ignoring NAT mapping: #{inspect(host_ip)} to #{inspect(external_ip)}, #{inspect(reason)}"
+      )
+
   defp loopback_if?({_int_name, int}) do
     :loopback in int[:flags]
   end

lib/ex_ice/priv/ice_agent.ex

@@ -10,7 +10,6 @@ defmodule ExICE.Priv.ICEAgent do
     ConnCheckHandler,
     Gatherer,
     IfDiscovery,
-    NATMapper,
     Transport,
     Utils
   }
@@ -91,6 +90,7 @@ defmodule ExICE.Priv.ICEAgent do
     stun_servers: [],
     turn_servers: [],
     resolved_turn_servers: [],
+    host_to_srflx_ip_mapper: nil,
     # stats
     bytes_sent: 0,
     bytes_received: 0,
@@ -99,8 +99,7 @@ defmodule ExICE.Priv.ICEAgent do
     selected_candidate_pair_changes: 0,
     # binding requests that failed to pass checks required to assign them to specific candidate pair
     # e.g. missing required attributes, role conflict, authentication, etc.
-    unmatched_requests: 0,
-    map_to_nat_ip: nil
+    unmatched_requests: 0
   ]
 
   @spec unmarshal_remote_candidate(String.t()) :: {:ok, Candidate.t()} | {:error, term()}
@@ -168,7 +167,7 @@ defmodule ExICE.Priv.ICEAgent do
       local_pwd: local_pwd,
       stun_servers: stun_servers,
       turn_servers: turn_servers,
-      map_to_nat_ip: opts[:map_to_nat_ip]
+      host_to_srflx_ip_mapper: opts[:host_to_srflx_ip_mapper]
     }
   end
 
@@ -328,9 +327,9 @@ defmodule ExICE.Priv.ICEAgent do
     ice_agent = %__MODULE__{ice_agent | local_preferences: local_preferences}
 
     srflx_cands =
-      NATMapper.create_srflx_candidates(
+      Gatherer.fabricate_srflx_candidates(
         host_cands,
-        ice_agent.map_to_nat_ip,
+        ice_agent.host_to_srflx_ip_mapper,
         ice_agent.local_preferences
       )
 

lib/ex_ice/priv/nat_mapper.ex

@@ -2631,11 +2631,11 @@ defmodule ExICE.Priv.ICEAgentTest do
     end
   end
 
-  describe "NAT mapping" do
+  describe "host to prefabricated srflx mapper" do
     alias ExICE.Priv.Candidate
 
     @ipv4 {10, 10, 10, 10}
-    @ipv6 {0, 0, 0, 0, 0, 0, 0, 1}
+    @ipv6 {64_512, 0, 0, 0, 0, 0, 0, 1}
     @invalid_ip :invalid_ip
 
     test "adds srflx candidate" do
@@ -2650,7 +2650,7 @@ defmodule ExICE.Priv.ICEAgentTest do
       assert srflx_cand =~ "typ srflx"
     end
 
-    test "creates only one candidate if external ip repeats itself" do
+    test "creates only one candidate if external ip is repeated" do
       ice_agent = spawn_ice_agent(IfDiscovery.MockMulti, fn _ip -> @ipv4 end)
 
       assert [%Candidate.Srflx{base: %{address: @ipv4}}] = srflx_candidates(ice_agent)
@@ -2701,7 +2701,7 @@ defmodule ExICE.Priv.ICEAgentTest do
           transport_module: Transport.Mock,
           if_discovery_module: IfDiscovery.MockSingle,
           ice_servers: [%{urls: "stun:192.168.0.3:19302"}],
-          map_to_nat_ip: fn _ip -> @ipv4 end
+          host_to_srflx_ip_mapper: fn _ip -> @ipv4 end
         )
         |> ICEAgent.set_remote_credentials("remoteufrag", "remotepwd")
         |> ICEAgent.gather_candidates()
@@ -2732,14 +2732,14 @@ defmodule ExICE.Priv.ICEAgentTest do
       assert [%Candidate.Srflx{}] = srflx_candidates(ice_agent)
     end
 
-    defp spawn_ice_agent(discovery_module, map_to_nat_ip) do
+    defp spawn_ice_agent(discovery_module, host_to_srflx_ip_mapper) do
       %ICEAgent{gathering_state: :complete} =
         ICEAgent.new(
           controlling_process: self(),
           role: :controlled,
           transport_module: Transport.Mock,
           if_discovery_module: discovery_module,
-          map_to_nat_ip: map_to_nat_ip
+          host_to_srflx_ip_mapper: host_to_srflx_ip_mapper
         )
         |> ICEAgent.set_remote_credentials("remoteufrag", "remotepwd")
         |> ICEAgent.gather_candidates()