fix(portal): Improve resource address validation (#8288)

We had a number of validation issues:

- DNS resources allow address `1.1.1.1` or `1.1.1.1/32`. These are not
valid and will cause issues during resolution.
- IP resources were allowing basically any string character on `edit`
caused by a logic bug in the changeset
- CIDR resources, same as above
- `*.*.*.*.google.com` and similar DNS wildcard resources were not
allowed

This PR beefs all of those up so that we have a higher degree of
certainty that our data is valid. If invalid data reaches connlib, it
will cause a panic.

This PR also introduces a migration to migrate any invalid resources
into the proper format in the DB.

Fixes #8287
This commit is contained in:
Jamil
2025-02-27 23:41:11 +00:00
committed by GitHub
parent 325604b3dd
commit d7be59707a
7 changed files with 462 additions and 90 deletions

View File

@@ -8,8 +8,7 @@ defmodule Domain.Resources.Resource.Changeset do
@replace_fields ~w[type address filters]a
@required_fields ~w[name type]a
# This list is based on the list of TLDs from the IANA but only contains
# all the country zones and most common generic zones.
# Reference list of common TLDs from IANA
@common_tlds ~w[
com org net edu gov mil biz info name mobi pro
ac ad ae af ag ai al am ao ar as at au aw ax az
@@ -26,9 +25,7 @@ defmodule Domain.Resources.Resource.Changeset do
va vc ve vg vi vn vu wf ws ye yt za zm zw
]
@prohibited_tlds ~w[
localhost
]
@prohibited_tlds ~w[localhost]
def create(%Accounts.Account{} = account, attrs, %Auth.Subject{} = subject) do
%Resource{connections: []}
@@ -51,6 +48,7 @@ defmodule Domain.Resources.Resource.Changeset do
|> cast(attrs, @fields)
|> changeset()
|> validate_required(@required_fields)
|> update_change(:address, &String.trim/1)
|> validate_address(account)
|> put_change(:persistent_id, Ecto.UUID.generate())
|> put_change(:account_id, account.id)
@@ -60,10 +58,43 @@ defmodule Domain.Resources.Resource.Changeset do
)
end
def update(%Resource{} = resource, attrs, %Auth.Subject{} = subject) do
resource
|> cast(attrs, @update_fields)
|> validate_required(@required_fields)
|> validate_address(subject.account)
|> changeset()
|> cast_assoc(:connections,
with: &Connection.Changeset.changeset(resource.account_id, &1, &2, subject),
required: true
)
|> maybe_breaking_change()
end
def delete(%Resource{} = resource) do
resource
|> change()
|> put_default_value(:deleted_at, DateTime.utc_now())
end
defp validate_address(changeset, account) do
if has_errors?(changeset, :type) do
changeset
else
# Force address revalidation if type has changed
changeset =
if Map.has_key?(changeset.changes, :type) do
case fetch_field(changeset, :address) do
{_, address} when not is_nil(address) ->
force_change(changeset, :address, address)
_ ->
changeset
end
else
changeset
end
case fetch_field(changeset, :type) do
{_data_or_changes, :dns} ->
changeset
@@ -92,34 +123,52 @@ defmodule Domain.Resources.Resource.Changeset do
defp validate_dns_address(changeset) do
changeset
|> validate_length(:address, min: 1, max: 253)
|> validate_format(:address, ~r/^[\p{L}\*\?0-9-]{1,63}(\.[\p{L}\*\?0-9-]{1,63})*$/iu)
|> validate_format(:address, ~r/^[^\.]/, message: "must start with a letter or number")
|> validate_format(:address, ~r/[^\.]$/, message: "must end with a letter or number")
|> validate_format(:address, ~r/[\w]/iu, message: "must contain at least one letter")
# Reject IPs (IPv4 and IPv6)
|> validate_change(:address, fn field, address ->
cond do
String.match?(address, ~r/^(\d+\.){3}\d+(\/\d+)?$/) ->
[{field, "IP addresses are not allowed, use an IP Resource instead"}]
String.match?(
address,
~r/^([0-9a-fA-F]{0,4}:){1,7}[0-9a-fA-F]{0,4}(\/\d+)?$|^::([0-9a-fA-F]{0,4}:){0,6}[0-9a-fA-F]{0,4}(\/\d+)?$/
) ->
[{field, "IP addresses are not allowed, use an IP Resource instead"}]
true ->
[]
end
end)
|> validate_format(
:address,
~r/^[a-zA-Z0-9\p{L}\-*?]+(?:\.[a-zA-Z0-9\p{L}\-*?]+)*$/u,
message:
"must be a valid hostname (letters, digits, hyphens, dots; wildcards *, ?, ** allowed)"
)
|> validate_change(:address, fn field, dns_address ->
{tld, domain} =
dns_address
|> String.split(".")
|> Enum.reverse()
|> case do
[tld, domain | _rest] -> {String.downcase(tld), String.downcase(domain)}
[tld | _rest] -> {String.downcase(tld), ""}
[] -> {"", ""}
parts = String.split(dns_address, ".")
{tld, domain_parts} =
case Enum.reverse(parts) do
[tld | rest] -> {String.downcase(tld), rest}
[] -> {"", []}
end
cond do
String.contains?(tld, ["*", "?"]) ->
[{field, {"TLD cannot contain wildcards", []}}]
[{field, "TLD cannot contain wildcards"}]
tld in @prohibited_tlds ->
[
{field,
{"#{tld} cannot be used as a TLD. " <>
"Try adding a DNS alias to /etc/hosts on the Gateway(s) instead", []}}
"#{tld} cannot be used as a TLD. Try adding a DNS alias to /etc/hosts on the Gateway(s) instead"}
]
tld in @common_tlds and String.contains?(domain, ["*", "?"]) ->
[{field, {"second level domain for IANA TLDs cannot contain wildcards", []}}]
Enum.all?(parts, &(&1 == "*")) ->
[{field, "wildcard pattern must include a valid domain"}]
tld in @common_tlds and Enum.all?(domain_parts, &String.match?(&1, ~r/^[\*\?]+$/)) ->
[{field, "domain for IANA TLDs cannot consist solely of wildcards"}]
true ->
[]
@@ -145,18 +194,12 @@ defmodule Domain.Resources.Resource.Changeset do
)
|> validate_not_in_cidr(
:address,
%Postgrex.INET{
address: {0, 0, 0, 0, 0, 0, 0, 0},
netmask: 128
},
%Postgrex.INET{address: {0, 0, 0, 0, 0, 0, 0, 0}, netmask: 128},
message: internet_resource_message
)
|> validate_not_in_cidr(
:address,
%Postgrex.INET{
address: {0, 0, 0, 0, 0, 0, 0, 1},
netmask: 128
},
%Postgrex.INET{address: {0, 0, 0, 0, 0, 0, 0, 1}, netmask: 128},
message: "cannot contain loopback addresses"
)
|> validate_address_is_not_in_private_range()
@@ -173,18 +216,12 @@ defmodule Domain.Resources.Resource.Changeset do
)
|> validate_not_in_cidr(
:address,
%Postgrex.INET{
address: {0, 0, 0, 0, 0, 0, 0, 0},
netmask: 128
},
%Postgrex.INET{address: {0, 0, 0, 0, 0, 0, 0, 0}, netmask: 128},
message: "cannot contain all IPv6 addresses"
)
|> validate_not_in_cidr(
:address,
%Postgrex.INET{
address: {0, 0, 0, 0, 0, 0, 0, 1},
netmask: 128
},
%Postgrex.INET{address: {0, 0, 0, 0, 0, 0, 0, 1}, netmask: 128},
message: "cannot contain loopback addresses"
)
|> validate_address_is_not_in_private_range()
@@ -209,22 +246,8 @@ defmodule Domain.Resources.Resource.Changeset do
end
end
def update(%Resource{} = resource, attrs, %Auth.Subject{} = subject) do
resource
|> cast(attrs, @update_fields)
|> validate_required(@required_fields)
|> changeset()
|> cast_assoc(:connections,
with: &Connection.Changeset.changeset(resource.account_id, &1, &2, subject),
required: true
)
|> maybe_breaking_change()
end
defp maybe_breaking_change(%{valid?: false} = changeset), do: {changeset, false}
defp maybe_breaking_change(%{valid?: false} = changeset),
do: {changeset, false}
# NOTE: Kept for backwards compatibility.
defp maybe_breaking_change(changeset) do
if any_field_changed?(changeset, @replace_fields) do
{changeset, true}
@@ -243,12 +266,6 @@ defmodule Domain.Resources.Resource.Changeset do
|> unique_constraint(:type, name: :unique_internet_resource_per_account)
end
def delete(%Resource{} = resource) do
resource
|> change()
|> put_default_value(:deleted_at, DateTime.utc_now())
end
defp cast_filter(%Resource.Filter{} = filter, attrs) do
filter
|> cast(attrs, [:protocol, :ports])

