From 39b7afd1cdd58ab279a6c5f4a52b1c52e83e39e5 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Fri, 14 Feb 2020 17:56:36 +0100 Subject: [PATCH] Blind key rotation and stale duration for profiles See https://blog.dereferenced.org/the-case-for-blind-key-rotation Signed-off-by: Thomas Citharel --- CHANGELOG.md | 5 ++ config/config.exs | 6 +- lib/federation/activity_pub/activity_pub.ex | 30 ++++++--- lib/mobilizon.ex | 6 ++ lib/mobilizon/actors/actor.ex | 6 +- lib/mobilizon/actors/actors.ex | 41 ++++++++++++ lib/service/workers/background.ex | 6 ++ ...135231_add_last_refreshed_at_to_actors.exs | 9 +++ .../activity_pub/activity_pub_test.exs | 65 ++++++++++++++++++- test/support/factory.ex | 1 + 10 files changed, 164 insertions(+), 11 deletions(-) create mode 100644 priv/repo/migrations/20200214135231_add_last_refreshed_at_to_actors.exs diff --git a/CHANGELOG.md b/CHANGELOG.md index 71ba656f3..7fbf76b58 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 1065721ec..cda25c0df 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 ba693fe4d..697d278f8 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 d8cce815e..f602364b3 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 e12e9b998..beb6afd60 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 c56b6ab55..f4b769570 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 816ee09b5..12196dd01 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/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 000000000..9b3e665c8 --- /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 923f508f3..acf5ab99a 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 f8fa75e8a..ae127b97f 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