Resolves IP Lookups (#313)

Closes #270 

Improve handling of IP lookup providers and fallbacks

config/config.exs

@@ -10,6 +10,7 @@ config :git_hooks,
         "mix format --check-formatted",
         "mix compile --warnings-as-errors",
         "mix credo",
+        "mix knigge.verify",
         "mix test --trace",
         "mix dialyzer"
       ]
@@ -124,6 +125,16 @@ config :archethic, ArchethicWeb.FaucetController,
     "3A7B579DBFB7CEBE26293850058F180A65D6A3D2F6964543F5EDE07BEB2EFDA4"
     |> Base.decode16!(case: :mixed)
 
+# -----Start-of-Networking-configs-----
+
+config :archethic, Archethic.Networking.IPLookup.NATDiscovery,
+  provider: Archethic.Networking.IPLookup.NATDiscovery.UPnPv1
+
+config :archethic, Archethic.Networking.IPLookup.RemoteDiscovery,
+  provider: Archethic.Networking.IPLookup.RemoteDiscovery.IPIFY
+
+# -----End-of-Networking-configs ------
+
 # Import environment specific config. This must remain at the bottom
 # of this file so it overrides the configuration defined above.
-import_config "#{Mix.env()}.exs"
+import_config("#{Mix.env()}.exs")

config/dev.exs

@@ -87,13 +87,19 @@ config :archethic, Archethic.OracleChain.Scheduler,
   # Aggregate chain at the 50th second
   summary_interval: "0 * * * * *"
 
+# -----Start-of-Networking-dev-configs-----
+config :archethic, Archethic.Networking,
+  validate_node_ip: System.get_env("ARCHETHIC_NODE_IP_VALIDATION", "false") == "true"
+
 config :archethic, Archethic.Networking.IPLookup, Archethic.Networking.IPLookup.Static
 
 config :archethic, Archethic.Networking.IPLookup.Static,
   hostname: System.get_env("ARCHETHIC_STATIC_IP", "127.0.0.1")
 
 config :archethic, Archethic.Networking.Scheduler, interval: "0 * * * * * *"
 
+# -----end-of-Networking-dev-configs-----
+
 config :archethic, Archethic.Reward.NetworkPoolScheduler,
   # At the 30th second
   interval: "30 * * * * *"

config/prod.exs

@@ -144,17 +144,22 @@ config :archethic, Archethic.Mining.PendingTransactionValidation,
         :software
     end)
 
+# -----Start-of-Networking-prod-configs-----
+
+config :archethic, Archethic.Networking,
+  validate_node_ip: System.get_env("ARCHETHIC_NODE_IP_VALIDATION", "true") == "true"
+
 config :archethic,
        Archethic.Networking.IPLookup,
        (case(System.get_env("ARCHETHIC_NETWORKING_IMPL", "NAT") |> String.upcase()) do
           "NAT" ->
-            Archethic.Networking.IPLookup.NAT
+            Archethic.Networking.IPLookup.NATDiscovery
 
           "STATIC" ->
             Archethic.Networking.IPLookup.Static
 
-          "IPFY" ->
-            Archethic.Networking.IPLookup.IPIFY
+          "REMOTE" ->
+            Archethic.Networking.IPLookup.RemoteDiscovery
         end)
 
 config :archethic, Archethic.Networking.PortForwarding,
@@ -170,6 +175,8 @@ config :archethic, Archethic.Networking.PortForwarding,
 config :archethic, Archethic.Networking.IPLookup.Static,
   hostname: System.get_env("ARCHETHIC_STATIC_IP")
 
+# -----end-of-Networking-prod-configs-----
+
 config :archethic, Archethic.Networking.Scheduler,
   interval: System.get_env("ARCHETHIC_NETWORKING_UPDATE_SCHEDULER", "0 0 * * * * *")
 
@@ -208,16 +215,6 @@ config :archethic, Archethic.P2P.BootstrappingSeeds,
   # TODO: define the default list of P2P seeds once the network will be more open to new miners
   genesis_seeds: System.get_env("ARCHETHIC_P2P_BOOTSTRAPPING_SEEDS")
 
-config :archethic, Archethic.Mining.PendingTransactionValidation,
-  validate_node_ip:
-    (case(System.get_env("ARCHETHIC_NODE_IP_VALIDATION", "true")) do
-       "true" ->
-         true
-
-       _ ->
-         false
-     end)
-
 config :archethic, ArchethicWeb.FaucetController,
   enabled: System.get_env("ARCHETHIC_NETWORK_TYPE") == "testnet"
 

config/test.exs