View File

@@ -0,0 +1,23 @@
defmodule Domain.Repo.Migrations.AppendDefaultNetmaskToEmptyCidr do
use Ecto.Migration
def change do
# Update the netmask of all empty IPv4 CIDR columns to 32
execute("""
UPDATE resources
SET address = address || '/32'
WHERE type = 'cidr'
AND address !~ '/'
AND address ~ '^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$'
""")
# Update the netmask of all empty IPv6 CIDR columns to 128
execute("""
UPDATE resources
SET address = address || '/128'
WHERE type = 'cidr'
AND address !~ '/'
AND address ~ '^[0-9a-fA-F:]+$'
""")
end
end

View File

@@ -0,0 +1,39 @@
defmodule Domain.Repo.Migrations.ConvertDnsIpResourcesToIp do
use Ecto.Migration
def change do
# Postgres doesn't natively support try_cast, but we can define our own
execute("""
CREATE OR REPLACE FUNCTION try_cast_inet(text) RETURNS inet AS $$
BEGIN
RETURN $1::inet;
EXCEPTION
WHEN OTHERS THEN
RETURN NULL;
END;
$$ LANGUAGE plpgsql
""")
# Convert IP addresses
execute("""
UPDATE resources
SET type = 'ip'
WHERE type = 'dns'
AND try_cast_inet(address) IS NOT NULL
AND address !~ '/'
""")
# Convert CIDR blocks
execute("""
UPDATE resources
SET type = 'cidr'
WHERE type = 'dns'
AND try_cast_inet(address) IS NOT NULL
AND address ~ '/'
""")
execute("""
DROP FUNCTION IF EXISTS try_cast_inet(text)
""")
end
end

