From 35ee759e74d0737598311d8e4245168f981812d3 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Mon, 5 Oct 2020 11:48:41 -0500 Subject: [PATCH 01/12] Add helper function to convert single IPs into CIDR format if they were not provided that way --- lib/pleroma/plugs/remote_ip.ex | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/plugs/remote_ip.ex b/lib/pleroma/plugs/remote_ip.ex index 0ac9050d0..d1b1f793a 100644 --- a/lib/pleroma/plugs/remote_ip.ex +++ b/lib/pleroma/plugs/remote_ip.ex @@ -47,8 +47,19 @@ defp remote_ip_opts(config) do config |> Keyword.get(:proxies, []) |> Enum.concat(reserved) - |> Enum.map(&InetCidr.parse/1) + |> Enum.map(&maybe_add_cidr/1) {headers, proxies} end + + defp maybe_add_cidr(proxy) when is_binary(proxy) do + proxy = + cond do + "/" in String.codepoints(proxy) -> proxy + InetCidr.v4?(InetCidr.parse_address!(proxy)) -> proxy <> "/32" + InetCidr.v6?(InetCidr.parse_address!(proxy)) -> proxy <> "/128" + end + + InetCidr.parse(proxy) + end end From 7aff2b47c56c5b41620445b7d49c429eb1866164 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 6 Oct 2020 15:47:11 -0500 Subject: [PATCH 02/12] Fix docs for default headers used by RemoteIp. We only use X-Forwarded-For by default. --- config/description.exs | 5 +++-- docs/configuration/cheatsheet.md | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/config/description.exs b/config/description.exs index ac3dfbb2b..f6331dd30 100644 --- a/config/description.exs +++ b/config/description.exs @@ -3262,8 +3262,9 @@ %{ key: :headers, type: {:list, :string}, - description: - "A list of strings naming the `req_headers` to use when deriving the `remote_ip`. Order does not matter. Default: `~w[forwarded x-forwarded-for x-client-ip x-real-ip]`." + description: """ + A list of strings naming the `req_headers` to use when deriving the `remote_ip`. Default: `["x-forwarded-for"]`. + """ }, %{ key: :proxies, diff --git a/docs/configuration/cheatsheet.md b/docs/configuration/cheatsheet.md index 42e5fe808..e0194525d 100644 --- a/docs/configuration/cheatsheet.md +++ b/docs/configuration/cheatsheet.md @@ -426,7 +426,7 @@ This will make Pleroma listen on `127.0.0.1` port `8080` and generate urls start Available options: * `enabled` - Enable/disable the plug. Defaults to `false`. -* `headers` - A list of strings naming the `req_headers` to use when deriving the `remote_ip`. Order does not matter. Defaults to `["x-forwarded-for"]`. +* `headers` - A list of strings naming the `req_headers` to use when deriving the `remote_ip`. Defaults to `["x-forwarded-for"]`. * `proxies` - A list of strings in [CIDR](https://en.wikipedia.org/wiki/CIDR) notation specifying the IPs of known proxies. Defaults to `[]`. * `reserved` - Defaults to [localhost](https://en.wikipedia.org/wiki/Localhost) and [private network](https://en.wikipedia.org/wiki/Private_network). From d43d05005ae4e8b0f069111baee867492d4f0c52 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 6 Oct 2020 17:02:46 -0500 Subject: [PATCH 03/12] Move hardcoded default configuration into config.exs --- config/config.exs | 13 ++++++++++++- lib/pleroma/plugs/remote_ip.ex | 31 +++++++------------------------ test/plugs/remote_ip_test.exs | 24 +++++++++++++++++------- 3 files changed, 36 insertions(+), 32 deletions(-) diff --git a/config/config.exs b/config/config.exs index 2e6b0796a..d53663d36 100644 --- a/config/config.exs +++ b/config/config.exs @@ -677,7 +677,18 @@ config :pleroma, Pleroma.Workers.PurgeExpiredActivity, enabled: true, min_lifetime: 600 -config :pleroma, Pleroma.Plugs.RemoteIp, enabled: true +config :pleroma, Pleroma.Plugs.RemoteIp, + enabled: true, + headers: ["x-forwarded-for"], + proxies: [], + reserved: [ + "127.0.0.0/8", + "::1/128", + "fc00::/7", + "10.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16" + ] config :pleroma, :static_fe, enabled: false diff --git a/lib/pleroma/plugs/remote_ip.ex b/lib/pleroma/plugs/remote_ip.ex index d1b1f793a..9487efa5f 100644 --- a/lib/pleroma/plugs/remote_ip.ex +++ b/lib/pleroma/plugs/remote_ip.ex @@ -7,45 +7,28 @@ defmodule Pleroma.Plugs.RemoteIp do This is a shim to call [`RemoteIp`](https://git.pleroma.social/pleroma/remote_ip) but with runtime configuration. """ + alias Pleroma.Config import Plug.Conn @behaviour Plug - @headers ~w[ - x-forwarded-for - ] - - # https://en.wikipedia.org/wiki/Localhost - # https://en.wikipedia.org/wiki/Private_network - @reserved ~w[ - 127.0.0.0/8 - ::1/128 - fc00::/7 - 10.0.0.0/8 - 172.16.0.0/12 - 192.168.0.0/16 - ] - def init(_), do: nil def call(%{remote_ip: original_remote_ip} = conn, _) do - config = Pleroma.Config.get(__MODULE__, []) - - if Keyword.get(config, :enabled, false) do - %{remote_ip: new_remote_ip} = conn = RemoteIp.call(conn, remote_ip_opts(config)) + if Config.get([__MODULE__, :enabled]) do + %{remote_ip: new_remote_ip} = conn = RemoteIp.call(conn, remote_ip_opts()) assign(conn, :remote_ip_found, original_remote_ip != new_remote_ip) else conn end end - defp remote_ip_opts(config) do - headers = config |> Keyword.get(:headers, @headers) |> MapSet.new() - reserved = Keyword.get(config, :reserved, @reserved) + defp remote_ip_opts() do + headers = Config.get([__MODULE__, :headers], []) |> MapSet.new() + reserved = Config.get([__MODULE__, :reserved], []) proxies = - config - |> Keyword.get(:proxies, []) + Config.get([__MODULE__, :proxies], []) |> Enum.concat(reserved) |> Enum.map(&maybe_add_cidr/1) diff --git a/test/plugs/remote_ip_test.exs b/test/plugs/remote_ip_test.exs index 752ab32e7..2dd1ac1f8 100644 --- a/test/plugs/remote_ip_test.exs +++ b/test/plugs/remote_ip_test.exs @@ -3,13 +3,27 @@ # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Plugs.RemoteIpTest do - use ExUnit.Case, async: true + use ExUnit.Case use Plug.Test alias Pleroma.Plugs.RemoteIp - import Pleroma.Tests.Helpers, only: [clear_config: 1, clear_config: 2] - setup do: clear_config(RemoteIp) + import Pleroma.Tests.Helpers, only: [clear_config: 2] + + setup do: + clear_config(RemoteIp, + enabled: true, + headers: ["x-forwarded-for"], + proxies: [], + reserved: [ + "127.0.0.0/8", + "::1/128", + "fc00::/7", + "10.0.0.0/8", + "172.16.0.0/12", + "192.168.0.0/16" + ] + ) test "disabled" do Pleroma.Config.put(RemoteIp, enabled: false) @@ -25,8 +39,6 @@ test "disabled" do end test "enabled" do - Pleroma.Config.put(RemoteIp, enabled: true) - conn = conn(:get, "/") |> put_req_header("x-forwarded-for", "1.1.1.1") @@ -54,8 +66,6 @@ test "custom headers" do end test "custom proxies" do - Pleroma.Config.put(RemoteIp, enabled: true) - conn = conn(:get, "/") |> put_req_header("x-forwarded-for", "173.245.48.1, 1.1.1.1, 173.245.48.2") From 9783e9cd8023533d05bf78e3db3375102a199fc0 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 6 Oct 2020 17:08:26 -0500 Subject: [PATCH 04/12] Add test for an entry without CIDR format --- test/plugs/remote_ip_test.exs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/plugs/remote_ip_test.exs b/test/plugs/remote_ip_test.exs index 2dd1ac1f8..849c7fc3d 100644 --- a/test/plugs/remote_ip_test.exs +++ b/test/plugs/remote_ip_test.exs @@ -82,4 +82,15 @@ test "custom proxies" do assert conn.remote_ip == {1, 1, 1, 1} end + + test "proxies set without CIDR format" do + Pleroma.Config.put([RemoteIp, :proxies], ["173.245.48.1"]) + + conn = + conn(:get, "/") + |> put_req_header("x-forwarded-for", "173.245.48.1, 1.1.1.1") + |> RemoteIp.call(nil) + + assert conn.remote_ip == {1, 1, 1, 1} + end end From b8c05f4876b8f48bcd93d7e5d60539101329065a Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 6 Oct 2020 17:21:27 -0500 Subject: [PATCH 05/12] Improve descriptions for reserved and proxies --- config/description.exs | 7 ++++--- docs/configuration/cheatsheet.md | 4 ++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/config/description.exs b/config/description.exs index f6331dd30..2c7d91ccc 100644 --- a/config/description.exs +++ b/config/description.exs @@ -3270,13 +3270,14 @@ key: :proxies, type: {:list, :string}, description: - "A list of strings in [CIDR](https://en.wikipedia.org/wiki/CIDR) notation specifying the IPs of known proxies. Default: `[]`." + "A list of upstream proxy IP subnets in CIDR notation. Defaults to `[]`. IPv4 entries without a bitmask will be assumed to be /32 and IPv6 /128." }, %{ key: :reserved, type: {:list, :string}, - description: - "Defaults to [localhost](https://en.wikipedia.org/wiki/Localhost) and [private network](https://en.wikipedia.org/wiki/Private_network)." + description: """ + A list of reserved IP subnets in CIDR notation which should be ignored if found in `headers`. Defaults to `["127.0.0.0/8", "::1/128", "fc00::/7", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"]` + """ } ] }, diff --git a/docs/configuration/cheatsheet.md b/docs/configuration/cheatsheet.md index e0194525d..22333c4f8 100644 --- a/docs/configuration/cheatsheet.md +++ b/docs/configuration/cheatsheet.md @@ -427,8 +427,8 @@ Available options: * `enabled` - Enable/disable the plug. Defaults to `false`. * `headers` - A list of strings naming the `req_headers` to use when deriving the `remote_ip`. Defaults to `["x-forwarded-for"]`. -* `proxies` - A list of strings in [CIDR](https://en.wikipedia.org/wiki/CIDR) notation specifying the IPs of known proxies. Defaults to `[]`. -* `reserved` - Defaults to [localhost](https://en.wikipedia.org/wiki/Localhost) and [private network](https://en.wikipedia.org/wiki/Private_network). +* `proxies` - A list of upstream proxy IP subnets in CIDR notation. Defaults to `[]`. IPv4 entries without a bitmask will be assumed to be /32 and IPv6 /128. +* `reserved` - A list of reserved IP subnets in CIDR notation which should be ignored if found in `headers`. Defaults to `["127.0.0.0/8", "::1/128", "fc00::/7", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"]`. ### :rate_limit From 7a2ed2fc90dd16a5ef45c4dd44a6e09bba035299 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 6 Oct 2020 17:26:31 -0500 Subject: [PATCH 06/12] Credo --- lib/pleroma/plugs/remote_ip.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/pleroma/plugs/remote_ip.ex b/lib/pleroma/plugs/remote_ip.ex index 9487efa5f..51cc87ad8 100644 --- a/lib/pleroma/plugs/remote_ip.ex +++ b/lib/pleroma/plugs/remote_ip.ex @@ -23,7 +23,7 @@ def call(%{remote_ip: original_remote_ip} = conn, _) do end end - defp remote_ip_opts() do + defp remote_ip_opts do headers = Config.get([__MODULE__, :headers], []) |> MapSet.new() reserved = Config.get([__MODULE__, :reserved], []) From e08eb4aba07ce843f3f1149b8c70fb6b4d855c44 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 6 Oct 2020 17:29:56 -0500 Subject: [PATCH 07/12] Don't leak internal variables in the docs. They're useless to users. --- config/description.exs | 2 +- docs/configuration/cheatsheet.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/description.exs b/config/description.exs index 2c7d91ccc..71cb5d913 100644 --- a/config/description.exs +++ b/config/description.exs @@ -3263,7 +3263,7 @@ key: :headers, type: {:list, :string}, description: """ - A list of strings naming the `req_headers` to use when deriving the `remote_ip`. Default: `["x-forwarded-for"]`. + A list of strings naming the HTTP headers to use when deriving the true client IP. Default: `["x-forwarded-for"]`. """ }, %{ diff --git a/docs/configuration/cheatsheet.md b/docs/configuration/cheatsheet.md index 22333c4f8..7f1dc0fe6 100644 --- a/docs/configuration/cheatsheet.md +++ b/docs/configuration/cheatsheet.md @@ -426,7 +426,7 @@ This will make Pleroma listen on `127.0.0.1` port `8080` and generate urls start Available options: * `enabled` - Enable/disable the plug. Defaults to `false`. -* `headers` - A list of strings naming the `req_headers` to use when deriving the `remote_ip`. Defaults to `["x-forwarded-for"]`. +* `headers` - A list of strings naming the HTTP headers to use when deriving the true client IP address. Defaults to `["x-forwarded-for"]`. * `proxies` - A list of upstream proxy IP subnets in CIDR notation. Defaults to `[]`. IPv4 entries without a bitmask will be assumed to be /32 and IPv6 /128. * `reserved` - A list of reserved IP subnets in CIDR notation which should be ignored if found in `headers`. Defaults to `["127.0.0.0/8", "::1/128", "fc00::/7", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"]`. From b90eda3d8bfa1faf5bdabce9539b601476abed94 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Tue, 6 Oct 2020 17:36:29 -0500 Subject: [PATCH 08/12] Improve description yet again --- config/description.exs | 2 +- docs/configuration/cheatsheet.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/config/description.exs b/config/description.exs index 71cb5d913..c6916ad14 100644 --- a/config/description.exs +++ b/config/description.exs @@ -3270,7 +3270,7 @@ key: :proxies, type: {:list, :string}, description: - "A list of upstream proxy IP subnets in CIDR notation. Defaults to `[]`. IPv4 entries without a bitmask will be assumed to be /32 and IPv6 /128." + "A list of upstream proxy IP subnets in CIDR notation from which we will parse the content of `headers`. Defaults to `[]`. IPv4 entries without a bitmask will be assumed to be /32 and IPv6 /128." }, %{ key: :reserved, diff --git a/docs/configuration/cheatsheet.md b/docs/configuration/cheatsheet.md index 7f1dc0fe6..ea7dfec98 100644 --- a/docs/configuration/cheatsheet.md +++ b/docs/configuration/cheatsheet.md @@ -427,7 +427,7 @@ Available options: * `enabled` - Enable/disable the plug. Defaults to `false`. * `headers` - A list of strings naming the HTTP headers to use when deriving the true client IP address. Defaults to `["x-forwarded-for"]`. -* `proxies` - A list of upstream proxy IP subnets in CIDR notation. Defaults to `[]`. IPv4 entries without a bitmask will be assumed to be /32 and IPv6 /128. +* `proxies` - A list of upstream proxy IP subnets in CIDR notation from which we will parse the content of `headers`. Defaults to `[]`. IPv4 entries without a bitmask will be assumed to be /32 and IPv6 /128. * `reserved` - A list of reserved IP subnets in CIDR notation which should be ignored if found in `headers`. Defaults to `["127.0.0.0/8", "::1/128", "fc00::/7", "10.0.0.0/8", "172.16.0.0/12", "192.168.0.0/16"]`. From d0eca5b12518b0b98ef53003d60b08a78decf35f Mon Sep 17 00:00:00 2001 From: feld Date: Wed, 7 Oct 2020 19:16:53 +0000 Subject: [PATCH 09/12] Apply 2 suggestion(s) to 2 file(s) --- lib/pleroma/plugs/remote_ip.ex | 2 +- test/plugs/remote_ip_test.exs | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/lib/pleroma/plugs/remote_ip.ex b/lib/pleroma/plugs/remote_ip.ex index 51cc87ad8..987022156 100644 --- a/lib/pleroma/plugs/remote_ip.ex +++ b/lib/pleroma/plugs/remote_ip.ex @@ -43,6 +43,6 @@ defp maybe_add_cidr(proxy) when is_binary(proxy) do InetCidr.v6?(InetCidr.parse_address!(proxy)) -> proxy <> "/128" end - InetCidr.parse(proxy) + InetCidr.parse(proxy, true) end end diff --git a/test/plugs/remote_ip_test.exs b/test/plugs/remote_ip_test.exs index 849c7fc3d..2da9f616b 100644 --- a/test/plugs/remote_ip_test.exs +++ b/test/plugs/remote_ip_test.exs @@ -92,5 +92,18 @@ test "proxies set without CIDR format" do |> RemoteIp.call(nil) assert conn.remote_ip == {1, 1, 1, 1} + + test "proxies set `nonsensical` CIDR" do + Pleroma.Config.put([RemoteIp, :reserved], ["127.0.0.0/8"]) + Pleroma.Config.put([RemoteIp, :proxies], ["10.0.0.3/24"]) + + conn = + conn(:get, "/") + |> put_req_header("x-forwarded-for", "10.0.0.3, 1.1.1.1") + |> RemoteIp.call(nil) + + assert conn.remote_ip == {1, 1, 1, 1} + end + end end From 8bfc5d9a0cf96739a6a73eae3c1d96277da8ae1b Mon Sep 17 00:00:00 2001 From: Maksim Date: Wed, 7 Oct 2020 19:32:09 +0000 Subject: [PATCH 10/12] Apply 1 suggestion(s) to 1 file(s) --- test/plugs/remote_ip_test.exs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/plugs/remote_ip_test.exs b/test/plugs/remote_ip_test.exs index 2da9f616b..5f1b8a539 100644 --- a/test/plugs/remote_ip_test.exs +++ b/test/plugs/remote_ip_test.exs @@ -92,6 +92,7 @@ test "proxies set without CIDR format" do |> RemoteIp.call(nil) assert conn.remote_ip == {1, 1, 1, 1} + end test "proxies set `nonsensical` CIDR" do Pleroma.Config.put([RemoteIp, :reserved], ["127.0.0.0/8"]) From 6ee20eb3285a99fab880150a9dfeebadc46fde76 Mon Sep 17 00:00:00 2001 From: Maksim Date: Wed, 7 Oct 2020 19:32:42 +0000 Subject: [PATCH 11/12] Apply 1 suggestion(s) to 1 file(s) --- test/plugs/remote_ip_test.exs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/plugs/remote_ip_test.exs b/test/plugs/remote_ip_test.exs index 5f1b8a539..b45baf75f 100644 --- a/test/plugs/remote_ip_test.exs +++ b/test/plugs/remote_ip_test.exs @@ -104,7 +104,6 @@ test "proxies set `nonsensical` CIDR" do |> RemoteIp.call(nil) assert conn.remote_ip == {1, 1, 1, 1} - end end end From a702f9fb5bff78c99014838eb8f678c30913bd59 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Wed, 7 Oct 2020 15:07:03 -0500 Subject: [PATCH 12/12] Lint --- test/plugs/remote_ip_test.exs | 1 - 1 file changed, 1 deletion(-) diff --git a/test/plugs/remote_ip_test.exs b/test/plugs/remote_ip_test.exs index b45baf75f..6d01c812d 100644 --- a/test/plugs/remote_ip_test.exs +++ b/test/plugs/remote_ip_test.exs @@ -104,6 +104,5 @@ test "proxies set `nonsensical` CIDR" do |> RemoteIp.call(nil) assert conn.remote_ip == {1, 1, 1, 1} - end end