Update DNS portal config (#2432)

Why:

* After further discussion around the Client DNS settings, it was
decided that keeping both `type` and `address` would be easier to help
with validation and parsing. At the moment, only IP DNS servers are
accepted, but placeholders for `DNS over TLS` and `DNS over HTTPS` have
been created.

elixir/apps/api/lib/api/client/views/interface.ex

@@ -1,10 +1,15 @@
 defmodule API.Client.Views.Interface do
   alias Domain.Clients
+  alias Domain.Config.Configuration.ClientsUpstreamDNS
 
   def render(%Clients.Client{} = client) do
     upstream_dns =
       Clients.fetch_client_config!(client)
       |> Keyword.fetch!(:upstream_dns)
+      |> Enum.map(fn dns_config ->
+        addr = ClientsUpstreamDNS.normalize_dns_address(dns_config)
+        %{dns_config | address: addr}
+      end)
 
     %{
       upstream_dns: upstream_dns,

elixir/apps/api/test/api/client/channel_test.exs

@@ -7,7 +7,10 @@ defmodule API.Client.ChannelTest do
 
     Fixtures.Config.upsert_configuration(
       account: account,
-      clients_upstream_dns: [%{address: "1.1.1.1"}]
+      clients_upstream_dns: [
+        %{protocol: "ip_port", address: "1.1.1.1"},
+        %{protocol: "ip_port", address: "8.8.8.8:53"}
+      ]
     )
 
     actor_group = Fixtures.Actors.create_group(account: account)
@@ -138,7 +141,14 @@ defmodule API.Client.ChannelTest do
                ipv4: client.ipv4,
                ipv6: client.ipv6,
                upstream_dns: [
-                 %Domain.Config.Configuration.ClientsUpstreamDNS{address: "1.1.1.1"}
+                 %Domain.Config.Configuration.ClientsUpstreamDNS{
+                   protocol: :ip_port,
+                   address: "1.1.1.1:53"
+                 },
+                 %Domain.Config.Configuration.ClientsUpstreamDNS{
+                   protocol: :ip_port,
+                   address: "8.8.8.8:53"
+                 }
                ]
              }
     end

elixir/apps/domain/lib/domain/config/configuration.ex

@@ -1,12 +1,10 @@
 defmodule Domain.Config.Configuration do
   use Domain, :schema
   alias Domain.Config.Logo
+  alias Domain.Config.Configuration.ClientsUpstreamDNS
 
   schema "configurations" do
-    embeds_many :clients_upstream_dns, ClientsUpstreamDNS, on_replace: :delete, primary_key: false do
-      field :address, :string
-    end
-
+    embeds_many :clients_upstream_dns, ClientsUpstreamDNS, on_replace: :delete
     embeds_one :logo, Logo, on_replace: :delete
 
     belongs_to :account, Domain.Accounts.Account

elixir/apps/domain/lib/domain/config/configuration/changeset.ex

@@ -1,6 +1,7 @@
 defmodule Domain.Config.Configuration.Changeset do
   use Domain, :changeset
   import Domain.Config, only: [config_changeset: 2]
+  alias Domain.Config.Configuration.ClientsUpstreamDNS
 
   @fields ~w[clients_upstream_dns logo]a
 
@@ -9,10 +10,8 @@ defmodule Domain.Config.Configuration.Changeset do
       configuration
       |> cast(attrs, [])
       |> cast_embed(:logo)
-      |> cast_embed(
-        :clients_upstream_dns,
-        with: &clients_upstream_dns_changeset/2
-      )
+      |> cast_embed(:clients_upstream_dns)
+      |> validate_unique_dns()
 
     Enum.reduce(@fields, changeset, fn field, changeset ->
       config_changeset(changeset, field)
@@ -20,21 +19,20 @@ defmodule Domain.Config.Configuration.Changeset do
     |> ensure_no_overridden_changes(configuration.account_id)
   end
 
-  def clients_upstream_dns_changeset(
-        dns_config \\ %Domain.Config.Configuration.ClientsUpstreamDNS{},
-        attrs
-      ) do
-    Ecto.Changeset.cast(
-      dns_config,
-      attrs,
-      [:address]
-    )
-    |> validate_required(:address)
-    |> trim_change(:address)
-    |> Domain.Validator.validate_one_of(:address, [
-      &Domain.Validator.validate_fqdn/2,
-      &Domain.Validator.validate_uri(&1, &2, schemes: ["https"])
-    ])
+  defp validate_unique_dns(changeset) do
+    dns_addrs =
+      apply_changes(changeset)
+      |> Map.get(:clients_upstream_dns)
+      |> Enum.map(&ClientsUpstreamDNS.normalize_dns_address/1)
+      |> Enum.reject(&is_nil/1)
+
+    duplicates = dns_addrs -- Enum.uniq(dns_addrs)
+
+    if duplicates != [] do
+      add_error(changeset, :clients_upstream_dns, "no duplicates allowed")
+    else
+      changeset
+    end
   end
 
   defp ensure_no_overridden_changes(changeset, account_id) do

elixir/apps/domain/lib/domain/config/configuration/clients_upstream_dns.ex

@@ -0,0 +1,63 @@
+defmodule Domain.Config.Configuration.ClientsUpstreamDNS do
+  @moduledoc """
+  Embedded Schema for Clients Upstream DNS
+  """
+  use Domain, :schema
+  import Domain.Validator
+  import Ecto.Changeset
+  alias Domain.Types.IPPort
+
+  @primary_key false
+  embedded_schema do
+    field :protocol, Ecto.Enum, values: [:ip_port, :dns_over_tls, :dns_over_http]
+    field :address, :string
+  end
+
+  def changeset(dns_config \\ %__MODULE__{}, attrs) do
+    dns_config
+    |> cast(attrs, [:protocol, :address])
+    |> validate_required([:protocol, :address])
+    |> trim_change(:address)
+    |> validate_inclusion(:protocol, supported_protocols(),
+      message: "this type of DNS provider is not supported yet"
+    )
+    |> validate_address()
+  end
+
+  def supported_protocols do
+    ~w[ip_port]a
+  end
+
+  def validate_address(changeset) do
+    if has_errors?(changeset, :protocol) do
+      changeset
+    else
+      case fetch_field(changeset, :protocol) do
+        {_changes_or_data, :ip_port} -> validate_ip_port(changeset)
+        :error -> changeset
+      end
+    end
+  end
+
+  def validate_ip_port(changeset) do
+    validate_change(changeset, :address, fn :address, address ->
+      case IPPort.cast(address) do
+        {:ok, _ip} -> []
+        _ -> [address: "must be a valid IP address"]
+      end
+    end)
+  end
+
+  def normalize_dns_address(%__MODULE__{protocol: :ip_port, address: address}) do
+    case IPPort.cast(address) do
+      {:ok, ip} -> IPPort.put_default_port(ip, default_dns_port()) |> to_string()
+      _ -> address
+    end
+  end
+
+  def normalize_dns_address(%__MODULE__{protocol: _, address: address}) do
+    address
+  end
+
+  def default_dns_port, do: 53
+end

elixir/apps/domain/lib/domain/config/definitions.ex

@@ -454,9 +454,9 @@ defmodule Domain.Config.Definitions do
   defconfig(
     :clients_upstream_dns,
     {:json_array, {:embed, Domain.Config.Configuration.ClientsUpstreamDNS},
-     validate_unique: true},
+     validate_unique: false},
     default: [],
-    changeset: {Domain.Config.Configuration.Changeset, :clients_upstream_dns_changeset, []}
+    changeset: {Domain.Config.Configuration.ClientsUpstreamDNS, :changeset, []}
   )
 
   ##############################################

elixir/apps/domain/lib/domain/types/ip_port.ex

@@ -44,7 +44,7 @@ defmodule Domain.Types.IPPort do
   def cast_address(address) do
     address
     |> String.to_charlist()
-    |> :inet.parse_address()
+    |> :inet.parse_strict_address()
   end
 
   defp cast_port(nil), do: {:ok, nil}
@@ -88,4 +88,9 @@ defmodule Domain.Types.IPPort do
     ip = ip |> :inet.ntoa() |> List.to_string()
     "[#{ip}]:#{port}"
   end
+
+  def put_default_port(ip_port, default_port) do
+    port = ip_port.port || default_port
+    %{ip_port | port: port}
+  end
 end

elixir/apps/domain/test/domain/config_test.exs

@@ -93,7 +93,18 @@ defmodule Domain.ConfigTest do
       assert fetch_resolved_configs!(account.id, [:clients_upstream_dns, :clients_upstream_dns]) ==
                %{
                  clients_upstream_dns: [
-                   %Domain.Config.Configuration.ClientsUpstreamDNS{address: "1.1.1.1"}
+                   %Domain.Config.Configuration.ClientsUpstreamDNS{
+                     protocol: :ip_port,
+                     address: "1.1.1.1"
+                   },
+                   %Domain.Config.Configuration.ClientsUpstreamDNS{
+                     protocol: :ip_port,
+                     address: "2606:4700:4700::1111"
+                   },
+                   %Domain.Config.Configuration.ClientsUpstreamDNS{
+                     protocol: :ip_port,
+                     address: "8.8.8.8:853"
+                   }
                  ]
                }
     end
@@ -143,7 +154,20 @@ defmodule Domain.ConfigTest do
                %{
                  clients_upstream_dns:
                    {{:db, :clients_upstream_dns},
-                    [%Domain.Config.Configuration.ClientsUpstreamDNS{address: "1.1.1.1"}]}
+                    [
+                      %Domain.Config.Configuration.ClientsUpstreamDNS{
+                        protocol: :ip_port,
+                        address: "1.1.1.1"
+                      },
+                      %Domain.Config.Configuration.ClientsUpstreamDNS{
+                        protocol: :ip_port,
+                        address: "2606:4700:4700::1111"
+                      },
+                      %Domain.Config.Configuration.ClientsUpstreamDNS{
+                        protocol: :ip_port,
+                        address: "8.8.8.8:853"
+                      }
+                    ]}
                }
     end
 
