diff --git a/lib/pleroma/signature.ex b/lib/pleroma/signature.ex index 1374ac068..cedfee2e5 100644 --- a/lib/pleroma/signature.ex +++ b/lib/pleroma/signature.ex @@ -8,32 +8,24 @@ defmodule Pleroma.Signature do alias Pleroma.User alias Pleroma.Web.ActivityPub.ActivityPub alias Pleroma.User.SigningKey + require Logger def key_id_to_actor_id(key_id) do # Given the key ID, first attempt to look it up in the signing keys table. - # If it's not found, then attempt to look it up via request to the remote instance. case SigningKey.key_id_to_ap_id(key_id) do nil -> - # this requires us to look up the url! - request_key_id_from_remote_instance(key_id) + # hm, we SHOULD have gotten this in the pipeline before we hit here! + Logger.error("Could not figure out who owns the key #{key_id}") + {:error, :key_owner_not_found} key -> {:ok, key} end end - def request_key_id_from_remote_instance(key_id) do - case SigningKey.fetch_remote_key(key_id) do - {:ok, key_id} -> - {:ok, key_id} - - {:error, _} -> - {:error, "Key ID not found"} - end - end - def fetch_public_key(conn) do with %{"keyId" => kid} <- HTTPSignatures.signature_for_conn(conn), + {:ok, %SigningKey{}} <- SigningKey.get_or_fetch_by_key_id(kid), {:ok, actor_id} <- key_id_to_actor_id(kid), {:ok, public_key} <- User.get_public_key_for_ap_id(actor_id) do {:ok, public_key} @@ -45,8 +37,8 @@ def fetch_public_key(conn) do def refetch_public_key(conn) do with %{"keyId" => kid} <- HTTPSignatures.signature_for_conn(conn), + {:ok, %SigningKey{}} <- SigningKey.get_or_fetch_by_key_id(kid), {:ok, actor_id} <- key_id_to_actor_id(kid), - {:ok, _user} <- ActivityPub.make_user_from_ap_id(actor_id), {:ok, public_key} <- User.get_public_key_for_ap_id(actor_id) do {:ok, public_key} else diff --git a/lib/pleroma/user/signing_key.ex b/lib/pleroma/user/signing_key.ex index 3d9b476f0..1fa574ae2 100644 --- a/lib/pleroma/user/signing_key.ex +++ b/lib/pleroma/user/signing_key.ex @@ -154,6 +154,21 @@ def private_key(%User{signing_key: %__MODULE__{private_key: private_key_pem}}) d {:ok, key} end + @spec get_or_fetch_by_key_id(String.t()) :: {:ok, __MODULE__} | {:error, String.t()} + @doc """ + Given a key ID, return the signing key associated with that key. + Will either return the key if it exists locally, or fetch it from the remote instance. + """ + def get_or_fetch_by_key_id(key_id) do + case key_id_to_user_id(key_id) do + nil -> + fetch_remote_key(key_id) + + user_id -> + {:ok, Repo.get_by(__MODULE__, user_id: user_id)} + end + end + @spec fetch_remote_key(String.t()) :: {:ok, __MODULE__} | {:error, String.t()} @doc """ Fetch a remote key by key ID. @@ -164,23 +179,33 @@ def private_key(%User{signing_key: %__MODULE__{private_key: private_key_pem}}) d So if we're rejected, we should probably just give up. """ def fetch_remote_key(key_id) do + Logger.debug("Fetching remote key: #{key_id}") # we should probably sign this, just in case resp = Pleroma.Object.Fetcher.get_object(key_id) - case handle_signature_response(resp) do - {:ok, ap_id, public_key_pem} -> - # fetch the user - user = User.get_or_fetch_by_ap_id(ap_id) - # store the key - key = %__MODULE__{ - user_id: user.id, - public_key: public_key_pem, - key_id: key_id - } + case resp do + {:ok, _original_url, body} -> + case handle_signature_response(resp) do + {:ok, ap_id, public_key_pem} -> + Logger.debug("Fetched remote key: #{ap_id}") + # fetch the user + {:ok, user} = User.get_or_fetch_by_ap_id(ap_id) + # store the key + key = %__MODULE__{ + user_id: user.id, + public_key: public_key_pem, + key_id: key_id + } - Repo.insert(key) + Repo.insert(key, on_conflict: :replace_all, conflict_target: :key_id) + + e -> + Logger.debug("Failed to fetch remote key: #{inspect(e)}") + {:error, "Could not fetch key"} + end _ -> + Logger.debug("Failed to fetch remote key: #{inspect(resp)}") {:error, "Could not fetch key"} end end @@ -196,7 +221,7 @@ defp extract_key_details(%{"id" => ap_id, "publicKey" => public_key}) do end end - defp handle_signature_response({:ok, %{status: status, body: body}}) when status in 200..299 do + defp handle_signature_response({:ok, _original_url, body}) do case Jason.decode(body) do {:ok, %{"id" => _user_id, "publicKey" => _public_key} = body} -> extract_key_details(body) diff --git a/lib/pleroma/web/activity_pub/views/user_view.ex b/lib/pleroma/web/activity_pub/views/user_view.ex index f91810fb8..ad6aeaff2 100644 --- a/lib/pleroma/web/activity_pub/views/user_view.ex +++ b/lib/pleroma/web/activity_pub/views/user_view.ex @@ -67,7 +67,12 @@ def render("user.json", %{user: %User{nickname: "internal." <> _} = user}), do: render("service.json", %{user: user}) |> Map.put("preferredUsername", user.nickname) def render("user.json", %{user: user}) do - {:ok, public_key} = User.SigningKey.public_key_pem(user) + public_key = + case User.SigningKey.public_key_pem(user) do + {:ok, public_key} -> public_key + _ -> nil + end + user = User.sanitize_html(user) endpoints = render("endpoints.json", %{user: user}) diff --git a/lib/pleroma/web/plugs/ensure_user_public_key_plug.ex b/lib/pleroma/web/plugs/ensure_user_public_key_plug.ex index c59053854..4da9215a3 100644 --- a/lib/pleroma/web/plugs/ensure_user_public_key_plug.ex +++ b/lib/pleroma/web/plugs/ensure_user_public_key_plug.ex @@ -14,7 +14,7 @@ def call(conn, _opts) do key_id = key_id_from_conn(conn) unless is_nil(key_id) do - SigningKey.fetch_remote_key(key_id) + User.SigningKey.fetch_remote_key(key_id) # now we SHOULD have the user that owns the key locally. maybe. # if we don't, we'll error out when we try to validate. end diff --git a/priv/repo/migrations/20240625213637_create_signing_key_table.exs b/priv/repo/migrations/20240625213637_create_signing_key_table.exs index 34169f78c..5bd9bbcf2 100644 --- a/priv/repo/migrations/20240625213637_create_signing_key_table.exs +++ b/priv/repo/migrations/20240625213637_create_signing_key_table.exs @@ -10,6 +10,6 @@ def change do timestamps() end - create index(:signing_keys, [:key_id]) + create unique_index(:signing_keys, [:key_id]) end end diff --git a/test/mix/tasks/pleroma/database_test.exs b/test/mix/tasks/pleroma/database_test.exs index 20f113e6e..4f97a978a 100644 --- a/test/mix/tasks/pleroma/database_test.exs +++ b/test/mix/tasks/pleroma/database_test.exs @@ -353,7 +353,7 @@ test "with the --keep-threads option it keeps old threads with bookmarked posts" test "We don't have unexpected tables which may contain objects that are referenced by activities" do # We can delete orphaned activities. For that we look for the objects they reference in the 'objects', 'activities', and 'users' table. - # If someone adds another table with objects (idk, maybe with separate relations, or collections or w/e), then we need to make sure we + # If someone adds another table with objects (idk, maybe with separate relations, or collections or w/e), then we need to make sure we # add logic for that in the 'prune_objects' task so that we don't wrongly delete their corresponding activities. # So when someone adds (or removes) a table, this test will fail. # Either the table contains objects which can be referenced from the activities table @@ -401,6 +401,7 @@ test "We don't have unexpected tables which may contain objects that are referen ["rich_media_card"], ["scheduled_activities"], ["schema_migrations"], + ["signing_keys"], ["thread_mutes"], ["user_follows_hashtag"], ["user_frontend_setting_profiles"], diff --git a/test/pleroma/signature_test.exs b/test/pleroma/signature_test.exs index 768c78f21..0e360d58d 100644 --- a/test/pleroma/signature_test.exs +++ b/test/pleroma/signature_test.exs @@ -35,25 +35,23 @@ defp make_fake_conn(key_id), do: %Plug.Conn{req_headers: %{"signature" => make_fake_signature(key_id <> "#main-key")}} describe "fetch_public_key/1" do - test "it returns key" do + test "it returns the key" do expected_result = {:ok, @rsa_public_key} - user = insert(:user, public_key: @public_key) + user = + insert(:user) + |> with_signing_key(public_key: @public_key) assert Signature.fetch_public_key(make_fake_conn(user.ap_id)) == expected_result end - test "it returns error when not found user" do - assert capture_log(fn -> - assert Signature.fetch_public_key(make_fake_conn("https://test-ap-id")) == - {:error, :error} - end) =~ "[error] Could not decode user" - end - test "it returns error if public key is nil" do - user = insert(:user, public_key: nil) + # this actually needs the URL to be valid + user = insert(:user) + key_id = user.ap_id <> "#main-key" + Tesla.Mock.mock(fn %{url: ^key_id} -> {:ok, %{status: 404}} end) - assert Signature.fetch_public_key(make_fake_conn(user.ap_id)) == {:error, :error} + assert {:error, _} = Signature.fetch_public_key(make_fake_conn(user.ap_id)) end end @@ -63,12 +61,6 @@ test "it returns key" do assert Signature.refetch_public_key(make_fake_conn(ap_id)) == {:ok, @rsa_public_key} end - - test "it returns error when not found user" do - assert capture_log(fn -> - {:error, _} = Signature.refetch_public_key(make_fake_conn("https://test-ap_id")) - end) =~ "[error] Could not decode user" - end end defp split_signature(sig) do @@ -104,9 +96,9 @@ defp assert_part_equal(part_a, part_b) do test "it returns signature headers" do user = insert(:user, %{ - ap_id: "https://mastodon.social/users/lambadalambda", - keys: @private_key + ap_id: "https://mastodon.social/users/lambadalambda" }) + |> with_signing_key(private_key: @private_key) headers = %{ host: "test.test", @@ -121,50 +113,15 @@ test "it returns signature headers" do "keyId=\"https://mastodon.social/users/lambadalambda#main-key\",algorithm=\"rsa-sha256\",headers=\"content-length host\",signature=\"sibUOoqsFfTDerquAkyprxzDjmJm6erYc42W5w1IyyxusWngSinq5ILTjaBxFvfarvc7ci1xAi+5gkBwtshRMWm7S+Uqix24Yg5EYafXRun9P25XVnYBEIH4XQ+wlnnzNIXQkU3PU9e6D8aajDZVp3hPJNeYt1gIPOA81bROI8/glzb1SAwQVGRbqUHHHKcwR8keiR/W2h7BwG3pVRy4JgnIZRSW7fQogKedDg02gzRXwUDFDk0pr2p3q6bUWHUXNV8cZIzlMK+v9NlyFbVYBTHctAR26GIAN6Hz0eV0mAQAePHDY1mXppbA8Gpp6hqaMuYfwifcXmcc+QFm4e+n3A==\"" ) end - - test "it returns error" do - user = insert(:user, %{ap_id: "https://mastodon.social/users/lambadalambda", keys: ""}) - - assert Signature.sign( - user, - %{host: "test.test", "content-length": "100"} - ) == {:error, []} - end end describe "key_id_to_actor_id/1" do - test "it properly deduces the actor id for misskey" do - assert Signature.key_id_to_actor_id("https://example.com/users/1234/publickey") == - {:ok, "https://example.com/users/1234"} - end + test "it reverses the key id to actor id" do + user = + insert(:user) + |> with_signing_key() - test "it properly deduces the actor id for mastodon and pleroma" do - assert Signature.key_id_to_actor_id("https://example.com/users/1234#main-key") == - {:ok, "https://example.com/users/1234"} - end - - test "it deduces the actor id for gotoSocial" do - assert Signature.key_id_to_actor_id("https://example.com/users/1234/main-key") == - {:ok, "https://example.com/users/1234"} - end - - test "it deduces the actor ID for streams" do - assert Signature.key_id_to_actor_id("https://example.com/users/1234?operation=getkey") == - {:ok, "https://example.com/users/1234"} - end - - test "it deduces the actor ID for bridgy" do - assert Signature.key_id_to_actor_id("https://example.com/1234#key") == - {:ok, "https://example.com/1234"} - end - - test "it calls webfinger for 'acct:' accounts" do - with_mock(Pleroma.Web.WebFinger, - finger: fn _ -> {:ok, %{"ap_id" => "https://gensokyo.2hu/users/raymoo"}} end - ) do - assert Signature.key_id_to_actor_id("acct:raymoo@gensokyo.2hu") == - {:ok, "https://gensokyo.2hu/users/raymoo"} - end + assert Signature.key_id_to_actor_id(user.signing_key.key_id) == {:ok, user.ap_id} end end diff --git a/test/pleroma/user_search_test.exs b/test/pleroma/user_search_test.exs index 2af19b6de..5d018a301 100644 --- a/test/pleroma/user_search_test.exs +++ b/test/pleroma/user_search_test.exs @@ -259,7 +259,7 @@ test "works with URIs" do |> Map.put(:multi_factor_authentication_settings, nil) |> Map.put(:notification_settings, nil) - assert user == expected + assert_user_match(user, expected) end test "excludes a blocked users from search result" do diff --git a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs index d67b59ef2..120fb6d1b 100644 --- a/test/pleroma/web/activity_pub/activity_pub_controller_test.exs +++ b/test/pleroma/web/activity_pub/activity_pub_controller_test.exs @@ -645,7 +645,7 @@ test "accept follow activity", %{conn: conn} do assert "ok" == conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{followed_relay.ap_id}/main-key\"") + |> put_req_header("signature", "keyId=\"#{followed_relay.ap_id}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/inbox", accept) |> json_response(200) @@ -682,6 +682,7 @@ test "accepts Add/Remove activities", %{conn: conn} do |> String.replace("{{nickname}}", "lain") actor = "https://example.com/users/lain" + key_id = "#{actor}/main-key" insert(:user, ap_id: actor, @@ -709,6 +710,16 @@ test "accepts Add/Remove activities", %{conn: conn} do headers: [{"content-type", "application/activity+json"}] } + %{ + method: :get, + url: ^key_id + } -> + %Tesla.Env{ + status: 200, + body: user, + headers: [{"content-type", "application/activity+json"}] + } + %{method: :get, url: "https://example.com/users/lain/collections/featured"} -> %Tesla.Env{ status: 200, @@ -782,6 +793,7 @@ test "mastodon pin/unpin", %{conn: conn} do |> String.replace("{{nickname}}", "lain") actor = "https://example.com/users/lain" + key_id = "#{actor}/main-key" sender = insert(:user, @@ -811,6 +823,15 @@ test "mastodon pin/unpin", %{conn: conn} do headers: [{"content-type", "application/activity+json"}] } + %{ + method: :get, + url: ^key_id + } -> + %Tesla.Env{ + status: 200, + body: user, + headers: [{"content-type", "application/activity+json"}] + } %{method: :get, url: "https://example.com/users/lain/collections/featured"} -> %Tesla.Env{ status: 200, @@ -907,6 +928,7 @@ test "it inserts an incoming activity into the database", %{conn: conn, data: da test "it accepts messages with to as string instead of array", %{conn: conn, data: data} do user = insert(:user) + |> with_signing_key() data = data @@ -952,6 +974,7 @@ test "it accepts messages with cc as string instead of array", %{conn: conn, dat test "it accepts messages with bcc as string instead of array", %{conn: conn, data: data} do user = insert(:user) + |> with_signing_key() data = data @@ -1273,7 +1296,7 @@ test "forwarded report from mastodon", %{conn: conn} do conn |> assign(:valid_signature, true) - |> put_req_header("signature", "keyId=\"#{remote_actor}/main-key\"") + |> put_req_header("signature", "keyId=\"#{remote_actor}#main-key\"") |> put_req_header("content-type", "application/activity+json") |> post("/users/#{reported_user.nickname}/inbox", data) |> json_response(200) diff --git a/test/pleroma/web/activity_pub/publisher_test.exs b/test/pleroma/web/activity_pub/publisher_test.exs index 87930b7b1..726d85958 100644 --- a/test/pleroma/web/activity_pub/publisher_test.exs +++ b/test/pleroma/web/activity_pub/publisher_test.exs @@ -141,6 +141,7 @@ test "publish to url with with different ports" do end) actor = insert(:user) + |> with_signing_key() assert {:ok, %{body: "port 42"}} = Publisher.publish_one(%{ @@ -166,6 +167,7 @@ test "publish to url with with different ports" do [:passthrough], [] do actor = insert(:user) + |> with_signing_key() inbox = "http://200.site/users/nick1/inbox" assert {:ok, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1}) @@ -177,6 +179,7 @@ test "publish to url with with different ports" do [:passthrough], [] do actor = insert(:user) + |> with_signing_key() inbox = "http://200.site/users/nick1/inbox" assert {:ok, _} = @@ -196,6 +199,7 @@ test "publish to url with with different ports" do [:passthrough], [] do actor = insert(:user) + |> with_signing_key() inbox = "http://200.site/users/nick1/inbox" assert {:ok, _} = @@ -215,6 +219,7 @@ test "publish to url with with different ports" do [:passthrough], [] do actor = insert(:user) + |> with_signing_key() inbox = "http://404.site/users/nick1/inbox" assert {:error, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1}) @@ -227,6 +232,7 @@ test "publish to url with with different ports" do [:passthrough], [] do actor = insert(:user) + |> with_signing_key() inbox = "http://connrefused.site/users/nick1/inbox" assert capture_log(fn -> @@ -242,6 +248,7 @@ test "publish to url with with different ports" do [:passthrough], [] do actor = insert(:user) + |> with_signing_key() inbox = "http://200.site/users/nick1/inbox" assert {:ok, _} = Publisher.publish_one(%{inbox: inbox, json: "{}", actor: actor, id: 1}) @@ -254,6 +261,7 @@ test "publish to url with with different ports" do [:passthrough], [] do actor = insert(:user) + |> with_signing_key() inbox = "http://connrefused.site/users/nick1/inbox" assert capture_log(fn -> @@ -295,6 +303,7 @@ test "publish to url with with different ports" do }) actor = insert(:user, follower_address: follower.ap_id) + |> with_signing_key() {:ok, follower, actor} = Pleroma.User.follow(follower, actor) {:ok, _another_follower, actor} = Pleroma.User.follow(another_follower, actor) @@ -366,6 +375,7 @@ test "publish to url with with different ports" do }) actor = insert(:user, follower_address: follower.ap_id) + |> with_signing_key() {:ok, follower, actor} = Pleroma.User.follow(follower, actor) actor = refresh_record(actor) diff --git a/test/pleroma/web/plugs/http_signature_plug_test.exs b/test/pleroma/web/plugs/http_signature_plug_test.exs index 7f623bfca..5236b519e 100644 --- a/test/pleroma/web/plugs/http_signature_plug_test.exs +++ b/test/pleroma/web/plugs/http_signature_plug_test.exs @@ -17,8 +17,8 @@ defmodule Pleroma.Web.Plugs.HTTPSignaturePlugTest do setup do user = :user - |> insert(%{ ap_id: "http://mastodon.example.org/users/admin" }) - |> with_signing_key(%{ key_id: "http://mastodon.example.org/users/admin#main-key" }) + |> insert(%{ap_id: "http://mastodon.example.org/users/admin"}) + |> with_signing_key(%{key_id: "http://mastodon.example.org/users/admin#main-key"}) {:ok, %{user: user}} end diff --git a/test/pleroma/web/plugs/mapped_signature_to_identity_plug_test.exs b/test/pleroma/web/plugs/mapped_signature_to_identity_plug_test.exs index 77b14bbd2..5789cd756 100644 --- a/test/pleroma/web/plugs/mapped_signature_to_identity_plug_test.exs +++ b/test/pleroma/web/plugs/mapped_signature_to_identity_plug_test.exs @@ -14,7 +14,11 @@ defmodule Pleroma.Web.Plugs.MappedSignatureToIdentityPlugTest do setup do mock(fn env -> apply(HttpRequestMock, :request, [env]) end) - user = insert(:user) + + user = + insert(:user) + |> with_signing_key() + {:ok, %{user: user}} end diff --git a/test/support/factory.ex b/test/support/factory.ex index 1f54c5b7a..54e5f91b7 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -96,9 +96,9 @@ def user_factory(attrs \\ %{}) do end def with_signing_key(%User{} = user, attrs \\ %{}) do - - signing_key = build(:signing_key, %{user: user, key_id: "#{user.ap_id}#main-key"}) - |> merge_attributes(attrs) + signing_key = + build(:signing_key, %{user: user, key_id: "#{user.ap_id}#main-key"}) + |> merge_attributes(attrs) insert(signing_key) %{user | signing_key: signing_key} @@ -111,8 +111,8 @@ def signing_key_factory(attrs \\ %{}) do %Pleroma.User.SigningKey{ user_id: user.id, - public_key: public_key, - private_key: pem, + public_key: attrs[:public_key] || public_key, + private_key: attrs[:private_key] || pem, key_id: attrs[:key_id] } end diff --git a/test/support/helpers.ex b/test/support/helpers.ex index 2dfff70a2..8d481b5f5 100644 --- a/test/support/helpers.ex +++ b/test/support/helpers.ex @@ -65,6 +65,7 @@ defmacro __using__(_opts) do clear_config: 1, clear_config: 2 ] + import Pleroma.Test.MatchingHelpers def time_travel(entity, seconds) do new_time = NaiveDateTime.add(entity.inserted_at, seconds) diff --git a/test/support/http_request_mock.ex b/test/support/http_request_mock.ex index ea06c4966..d14434333 100644 --- a/test/support/http_request_mock.ex +++ b/test/support/http_request_mock.ex @@ -424,6 +424,15 @@ def get("http://mastodon.example.org/users/admin", _, _, _) do }} end + def get("http://mastodon.example.org/users/admin/main-key", _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/tesla_mock/admin@mastdon.example.org.json"), + headers: activitypub_object_headers() + }} + end + def get( "http://mastodon.example.org/users/admin/statuses/99512778738411822/replies?min_id=99512778738411824&page=true", _, @@ -958,6 +967,15 @@ def get("https://mastodon.social/users/lambadalambda", _, _, _) do }} end + def get("https://mastodon.social/users/lambadalambda#main-key", _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/lambadalambda.json"), + headers: activitypub_object_headers() + }} + end + def get("https://mastodon.social/users/lambadalambda/collections/featured", _, _, _) do {:ok, %Tesla.Env{ @@ -1403,6 +1421,15 @@ def get("https://relay.mastodon.host/actor", _, _, _) do }} end + def get("https://relay.mastodon.host/actor#main-key", _, _, _) do + {:ok, + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/relay/relay.json"), + headers: activitypub_object_headers() + }} + end + def get("http://localhost:4001/", _, "", [{"accept", "text/html"}]) do {:ok, %Tesla.Env{status: 200, body: File.read!("test/fixtures/tesla_mock/7369654.html")}} end diff --git a/test/support/matching_helpers.ex b/test/support/matching_helpers.ex new file mode 100644 index 000000000..39963171f --- /dev/null +++ b/test/support/matching_helpers.ex @@ -0,0 +1,9 @@ +defmodule Pleroma.Test.MatchingHelpers do + import ExUnit.Assertions + @assoc_fields [ + :signing_key + ] + def assert_user_match(actor1, actor2) do + assert Ecto.reset_fields(actor1, @assoc_fields) == Ecto.reset_fields(actor2, @assoc_fields) + end +end