@@ -85,10 +85,23 @@ config :archethic, Archethic.OracleChain.Scheduler,
 
 config :archethic, Archethic.OracleChain.Services.UCOPrice, provider: MockUCOPriceProvider
 
+# -----Start-of-Networking-tests-configs-----
+
+config :archethic, Archethic.Networking, validate_node_ip: false
+
 config :archethic, Archethic.Networking.IPLookup, MockIPLookup
+config :archethic, Archethic.Networking.IPLookup.Static, MockStatic
+
+# Regardless of default upnpV1 use MockNATDiscovery
+config :archethic, Archethic.Networking.IPLookup.NATDiscovery, provider: MockNATDiscovery
+# Regardless of default IPIFY use MockRemoteDiscovery
+config :archethic, Archethic.Networking.IPLookup.RemoteDiscovery, provider: MockRemoteDiscovery
+
 config :archethic, Archethic.Networking.PortForwarding, MockPortForwarding
 config :archethic, Archethic.Networking.Scheduler, enabled: false
 
+# -----End-of-Networking-tests-configs ------
+
 config :archethic, Archethic.P2P.Listener, enabled: false
 config :archethic, Archethic.P2P.MemTableLoader, enabled: false
 config :archethic, Archethic.P2P.MemTable, enabled: false

lib/archethic/mining/pending_transaction_validation.ex

@@ -137,8 +137,8 @@ defmodule Archethic.Mining.PendingTransactionValidation do
               key_certificate,
               root_ca_public_key
             )},
-         {:conn, true} <-
-           {:conn, valid_connection?(ip, port, previous_public_key, should_validate_node_ip?())} do
+         {:conn, :ok} <-
+           {:conn, valid_connection(ip, port, previous_public_key)} do
       :ok
     else
       :error ->
@@ -150,8 +150,12 @@ defmodule Archethic.Mining.PendingTransactionValidation do
       {:auth_origin, false} ->
         {:error, "Invalid node transaction with invalid key origin"}
 
-      {:conn, false} ->
-        {:error, "Invalid node connection (IP/Port) for the given public key"}
+      {:conn, {:error, :invalid_ip}} ->
+        {:error, "Invalid node's IP address"}
+
+      {:conn, {:error, :existing_node}} ->
+        {:error,
+         "Invalid node connection (IP/Port) for for the given public key - already existing"}
     end
   end
 
@@ -347,29 +351,16 @@ defmodule Archethic.Mining.PendingTransactionValidation do
 
   defp get_first_public_key([], _), do: {:error, :network_issue}
 
-  defp valid_connection?(ip, port, previous_public_key, _check_ip? = true) do
-    with true <- Networking.valid_ip?(ip),
+  defp valid_connection(ip, port, previous_public_key) do
+    with :ok <- Networking.validate_ip(ip),
          false <- P2P.duplicating_node?(ip, port, previous_public_key) do
-      true
+      :ok
     else
-      _ ->
-        false
-    end
-  end
-
-  defp valid_connection?(ip, port, previous_public_key, _check_ip? = false) do
-    case P2P.duplicating_node?(ip, port, previous_public_key) do
-      false ->
-        true
-
       true ->
-        false
-    end
-  end
+        {:error, :existing_node}
 
-  defp should_validate_node_ip? do
-    :archethic
-    |> Application.get_env(__MODULE__, [])
-    |> Keyword.get(:validate_node_ip, false)
+      {:error, :invalid_ip} ->
+        {:error, :invalid_ip}
+    end
   end
 end

lib/archethic/networking.ex

@@ -7,6 +7,43 @@ defmodule Archethic.Networking do
   alias __MODULE__.PortForwarding
 
   @ip_validate_regex ~r/(^0\.)|(^127\.)|(^10\.)|(^172\.1[6-9]\.)|(^172\.2[0-9]\.)|(^172\.3[0-1]\.)|(^192\.168\.)/