@@ -444,25 +468,25 @@ defmodule Domain.ConfigTest do
       config = get_account_config_by_account_id(account.id)
 
       attrs = %{
-        clients_upstream_dns: [%{address: "!!!"}]
+        clients_upstream_dns: [%{protocol: "ip_port", address: "!!!"}]
       }
 
       assert {:error, changeset} = update_config(config, attrs)
 
       assert errors_on(changeset) == %{
                clients_upstream_dns: [
-                 %{address: ["does not contain a scheme or a host", "!!! is not a valid FQDN"]}
+                 %{address: ["must be a valid IP address"]}
                ]
              }
     end
 
     test "returns error when trying to change overridden value", %{account: account} do
-      put_system_env_override(:clients_upstream_dns, [%{address: "1.2.3.4"}])
+      put_system_env_override(:clients_upstream_dns, [%{protocol: "ip_port", address: "1.2.3.4"}])
 
       config = get_account_config_by_account_id(account.id)
 
       attrs = %{
-        clients_upstream_dns: [%{address: "4.1.2.3"}]
+        clients_upstream_dns: [%{protocol: "ip_port", address: "4.1.2.3"}]
       }
 
       assert {:error, changeset} = update_config(config, attrs)
@@ -479,25 +503,47 @@ defmodule Domain.ConfigTest do
       config = get_account_config_by_account_id(account.id)
 
       attrs = %{
-        clients_upstream_dns: [%{address: "   foobar.com"}, %{address: "google.com   "}]
+        clients_upstream_dns: [
+          %{protocol: "ip_port", address: "   1.1.1.1"},
+          %{protocol: "ip_port", address: "8.8.8.8   "}
+        ]
       }
 
       assert {:ok, config} = update_config(config, attrs)
 
       assert config.clients_upstream_dns == [
-               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "foobar.com"},
-               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "google.com"}
+               %Domain.Config.Configuration.ClientsUpstreamDNS{
+                 protocol: :ip_port,
+                 address: "1.1.1.1"
+               },
+               %Domain.Config.Configuration.ClientsUpstreamDNS{
+                 protocol: :ip_port,
+                 address: "8.8.8.8"
+               }
              ]
     end
 
     test "changes database config value when it did not exist", %{account: account} do
       config = get_account_config_by_account_id(account.id)
