From f35db6540bb9df342a88fce9bef7c674cadfad50 Mon Sep 17 00:00:00 2001 From: Thomas Citharel Date: Tue, 16 Nov 2021 15:47:53 +0100 Subject: [PATCH] Various HTTP signature code improvements Signed-off-by: Thomas Citharel --- lib/federation/http_signatures/signature.ex | 50 ++++++++++--------- lib/web/plugs/http_signatures.ex | 14 +++--- lib/web/plugs/mapped_signature_to_identity.ex | 35 ++++++++----- 3 files changed, 55 insertions(+), 44 deletions(-) diff --git a/lib/federation/http_signatures/signature.ex b/lib/federation/http_signatures/signature.ex index f99dad4ee..732738b61 100644 --- a/lib/federation/http_signatures/signature.ex +++ b/lib/federation/http_signatures/signature.ex @@ -10,10 +10,9 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do @behaviour HTTPSignatures.Adapter + alias Mobilizon.Actors alias Mobilizon.Actors.Actor - alias Mobilizon.Federation.ActivityPub.Actor, as: ActivityPubActor - alias Mobilizon.Service.ErrorReporting.Sentry require Logger @@ -52,36 +51,37 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do # Gets a public key for a given ActivityPub actor ID (url). @spec get_public_key_for_url(String.t()) :: {:ok, String.t()} - | {:error, :actor_fetch_error | :pem_decode_error | :actor_not_fetchable} + | {:error, :actor_not_found | :pem_decode_error} defp get_public_key_for_url(url) do - case ActivityPubActor.get_or_fetch_actor_by_url(url) do - {:ok, %Actor{keys: keys}} -> - case prepare_public_key(keys) do - {:ok, public_key} -> - {:ok, public_key} + case Actors.get_actor_by_url(url) do + {:ok, %Actor{} = actor} -> + get_actor_public_key(actor) - {:error, :pem_decode_error} -> - Logger.error("Error while decoding PEM") - - {:error, :pem_decode_error} - end - - {:error, err} -> - Sentry.capture_message("Unable to fetch actor, so no keys for you", - extra: %{url: url} + {:error, :actor_not_found} -> + Logger.info( + "Unable to get actor from URL from local database, returning empty keys to trigger refreshment" ) - Logger.error("Unable to fetch actor, so no keys for you") - Logger.error(inspect(err)) + {:ok, ""} + end + end - {:error, :actor_fetch_error} + @spec get_actor_public_key(Actor.t()) :: {:ok, String.t()} | {:error, :pem_decode_error} + defp get_actor_public_key(%Actor{keys: keys}) do + case prepare_public_key(keys) do + {:ok, public_key} -> + {:ok, public_key} + + {:error, :pem_decode_error} -> + Logger.error("Error while decoding PEM") + + {:error, :pem_decode_error} end end @spec fetch_public_key(Plug.Conn.t()) :: {:ok, String.t()} - | {:error, - :actor_fetch_error | :actor_not_fetchable | :pem_decode_error | :no_signature_in_conn} + | {:error, :actor_not_found | :pem_decode_error | :no_signature_in_conn} def fetch_public_key(conn) do case HTTPSignatures.signature_for_conn(conn) do %{"keyId" => kid} -> @@ -100,8 +100,8 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do :actor_is_local} def refetch_public_key(conn) do %{"keyId" => kid} = HTTPSignatures.signature_for_conn(conn) - actor_id = key_id_to_actor_url(kid) - Logger.debug("Refetching public key for #{actor_id}") + actor_url = key_id_to_actor_url(kid) + Logger.debug("Refetching public key for #{actor_url}") with {:ok, %Actor{} = actor} <- ActivityPubActor.make_actor_from_url(actor_url, ignore_sign_object_fetches: true) do @@ -134,6 +134,8 @@ defmodule Mobilizon.Federation.HTTPSignatures.Signature do @spec generate_date_header(NaiveDateTime.t()) :: String.t() def generate_date_header(%NaiveDateTime{} = date) do + # We make sure the format is correct + # TODO: Remove Timex, as this is the only usage (with parsing) Timex.lformat!(date, "{WDshort}, {0D} {Mshort} {YYYY} {h24}:{m}:{s} GMT", "en") end diff --git a/lib/web/plugs/http_signatures.ex b/lib/web/plugs/http_signatures.ex index 48866fef8..3d0bfd6c3 100644 --- a/lib/web/plugs/http_signatures.ex +++ b/lib/web/plugs/http_signatures.ex @@ -36,14 +36,7 @@ defmodule Mobilizon.Web.Plugs.HTTPSignatures do "(request-target)", String.downcase("#{conn.method}") <> " #{conn.request_path}" ) - - conn = - if conn.assigns[:digest] do - conn - |> put_req_header("digest", conn.assigns[:digest]) - else - conn - end + |> maybe_put_digest_header() signature_valid = HTTPSignatures.validate_conn(conn) Logger.debug("Is signature valid ? #{inspect(signature_valid)}") @@ -53,6 +46,11 @@ defmodule Mobilizon.Web.Plugs.HTTPSignatures do end end + defp maybe_put_digest_header(%Plug.Conn{assigns: %{digest: digest}} = conn), + do: put_req_header(conn, "digest", digest) + + defp maybe_put_digest_header(%Plug.Conn{} = conn), do: conn + @spec date_valid?(Plug.Conn.t()) :: boolean() defp date_valid?(conn) do date = conn |> get_req_header("date") |> List.first() diff --git a/lib/web/plugs/mapped_signature_to_identity.ex b/lib/web/plugs/mapped_signature_to_identity.ex index 550e1e6f9..351daba34 100644 --- a/lib/web/plugs/mapped_signature_to_identity.ex +++ b/lib/web/plugs/mapped_signature_to_identity.ex @@ -11,7 +11,6 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do import Plug.Conn alias Mobilizon.Actors.Actor - alias Mobilizon.Federation.ActivityPub.Actor, as: ActivityPubActor alias Mobilizon.Federation.ActivityPub.Utils alias Mobilizon.Federation.HTTPSignatures.Signature @@ -32,16 +31,20 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do end end - @spec actor_from_key_id(Plug.Conn.t()) :: Actor.t() | nil + @spec actor_from_key_id(Plug.Conn.t()) :: + {:ok, Actor.t()} | {:error, :actor_not_found | :no_key_in_conn} defp actor_from_key_id(conn) do Logger.debug("Determining actor from connection signature") - with key_actor_id when is_binary(key_actor_id) <- key_id_from_conn(conn), - {:ok, %Actor{} = actor} <- ActivityPubActor.get_or_fetch_actor_by_url(key_actor_id) do - actor - else - _ -> - nil + case key_id_from_conn(conn) do + key_actor_id when is_binary(key_actor_id) -> + # We don't need to call refreshment here since + # the Mobilizon.Federation.HTTPSignatures.Signature plug + # should already have refreshed the actor if needed + ActivityPubActor.make_actor_from_url(key_actor_id, ignore_sign_object_fetches: true) + + nil -> + {:error, :no_key_in_conn} end end @@ -51,7 +54,7 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do # if this has payload make sure it is signed by the same actor that made it def call(%{assigns: %{valid_signature: true}, params: %{"actor" => actor}} = conn, _opts) do with actor_id when actor_id != nil <- Utils.get_url(actor), - {:actor, %Actor{} = actor} <- {:actor, actor_from_key_id(conn)}, + {:ok, %Actor{} = actor} <- actor_from_key_id(conn), {:actor_match, true} <- {:actor_match, actor.url == actor_id} do Logger.debug("Mapped identity to #{actor.url} from actor param") assign(conn, :actor, actor) @@ -61,8 +64,12 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do Logger.debug("key_id=#{key_id_from_conn(conn)}, actor=#{actor}") assign(conn, :valid_signature, false) + {:error, :no_key_in_conn} -> + Logger.debug("There was no key in conn") + conn + # TODO: remove me once testsuite uses mapped capabilities instead of what we do now - {:actor, nil} -> + {:error, :actor_not_found} -> Logger.debug("Failed to map identity from signature (lookup failure)") Logger.debug("key_id=#{key_id_from_conn(conn)}, actor=#{actor}") conn @@ -72,11 +79,15 @@ defmodule Mobilizon.Web.Plugs.MappedSignatureToIdentity do # no payload, probably a signed fetch def call(%{assigns: %{valid_signature: true}} = conn, _opts) do case actor_from_key_id(conn) do - %Actor{} = actor -> + {:ok, %Actor{} = actor} -> Logger.debug("Mapped identity to #{actor.url} from signed fetch") assign(conn, :actor, actor) - _ -> + {:error, :no_key_in_conn} -> + Logger.debug("There was no key in conn") + assign(conn, :valid_signature, false) + + {:error, :actor_not_found} -> Logger.debug("Failed to map identity from signature (no payload actor mismatch)") Logger.debug("key_id=#{key_id_from_conn(conn)}") assign(conn, :valid_signature, false)