diff --git a/CHANGELOG.md b/CHANGELOG.md index 71ba656f..7fbf76b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,11 @@ Also make sure to remove the `EnvironmentFile=` line from the systemd service an - Possibility to change email address for the account - Possibility to delete your account +### Changed +- Signature validation also now checks if `Date` header has acceptable values +- Actor profiles are now stale after two days and have to be refetched +- Actor keys are rotated some time after sending a `Delete` activity + ### Fixed - Fixed URL search - Fixed content accessed through URL search being public diff --git a/config/config.exs b/config/config.exs index 1065721e..cda25c0d 100644 --- a/config/config.exs +++ b/config/config.exs @@ -146,7 +146,11 @@ config :ex_cldr, config :http_signatures, adapter: Mobilizon.Federation.HTTPSignatures.Signature -config :mobilizon, :activitypub, sign_object_fetches: true +config :mobilizon, :activitypub, + # One day + actor_stale_period: 3_600 * 48, + actor_key_rotation_delay: 3_600 * 48, + sign_object_fetches: true config :mobilizon, Mobilizon.Service.Geospatial, service: Mobilizon.Service.Geospatial.Nominatim diff --git a/lib/federation/activity_pub/activity_pub.ex b/lib/federation/activity_pub/activity_pub.ex index ba693fe4..697d278f 100644 --- a/lib/federation/activity_pub/activity_pub.ex +++ b/lib/federation/activity_pub/activity_pub.ex @@ -73,8 +73,8 @@ defmodule Mobilizon.Federation.ActivityPub do with {:not_http, true} <- {:not_http, String.starts_with?(url, "http")}, {:existing_event, nil} <- {:existing_event, Events.get_event_by_url(url)}, {:existing_comment, nil} <- {:existing_comment, Events.get_comment_from_url(url)}, - {:existing_actor, {:error, :actor_not_found}} <- - {:existing_actor, Actors.get_actor_by_url(url)}, + {:existing_actor, {:error, _err}} <- + {:existing_actor, get_or_fetch_actor_by_url(url)}, {:ok, %{body: body, status_code: code}} when code in 200..299 <- HTTPoison.get( url, @@ -126,7 +126,7 @@ defmodule Mobilizon.Federation.ActivityPub do end @doc """ - Getting an actor from url, eventually creating it + Getting an actor from url, eventually creating it if we don't have it locally or if it needs an update """ @spec get_or_fetch_actor_by_url(String.t(), boolean) :: {:ok, Actor.t()} | {:error, String.t()} def get_or_fetch_actor_by_url(url, preload \\ false) @@ -138,12 +138,13 @@ defmodule Mobilizon.Federation.ActivityPub do end def get_or_fetch_actor_by_url(url, preload) do - case Actors.get_actor_by_url(url, preload) do - {:ok, %Actor{} = actor} -> - {:ok, actor} - + with {:ok, %Actor{} = cached_actor} <- Actors.get_actor_by_url(url, preload), + false <- Actors.needs_update?(cached_actor) do + {:ok, cached_actor} + else _ -> - case make_actor_from_url(url, preload) do + # For tests, see https://github.com/jjh42/mock#not-supported---mocking-internal-function-calls and Mobilizon.Federation.ActivityPubTest + case __MODULE__.make_actor_from_url(url, preload) do {:ok, %Actor{} = actor} -> {:ok, actor} @@ -351,6 +352,7 @@ defmodule Mobilizon.Federation.ActivityPub do {:ok, %Tombstone{} = _tombstone} <- Tombstone.create_tombstone(%{uri: event.url, actor_id: actor.id}), Share.delete_all_by_uri(event.url), + :ok <- check_for_actor_key_rotation(actor), {:ok, activity} <- create_activity(Map.merge(data, audience), local), :ok <- maybe_federate(activity) do {:ok, activity, event} @@ -374,6 +376,7 @@ defmodule Mobilizon.Federation.ActivityPub do {:ok, %Tombstone{} = _tombstone} <- Tombstone.create_tombstone(%{uri: comment.url, actor_id: actor.id}), Share.delete_all_by_uri(comment.url), + :ok <- check_for_actor_key_rotation(actor), {:ok, activity} <- create_activity(Map.merge(data, audience), local), :ok <- maybe_federate(activity) do {:ok, activity, comment} @@ -1012,4 +1015,15 @@ defmodule Mobilizon.Federation.ActivityPub do }) end end + + defp check_for_actor_key_rotation(%Actor{} = actor) do + if Actors.should_rotate_actor_key(actor) do + Actors.schedule_key_rotation( + actor, + Application.get_env(:mobilizon, :activitypub)[:actor_key_rotation_delay] + ) + end + + :ok + end end diff --git a/lib/mobilizon.ex b/lib/mobilizon.ex index d8cce815..f602364b 100644 --- a/lib/mobilizon.ex +++ b/lib/mobilizon.ex @@ -47,6 +47,12 @@ defmodule Mobilizon do ActivityPub.Federator, cachex_spec(:feed, 2500, 60, 60, &Feed.create_cache/1), cachex_spec(:ics, 2500, 60, 60, &ICalendar.create_cache/1), + cachex_spec( + :actor_key_rotation, + 2500, + div(Application.get_env(:mobilizon, :activitypub)[:actor_key_rotation_delay], 60), + 60 * 30 + ), cachex_spec(:statistics, 10, 60, 60), cachex_spec(:config, 10, 60, 60), cachex_spec(:activity_pub, 2500, 3, 15) diff --git a/lib/mobilizon/actors/actor.ex b/lib/mobilizon/actors/actor.ex index e12e9b99..beb6afd6 100644 --- a/lib/mobilizon/actors/actor.ex +++ b/lib/mobilizon/actors/actor.ex @@ -50,7 +50,8 @@ defmodule Mobilizon.Actors.Actor do mentions: [Mention.t()], shares: [Share.t()], owner_shares: [Share.t()], - memberships: [t] + memberships: [t], + last_refreshed_at: DateTime.t() } @required_attrs [:preferred_username, :keys, :suspended, :url] @@ -65,6 +66,7 @@ defmodule Mobilizon.Actors.Actor do :domain, :summary, :manually_approves_followers, + :last_refreshed_at, :user_id ] @attrs @required_attrs ++ @optional_attrs @@ -118,6 +120,7 @@ defmodule Mobilizon.Actors.Actor do field(:openness, ActorOpenness, default: :moderated) field(:visibility, ActorVisibility, default: :private) field(:suspended, :boolean, default: false) + field(:last_refreshed_at, :utc_datetime) embeds_one(:avatar, File, on_replace: :update) embeds_one(:banner, File, on_replace: :update) @@ -261,6 +264,7 @@ defmodule Mobilizon.Actors.Actor do |> unique_constraint(:url, name: :actors_url_index) |> unique_constraint(:preferred_username, name: :actors_preferred_username_domain_type_index) |> validate_format(:preferred_username, ~r/[a-z0-9_]+/) + |> put_change(:last_refreshed_at, DateTime.utc_now() |> DateTime.truncate(:second)) end @doc """ diff --git a/lib/mobilizon/actors/actors.ex b/lib/mobilizon/actors/actors.ex index c56b6ab5..f4b76957 100644 --- a/lib/mobilizon/actors/actors.ex +++ b/lib/mobilizon/actors/actors.ex @@ -260,6 +260,13 @@ defmodule Mobilizon.Actors do end end + @spec actor_key_rotation(Actor.t()) :: {:ok, Actor.t()} | {:error, Ecto.Changeset.t()} + def actor_key_rotation(%Actor{} = actor) do + actor + |> Actor.changeset(%{keys: Crypto.generate_rsa_2048_private_key()}) + |> Repo.update() + end + @doc """ Returns the list of actors. """ @@ -746,6 +753,40 @@ defmodule Mobilizon.Actors do get_follower_by_followed_and_following(followed_actor, follower_actor) end + @doc """ + Whether the actor needs to be updated. + + Local actors obviously don't need to be updated + """ + @spec needs_update?(Actor.t()) :: boolean + def needs_update?(%Actor{domain: nil}), do: false + + def needs_update?(%Actor{last_refreshed_at: nil, domain: domain}) when not is_nil(domain), + do: true + + def needs_update?(%Actor{domain: domain} = actor) when not is_nil(domain) do + DateTime.diff(DateTime.utc_now(), actor.last_refreshed_at) >= + Application.get_env(:mobilizon, :activitypub)[:actor_stale_period] + end + + def needs_update?(_), do: true + + @spec should_rotate_actor_key(Actor.t()) :: boolean + def should_rotate_actor_key(%Actor{id: actor_id}) do + with {:ok, value} when is_boolean(value) <- Cachex.exists?(:actor_key_rotation, actor_id) do + value + end + end + + @spec schedule_key_rotation(Actor.t(), integer()) :: nil + def schedule_key_rotation(%Actor{id: actor_id} = actor, delay) do + Cachex.put(:actor_key_rotation, actor_id, true) + + Workers.Background.enqueue("actor_key_rotation", %{"actor_id" => actor.id}, schedule_in: delay) + + :ok + end + @spec remove_banner(Actor.t()) :: {:ok, Actor.t()} defp remove_banner(%Actor{banner: nil} = actor), do: {:ok, actor} diff --git a/lib/service/workers/background.ex b/lib/service/workers/background.ex index 816ee09b..12196dd0 100644 --- a/lib/service/workers/background.ex +++ b/lib/service/workers/background.ex @@ -14,4 +14,10 @@ defmodule Mobilizon.Service.Workers.Background do Actors.perform(:delete_actor, actor) end end + + def perform(%{"op" => "actor_key_rotation", "actor_id" => actor_id}, _job) do + with %Actor{} = actor <- Actors.get_actor(actor_id) do + Actors.actor_key_rotation(actor) + end + end end diff --git a/lib/web/plugs/http_signatures.ex b/lib/web/plugs/http_signatures.ex index fa4ae1fb..80333a78 100644 --- a/lib/web/plugs/http_signatures.ex +++ b/lib/web/plugs/http_signatures.ex @@ -43,7 +43,8 @@ defmodule Mobilizon.Web.Plugs.HTTPSignatures do signature_valid = HTTPSignatures.validate_conn(conn) Logger.debug("Is signature valid ? #{inspect(signature_valid)}") - assign(conn, :valid_signature, signature_valid) + date_valid = date_valid?(conn) + assign(conn, :valid_signature, signature_valid && date_valid) else Logger.debug("No signature header!") conn @@ -53,4 +54,15 @@ defmodule Mobilizon.Web.Plugs.HTTPSignatures do conn end end + + @spec date_valid?(Plug.Conn.t()) :: boolean() + defp date_valid?(conn) do + with [date | _] <- get_req_header(conn, "date") || [""], + {:ok, date} <- Timex.parse(date, "{WDshort}, {0D} {Mshort} {YYYY} {h24}:{m}:{s} GMT") do + Timex.diff(date, DateTime.utc_now(), :hours) <= 12 && + Timex.diff(date, DateTime.utc_now(), :hours) >= -12 + else + _ -> false + end + end end diff --git a/priv/repo/migrations/20200214135231_add_last_refreshed_at_to_actors.exs b/priv/repo/migrations/20200214135231_add_last_refreshed_at_to_actors.exs new file mode 100644 index 00000000..9b3e665c --- /dev/null +++ b/priv/repo/migrations/20200214135231_add_last_refreshed_at_to_actors.exs @@ -0,0 +1,9 @@ +defmodule Mobilizon.Storage.Repo.Migrations.AddLastRefreshedAtToActors do + use Ecto.Migration + + def change do + alter table(:actors) do + add(:last_refreshed_at, :utc_datetime, null: true) + end + end +end diff --git a/test/federation/activity_pub/activity_pub_test.exs b/test/federation/activity_pub/activity_pub_test.exs index 923f508f..acf5ab99 100644 --- a/test/federation/activity_pub/activity_pub_test.exs +++ b/test/federation/activity_pub/activity_pub_test.exs @@ -10,6 +10,7 @@ defmodule Mobilizon.Federation.ActivityPubTest do import Mock import Mobilizon.Factory + alias Mobilizon.Actors alias Mobilizon.Actors.Actor alias Mobilizon.Events @@ -47,10 +48,72 @@ defmodule Mobilizon.Federation.ActivityPubTest do end end + @actor_url "https://framapiaf.org/users/tcit" test "returns an actor from url" do + # Initial fetch use_cassette "activity_pub/fetch_framapiaf.org_users_tcit" do assert {:ok, %Actor{preferred_username: "tcit", domain: "framapiaf.org"}} = - ActivityPub.get_or_fetch_actor_by_url("https://framapiaf.org/users/tcit") + ActivityPub.get_or_fetch_actor_by_url(@actor_url) + end + + # Fetch uses cache if Actors.needs_update? returns false + with_mocks([ + {Actors, [:passthrough], + [ + get_actor_by_url: fn @actor_url, false -> + {:ok, + %Actor{ + preferred_username: "tcit", + domain: "framapiaf.org" + }} + end, + needs_update?: fn _ -> false end + ]}, + {ActivityPub, [:passthrough], + make_actor_from_url: fn @actor_url, false -> + {:ok, + %Actor{ + preferred_username: "tcit", + domain: "framapiaf.org" + }} + end} + ]) do + assert {:ok, %Actor{preferred_username: "tcit", domain: "framapiaf.org"}} = + ActivityPub.get_or_fetch_actor_by_url(@actor_url) + + assert_called(Actors.needs_update?(:_)) + refute called(ActivityPub.make_actor_from_url(@actor_url, false)) + end + + # Fetch doesn't use cache if Actors.needs_update? returns true + with_mocks([ + {Actors, [:passthrough], + [ + get_actor_by_url: fn @actor_url, false -> + {:ok, + %Actor{ + preferred_username: "tcit", + domain: "framapiaf.org" + }} + end, + needs_update?: fn _ -> true end + ]}, + {ActivityPub, [:passthrough], + make_actor_from_url: fn @actor_url, false -> + {:ok, + %Actor{ + preferred_username: "tcit", + domain: "framapiaf.org" + }} + end} + ]) do + assert {:ok, %Actor{preferred_username: "tcit", domain: "framapiaf.org"}} = + ActivityPub.get_or_fetch_actor_by_url(@actor_url) + + assert_called(ActivityPub.get_or_fetch_actor_by_url(@actor_url)) + assert_called(Actors.get_actor_by_url(@actor_url, false)) + assert_called(Actors.needs_update?(:_)) + assert_called(ActivityPub.make_actor_from_url(@actor_url, false)) end end end diff --git a/test/support/factory.ex b/test/support/factory.ex index f8fa75e8..ae127b97 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -39,6 +39,7 @@ defmodule Mobilizon.Factory do following_url: Actor.build_url(preferred_username, :following), inbox_url: Actor.build_url(preferred_username, :inbox), outbox_url: Actor.build_url(preferred_username, :outbox), + last_refreshed_at: DateTime.utc_now(), user: build(:user) } end diff --git a/test/web/plugs/http_signatures_test.exs b/test/web/plugs/http_signatures_test.exs new file mode 100644 index 00000000..581e4419 --- /dev/null +++ b/test/web/plugs/http_signatures_test.exs @@ -0,0 +1,52 @@ +defmodule Mobilizon.Web.Plug.HTTPSignaturesTest do + use Mobilizon.Web.ConnCase + import Mock + + alias Mobilizon.Federation.HTTPSignatures.Signature + alias Mobilizon.Web.Plugs.HTTPSignatures, as: HTTPSignaturesPlug + + test "tests that date window is acceptable" do + date = NaiveDateTime.utc_now() |> Timex.shift(hours: -3) |> Signature.generate_date_header() + + with_mock HTTPSignatures, validate_conn: fn _ -> true end do + conn = + build_conn() + |> put_req_header("signature", "signature") + |> put_req_header("date", date) + |> HTTPSignaturesPlug.call(%{}) + + assert called(HTTPSignatures.validate_conn(:_)) + assert conn.assigns.valid_signature + end + end + + test "tests that date window is not acceptable (already passed)" do + date = NaiveDateTime.utc_now() |> Timex.shift(hours: -30) |> Signature.generate_date_header() + + with_mock HTTPSignatures, validate_conn: fn _ -> true end do + conn = + build_conn() + |> put_req_header("signature", "signature") + |> put_req_header("date", date) + |> HTTPSignaturesPlug.call(%{}) + + refute conn.assigns.valid_signature + assert called(HTTPSignatures.validate_conn(:_)) + end + end + + test "tests that date window is not acceptable (in the future)" do + date = NaiveDateTime.utc_now() |> Timex.shift(hours: 30) |> Signature.generate_date_header() + + with_mock HTTPSignatures, validate_conn: fn _ -> true end do + conn = + build_conn() + |> put_req_header("signature", "signature") + |> put_req_header("date", date) + |> HTTPSignaturesPlug.call(%{}) + + refute conn.assigns.valid_signature + assert called(HTTPSignatures.validate_conn(:_)) + end + end +end