-      attrs = %{clients_upstream_dns: [%{address: "foobar.com"}, %{address: "google.com"}]}
+
+      attrs = %{
+        clients_upstream_dns: [
+          %{protocol: "ip_port", address: "1.1.1.1"},
+          %{protocol: "ip_port", address: "8.8.8.8"}
+        ]
+      }
+
       assert {:ok, config} = update_config(config, attrs)
 
       assert config.clients_upstream_dns == [
-               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "foobar.com"},
-               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "google.com"}
+               %Domain.Config.Configuration.ClientsUpstreamDNS{
+                 protocol: :ip_port,
+                 address: "1.1.1.1"
+               },
+               %Domain.Config.Configuration.ClientsUpstreamDNS{
+                 protocol: :ip_port,
+                 address: "8.8.8.8"
+               }
              ]
     end
 
@@ -505,12 +551,25 @@ defmodule Domain.ConfigTest do
       Fixtures.Config.upsert_configuration(account: account)
 
       config = get_account_config_by_account_id(account.id)
-      attrs = %{clients_upstream_dns: [%{address: "foobar.com"}, %{address: "google.com"}]}
+
+      attrs = %{
+        clients_upstream_dns: [
+          %{protocol: "ip_port", address: "8.8.8.8"},
+          %{protocol: "ip_port", address: "8.8.4.4"}
+        ]
+      }
+
       assert {:ok, config} = update_config(config, attrs)
 
       assert config.clients_upstream_dns == [
-               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "foobar.com"},
-               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "google.com"}
+               %Domain.Config.Configuration.ClientsUpstreamDNS{
+                 protocol: :ip_port,
+                 address: "8.8.8.8"
+               },
+               %Domain.Config.Configuration.ClientsUpstreamDNS{
+                 protocol: :ip_port,
+                 address: "8.8.4.4"
+               }
              ]
     end
   end

