Merge branch 'federation-improvements' into 'master'

Federation improvements

See merge request framasoft/mobilizon!398

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

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
 

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, {:error, _err}} <-
-           {:existing_actor, Actors.get_actor_by_url(url)},
+           {: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
+    with {:ok, %Actor{} = cached_actor} <- Actors.get_actor_by_url(url, preload),
-      {:ok, %Actor{} = actor} ->
+         false <- Actors.needs_update?(cached_actor) do
-        {:ok, actor}
+      {: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

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)

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 """

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}
 

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

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

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

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

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

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