+
+  @doc ~S"""
+  Validates whether a given IP is a valid Public IP depending
+
+  upon whether it should be validated or not?
+
+  When mix_env = :dev , Use of Private IP : allowed , returns :ok
+  Static and Subnet(NAT), Private IP not to be validated to be public IP.
+
+  When mix_env = :prod, Use of Private IP : not allowed,
+  IP must be validated for a valid Public IP, otherwise return error
+
+  ## Example
+
+      iex> Archethic.Networking.validate_ip({0,0,0,0}, false)
+      :ok
+
+      iex> Archethic.Networking.validate_ip({127,0,0,1}, true)
+      {:error, :invalid_ip}
+
+
+      iex> Archethic.Networking.validate_ip({54,39,186,147},true)
+      :ok
+
+  """
+  @spec validate_ip(:inet.ip_address(), boolean()) :: :ok | {:error, :invalid_ip}
+  def validate_ip(ip, ip_validation? \\ should_validate_node_ip?())
+  def validate_ip(_ip, false), do: :ok
+
+  def validate_ip(ip, true) do
+    if valid_ip?(ip) do
+      :ok
+    else
+      {:error, :invalid_ip}
+    end
+  end
+
   @doc """
   Provides current host IP address by leveraging the IP lookup provider.
 
@@ -63,4 +100,10 @@ defmodule Archethic.Networking do
         )
     end
   end
+
+  defp should_validate_node_ip?() do
+    :archethic
+    |> Application.get_env(__MODULE__, [])
+    |> Keyword.get(:validate_node_ip, false)
+  end
 end

lib/archethic/networking/ip_lookup.ex

@@ -1,26 +1,27 @@
 defmodule Archethic.Networking.IPLookup do
   @moduledoc false
 
-  alias __MODULE__.IPIFY
-  alias __MODULE__.NAT
-
   require Logger
 
+  alias Archethic.Networking
+  alias Archethic.Networking.IPLookup.RemoteDiscovery
+  alias Archethic.Networking.IPLookup.NATDiscovery
+
   @doc """
   Get the node public ip with a fallback capability
 
   For example, using the NAT provider, if the UPnP discovery failed, it switches to the IPIFY to get the external public ip
   """
   @spec get_node_ip() :: :inet.ip_address()
-  def get_node_ip do
-    provider = get_provider()
+  def get_node_ip() do
+    provider = provider()
 
     ip =
-      case apply(provider, :get_node_ip, []) do
-        {:ok, ip} ->
-          Logger.info("Node IP discovered by #{provider}")
-          ip
-
+      with {:ok, ip} <- provider.get_node_ip(),
+           :ok <- Networking.validate_ip(ip) do
+        Logger.info("Node IP discovered by #{provider}")
+        ip
+      else
         {:error, reason} ->
           fallback(provider, reason)
       end
@@ -29,24 +30,24 @@ defmodule Archethic.Networking.IPLookup do
     ip
   end
 
-  defp get_provider do
-    Application.get_env(:archethic, __MODULE__)
-  end
-
-  defp fallback(NAT, reason) do
-    Logger.warning("Cannot use NAT IP lookup - #{inspect(reason)}")
-    Logger.info("Trying IPFY as fallback")
+  defp fallback(NATDiscovery, reason) do
+    Logger.warning("Cannot use NATDiscovery: NAT IP lookup - #{inspect(reason)}")
+    Logger.info("Trying PublicGateway: IPIFY as fallback")
 
-    case IPIFY.get_node_ip() do
+    case RemoteDiscovery.get_node_ip() do
       {:ok, ip} ->
         ip
 
       {:error, reason} ->
-        fallback(IPIFY, reason)
+        fallback(RemoteDiscovery, reason)
     end
   end
 
   defp fallback(provider, reason) do
     raise "Cannot use #{provider} IP lookup - #{inspect(reason)}"
   end
+
+  defp provider() do
+    Application.get_env(:archethic, __MODULE__)
+  end
 end

lib/archethic/networking/ip_lookup/nat_discovery.ex

@@ -0,0 +1,55 @@
+defmodule Archethic.Networking.IPLookup.NATDiscovery do
+  @moduledoc """
+  Provide abstraction over :natupnp_v1, :natupnp_v2, :natpmp
+  """
+
+  alias Archethic.Networking.IPLookup.Impl
+
+  alias __MODULE__.UPnPv1
+  alias __MODULE__.UPnPv2
+  alias __MODULE__.PMP
+
+  require Logger
+
+  @behaviour Impl
+  def get_node_ip() do
+    provider = provider()
+    do_get_node_ip(provider)
+  end
+
+  defp do_get_node_ip(provider) do
+    case provider.get_node_ip() do
+      {:ok, ip} ->
+        {:ok, ip}
+
+      {:error, reason} ->
+        Logger.error(
+          "Cannot use the provider #{provider} for IP Lookup - reason: #{inspect(reason)}"
+        )
+
+        fallback(provider, reason)
+    end
+  end
+
+  defp fallback(UPnPv1, _reason) do
+    do_get_node_ip(UPnPv2)
+  end
+
+  defp fallback(UPnPv2, _reason) do
+    do_get_node_ip(PMP)
+  end
+
+  defp fallback(PMP, reason) do
+    {:error, reason}
+  end
+
+  defp fallback(_provider, reason) do
+    {:error, reason}
+  end
+
+  defp provider() do
+    :archethic
+    |> Application.get_env(__MODULE__, [])
+    |> Keyword.get(:provider, UPnPv1)
+  end
+end