elixir/apps/domain/test/domain/types/ip_port_test.exs

@@ -0,0 +1,47 @@
+defmodule Domain.Types.IPPortTest do
+  use ExUnit.Case, async: true
+  import Domain.Types.IPPort
+
+  describe "cast_address/1" do
+    test "uses strict parsing for IP addresses" do
+      assert cast_address("1.1.1") == {:error, :einval}
+      assert cast_address("1.1.1.1") == {:ok, {1, 1, 1, 1}}
+    end
+  end
+
+  describe "cast/1" do
+    test "only allows port numbers between 1 - 65_535" do
+      assert cast("1.1.1.1:0") == {:error, [message: "is invalid"]}
+
+      assert cast("1.1.1.1:1") ==
+               {:ok, %Domain.Types.IPPort{type: :ipv4, address: {1, 1, 1, 1}, port: 1}}
+
+      assert cast("1.1.1.1:65535") ==
+               {:ok, %Domain.Types.IPPort{type: :ipv4, address: {1, 1, 1, 1}, port: 65_535}}
+
+      assert cast("1.1.1.1:65536") == {:error, [message: "is invalid"]}
+    end
+  end
+
+  describe "put_default_port/1" do
+    test "sets default port when one does not exist" do
+      {:ok, ip_port} = cast("1.1.1.1")
+
+      assert put_default_port(ip_port, 53) == %Domain.Types.IPPort{
+               type: :ipv4,
+               address: {1, 1, 1, 1},
+               port: 53
+             }
+    end
+
+    test "does not set default port when port exists" do
+      {:ok, ip_port} = cast("1.1.1.1:853")
+
+      assert put_default_port(ip_port, 53) == %Domain.Types.IPPort{
+               type: :ipv4,
+               address: {1, 1, 1, 1},
+               port: 853
+             }
+    end
+  end
+end

elixir/apps/domain/test/support/fixtures/config.ex

@@ -4,7 +4,11 @@ defmodule Domain.Fixtures.Config do
 
   def configuration_attrs(attrs \\ %{}) do
     Enum.into(attrs, %{
-      clients_upstream_dns: [%{address: "1.1.1.1"}]
+      clients_upstream_dns: [
+        %{protocol: "ip_port", address: "1.1.1.1"},
+        %{protocol: "ip_port", address: "2606:4700:4700::1111"},
+        %{protocol: "ip_port", address: "8.8.8.8:853"}
+      ]
     })
   end
 

elixir/apps/web/lib/web/live/settings/dns.ex

@@ -1,6 +1,7 @@
 defmodule Web.Settings.DNS do
   use Web, :live_view
   alias Domain.Config
+  alias Domain.Config.Configuration.ClientsUpstreamDNS
 
   def mount(_params, _session, socket) do
     {:ok, config} = Config.fetch_account_config(socket.assigns.subject)
@@ -56,14 +57,26 @@ defmodule Web.Settings.DNS do
             <div class="grid gap-4 mb-4 sm:grid-cols-1 sm:gap-6 sm:mb-6">
               <div>
                 <.inputs_for :let={dns} field={@form[:clients_upstream_dns]}>
-                  <div class="mb-4">
-                    <.input label="Address" field={dns[:address]} placeholder="DNS Server Address" />
+                  <div class="flex gap-4 items-start mb-2">
+                    <div class="w-1/4">
+                      <.input
+                        type="select"
+                        label="Protocol"
+                        field={dns[:protocol]}
+                        placeholder="Protocol"
+                        options={dns_options()}
+                        value={dns[:protocol].value}
+                      />
+                    </div>
+                    <div class="w-3/4">
+                      <.input label="Address" field={dns[:address]} placeholder="DNS Server Address" />
+                    </div>
                   </div>
                 </.inputs_for>