View File

@@ -39,9 +39,28 @@ defmodule Domain.Config.ValidatorTest do
type = {:one_of, [Types.IP, Types.CIDR]}
assert validate(:key, "::1", type, []) ==
{:ok, %Postgrex.INET{address: {0, 0, 0, 0, 0, 0, 0, 1}, netmask: nil}}
assert validate(:key, "::1/foo", type, []) ==
{:error,
{"::1/foo",
[
"must be one of: Elixir.Domain.Types.IP, Elixir.Domain.Types.CIDR",
"CIDR netmask is invalid or missing"
]}}
assert validate(:key, "1.1.1.1", type, []) ==
{:ok, %Postgrex.INET{address: {1, 1, 1, 1}, netmask: nil}}
assert validate(:key, "1.1.1.1/foo", type, []) ==
{:error,
{"1.1.1.1/foo",
[
"must be one of: Elixir.Domain.Types.IP, Elixir.Domain.Types.CIDR",
"CIDR netmask is invalid or missing"
]}}
assert validate(:key, "127.0.0.1/24", type, []) ==
{:ok, %Postgrex.INET{address: {127, 0, 0, 1}, netmask: 24}}

View File

@@ -2,6 +2,16 @@ defmodule Domain.Resources.Resource.ChangesetTest do
use Domain.DataCase, async: true
import Domain.Resources.Resource.Changeset
setup do
account = Fixtures.Accounts.create_account()
actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account)
identity = Fixtures.Auth.create_identity(account: account, actor: actor)
subject = Fixtures.Auth.create_subject(identity: identity)
resource = Fixtures.Resources.create_resource(account: account)
%{account: account, actor: actor, identity: identity, subject: subject, resource: resource}
end
describe "create/2" do
test "validates and normalizes CIDR ranges" do
for {string, cidr} <- [
@@ -23,17 +33,23 @@ defmodule Domain.Resources.Resource.ChangesetTest do
assert changeset.valid?
end
refute create(%{type: :cidr, address: "192.168.1.256/28"}).valid?
refute create(%{type: :cidr, address: "100.64.0.0/8"}).valid?
refute create(%{type: :cidr, address: "fd00:2021:1111::/102"}).valid?
refute create(%{type: :cidr, address: "0.0.0.0/32"}).valid?
refute create(%{type: :cidr, address: "0.0.0.0/16"}).valid?
refute create(%{type: :cidr, address: "0.0.0.0/0"}).valid?
refute create(%{type: :cidr, address: "127.0.0.1/32"}).valid?
refute create(%{type: :cidr, address: "::0/32"}).valid?
refute create(%{type: :cidr, address: "::1/128"}).valid?
refute create(%{type: :cidr, address: "::8/8"}).valid?
refute create(%{type: :cidr, address: "2607:f8b0:4012:0::200e/128:80"}).valid?
[
"foobar",
"192.168.1.256/28",
"100.64.0.0/8",
"fd00:2021:1111::/102",
"0.0.0.0/32",
"0.0.0.0/16",
"0.0.0.0/0",
"127.0.0.1/32",
"::0/32",
"::1/128",
"::8/8",
"2607:f8b0:4012:0::200e/128:80"
]
|> Enum.each(fn string ->
refute create(%{type: :cidr, address: string}).valid?
end)
end
test "validates and normalizes IP addresses" do
@@ -55,18 +71,35 @@ defmodule Domain.Resources.Resource.ChangesetTest do
assert changeset.valid?
end
refute create(%{type: :ip, address: "192.168.1.256"}).valid?
refute create(%{type: :ip, address: "100.64.0.0"}).valid?
refute create(%{type: :ip, address: "fd00:2021:1111::"}).valid?
refute create(%{type: :ip, address: "0.0.0.0"}).valid?
refute create(%{type: :ip, address: "::0"}).valid?
refute create(%{type: :ip, address: "127.0.0.1"}).valid?
refute create(%{type: :ip, address: "::1"}).valid?
refute create(%{type: :ip, address: "[2607:f8b0:4012:0::200e]:80"}).valid?
[
"foobar",
"192.168.1.256",
"100.64.0.0",
"fd00:2021:1111::",
"0.0.0.0",
"::0",
"127.0.0.1",
"::1",
"[2607:f8b0:4012:0::200e]:80"
]
|> Enum.each(fn string ->
refute create(%{type: :ip, address: string}).valid?
end)
end
test "accepts valid DNS addresses" do
for valid_address <- [
"**.foo.google.com",
"?.foo.google.com",
"web-*.foo.google.com",
"web-?.foo.google.com",
"web-*.google.com",
"web-?.google.com",
"**.*.?.foo.foo.com",
"**.google.com",
"?.google.com",
"*.*.*.*.google.com",
"?.?.?.?.google.com",
"*.google",
"?.google",
"google",
@@ -88,17 +121,183 @@ defmodule Domain.Resources.Resource.ChangesetTest do
assert changeset.valid?
end
refute create(%{type: :dns, address: "1.1.1.1"}).valid?
refute create(%{type: :dns, address: ".example.com"}).valid?
refute create(%{type: :dns, address: "example.com."}).valid?
refute create(%{type: :dns, address: "exa&mple.com"}).valid?
refute create(%{type: :dns, address: ""}).valid?
refute create(%{type: :dns, address: "http://example.com/"}).valid?
refute create(%{type: :dns, address: "//example.com/"}).valid?
refute create(%{type: :dns, address: "example.com/"}).valid?
refute create(%{type: :dns, address: ".example.com"}).valid?
refute create(%{type: :dns, address: "example."}).valid?
refute create(%{type: :dns, address: "example.com:80"}).valid?
[
"1.1.1.1/32",
"1.1.1.1",
".example.com",
"example..com",
"**",
"**.",
"*.",
"*.*",
"?",
"?.",
"example.com.",
"exa&mple.com",
"",
"http://example.com/",
"//example.com/",
"example.com/",
".example.com",
"example.",
"example.com:80"
]
|> Enum.each(fn invalid_address ->
refute create(%{type: :dns, address: invalid_address}).valid?
end)
end
end
describe "update/2" do
test "validates and normalizes CIDR ranges", %{resource: resource, subject: subject} do
for {string, cidr} <- [
{"192.168.1.1/24", "192.168.1.0/24"},
{"101.100.100.0/28", "101.100.100.0/28"},
{"192.168.1.255/28", "192.168.1.240/28"},
{"192.168.1.255/32", "192.168.1.255/32"},
{"2607:f8b0:4012:0::200e/128", "2607:f8b0:4012::200e/128"}
] do
{changeset, _} =
update(
resource,
%{
name: "foo",
type: :cidr,
address: string,
address_description: string
},
subject
)
assert changeset.changes.address == cidr
assert changeset.valid?
end
[
"foobar",
"192.168.1.256/28",
"100.64.0.0/8",
"fd00:2021:1111::/102",
"0.0.0.0/32",
"0.0.0.0/16",
"0.0.0.0/0",
"127.0.0.1/32",
"::0/32",
"::1/128",
"::8/8",
"2607:f8b0:4012:0::200e/128:80"
]
|> Enum.each(fn invalid_cidr ->
{changeset, _} = update(resource, %{type: :cidr, address: invalid_cidr}, subject)
refute changeset.valid?
end)
end
test "validates and normalizes IP addresses", %{resource: resource, subject: subject} do
for {string, ip} <- [
{"192.168.1.1", "192.168.1.1"},
{"101.100.100.0", "101.100.100.0"},
{"192.168.1.255", "192.168.1.255"},
{"2607:f8b0:4012:0::200e", "2607:f8b0:4012::200e"}
] do
{changeset, _} =
update(
resource,
%{
name: "foo",
type: :ip,
address: string,
address_description: string
},
subject
)
assert changeset.changes.address == ip
assert changeset.valid?
end
[
"foobar",
"192.168.1.256",
"100.64.0.0",
"fd00:2021:1111::",
"0.0.0.0",
"::0",
"127.0.0.1",
"::1",
"[2607:f8b0:4012:0::200e]:80"
]
|> Enum.each(fn invalid_ip ->
{changeset, _} = update(resource, %{type: :ip, address: invalid_ip}, subject)
refute changeset.valid?
end)
end
test "accepts valid DNS addresses", %{resource: resource, subject: subject} do
for valid_address <- [
"**.foo.google.com",
"?.foo.google.com",
"web-*.foo.google.com",
"web-?.foo.google.com",
"web-*.google.com",
"web-?.google.com",
"**.*.?.foo.foo.com",
"**.google.com",
"?.google.com",
"*.*.*.*.google.com",
"?.?.?.?.google.com",
"*.google",
"?.google",
"google",
"example.com",
"example.weird",
"1234567890.com",
"#{String.duplicate("a", 63)}.com",
"такі.справи",
"subdomain.subdomain2.example.space"
] do
{changeset, _} =
update(
resource,
%{
name: "foo",
type: :dns,
address: valid_address,
address_description: valid_address
},
subject
)
assert changeset.valid?
end
[
"2600::1/32",
"2600::1",
"1.1.1.1/32",
"1.1.1.1",
".example.com",
"example..com",
"**",
"**.",
"*.",
"*.*",
"?",
"?.",
"example.com.",
"exa&mple.com",
"",
"http://example.com/",
"//example.com/",
"example.com/",
".example.com",
"example.",
"example.com:80"
]
|> Enum.each(fn invalid_address ->
{changeset, _} = update(resource, %{type: :dns, address: invalid_address}, subject)
refute changeset.valid?
end)
end
end

View File

@@ -1129,7 +1129,7 @@ defmodule Domain.ResourcesTest do
attrs = Fixtures.Resources.resource_attrs(address: "*.com")
assert {:error, changeset} = create_resource(attrs, subject)
error = "second level domain for IANA TLDs cannot contain wildcards"
error = "domain for IANA TLDs cannot consist solely of wildcards"
assert error in errors_on(changeset).address
attrs = Fixtures.Resources.resource_attrs(address: "foo.*")

View File

@@ -456,6 +456,81 @@ defmodule Web.Live.Resources.EditTest do
assert_redirect(lv, ~p"/#{account}/resources")
end
test "prevents updating resource type to ip when address is not an IP address", %{
account: account,
identity: identity,
resource: resource,
conn: conn
} do
{:ok, lv, _html} =
conn
|> authorize_conn(identity)
|> live(~p"/#{account}/resources/#{resource}/edit")
attrs = %{
"type" => "ip"
}
assert lv
|> form("form", resource: attrs)
|> render_submit()
|> form_validation_errors() == %{
"resource[address]" => ["is not a valid IP address"]
}
end
test "prevents updating resource type to dns when address is an IP", %{
account: account,
identity: identity,
conn: conn
} do
resource = Fixtures.Resources.create_resource(account: account, type: :ip, address: "1.1.1.1")
{:ok, lv, _html} =
conn
|> authorize_conn(identity)
|> live(~p"/#{account}/resources/#{resource}/edit")
attrs = %{
"type" => "dns"
}
assert lv
|> form("form", resource: attrs)
|> render_submit()
|> form_validation_errors() == %{
"resource[address]" => ["IP addresses are not allowed, use an IP Resource instead"]
}
end
test "prevents updating resource type to dns when address is a CIDR", %{
account: account,
identity: identity,
conn: conn
} do
resource =
Fixtures.Resources.create_resource(account: account, type: :cidr, address: "1.1.1.1/32")
{:ok, lv, _html} =
conn
|> authorize_conn(identity)
|> live(~p"/#{account}/resources/#{resource}/edit")
attrs = %{
"type" => "dns"
}
assert lv
|> form("form", resource: attrs)
|> render_submit()
|> form_validation_errors() == %{
"resource[address]" => [
"must be a valid hostname (letters, digits, hyphens, dots; wildcards *, ?, ** allowed)",
"IP addresses are not allowed, use an IP Resource instead"
]
}
end
test "redirects to resources page when resource address is edited", %{
account: account,
identity: identity,