From 038f025220832f20fdabbe38f39a905adb8fc090 Mon Sep 17 00:00:00 2001 From: Gabi Date: Fri, 5 Aug 2022 20:11:33 -0300 Subject: [PATCH] Re-add port-based rules and gate them behind kernel version (#890) * Revert "Revert "Add initial rough version of port based rules (#874)" (#888)" This reverts commit 58e48457ad1cd75bd39e7e8d3cfe730f6dc931d7. * gate port rule depending on kernel version * fix version comparision * allow for no port-related values when creating rule event * Fix struct accessor * fix getting port type in rule list component * small fix * oops * hide port-related display on disabled port-rules * Gate table headers * update port-based rule for boot-up only and update ui * fix tests * fix disable button * Minor UI and wording update * Add firewall functional tests * fix functional testing * add debug log for functional debugging * fix * Fix functional testing by preventing overlap * remove sudo from functional firewall tests * fix error message * fix firewall ci * re-adding sudo to functional test * fix expected results in functional test * Apply suggestions * Update apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.html.heex Signed-off-by: Jamil Co-authored-by: Jamil --- .ci/functional_test.sh | 19 ++- apps/fz_common/lib/fz_kernel_version.ex | 15 ++ apps/fz_http/lib/fz_http/int4range.ex | 90 +++++++++++ apps/fz_http/lib/fz_http/rules.ex | 24 ++- apps/fz_http/lib/fz_http/rules/rule.ex | 24 ++- .../fz_http/lib/fz_http/rules/rule_setting.ex | 8 +- .../live/rule_live/rule_list_component.ex | 57 +++++-- .../rule_live/rule_list_component.html.heex | 53 +++++- .../20220726205646_add_rule_port_range.exs | 51 ++++++ apps/fz_http/test/fz_http/events_test.exs | 25 ++- .../test/fz_http/repo/notifier_test.exs | 10 +- apps/fz_http/test/fz_http/rules_test.exs | 59 +++++++ apps/fz_http/test/support/test_helpers.ex | 6 + apps/fz_wall/lib/fz_wall/cli/helpers/nft.ex | 145 +++++++++++++---- apps/fz_wall/lib/fz_wall/cli/helpers/sets.ex | 65 +++++--- apps/fz_wall/lib/fz_wall/cli/live.ex | 151 ++++++++---------- apps/fz_wall/lib/fz_wall/cli/sandbox.ex | 1 - .../fz_wall/test/fz_wall/cli/sandbox_test.exs | 9 -- config/config.exs | 3 +- config/runtime.exs | 5 +- 20 files changed, 654 insertions(+), 166 deletions(-) create mode 100644 apps/fz_common/lib/fz_kernel_version.ex create mode 100644 apps/fz_http/lib/fz_http/int4range.ex create mode 100644 apps/fz_http/priv/repo/migrations/20220726205646_add_rule_port_range.exs delete mode 100644 apps/fz_wall/test/fz_wall/cli/sandbox_test.exs diff --git a/.ci/functional_test.sh b/.ci/functional_test.sh index 3b8634e5c..371ae3cc7 100755 --- a/.ci/functional_test.sh +++ b/.ci/functional_test.sh @@ -67,9 +67,10 @@ else exit 1 fi -echo "Testing FzVpn.Interface module works with WireGuard" fz_bin="/opt/firezone/embedded/service/firezone/bin/firezone" ok_res=":ok" + +echo "Testing FzVpn.Interface module works with WireGuard" set_interface=`sudo $fz_bin rpc "IO.inspect(FzVpn.Interface.set(\"wg-fz-test\", %{}))"` del_interface=`sudo $fz_bin rpc "IO.inspect(FzVpn.Interface.delete(\"wg-fz-test\"))"` @@ -77,3 +78,19 @@ if [[ "$set_interface" != $ok_res || "$del_interface" != $ok_res ]]; then echo "WireGuard test failed!" exit 1 fi + +echo "Testing Firewall Rules" +user_id="5" # Picking a high enough user_id so there is no overlap +device="%{ip: \"10.0.0.1\", ip6: \"fd00::3:2:1\", user_id: $user_id}" +rule="%{destination: \"10.0.0.2\", user_id: $user_id, action: :drop, port_type: nil, port_range: nil}" +add_user=`sudo $fz_bin rpc "IO.inspect(FzWall.CLI.Live.add_user($user_id))"` +add_device=`sudo $fz_bin rpc "IO.inspect(FzWall.CLI.Live.add_device($device))"` +add_rule=`sudo $fz_bin rpc "IO.inspect(FzWall.CLI.Live.add_rule($rule))"` +del_rule=`sudo $fz_bin rpc "IO.inspect(FzWall.CLI.Live.delete_rule($rule))"` +del_device=`sudo $fz_bin rpc "IO.inspect(FzWall.CLI.Live.delete_device($device))"` +del_user=`sudo $fz_bin rpc "IO.inspect(FzWall.CLI.Live.delete_user($user_id))"` + +if [[ "$add_user" != $ok_res || "$add_device" != $ok_res || "$add_rule" != '""' || "$del_rule" != '""' || "$del_device" != $ok_res || "$del_user" != $ok_res ]]; then + echo "Firewall test failed!" + exit 1 +fi diff --git a/apps/fz_common/lib/fz_kernel_version.ex b/apps/fz_common/lib/fz_kernel_version.ex new file mode 100644 index 000000000..1332b5b1e --- /dev/null +++ b/apps/fz_common/lib/fz_kernel_version.ex @@ -0,0 +1,15 @@ +defmodule FzCommon.FzKernelVersion do + @moduledoc """ + Helpers related to kernel version + """ + + @doc """ + Compares version tuple to current kernel version + """ + def is_version_greater_than?(val) do + case :os.version() do + v when is_tuple(v) -> v > val + _ -> false + end + end +end diff --git a/apps/fz_http/lib/fz_http/int4range.ex b/apps/fz_http/lib/fz_http/int4range.ex new file mode 100644 index 000000000..8f489273b --- /dev/null +++ b/apps/fz_http/lib/fz_http/int4range.ex @@ -0,0 +1,90 @@ +defmodule FzHttp.Int4Range do + @moduledoc """ + Ecto type for Postgres' Int4Range type + """ + # Note: we represent a port range as a string: lower - upper for ease of use + # with Phoenix LiveView and nftables + use Ecto.Type + @format_error "Range Error: Bad format" + + def type, do: :int4range + + def cast(str) when is_binary(str) do + # We need to handle this case since postgre notifies + # before inserting the range in the database using this format + parse_str = + if String.starts_with?(str, ["[", "("]) do + &parse_bracket/1 + else + &parse_range/1 + end + + case parse_str.(str) do + {:ok, range} -> cast(range) + err -> err + end + end + + def cast([num, num]) when is_number(num) do + {:ok, Integer.to_string(num)} + end + + def cast([lower, upper]) when upper >= lower, do: {:ok, "#{lower} - #{upper}"} + def cast([_, _]), do: {:error, message: "Range Error: Lower bound higher than upper bound"} + + def load(%Postgrex.Range{ + lower: lower, + upper: upper, + lower_inclusive: lower_inclusive, + upper_inclusive: upper_inclusive + }) do + upper = if upper != :unbound, do: upper - to_num(!upper_inclusive), else: nil + lower = if lower != :unbound, do: lower + to_num(!lower_inclusive), else: nil + cast([lower, upper]) + end + + def dump(range) when is_binary(range) do + {:ok, range_list} = parse_range(range) + dump(range_list) + end + + def dump([lower, upper]) do + {:ok, + %Postgrex.Range{lower: lower, upper: upper, upper_inclusive: true, lower_inclusive: true}} + end + + def dump(_), do: :error + + defp parse_range(range) do + res = + String.trim(range) + |> String.split("-", trim: true, parts: 2) + |> Enum.map(&String.trim/1) + |> Enum.map(&Integer.parse/1) + + case res do + [{lower, _}, {upper, _}] -> {:ok, [lower, upper]} + [{num, _}] -> {:ok, [num, num]} + _ -> {:error, message: @format_error} + end + end + + defp parse_bracket(bracket) do + res = + Regex.named_captures( + ~r/(?[\[|(])\s*(?\d+),\s*(?\d+)\s*(?[\]|\)])/, + bracket + ) + + if is_nil(res) || Enum.any?(["lower", "upper", "start", "end"], &is_nil(res[&1])) do + {:error, message: @format_error} + else + lower = String.to_integer(res["lower"]) + to_num(res["start"] == "(") + upper = String.to_integer(res["upper"]) - to_num(res["end"] == ")") + {:ok, [lower, upper]} + end + end + + defp to_num(b) when b, do: 1 + defp to_num(_b), do: 0 +end diff --git a/apps/fz_http/lib/fz_http/rules.ex b/apps/fz_http/lib/fz_http/rules.ex index b9edc319e..45c567c20 100644 --- a/apps/fz_http/lib/fz_http/rules.ex +++ b/apps/fz_http/lib/fz_http/rules.ex @@ -4,9 +4,19 @@ defmodule FzHttp.Rules do """ import Ecto.Query, warn: false - + import Ecto.Changeset alias FzHttp.{Repo, Rules.Rule, Rules.RuleSetting, Telemetry} + def port_rules_supported?, do: Application.fetch_env!(:fz_wall, :port_based_rules_supported) + + defp scope(port_based_rules) when port_based_rules == true do + Rule + end + + defp scope(port_based_rules) when port_based_rules == false do + from r in Rule, where: is_nil(r.port_type) + end + def list_rules, do: Repo.all(Rule) def list_rules(user_id) do @@ -17,7 +27,9 @@ defmodule FzHttp.Rules do end def as_settings do - Repo.all(from(Rule)) + port_rules_supported?() + |> scope() + |> Repo.all() |> Enum.map(&setting_projection/1) |> MapSet.new() end @@ -38,6 +50,14 @@ defmodule FzHttp.Rules do |> Rule.changeset(attrs) end + def defaults(changeset) do + %{port_type: get_field(changeset, :port_type)} + end + + def defaults do + defaults(new_rule()) + end + def create_rule(attrs \\ %{}) do result = attrs diff --git a/apps/fz_http/lib/fz_http/rules/rule.ex b/apps/fz_http/lib/fz_http/rules/rule.ex index f67d0de07..301e28ebb 100644 --- a/apps/fz_http/lib/fz_http/rules/rule.ex +++ b/apps/fz_http/lib/fz_http/rules/rule.ex @@ -7,11 +7,15 @@ defmodule FzHttp.Rules.Rule do import Ecto.Changeset @exclusion_msg "Destination overlaps with an existing rule" + @port_range_msg "Port is not within valid range" + @port_type_msg "Please specify a port-range for the given port type" schema "rules" do field :uuid, Ecto.UUID, autogenerate: true field :destination, EctoNetwork.INET, read_after_writes: true field :action, Ecto.Enum, values: [:drop, :accept], default: :drop + field :port_type, Ecto.Enum, values: [:tcp, :udp], default: nil + field :port_range, FzHttp.Int4Range, default: nil belongs_to :user, FzHttp.Users.User timestamps(type: :utc_datetime_usec) @@ -22,9 +26,19 @@ defmodule FzHttp.Rules.Rule do |> cast(attrs, [ :user_id, :action, - :destination + :destination, + :port_type, + :port_range ]) |> validate_required([:action, :destination]) + |> check_constraint(:port_range, + message: @port_range_msg, + name: :port_range_is_within_valid_values + ) + |> check_constraint(:port_type, + message: @port_type_msg, + name: :port_range_needs_type + ) |> exclusion_constraint(:destination, message: @exclusion_msg, name: :destination_overlap_excl_usr_rule @@ -33,5 +47,13 @@ defmodule FzHttp.Rules.Rule do message: @exclusion_msg, name: :destination_overlap_excl ) + |> exclusion_constraint(:destination, + message: @exclusion_msg, + name: :destination_overlap_excl_port + ) + |> exclusion_constraint(:destination, + message: @exclusion_msg, + name: :destination_overlap_excl_usr_rule_port + ) end end diff --git a/apps/fz_http/lib/fz_http/rules/rule_setting.ex b/apps/fz_http/lib/fz_http/rules/rule_setting.ex index 46d0740bc..253dd7401 100644 --- a/apps/fz_http/lib/fz_http/rules/rule_setting.ex +++ b/apps/fz_http/lib/fz_http/rules/rule_setting.ex @@ -12,19 +12,23 @@ defmodule FzHttp.Rules.RuleSetting do field :action, Ecto.Enum, values: [:drop, :accept] field :destination, :string field :user_id, :integer + field :port_type, Ecto.Enum, values: [:tcp, :udp], default: nil + field :port_range, FzHttp.Int4Range, default: nil end def parse(rule) when is_struct(rule) do %__MODULE__{ destination: decode(rule.destination), action: rule.action, - user_id: rule.user_id + user_id: rule.user_id, + port_type: rule.port_type, + port_range: rule.port_range } end def parse(rule) when is_map(rule) do %__MODULE__{} - |> cast(rule, [:action, :destination, :user_id]) + |> cast(rule, [:action, :destination, :user_id, :port_type, :port_range]) |> apply_changes() end end diff --git a/apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.ex b/apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.ex index 72b3533a0..76367bcea 100644 --- a/apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.ex +++ b/apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.ex @@ -12,23 +12,42 @@ defmodule FzHttpWeb.RuleLive.RuleListComponent do {:ok, socket |> assign(assigns) + |> assign(Rules.defaults()) |> assign( action: action(assigns.id), rule_list: rule_list(assigns), users: users(), - changeset: Rules.new_rule() + changeset: Rules.new_rule(), + port_rules_supported: Rules.port_rules_supported?() )} end + @impl Phoenix.LiveComponent + def handle_event("change", %{"rule" => rule_params}, socket) do + changeset = Rules.new_rule(rule_params) + + {:noreply, + socket + |> assign(:changeset, changeset) + |> assign(Rules.defaults(changeset))} + end + @impl true def handle_event("add_rule", %{"rule" => rule_params}, socket) do - case Rules.create_rule(rule_params) do - {:ok, _rule} -> - {:noreply, - assign(socket, changeset: Rules.new_rule(), rule_list: rule_list(socket.assigns))} + if Rules.port_rules_supported?() || Map.get(rule_params, :port_type) == nil do + case Rules.create_rule(rule_params) do + {:ok, _rule} -> + {:noreply, + assign(socket, changeset: Rules.new_rule(), rule_list: rule_list(socket.assigns)) + |> assign(Rules.defaults())} - {:error, changeset} -> - {:noreply, assign(socket, changeset: changeset)} + {:error, changeset} -> + {:noreply, assign(socket, changeset: changeset)} + end + else + # While using the UI this should never happen + {:noreply, + put_flash(socket, :error, "Couldn't add rule. Port-based rules are not supported.")} end end @@ -36,12 +55,18 @@ defmodule FzHttpWeb.RuleLive.RuleListComponent do def handle_event("delete_rule", %{"rule_id" => rule_id}, socket) do rule = Rules.get_rule!(rule_id) - case Rules.delete_rule(rule) do - {:ok, _rule} -> - {:noreply, assign(socket, rule_list: rule_list(socket.assigns))} + if Rules.port_rules_supported?() || rule.port_type == nil do + case Rules.delete_rule(rule) do + {:ok, _rule} -> + {:noreply, assign(socket, rule_list: rule_list(socket.assigns))} - {:error, msg} -> - {:noreply, put_flash(socket, :error, "Couldn't delete rule. #{msg}")} + {:error, msg} -> + {:noreply, put_flash(socket, :error, "Couldn't delete rule. #{msg}")} + end + else + # While using the UI this should never happen + {:noreply, + put_flash(socket, :error, "Couldn't delete rule. Port-based rules are not supported.")} end end @@ -74,4 +99,12 @@ defmodule FzHttpWeb.RuleLive.RuleListComponent do defp user_options(users) do Enum.map(users, fn {id, email} -> {email, id} end) end + + defp port_type_options do + %{TCP: :tcp, UDP: :udp} + end + + defp port_type_display(nil), do: nil + defp port_type_display(:tcp), do: "TCP" + defp port_type_display(:udp), do: "UDP" end diff --git a/apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.html.heex b/apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.html.heex index 78aa0d8e2..19dc7a2ca 100644 --- a/apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.html.heex +++ b/apps/fz_http/lib/fz_http_web/live/rule_live/rule_list_component.html.heex @@ -6,7 +6,7 @@

- <.form let={f} for={@changeset} id={"#{@action}-form"} phx-target={@myself} phx-submit="add_rule"> + <.form let={f} for={@changeset} id={"#{@action}-form"} phx-change="change" phx-target={@myself} phx-submit="add_rule"> <%= hidden_input f, :action, value: @action %>
@@ -23,6 +23,32 @@ prompt: "Optional user scope" %>
+
+
+ <%= select f, + :port_type, + port_type_options(), + prompt: "Optional port range", + disabled: !@port_rules_supported %> +
+

+ <%= error_tag f, :port_type %> +

+
+
+
+ <%= text_input f, :port_range, + class: "input #{input_error_class(f, :port_range)}", + placeholder: "Port Range", + disabled: @port_type == nil %> +
+

+ Formatted as 'start - stop' or 'port' e.g. 20-80 or 80 (Requires port type) +

+

+ <%= error_tag f, :port_range %> +

+
<%= submit "Add", class: "button is-primary" %>
@@ -30,6 +56,11 @@

<%= error_tag f, :destination %>

+ <%= if !@port_rules_supported do %> +

+ Minimum kernel version 5.6.9 required for port-based rules +

+ <% end %> @@ -38,12 +69,14 @@ + + <%= for rule <- @rule_list do %> - + + + <% end %> + + <%= if !@port_rules_supported && Enum.any?(@rule_list, fn rule -> rule.port_range != nil end) do %> +

+ Port-based rules are only applied when Linux Kernel is 5.6.9 or greater +

+ <% end %> <% end %>
Destination User ScopePort TypePort Range
<%= rule.destination %> @@ -52,16 +85,30 @@
<%= @users[rule.user_id] %> + <%= port_type_display(rule.port_type) %> + + <%= if rule.port_range != nil, do: rule.port_range %> + + phx-target={@myself} + disabled={!@port_rules_supported && rule.port_range != nil} > Delete
diff --git a/apps/fz_http/priv/repo/migrations/20220726205646_add_rule_port_range.exs b/apps/fz_http/priv/repo/migrations/20220726205646_add_rule_port_range.exs new file mode 100644 index 000000000..5bbb6beed --- /dev/null +++ b/apps/fz_http/priv/repo/migrations/20220726205646_add_rule_port_range.exs @@ -0,0 +1,51 @@ +defmodule FzHttp.Repo.Migrations.AddRulePortRange do + use Ecto.Migration + + @create_query "CREATE TYPE port_type_enum AS ENUM ('tcp', 'udp')" + @drop_query "DROP TYPE port_type_enum" + + def change do + execute("ALTER TABLE rules DROP CONSTRAINT destination_overlap_excl_usr_rule") + + execute("ALTER TABLE rules DROP CONSTRAINT destination_overlap_excl") + + execute(@create_query, @drop_query) + + alter table(:rules) do + add :port_range, :int4range, default: nil + add :port_type, :port_type_enum, default: nil + end + + create constraint("rules", :port_range_needs_type, + check: "(port_range IS NULL) = (port_type IS NULL)" + ) + + create constraint("rules", :port_range_is_within_valid_values, + check: "port_range <@ int4range(1, 65535)" + ) + + execute( + "ALTER TABLE rules + ADD CONSTRAINT destination_overlap_excl EXCLUDE USING gist (destination inet_ops WITH &&, action WITH =) WHERE (user_id IS NULL AND port_range IS NULL)", + "ALTER TABLE rules DROP CONSTRAINT destination_overlap_excl" + ) + + execute( + "ALTER TABLE rules + ADD CONSTRAINT destination_overlap_excl_usr_rule EXCLUDE USING gist (destination inet_ops WITH &&, user_id WITH =, action WITH =) WHERE (user_id IS NOT NULL AND port_range IS NULL)", + "ALTER TABLE rules DROP CONSTRAINT destination_overlap_excl_usr_rule" + ) + + execute( + "ALTER TABLE rules + ADD CONSTRAINT destination_overlap_excl_port EXCLUDE USING gist (destination inet_ops WITH &&, action WITH =, port_range WITH &&, port_type WITH =) WHERE (user_id IS NULL AND port_range IS NOT NULL)", + "ALTER TABLE rules DROP CONSTRAINT destination_overlap_excl_rule_port" + ) + + execute( + "ALTER TABLE rules + ADD CONSTRAINT destination_overlap_excl_usr_rule_port EXCLUDE USING gist (destination inet_ops WITH &&, user_id WITH =, action WITH =, port_range WITH &&, port_type WITH =) WHERE (user_id IS NOT NULL AND port_range IS NOT NULL)", + "ALTER TABLE rules DROP CONSTRAINT destination_overlap_excl_usr_rule_port" + ) + end +end diff --git a/apps/fz_http/test/fz_http/events_test.exs b/apps/fz_http/test/fz_http/events_test.exs index 8cc682257..ec8d4fce9 100644 --- a/apps/fz_http/test/fz_http/events_test.exs +++ b/apps/fz_http/test/fz_http/events_test.exs @@ -87,7 +87,16 @@ defmodule FzHttp.EventsTest do %{ users: MapSet.new(), devices: MapSet.new(), - rules: MapSet.new([%{destination: "10.10.10.0/24", user_id: nil, action: :drop}]) + rules: + MapSet.new([ + %{ + destination: "10.10.10.0/24", + port_range: nil, + port_type: nil, + user_id: nil, + action: :drop + } + ]) } end end @@ -103,7 +112,15 @@ defmodule FzHttp.EventsTest do users: MapSet.new(), devices: MapSet.new(), rules: - MapSet.new([%{destination: "10.10.10.0/24", user_id: nil, action: :accept}]) + MapSet.new([ + %{ + destination: "10.10.10.0/24", + user_id: nil, + action: :accept, + port_type: nil, + port_range: nil + } + ]) } end end @@ -167,7 +184,9 @@ defmodule FzHttp.EventsTest do %{ user_id: rule.user_id, destination: FzHttp.Devices.decode(rule.destination), - action: rule.action + action: rule.action, + port_range: nil, + port_type: nil } end) ) diff --git a/apps/fz_http/test/fz_http/repo/notifier_test.exs b/apps/fz_http/test/fz_http/repo/notifier_test.exs index 626ddda45..3682ccd41 100644 --- a/apps/fz_http/test/fz_http/repo/notifier_test.exs +++ b/apps/fz_http/test/fz_http/repo/notifier_test.exs @@ -52,7 +52,15 @@ defmodule FzHttp.Repo.NotifierTest do expected_state = %{ users: MapSet.new([]), rules: - MapSet.new([%{action: rule.action, destination: "10.10.10.0/24", user_id: rule.user_id}]), + MapSet.new([ + %{ + action: rule.action, + destination: "10.10.10.0/24", + user_id: rule.user_id, + port_range: nil, + port_type: nil + } + ]), devices: MapSet.new([]) } diff --git a/apps/fz_http/test/fz_http/rules_test.exs b/apps/fz_http/test/fz_http/rules_test.exs index a31bb8eaa..bfe94ab8f 100644 --- a/apps/fz_http/test/fz_http/rules_test.exs +++ b/apps/fz_http/test/fz_http/rules_test.exs @@ -53,12 +53,57 @@ defmodule FzHttp.RulesTest do assert rule.user_id == nil end + test "creates rule with port" do + {:ok, rule} = + Rules.create_rule(%{destination: "10.0.0.0/24", port_type: :tcp, port_range: "100-200"}) + + assert !is_nil(rule.id) + assert rule.action == :drop + assert rule.user_id == nil + assert rule.port_type == :tcp + assert rule.port_range == "100 - 200" + end + test "prevents invalid CIDRs" do {:error, changeset} = Rules.create_rule(%{destination: "10.0 0.0/24"}) assert changeset.errors[:destination] == {"is invalid", [type: EctoNetwork.INET, validation: :cast]} end + + test "prevents invalid port_range: no port_type" do + {:error, changeset} = Rules.create_rule(%{destination: "10.0.0.0/24", port_range: "10-20"}) + + assert changeset.errors[:port_type] == + {"Please specify a port-range for the given port type", + [constraint: :check, constraint_name: "port_range_needs_type"]} + end + + test "prevents invalid port_type: no port_range" do + {:error, changeset} = Rules.create_rule(%{destination: "10.0.0.0/24", port_type: :tcp}) + + assert changeset.errors[:port_type] == + {"Please specify a port-range for the given port type", + [constraint: :check, constraint_name: "port_range_needs_type"]} + end + + test "prevents invalid port_range: outside port range" do + {:error, changeset} = + Rules.create_rule(%{destination: "10.0.0.0/24", port_type: :tcp, port_range: "10-90000"}) + + assert changeset.errors[:port_range] == + {"Port is not within valid range", + [constraint: :check, constraint_name: "port_range_is_within_valid_values"]} + end + + test "prevents invalid port_range: " do + {:error, changeset} = + Rules.create_rule(%{destination: "10.0.0.0/24", port_type: :tcp, port_range: "20-10"}) + + assert changeset.errors[:port_range] == + {"Range Error: Lower bound higher than upper bound", + [type: FzHttp.Int4Range, validation: :cast]} + end end describe "delete_rule/1" do @@ -110,6 +155,20 @@ defmodule FzHttp.RulesTest do end end + describe "setting_projection/1 with ports" do + setup [:create_rule_with_ports] + + test "projects expected fields", %{rule: rule} do + assert %{ + destination: "10.10.10.0/24", + port_type: :udp, + port_range: "10 - 20", + action: :drop, + user_id: nil + } = Rules.setting_projection(rule) + end + end + describe "as_settings/0" do setup [:create_rules] diff --git a/apps/fz_http/test/support/test_helpers.ex b/apps/fz_http/test/support/test_helpers.ex index 1b487c9f8..d23726ae5 100644 --- a/apps/fz_http/test/support/test_helpers.ex +++ b/apps/fz_http/test/support/test_helpers.ex @@ -187,6 +187,12 @@ defmodule FzHttp.TestHelpers do {:ok, rule: rule, user: user} end + def create_rule_with_ports(opts \\ %{}) do + rule = RulesFixtures.rule(Map.merge(%{port_range: "10 - 20", port_type: :udp}, opts)) + + {:ok, rule: rule} + end + def create_user_with_valid_sign_in_token(_) do {:ok, user: %User{} = UsersFixtures.user(Users.sign_in_keys())} end diff --git a/apps/fz_wall/lib/fz_wall/cli/helpers/nft.ex b/apps/fz_wall/lib/fz_wall/cli/helpers/nft.ex index 240e3bf1b..41e649e85 100644 --- a/apps/fz_wall/lib/fz_wall/cli/helpers/nft.ex +++ b/apps/fz_wall/lib/fz_wall/cli/helpers/nft.ex @@ -6,56 +6,88 @@ defmodule FzWall.CLI.Helpers.Nft do import FzCommon.FzNet, only: [standardized_inet: 1] require Logger @table_name "firezone" + @main_chain "forward" @doc """ - Insert a nft rule + Insert a nft filter rule """ - def insert_rule(type, source_set, dest_set, action) do - exec!(""" - #{nft()} 'insert rule inet #{@table_name} forward #{rule_match_str(type, source_set, dest_set, action)}' - """) + def insert_filter_rule(chain, type, dest_set, action, layer4) do + insert_rule(chain, rule_filter_match_str(type, dest_set, action, layer4)) end @doc """ - Removes a nft rule + Insert a nft jump rule """ - def remove_rule(type, source_set, dest_set, action) do - delete_rule_matching(rule_match_str(type, source_set, dest_set, action)) + def insert_dev_rule(ip_type, source_set, jump_chain) do + insert_rule(@main_chain, rule_dev_match_str(ip_type, source_set, jump_chain)) end @doc """ - Adds an element from a nft set + Remove device-related rule """ + # Note: we don't need remove_filter_rule because the chains are removed all together + def remove_dev_rule(ip_type, source_set, jump_chain) do + delete_rule_matching(rule_dev_match_str(ip_type, source_set, jump_chain)) + end + + @doc """ + Add element to set + """ + def add_elem(set, ip, nil, nil) do + add_ip_elem(set, ip) + end + + def add_elem(set, ip, proto, ports) do + add_elem_exec(set, get_elem(ip, proto, ports)) + end + def add_elem(set, ip) do - exec!(""" - #{nft()} 'add element inet #{@table_name} #{set} { #{standardized_inet(ip)} }' - """) + add_ip_elem(set, ip) + end + + defp add_ip_elem(set, ip) do + add_elem_exec(set, get_elem(ip)) end @doc """ Deletes an element from a nft set """ def delete_elem(set, ip) do - exec!(""" - #{nft()} 'delete element inet #{@table_name} #{set} { #{standardized_inet(ip)} }' - """) + delete_ip_elem(set, ip) + end + + def delete_elem(set, ip, nil, nil) do + delete_ip_elem(set, ip) + end + + def delete_elem(set, ip, proto, ports) do + delete_elem_exec(set, get_elem(ip, proto, ports)) + end + + defp delete_ip_elem(set, ip) do + delete_elem_exec(set, get_elem(ip)) end @doc """ - Adds a nft set + Adds a nft dev set """ - def add_set(set_spec) do - exec!(""" - #{nft()} 'add set inet #{@table_name} #{set_spec.name} { type #{set_type(set_spec.type)} ; flags interval ; }' - """) + def add_dev_set(name, ip_type) do + add_set(name, dev_set_type(ip_type)) + end + + @doc """ + Adds a nft filter set + """ + def add_filter_set(name, ip_type, layer4) do + add_set(name, filter_set_type(ip_type, layer4)) end @doc """ Deletes a nft set """ - def delete_set(set_spec) do + def delete_set(name) do exec!(""" - #{nft()} 'delete set inet #{@table_name} #{set_spec.name}' + #{nft()} 'delete set inet #{@table_name} #{name}' """) end @@ -71,7 +103,7 @@ defmodule FzWall.CLI.Helpers.Nft do """ def setup_chains do exec!( - "#{nft()} 'add chain inet #{@table_name} forward " <> + "#{nft()} 'add chain inet #{@table_name} #{@main_chain} " <> "{ type filter hook forward priority 0 ; policy accept ; }'" ) @@ -83,6 +115,20 @@ defmodule FzWall.CLI.Helpers.Nft do setup_masquerade() end + @doc """ + Adds a regular nftable chain(not base) + """ + def add_chain(chain_name) do + exec!("#{nft()} 'add chain inet #{@table_name} #{chain_name}'") + end + + @doc """ + Deletes a regular nftable chain(not base) + """ + def delete_chain(chain_name) do + exec!("#{nft()} 'delete chain inet #{@table_name} #{chain_name}'") + end + defp setup_masquerade do if masquerade_ipv4?() do setup_masquerade(:ipv4) @@ -160,7 +206,7 @@ defmodule FzWall.CLI.Helpers.Nft do ) handle -> - exec!("#{nft()} delete rule inet #{@table_name} forward handle #{handle}") + exec!("#{nft()} delete rule inet #{@table_name} #{@main_chain} handle #{handle}") end end @@ -172,14 +218,55 @@ defmodule FzWall.CLI.Helpers.Nft do Regex.run(regex, rules, capture: :all_names) end - defp set_type(:ip), do: "ipv4_addr" - defp set_type(:ip6), do: "ipv6_addr" + defp filter_set_type(:ip, false), do: "ipv4_addr" + defp filter_set_type(:ip6, false), do: "ipv6_addr" - defp rule_match_str(type, nil, dest_set, action) do + defp filter_set_type(ip_type, true), + do: "#{filter_set_type(ip_type, false)} . inet_proto . inet_service" + + defp dev_set_type(ip_type), do: filter_set_type(ip_type, false) + + defp rule_filter_match_str(type, dest_set, action, false) do "#{type} daddr @#{dest_set} ct state != established #{action}" end - defp rule_match_str(type, source_set, dest_set, action) do - "#{type} saddr @#{source_set} #{rule_match_str(type, nil, dest_set, action)}" + defp rule_filter_match_str(type, dest_set, action, true) do + "#{type} daddr . meta l4proto . th dport @#{dest_set} ct state != established #{action}" + end + + defp rule_dev_match_str(ip_type, source_set, jump_chain) do + "#{ip_type} saddr @#{source_set} jump #{jump_chain}" + end + + defp insert_rule(chain, rule_str) do + exec!(""" + #{nft()} 'insert rule inet #{@table_name} #{chain} #{rule_str}' + """) + end + + defp delete_elem_exec(set, elem) do + exec!(""" + #{nft()} 'delete element inet #{@table_name} #{set} { #{elem} }' + """) + end + + defp add_set(name, type) do + exec!(""" + #{nft()} 'add set inet #{@table_name} #{name} { type #{type} ; flags interval ; }' + """) + end + + defp add_elem_exec(set, elem) do + exec!(""" + #{nft()} 'add element inet #{@table_name} #{set} { #{elem} }' + """) + end + + def get_elem(ip) do + "#{standardized_inet(ip)}" + end + + def get_elem(ip, proto, ports) do + "#{standardized_inet(ip)} . #{proto} . #{ports}" end end diff --git a/apps/fz_wall/lib/fz_wall/cli/helpers/sets.ex b/apps/fz_wall/lib/fz_wall/cli/helpers/sets.ex index 82e41f896..1848a2c1e 100644 --- a/apps/fz_wall/lib/fz_wall/cli/helpers/sets.ex +++ b/apps/fz_wall/lib/fz_wall/cli/helpers/sets.ex @@ -4,39 +4,66 @@ defmodule FzWall.CLI.Helpers.Sets do """ @actions [:drop, :accept] - @types [:ip, :ip6] + @ip_types [:ip, :ip6] - def list_dest_sets(user_id) do - cross(@types, @actions) - |> Enum.map(fn {type, action} -> - %{name: get_dest_set_name(user_id, type, action), type: type} + defp port_rules_supported?, do: Application.fetch_env!(:fz_wall, :port_based_rules_supported) + + def list_filter_sets(user_id) do + get_all_filter_sets(user_id, port_rules_supported?()) + end + + defp get_all_filter_sets(user_id, false) do + get_filter_sets_spec(user_id, false) + end + + defp get_all_filter_sets(user_id, true) do + get_all_filter_sets(user_id, false) ++ get_filter_sets_spec(user_id, true) + end + + defp get_filter_sets_spec(user_id, layer4) do + cross(@ip_types, @actions) + |> Enum.map(fn {ip_type, action} -> + %{ + name: get_filter_set_name(user_id, ip_type, action, layer4), + ip_type: ip_type, + action: action, + layer4: layer4 + } end) end - def list_sets(nil), do: list_dest_sets(nil) - - def list_sets(user_id) do - list_dest_sets(user_id) ++ - Enum.map(@types, fn type -> %{name: get_device_set_name(user_id, type), type: type} end) + def list_dev_sets(user_id) do + Enum.map(@ip_types, fn type -> %{name: get_device_set_name(user_id, type), ip_type: type} end) end - def get_types do - @types + def get_ip_types do + @ip_types end def get_actions do @actions end - def get_dest_set_name(nil, type, action), do: "#{type}_#{action}" - def get_dest_set_name(user_id, type, action), do: "user_#{user_id}_#{type}_#{action}" - def get_device_set_name(nil, _type), do: nil - def get_device_set_name(user_id, type), do: "user_#{user_id}_#{type}_devices" + def get_device_set_name(user_id, type), do: "user#{user_id}_#{type}_devices" + def get_user_chain(nil), do: "forward" + def get_user_chain(user_id), do: "user#{user_id}" - def cross([x | a], [y | b]) do + def get_filter_set_name(nil, ip_type, action, false), + do: "#{ip_type}_#{action}" + + def get_filter_set_name(user_id, ip_type, action, false), + do: "user#{user_id}_#{ip_type}_#{action}" + + def get_filter_set_name(nil, ip_type, action, true), + do: "#{ip_type}_#{action}_layer4" + + def get_filter_set_name(user_id, ip_type, action, true), + do: "user#{user_id}_#{ip_type}_#{action}_layer4" + + defp cross([x | a], [y | b]) do [{x, y}] ++ cross([x], b) ++ cross(a, [y | b]) end - def cross([], _b), do: [] - def cross(_a, []), do: [] + defp cross([], _b), do: [] + defp cross(_a, []), do: [] end diff --git a/apps/fz_wall/lib/fz_wall/cli/live.ex b/apps/fz_wall/lib/fz_wall/cli/live.ex index 7ab11214d..5f179c441 100644 --- a/apps/fz_wall/lib/fz_wall/cli/live.ex +++ b/apps/fz_wall/lib/fz_wall/cli/live.ex @@ -8,7 +8,6 @@ defmodule FzWall.CLI.Live do import FzWall.CLI.Helpers.Sets import FzWall.CLI.Helpers.Nft - import FzCommon.CLI import FzCommon.FzNet, only: [ip_type: 1] require Logger @@ -19,60 +18,81 @@ defmodule FzWall.CLI.Live do teardown_table() setup_table() setup_chains() - setup_rules() + setup_rules(nil) end @doc """ Adds user sets and rules. """ def add_user(user_id) do - add_sets(user_id) - add_rules(user_id) + add_user_set(user_id) + add_chain(get_user_chain(user_id)) + set_jump_rule(user_id) + setup_rules(user_id) + end + + defp add_user_set(user_id) do + list_dev_sets(user_id) + |> Enum.map(fn set_spec -> add_dev_set(set_spec.name, set_spec.ip_type) end) + end + + defp delete_user_set(user_id) do + list_dev_sets(user_id) + |> Enum.map(fn set_spec -> delete_set(set_spec.name) end) end @doc """ Remove user sets and rules. """ def delete_user(user_id) do - delete_rules(user_id) - delete_sets(user_id) + delete_jump_rules(user_id) + delete_user_set(user_id) + delete_chain(get_user_chain(user_id)) + delete_filter_sets(user_id) end @doc """ Adds general sets and rules. """ - def setup_rules do - add_sets(nil) - add_rules(nil) + def setup_rules(user_id) do + add_filter_sets(user_id) + add_filter_rules(user_id) + end + + def set_jump_rule(user_id) do + list_dev_sets(user_id) + |> Enum.each(fn set_spec -> + insert_dev_rule(set_spec.ip_type, set_spec.name, get_user_chain(user_id)) + end) end @doc """ Adds device ip to the user's sets. """ def add_device(device) do - get_types() - |> Enum.each(fn type -> add_to_set(device.user_id, device[type], type) end) + list_dev_sets(device.user_id) + |> Enum.each(fn set_spec -> add_elem(set_spec.name, device[set_spec.ip_type]) end) end @doc """ Adds rule ip to its corresponding sets. """ def add_rule(rule) do - add_to_set(rule.user_id, rule.destination, proto(rule.destination), rule.action) + modify_elem(&add_elem/4, rule) end @doc """ Delete rule destination ip from its corresponding sets. """ def delete_rule(rule) do - remove_from_set(rule.user_id, rule.destination, proto(rule.destination), rule.action) + modify_elem(&delete_elem/4, rule) end @doc """ Eliminates device rules from its corresponding sets. """ def delete_device(device) do - get_types() + get_ip_types() |> Enum.each(fn type -> remove_from_set(device.user_id, device[type], type) end) end @@ -83,54 +103,35 @@ defmodule FzWall.CLI.Live do |> delete_elem(ip) end - defp remove_from_set(user_id, ip, type, action) do - get_dest_set_name(user_id, type, action) - |> delete_elem(ip) + defp add_filter_sets(user_id) do + list_filter_sets(user_id) + |> Enum.each(fn set_spec -> + add_filter_set(set_spec.name, set_spec.ip_type, set_spec.layer4) + end) end - defp add_to_set(_user_id, nil, _type), do: :no_ip - - defp add_to_set(user_id, ip, type) do - get_device_set_name(user_id, type) - |> add_elem(ip) + defp delete_filter_sets(user_id) do + list_filter_sets(user_id) + |> Enum.each(fn set_spec -> delete_set(set_spec.name) end) end - defp add_to_set(user_id, ip, type, action) do - get_dest_set_name(user_id, type, action) - |> add_elem(ip) - end - - defp add_sets(user_id) do - list_sets(user_id) - |> Enum.each(&add_set/1) - end - - defp delete_sets(user_id) do - list_sets(user_id) - |> Enum.each(&delete_set/1) - end - - defp add_rules(user_id) do - cross(get_types(), get_actions()) - |> Enum.each(fn {type, action} -> - insert_rule( - type, - get_device_set_name(user_id, type), - get_dest_set_name(user_id, type, action), - action + defp add_filter_rules(user_id) do + list_filter_sets(user_id) + |> Enum.each(fn set_spec -> + insert_filter_rule( + get_user_chain(user_id), + set_spec.ip_type, + set_spec.name, + set_spec.action, + set_spec.layer4 ) end) end - defp delete_rules(user_id) do - cross(get_types(), get_actions()) - |> Enum.each(fn {type, action} -> - remove_rule( - type, - get_device_set_name(user_id, type), - get_dest_set_name(user_id, type, action), - action - ) + defp delete_jump_rules(user_id) do + list_dev_sets(user_id) + |> Enum.each(fn set_spec -> + remove_dev_rule(set_spec.ip_type, set_spec.name, get_user_chain(user_id)) end) end @@ -141,36 +142,24 @@ defmodule FzWall.CLI.Live do Enum.each(rules, &add_rule/1) end - def egress_address do - case :os.type() do - {:unix, :linux} -> - cmd = "ip address show dev #{egress_interface()} | grep 'inet ' | awk '{print $2}'" - - exec!(cmd) - |> String.trim() - |> String.split("/") - |> List.first() - - {:unix, :darwin} -> - cmd = "ipconfig getifaddr #{egress_interface()}" - - exec!(cmd) - |> String.trim() - - _ -> - raise "OS not supported (yet)" - end - end - - defp egress_interface do - Application.fetch_env!(:fz_wall, :egress_interface) - end - defp proto(ip) do case ip_type("#{ip}") do - "IPv4" -> "ip" - "IPv6" -> "ip6" + "IPv4" -> :ip + "IPv6" -> :ip6 "unknown" -> raise "Unknown protocol." end end + + defp modify_elem(action, rule) do + ip_type = proto(rule.destination) + port_type = rule.port_type + layer4 = port_type != nil + + action.( + get_filter_set_name(rule.user_id, ip_type, rule.action, layer4), + rule.destination, + port_type, + rule.port_range + ) + end end diff --git a/apps/fz_wall/lib/fz_wall/cli/sandbox.ex b/apps/fz_wall/lib/fz_wall/cli/sandbox.ex index 4c1a57183..a7ee5670f 100644 --- a/apps/fz_wall/lib/fz_wall/cli/sandbox.ex +++ b/apps/fz_wall/lib/fz_wall/cli/sandbox.ex @@ -13,5 +13,4 @@ defmodule FzWall.CLI.Sandbox do def delete_device(_device), do: @default_returned def add_user(_user), do: @default_returned def delete_user(_user), do: @default_returned - def egress_address, do: "10.0.0.1" end diff --git a/apps/fz_wall/test/fz_wall/cli/sandbox_test.exs b/apps/fz_wall/test/fz_wall/cli/sandbox_test.exs deleted file mode 100644 index 8a5ca1bda..000000000 --- a/apps/fz_wall/test/fz_wall/cli/sandbox_test.exs +++ /dev/null @@ -1,9 +0,0 @@ -defmodule FzWall.CLI.SandboxTest do - use ExUnit.Case, async: true - - import FzWall.CLI - - test "egress_address()" do - assert is_binary(cli().egress_address()) - end -end diff --git a/config/config.exs b/config/config.exs index d8943f473..de36216fe 100644 --- a/config/config.exs +++ b/config/config.exs @@ -95,7 +95,8 @@ config :fz_wall, wireguard_ipv6_masquerade: true, server_process_opts: [name: {:global, :fz_wall_server}], egress_interface: "dummy", - wireguard_interface_name: "wg-firezone" + wireguard_interface_name: "wg-firezone", + port_based_rules_supported: true config :hammer, backend: {Hammer.Backend.ETS, [expiry_ms: 60_000 * 60 * 4, cleanup_interval_ms: 60_000 * 10]} diff --git a/config/runtime.exs b/config/runtime.exs index 341397824..557a9c3f1 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -5,7 +5,7 @@ import Config -alias FzCommon.{CLI, FzInteger, FzString} +alias FzCommon.{CLI, FzInteger, FzString, FzKernelVersion} # external_url is important external_url = System.get_env("EXTERNAL_URL", "https://localhost") @@ -17,6 +17,9 @@ config :fz_http, FzHttpWeb.Endpoint, url: [host: host, scheme: scheme, port: port, path: path], check_origin: ["//127.0.0.1", "//localhost", "//#{host}"] +config :fz_wall, + port_based_rules_supported: FzKernelVersion.is_version_greater_than?({5, 6, 8}) + # Formerly releases.exs - Only evaluated in production if config_env() == :prod do # For releases, require that all these are set