+                <.error :for={msg <- @errors} data-validation-error-for="clients_upstream_dns">
+                  <%= msg %>
+                </.error>
               </div>
-              <.error :for={msg <- @errors} data-validation-error-for="clients_upstream_dns">
-                <%= msg %>
-              </.error>
               <.submit_button>
                 Save
               </.submit_button>
@@ -157,4 +170,21 @@ defmodule Web.Settings.DNS do
       existing_servers ++ [%{address: ""}]
     )
   end
+
+  defp dns_options do
+    options = [
+      [key: "IP", value: "ip_port"],
+      [key: "DNS over TLS", value: "dns_over_tls"],
+      [key: "DNS over HTTPS", value: "dns_over_https"]
+    ]
+
+    supported = Enum.map(ClientsUpstreamDNS.supported_protocols(), &to_string/1)
+
+    Enum.map(options, fn option ->
+      case option[:value] in supported do
+        true -> option
+        false -> option ++ [disabled: true]
+      end
+    end)
+  end
 end

elixir/apps/web/test/web/live/settings/dns/index_test.exs

@@ -52,7 +52,8 @@ defmodule Web.Live.Settings.DNS.IndexTest do
 
     assert find_inputs(form) == [
              "configuration[clients_upstream_dns][0][_persistent_id]",
-             "configuration[clients_upstream_dns][0][address]"
+             "configuration[clients_upstream_dns][0][address]",
+             "configuration[clients_upstream_dns][0][protocol]"
            ]
   end
 
@@ -77,8 +78,10 @@ defmodule Web.Live.Settings.DNS.IndexTest do
            |> find_inputs() == [
              "configuration[clients_upstream_dns][0][_persistent_id]",
              "configuration[clients_upstream_dns][0][address]",
+             "configuration[clients_upstream_dns][0][protocol]",
              "configuration[clients_upstream_dns][1][_persistent_id]",
-             "configuration[clients_upstream_dns][1][address]"
+             "configuration[clients_upstream_dns][1][address]",
+             "configuration[clients_upstream_dns][1][protocol]"
            ]
   end
 
@@ -107,8 +110,10 @@ defmodule Web.Live.Settings.DNS.IndexTest do
            |> find_inputs() == [
              "configuration[clients_upstream_dns][0][_persistent_id]",
              "configuration[clients_upstream_dns][0][address]",
+             "configuration[clients_upstream_dns][0][protocol]",
              "configuration[clients_upstream_dns][1][_persistent_id]",
-             "configuration[clients_upstream_dns][1][address]"
+             "configuration[clients_upstream_dns][1][address]",
+             "configuration[clients_upstream_dns][1][protocol]"
            ]
 
     empty_attrs = %{
@@ -123,20 +128,23 @@ defmodule Web.Live.Settings.DNS.IndexTest do
            |> form("form")
            |> find_inputs() == [
              "configuration[clients_upstream_dns][0][_persistent_id]",
-             "configuration[clients_upstream_dns][0][address]"
+             "configuration[clients_upstream_dns][0][address]",
+             "configuration[clients_upstream_dns][0][protocol]"
            ]
   end
 
-  test "warns when duplicates found", %{
+  test "warns when duplicate IPv4 addresses found", %{
     account: account,
     identity: identity,
     conn: conn
   } do
-    addr = %{address: "8.8.8.8"}
+    addr1 = %{address: "8.8.8.8"}
+    addr1_dup = %{address: "8.8.8.8:53"}
+    addr2 = %{address: "1.1.1.1"}
 
     attrs = %{
       configuration: %{
-        clients_upstream_dns: %{"0" => addr}
+        clients_upstream_dns: %{"0" => addr1}
       }
     }
 
@@ -150,8 +158,16 @@ defmodule Web.Live.Settings.DNS.IndexTest do
     |> render_submit()
 
     assert lv
-           |> form("form", %{configuration: %{clients_upstream_dns: %{"1" => addr}}})
-           |> render_change() =~ "should not contain duplicates"
+           |> form("form", %{configuration: %{clients_upstream_dns: %{"1" => addr1}}})
+           |> render_change() =~ "no duplicates allowed"
+
+    refute lv
+           |> form("form", %{configuration: %{clients_upstream_dns: %{"1" => addr2}}})
+           |> render_change() =~ "no duplicates allowed"
+
+    assert lv
+           |> form("form", %{configuration: %{clients_upstream_dns: %{"1" => addr1_dup}}})
+           |> render_change() =~ "no duplicates allowed"
   end
 
   test "does not display 'cannot be empty' error message", %{