From 1a482a73c3b99f7fdc512b734dd746e9f9cd396d Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 25 Jul 2024 11:18:27 -0400 Subject: [PATCH 1/6] Fix Optimistic Inbox for failed signatures When signatures fail on incoming activities we put the job into Oban to be processed later instead of doing the user fetching and validation inline which is expensive and increases latency on the incoming POST request. Unfortunately we did not retain the :method, :request_path, and :query_string parameters from the conn so the signature validation and Oban Job would always fail. This was most obvious when Mastodon sends Deletes for users your server has never seen before. --- changelog.d/optimistic-inbox-sigs.fix | 1 + .../web/activity_pub/activity_pub_controller.ex | 11 +++++++++-- lib/pleroma/workers/receiver_worker.ex | 17 +++++++++++++++-- 3 files changed, 25 insertions(+), 4 deletions(-) create mode 100644 changelog.d/optimistic-inbox-sigs.fix diff --git a/changelog.d/optimistic-inbox-sigs.fix b/changelog.d/optimistic-inbox-sigs.fix new file mode 100644 index 000000000..53ffe6b5b --- /dev/null +++ b/changelog.d/optimistic-inbox-sigs.fix @@ -0,0 +1 @@ +Fix Optimistic Inbox for failed signatures diff --git a/lib/pleroma/web/activity_pub/activity_pub_controller.ex b/lib/pleroma/web/activity_pub/activity_pub_controller.ex index e6161455d..cdd054e1a 100644 --- a/lib/pleroma/web/activity_pub/activity_pub_controller.ex +++ b/lib/pleroma/web/activity_pub/activity_pub_controller.ex @@ -293,8 +293,15 @@ def inbox(%{assigns: %{valid_signature: true}} = conn, params) do json(conn, "ok") end - def inbox(%{assigns: %{valid_signature: false}, req_headers: req_headers} = conn, params) do - Federator.incoming_ap_doc(%{req_headers: req_headers, params: params}) + def inbox(%{assigns: %{valid_signature: false}} = conn, params) do + Federator.incoming_ap_doc(%{ + method: conn.method, + req_headers: conn.req_headers, + request_path: conn.request_path, + params: params, + query_string: conn.query_string + }) + json(conn, "ok") end diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index d01813c25..1a2881cd1 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -12,13 +12,26 @@ defmodule Pleroma.Workers.ReceiverWorker do @impl Oban.Worker def perform(%Job{ - args: %{"op" => "incoming_ap_doc", "req_headers" => req_headers, "params" => params} + args: %{ + "op" => "incoming_ap_doc", + "method" => method, + "params" => params, + "req_headers" => req_headers, + "request_path" => request_path, + "query_string" => query_string + } }) do # Oban's serialization converts our tuple headers to lists. # Revert it for the signature validation. req_headers = Enum.into(req_headers, [], &List.to_tuple(&1)) - conn_data = %{params: params, req_headers: req_headers} + conn_data = %{ + method: method, + params: params, + req_headers: req_headers, + request_path: request_path, + query_string: query_string + } with {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), From 1b9c887dbb8d87814f8d9cc11cfcbc8802348b22 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 25 Jul 2024 12:54:27 -0400 Subject: [PATCH 2/6] Extract validate_signature/2 from the HTTPSignaturePlug This logic only exists in the Plug, so attempting to validate the signature by calling the library function HTTPSignature.validate_conn/2 directly will never work because we do not attempt to construct the (request-target) and @request-target headers with both the commonly misinterpreted and correct implementation of this field. Therefore all attempts to validate a signature from an Oban Job will fail. --- config/test.exs | 3 +- lib/pleroma/signature.ex | 52 ++++++++++++++++++++ lib/pleroma/web/plugs/http_signature_plug.ex | 50 +------------------ lib/pleroma/workers/receiver_worker.ex | 2 +- 4 files changed, 56 insertions(+), 51 deletions(-) diff --git a/config/test.exs b/config/test.exs index 5d9541f43..8a5694054 100644 --- a/config/test.exs +++ b/config/test.exs @@ -158,8 +158,7 @@ config :pleroma, Pleroma.Web.Plugs.HTTPSecurityPlug, config_impl: Pleroma.StaticStubbedConfigMock config :pleroma, Pleroma.Web.Plugs.HTTPSignaturePlug, config_impl: Pleroma.StaticStubbedConfigMock -config :pleroma, Pleroma.Web.Plugs.HTTPSignaturePlug, - http_signatures_impl: Pleroma.StubbedHTTPSignaturesMock +config :pleroma, Pleroma.Signature, http_signatures_impl: Pleroma.StubbedHTTPSignaturesMock peer_module = if String.to_integer(System.otp_release()) >= 25 do diff --git a/lib/pleroma/signature.ex b/lib/pleroma/signature.ex index 900d40c4b..51dac2402 100644 --- a/lib/pleroma/signature.ex +++ b/lib/pleroma/signature.ex @@ -10,6 +10,14 @@ defmodule Pleroma.Signature do alias Pleroma.User alias Pleroma.Web.ActivityPub.ActivityPub + import Plug.Conn, only: [put_req_header: 3] + + @http_signatures_impl Application.compile_env( + :pleroma, + [__MODULE__, :http_signatures_impl], + HTTPSignatures + ) + @known_suffixes ["/publickey", "/main-key"] def key_id_to_actor_id(key_id) do @@ -85,4 +93,48 @@ def signed_date, do: signed_date(NaiveDateTime.utc_now()) def signed_date(%NaiveDateTime{} = date) do Timex.format!(date, "{WDshort}, {0D} {Mshort} {YYYY} {h24}:{m}:{s} GMT") end + + @spec validate_signature(map(), String.t()) :: boolean() + def validate_signature(conn, request_target) do + # Newer drafts for HTTP signatures now use @request-target instead of the + # old (request-target). We'll now support both for incoming signatures. + conn = + conn + |> put_req_header("(request-target)", request_target) + |> put_req_header("@request-target", request_target) + + @http_signatures_impl.validate_conn(conn) + end + + @spec validate_signature(map()) :: boolean() + def validate_signature(conn) do + # This (request-target) is non-standard, but many implementations do it + # this way due to a misinterpretation of + # https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures-06 + # "path" was interpreted as not having the query, though later examples + # show that it must be the absolute path + query. This behavior is kept to + # make sure most software (Pleroma itself, Mastodon, and probably others) + # do not break. + request_target = String.downcase("#{conn.method}") <> " #{conn.request_path}" + + # This is the proper way to build the @request-target, as expected by + # many HTTP signature libraries, clarified in the following draft: + # https://www.ietf.org/archive/id/draft-ietf-httpbis-message-signatures-11.html#section-2.2.6 + # It is the same as before, but containing the query part as well. + proper_target = request_target <> "?#{conn.query_string}" + + cond do + # Normal, non-standard behavior but expected by Pleroma and more. + validate_signature(conn, request_target) -> + true + + # Has query string and the previous one failed: let's try the standard. + conn.query_string != "" -> + validate_signature(conn, proper_target) + + # If there's no query string and signature fails, it's rotten. + true -> + false + end + end end diff --git a/lib/pleroma/web/plugs/http_signature_plug.ex b/lib/pleroma/web/plugs/http_signature_plug.ex index 6bf2dd432..67974599a 100644 --- a/lib/pleroma/web/plugs/http_signature_plug.ex +++ b/lib/pleroma/web/plugs/http_signature_plug.ex @@ -8,16 +8,12 @@ defmodule Pleroma.Web.Plugs.HTTPSignaturePlug do import Plug.Conn import Phoenix.Controller, only: [get_format: 1, text: 2] + alias Pleroma.Signature alias Pleroma.Web.ActivityPub.MRF require Logger @config_impl Application.compile_env(:pleroma, [__MODULE__, :config_impl], Pleroma.Config) - @http_signatures_impl Application.compile_env( - :pleroma, - [__MODULE__, :http_signatures_impl], - HTTPSignatures - ) def init(options) do options @@ -39,48 +35,6 @@ def call(conn, _opts) do end end - defp validate_signature(conn, request_target) do - # Newer drafts for HTTP signatures now use @request-target instead of the - # old (request-target). We'll now support both for incoming signatures. - conn = - conn - |> put_req_header("(request-target)", request_target) - |> put_req_header("@request-target", request_target) - - @http_signatures_impl.validate_conn(conn) - end - - defp validate_signature(conn) do - # This (request-target) is non-standard, but many implementations do it - # this way due to a misinterpretation of - # https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures-06 - # "path" was interpreted as not having the query, though later examples - # show that it must be the absolute path + query. This behavior is kept to - # make sure most software (Pleroma itself, Mastodon, and probably others) - # do not break. - request_target = String.downcase("#{conn.method}") <> " #{conn.request_path}" - - # This is the proper way to build the @request-target, as expected by - # many HTTP signature libraries, clarified in the following draft: - # https://www.ietf.org/archive/id/draft-ietf-httpbis-message-signatures-11.html#section-2.2.6 - # It is the same as before, but containing the query part as well. - proper_target = request_target <> "?#{conn.query_string}" - - cond do - # Normal, non-standard behavior but expected by Pleroma and more. - validate_signature(conn, request_target) -> - true - - # Has query string and the previous one failed: let's try the standard. - conn.query_string != "" -> - validate_signature(conn, proper_target) - - # If there's no query string and signature fails, it's rotten. - true -> - false - end - end - defp maybe_assign_valid_signature(conn) do if has_signature_header?(conn) do # we replace the digest header with the one we computed in DigestPlug @@ -90,7 +44,7 @@ defp maybe_assign_valid_signature(conn) do conn -> conn end - assign(conn, :valid_signature, validate_signature(conn)) + assign(conn, :valid_signature, Signature.validate_signature(conn)) else Logger.debug("No signature header!") conn diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 1a2881cd1..94624579e 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -35,7 +35,7 @@ def perform(%Job{ with {:ok, %User{} = _actor} <- User.get_or_fetch_by_ap_id(conn_data.params["actor"]), {:ok, _public_key} <- Signature.refetch_public_key(conn_data), - {:signature, true} <- {:signature, HTTPSignatures.validate_conn(conn_data)}, + {:signature, true} <- {:signature, Signature.validate_signature(conn_data)}, {:ok, res} <- Federator.perform(:incoming_ap_doc, params) do {:ok, res} else From a964368e31ddf8a22dd36bbf2aae51f7d91bbdaf Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 25 Jul 2024 14:33:51 -0400 Subject: [PATCH 3/6] Add test to fetch and validate an activity that originally failed signature --- lib/pleroma/web/federator.ex | 6 +- lib/pleroma/workers/receiver_worker.ex | 2 +- test/fixtures/bastianallgeier.json | 117 +++++++++++ test/fixtures/denniskoch.json | 112 ++++++++++ .../receiver_worker_signature_activity.json | 62 ++++++ test/pleroma/workers/receiver_worker_test.exs | 196 ++++++++++++++++++ 6 files changed, 492 insertions(+), 3 deletions(-) create mode 100644 test/fixtures/bastianallgeier.json create mode 100644 test/fixtures/denniskoch.json create mode 100644 test/fixtures/receiver_worker_signature_activity.json diff --git a/lib/pleroma/web/federator.ex b/lib/pleroma/web/federator.ex index 4b30fd21d..3d3101d61 100644 --- a/lib/pleroma/web/federator.ex +++ b/lib/pleroma/web/federator.ex @@ -35,10 +35,12 @@ def allowed_thread_distance?(distance) do end # Client API - def incoming_ap_doc(%{params: params, req_headers: req_headers}) do + def incoming_ap_doc(%{params: _params, req_headers: _req_headers} = args) do + job_args = Enum.into(args, %{}, fn {k, v} -> {Atom.to_string(k), v} end) + ReceiverWorker.enqueue( "incoming_ap_doc", - %{"req_headers" => req_headers, "params" => params, "timeout" => :timer.seconds(20)}, + Map.put(job_args, "timeout", :timer.seconds(20)), priority: 2 ) end diff --git a/lib/pleroma/workers/receiver_worker.ex b/lib/pleroma/workers/receiver_worker.ex index 94624579e..fd5c13fca 100644 --- a/lib/pleroma/workers/receiver_worker.ex +++ b/lib/pleroma/workers/receiver_worker.ex @@ -25,7 +25,7 @@ def perform(%Job{ # Revert it for the signature validation. req_headers = Enum.into(req_headers, [], &List.to_tuple(&1)) - conn_data = %{ + conn_data = %Plug.Conn{ method: method, params: params, req_headers: req_headers, diff --git a/test/fixtures/bastianallgeier.json b/test/fixtures/bastianallgeier.json new file mode 100644 index 000000000..6b47e7db9 --- /dev/null +++ b/test/fixtures/bastianallgeier.json @@ -0,0 +1,117 @@ +{ + "@context": [ + "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1", + { + "Curve25519Key": "toot:Curve25519Key", + "Device": "toot:Device", + "Ed25519Key": "toot:Ed25519Key", + "Ed25519Signature": "toot:Ed25519Signature", + "EncryptedMessage": "toot:EncryptedMessage", + "PropertyValue": "schema:PropertyValue", + "alsoKnownAs": { + "@id": "as:alsoKnownAs", + "@type": "@id" + }, + "cipherText": "toot:cipherText", + "claim": { + "@id": "toot:claim", + "@type": "@id" + }, + "deviceId": "toot:deviceId", + "devices": { + "@id": "toot:devices", + "@type": "@id" + }, + "discoverable": "toot:discoverable", + "featured": { + "@id": "toot:featured", + "@type": "@id" + }, + "featuredTags": { + "@id": "toot:featuredTags", + "@type": "@id" + }, + "fingerprintKey": { + "@id": "toot:fingerprintKey", + "@type": "@id" + }, + "focalPoint": { + "@container": "@list", + "@id": "toot:focalPoint" + }, + "identityKey": { + "@id": "toot:identityKey", + "@type": "@id" + }, + "indexable": "toot:indexable", + "manuallyApprovesFollowers": "as:manuallyApprovesFollowers", + "memorial": "toot:memorial", + "messageFranking": "toot:messageFranking", + "messageType": "toot:messageType", + "movedTo": { + "@id": "as:movedTo", + "@type": "@id" + }, + "publicKeyBase64": "toot:publicKeyBase64", + "schema": "http://schema.org#", + "suspended": "toot:suspended", + "toot": "http://joinmastodon.org/ns#", + "value": "schema:value" + } + ], + "attachment": [ + { + "name": "Website", + "type": "PropertyValue", + "value": "https://bastianallgeier.com" + }, + { + "name": "Project", + "type": "PropertyValue", + "value": "https://getkirby.com" + }, + { + "name": "Github", + "type": "PropertyValue", + "value": "https://github.com/bastianallgeier" + } + ], + "devices": "https://mastodon.social/users/bastianallgeier/collections/devices", + "discoverable": true, + "endpoints": { + "sharedInbox": "https://mastodon.social/inbox" + }, + "featured": "https://mastodon.social/users/bastianallgeier/collections/featured", + "featuredTags": "https://mastodon.social/users/bastianallgeier/collections/tags", + "followers": "https://mastodon.social/users/bastianallgeier/followers", + "following": "https://mastodon.social/users/bastianallgeier/following", + "icon": { + "mediaType": "image/jpeg", + "type": "Image", + "url": "https://files.mastodon.social/accounts/avatars/000/007/393/original/0180a20079617c71.jpg" + }, + "id": "https://mastodon.social/users/bastianallgeier", + "image": { + "mediaType": "image/jpeg", + "type": "Image", + "url": "https://files.mastodon.social/accounts/headers/000/007/393/original/13d644ab46d50478.jpeg" + }, + "inbox": "https://mastodon.social/users/bastianallgeier/inbox", + "indexable": false, + "manuallyApprovesFollowers": false, + "memorial": false, + "name": "Bastian Allgeier", + "outbox": "https://mastodon.social/users/bastianallgeier/outbox", + "preferredUsername": "bastianallgeier", + "publicKey": { + "id": "https://mastodon.social/users/bastianallgeier#main-key", + "owner": "https://mastodon.social/users/bastianallgeier", + "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA3fz+hpgVztO9z6HUhyzv\nwP++ERBBoIwSLKf1TyIM8bvzGFm2YXaO5uxu1HvumYFTYc3ACr3q4j8VUb7NMxkQ\nlzu4QwPjOFJ43O+fY+HSPORXEDW5fXDGC5DGpox4+i08LxRmx7L6YPRUSUuPN8nI\nWyq1Qsq1zOQrNY/rohMXkBdSXxqC3yIRqvtLt4otCgay/5tMogJWkkS6ZKyFhb9z\nwVVy1fsbV10c9C+SHy4NH26CKaTtpTYLRBMjhTCS8bX8iDSjGIf2aZgYs1ir7gEz\n9wf5CvLiENmVWGwm64t6KSEAkA4NJ1hzgHUZPCjPHZE2SmhO/oHaxokTzqtbbENJ\n1QIDAQAB\n-----END PUBLIC KEY-----\n" + }, + "published": "2016-11-01T00:00:00Z", + "summary": "

Designer & developer. Creator of Kirby CMS

", + "tag": [], + "type": "Person", + "url": "https://mastodon.social/@bastianallgeier" +} diff --git a/test/fixtures/denniskoch.json b/test/fixtures/denniskoch.json new file mode 100644 index 000000000..7aa4de508 --- /dev/null +++ b/test/fixtures/denniskoch.json @@ -0,0 +1,112 @@ +{ + "@context": [ + "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1", + { + "Curve25519Key": "toot:Curve25519Key", + "Device": "toot:Device", + "Ed25519Key": "toot:Ed25519Key", + "Ed25519Signature": "toot:Ed25519Signature", + "EncryptedMessage": "toot:EncryptedMessage", + "PropertyValue": "schema:PropertyValue", + "alsoKnownAs": { + "@id": "as:alsoKnownAs", + "@type": "@id" + }, + "cipherText": "toot:cipherText", + "claim": { + "@id": "toot:claim", + "@type": "@id" + }, + "deviceId": "toot:deviceId", + "devices": { + "@id": "toot:devices", + "@type": "@id" + }, + "discoverable": "toot:discoverable", + "featured": { + "@id": "toot:featured", + "@type": "@id" + }, + "featuredTags": { + "@id": "toot:featuredTags", + "@type": "@id" + }, + "fingerprintKey": { + "@id": "toot:fingerprintKey", + "@type": "@id" + }, + "focalPoint": { + "@container": "@list", + "@id": "toot:focalPoint" + }, + "identityKey": { + "@id": "toot:identityKey", + "@type": "@id" + }, + "indexable": "toot:indexable", + "manuallyApprovesFollowers": "as:manuallyApprovesFollowers", + "memorial": "toot:memorial", + "messageFranking": "toot:messageFranking", + "messageType": "toot:messageType", + "movedTo": { + "@id": "as:movedTo", + "@type": "@id" + }, + "publicKeyBase64": "toot:publicKeyBase64", + "schema": "http://schema.org#", + "suspended": "toot:suspended", + "toot": "http://joinmastodon.org/ns#", + "value": "schema:value" + } + ], + "attachment": [ + { + "name": "GitHub", + "type": "PropertyValue", + "value": "https://github.com/pxlrbt/" + }, + { + "name": "Discord", + "type": "PropertyValue", + "value": "pxlrbt#6029" + } + ], + "devices": "https://phpc.social/users/denniskoch/collections/devices", + "discoverable": true, + "endpoints": { + "sharedInbox": "https://phpc.social/inbox" + }, + "featured": "https://phpc.social/users/denniskoch/collections/featured", + "featuredTags": "https://phpc.social/users/denniskoch/collections/tags", + "followers": "https://phpc.social/users/denniskoch/followers", + "following": "https://phpc.social/users/denniskoch/following", + "icon": { + "mediaType": "image/jpeg", + "type": "Image", + "url": "https://media.phpc.social/accounts/avatars/109/364/097/179/042/485/original/6e770c7b3f5ef72d.jpg" + }, + "id": "https://phpc.social/users/denniskoch", + "image": { + "mediaType": "image/jpeg", + "type": "Image", + "url": "https://media.phpc.social/accounts/headers/109/364/097/179/042/485/original/709da24705260c04.jpg" + }, + "inbox": "https://phpc.social/users/denniskoch/inbox", + "indexable": true, + "manuallyApprovesFollowers": false, + "memorial": false, + "name": "Dennis Koch", + "outbox": "https://phpc.social/users/denniskoch/outbox", + "preferredUsername": "denniskoch", + "publicKey": { + "id": "https://phpc.social/users/denniskoch#main-key", + "owner": "https://phpc.social/users/denniskoch", + "publicKeyPem": "-----BEGIN PUBLIC KEY-----\nMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA4dmcSlqLj18gPvuslkmt\nQTniZ8ybO4pgvMvPLYtBuTBUjo49vJ/8Sw6jB5zcKb1haqIdny7Rv/vY3kCdCXcP\nloh1I+jthEgqLT8JpZWGwLGwg9piFhrMGADmt3N8du7HfglzuZ8LlVpnZ8feCw7I\nS2ua/ZCxE47mI45Z3ed2kkFYKWopWWqFn2lan/1OyHrcFKtCvaVjRdvo0UUt2tgl\nvyJI4+zN8FnrCbsMtcbI5nSzfJIrOc4LeaGmLJh+0o2rwoOQZc2487XWbeyfhjsq\nPRBpYN7pfHWQDvzQIN075LHTf9zDFsm6+HqY7Zs5rYxr72rvcX7d9JcP6CasIosY\nqwIDAQAB\n-----END PUBLIC KEY-----\n" + }, + "published": "2022-11-18T00:00:00Z", + "summary": "

🧑‍💻 Full Stack Developer
🚀 Laravel, Filament, Livewire, Vue, Inertia
🌍 Germany

", + "tag": [], + "type": "Person", + "url": "https://phpc.social/@denniskoch" +} diff --git a/test/fixtures/receiver_worker_signature_activity.json b/test/fixtures/receiver_worker_signature_activity.json new file mode 100644 index 000000000..3c3fb3fd2 --- /dev/null +++ b/test/fixtures/receiver_worker_signature_activity.json @@ -0,0 +1,62 @@ +{ + "@context": [ + "https://www.w3.org/ns/activitystreams", + { + "atomUri": "ostatus:atomUri", + "blurhash": "toot:blurhash", + "conversation": "ostatus:conversation", + "focalPoint": { + "@container": "@list", + "@id": "toot:focalPoint" + }, + "inReplyToAtomUri": "ostatus:inReplyToAtomUri", + "ostatus": "http://ostatus.org#", + "sensitive": "as:sensitive", + "toot": "http://joinmastodon.org/ns#", + "votersCount": "toot:votersCount" + } + ], + "atomUri": "https://chaos.social/users/distantnative/statuses/109336635639931467", + "attachment": [ + { + "blurhash": "UAK1zS00OXIUxuMxIUM{?b-:-;W:Di?b%2M{", + "height": 960, + "mediaType": "image/jpeg", + "name": null, + "type": "Document", + "url": "https://assets.chaos.social/media_attachments/files/109/336/634/286/114/657/original/2e6122063d8bfb26.jpeg", + "width": 346 + } + ], + "attributedTo": "https://chaos.social/users/distantnative", + "cc": [ + "https://chaos.social/users/distantnative/followers" + ], + "content": "

Favorite piece of anthropology meta discourse.

", + "contentMap": { + "en": "

Favorite piece of anthropology meta discourse.

" + }, + "conversation": "tag:chaos.social,2022-11-13:objectId=71843781:objectType=Conversation", + "id": "https://chaos.social/users/distantnative/statuses/109336635639931467", + "inReplyTo": null, + "inReplyToAtomUri": null, + "published": "2022-11-13T13:04:20Z", + "replies": { + "first": { + "items": [], + "next": "https://chaos.social/users/distantnative/statuses/109336635639931467/replies?only_other_accounts=true&page=true", + "partOf": "https://chaos.social/users/distantnative/statuses/109336635639931467/replies", + "type": "CollectionPage" + }, + "id": "https://chaos.social/users/distantnative/statuses/109336635639931467/replies", + "type": "Collection" + }, + "sensitive": false, + "summary": null, + "tag": [], + "to": [ + "https://www.w3.org/ns/activitystreams#Public" + ], + "type": "Note", + "url": "https://chaos.social/@distantnative/109336635639931467" +} diff --git a/test/pleroma/workers/receiver_worker_test.exs b/test/pleroma/workers/receiver_worker_test.exs index 2b8bd3c40..33be91085 100644 --- a/test/pleroma/workers/receiver_worker_test.exs +++ b/test/pleroma/workers/receiver_worker_test.exs @@ -9,6 +9,7 @@ defmodule Pleroma.Workers.ReceiverWorkerTest do import Mock import Pleroma.Factory + alias Pleroma.Web.Federator alias Pleroma.Workers.ReceiverWorker test "it does not retry MRF reject" do @@ -49,4 +50,199 @@ test "it does not retry duplicates" do args: %{"op" => "incoming_ap_doc", "params" => params} }) end + + test "it can validate the signature" do + Tesla.Mock.mock(fn + %{url: "https://mastodon.social/users/bastianallgeier"} -> + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/bastianallgeier.json"), + headers: [{"content-type", "application/activity+json"}] + } + + %{url: "https://mastodon.social/users/bastianallgeier/collections/featured"} -> + %Tesla.Env{ + status: 200, + headers: [{"content-type", "application/activity+json"}], + body: + File.read!("test/fixtures/users_mock/masto_featured.json") + |> String.replace("{{domain}}", "mastodon.social") + |> String.replace("{{nickname}}", "bastianallgeier") + } + + %{url: "https://phpc.social/users/denniskoch"} -> + %Tesla.Env{ + status: 200, + body: File.read!("test/fixtures/denniskoch.json"), + headers: [{"content-type", "application/activity+json"}] + } + + %{url: "https://phpc.social/users/denniskoch/collections/featured"} -> + %Tesla.Env{ + status: 200, + headers: [{"content-type", "application/activity+json"}], + body: + File.read!("test/fixtures/users_mock/masto_featured.json") + |> String.replace("{{domain}}", "phpc.social") + |> String.replace("{{nickname}}", "denniskoch") + } + + %{url: "https://mastodon.social/users/bastianallgeier/statuses/112846516276907281"} -> + %Tesla.Env{ + status: 200, + headers: [{"content-type", "application/activity+json"}], + body: File.read!("test/fixtures/receiver_worker_signature_activity.json") + } + end) + + params = %{ + "@context" => [ + "https://www.w3.org/ns/activitystreams", + "https://w3id.org/security/v1", + %{ + "claim" => %{"@id" => "toot:claim", "@type" => "@id"}, + "memorial" => "toot:memorial", + "atomUri" => "ostatus:atomUri", + "manuallyApprovesFollowers" => "as:manuallyApprovesFollowers", + "blurhash" => "toot:blurhash", + "ostatus" => "http://ostatus.org#", + "discoverable" => "toot:discoverable", + "focalPoint" => %{"@container" => "@list", "@id" => "toot:focalPoint"}, + "votersCount" => "toot:votersCount", + "Hashtag" => "as:Hashtag", + "Emoji" => "toot:Emoji", + "alsoKnownAs" => %{"@id" => "as:alsoKnownAs", "@type" => "@id"}, + "sensitive" => "as:sensitive", + "movedTo" => %{"@id" => "as:movedTo", "@type" => "@id"}, + "inReplyToAtomUri" => "ostatus:inReplyToAtomUri", + "conversation" => "ostatus:conversation", + "Device" => "toot:Device", + "schema" => "http://schema.org#", + "toot" => "http://joinmastodon.org/ns#", + "cipherText" => "toot:cipherText", + "suspended" => "toot:suspended", + "messageType" => "toot:messageType", + "featuredTags" => %{"@id" => "toot:featuredTags", "@type" => "@id"}, + "Curve25519Key" => "toot:Curve25519Key", + "deviceId" => "toot:deviceId", + "Ed25519Signature" => "toot:Ed25519Signature", + "featured" => %{"@id" => "toot:featured", "@type" => "@id"}, + "devices" => %{"@id" => "toot:devices", "@type" => "@id"}, + "value" => "schema:value", + "PropertyValue" => "schema:PropertyValue", + "messageFranking" => "toot:messageFranking", + "publicKeyBase64" => "toot:publicKeyBase64", + "identityKey" => %{"@id" => "toot:identityKey", "@type" => "@id"}, + "Ed25519Key" => "toot:Ed25519Key", + "indexable" => "toot:indexable", + "EncryptedMessage" => "toot:EncryptedMessage", + "fingerprintKey" => %{"@id" => "toot:fingerprintKey", "@type" => "@id"} + } + ], + "actor" => "https://phpc.social/users/denniskoch", + "cc" => [ + "https://phpc.social/users/denniskoch/followers", + "https://mastodon.social/users/bastianallgeier", + "https://chaos.social/users/distantnative", + "https://fosstodon.org/users/kev" + ], + "id" => "https://phpc.social/users/denniskoch/statuses/112847382711461301/activity", + "object" => %{ + "atomUri" => "https://phpc.social/users/denniskoch/statuses/112847382711461301", + "attachment" => [], + "attributedTo" => "https://phpc.social/users/denniskoch", + "cc" => [ + "https://phpc.social/users/denniskoch/followers", + "https://mastodon.social/users/bastianallgeier", + "https://chaos.social/users/distantnative", + "https://fosstodon.org/users/kev" + ], + "content" => + "

@bastianallgeier @distantnative @kev Another main argument: Discord is popular. Many people have an account, so you can just join an server quickly. Also you know the app and how to get around.

", + "contentMap" => %{ + "en" => + "

@bastianallgeier @distantnative @kev Another main argument: Discord is popular. Many people have an account, so you can just join an server quickly. Also you know the app and how to get around.

" + }, + "conversation" => + "tag:mastodon.social,2024-07-25:objectId=760068442:objectType=Conversation", + "id" => "https://phpc.social/users/denniskoch/statuses/112847382711461301", + "inReplyTo" => + "https://mastodon.social/users/bastianallgeier/statuses/112846516276907281", + "inReplyToAtomUri" => + "https://mastodon.social/users/bastianallgeier/statuses/112846516276907281", + "published" => "2024-07-25T13:33:29Z", + "replies" => %{ + "first" => %{ + "items" => [], + "next" => + "https://phpc.social/users/denniskoch/statuses/112847382711461301/replies?only_other_accounts=true&page=true", + "partOf" => + "https://phpc.social/users/denniskoch/statuses/112847382711461301/replies", + "type" => "CollectionPage" + }, + "id" => "https://phpc.social/users/denniskoch/statuses/112847382711461301/replies", + "type" => "Collection" + }, + "sensitive" => false, + "tag" => [ + %{ + "href" => "https://mastodon.social/users/bastianallgeier", + "name" => "@bastianallgeier@mastodon.social", + "type" => "Mention" + }, + %{ + "href" => "https://chaos.social/users/distantnative", + "name" => "@distantnative@chaos.social", + "type" => "Mention" + }, + %{ + "href" => "https://fosstodon.org/users/kev", + "name" => "@kev@fosstodon.org", + "type" => "Mention" + } + ], + "to" => ["https://www.w3.org/ns/activitystreams#Public"], + "type" => "Note", + "url" => "https://phpc.social/@denniskoch/112847382711461301" + }, + "published" => "2024-07-25T13:33:29Z", + "signature" => %{ + "created" => "2024-07-25T13:33:29Z", + "creator" => "https://phpc.social/users/denniskoch#main-key", + "signatureValue" => + "slz9BKJzd2n1S44wdXGOU+bV/wsskdgAaUpwxj8R16mYOL8+DTpE6VnfSKoZGsBBJT8uG5gnVfVEz1YsTUYtymeUgLMh7cvd8VnJnZPS+oixbmBRVky/Myf91TEgQQE7G4vDmTdB4ii54hZrHcOOYYf5FKPNRSkMXboKA6LMqNtekhbI+JTUJYIB02WBBK6PUyo15f6B1RJ6HGWVgud9NE0y1EZXfrkqUt682p8/9D49ORf7AwjXUJibKic2RbPvhEBj70qUGfBm4vvgdWhSUn1IG46xh+U0+NrTSUED82j1ZVOeua/2k/igkGs8cSBkY35quXTkPz6gbqCCH66CuA==", + "type" => "RsaSignature2017" + }, + "to" => ["https://www.w3.org/ns/activitystreams#Public"], + "type" => "Create" + } + + req_headers = [ + ["accept-encoding", "gzip"], + ["content-length", "5184"], + ["content-type", "application/activity+json"], + ["date", "Thu, 25 Jul 2024 13:33:31 GMT"], + ["digest", "SHA-256=ouge/6HP2/QryG6F3JNtZ6vzs/hSwMk67xdxe87eH7A="], + ["host", "bikeshed.party"], + [ + "signature", + "keyId=\"https://mastodon.social/users/bastianallgeier#main-key\",algorithm=\"rsa-sha256\",headers=\"(request-target) host date digest content-type\",signature=\"ymE3vn5Iw50N6ukSp8oIuXJB5SBjGAGjBasdTDvn+ahZIzq2SIJfmVCsIIzyqIROnhWyQoTbavTclVojEqdaeOx+Ejz2wBnRBmhz5oemJLk4RnnCH0lwMWyzeY98YAvxi9Rq57Gojuv/1lBqyGa+rDzynyJpAMyFk17XIZpjMKuTNMCbjMDy76ILHqArykAIL/v1zxkgwxY/+ELzxqMpNqtZ+kQ29znNMUBB3eVZ/mNAHAz6o33Y9VKxM2jw+08vtuIZOusXyiHbRiaj2g5HtN2WBUw1MzzfRfHF2/yy7rcipobeoyk5RvP5SyHV3WrIeZ3iyoNfmv33y8fxllF0EA==\"" + ], + [ + "user-agent", + "http.rb/5.2.0 (Mastodon/4.3.0-nightly.2024-07-25; +https://mastodon.social/)" + ] + ] + + {:ok, oban_job} = + Federator.incoming_ap_doc(%{ + method: "POST", + req_headers: req_headers, + request_path: "/inbox", + params: params, + query_string: "" + }) + + assert {:ok, %Pleroma.Activity{}} = ReceiverWorker.perform(oban_job) + end end From 84b15ac1119396eeb9827fc5242489a4f5cb820b Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 25 Jul 2024 16:18:31 -0400 Subject: [PATCH 4/6] Improve specs and matching --- lib/pleroma/signature.ex | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/pleroma/signature.ex b/lib/pleroma/signature.ex index 51dac2402..0f3362ebe 100644 --- a/lib/pleroma/signature.ex +++ b/lib/pleroma/signature.ex @@ -94,8 +94,8 @@ def signed_date(%NaiveDateTime{} = date) do Timex.format!(date, "{WDshort}, {0D} {Mshort} {YYYY} {h24}:{m}:{s} GMT") end - @spec validate_signature(map(), String.t()) :: boolean() - def validate_signature(conn, request_target) do + @spec validate_signature(Plug.Conn.t(), String.t()) :: boolean() + def validate_signature(%Plug.Conn{} = conn, request_target) do # Newer drafts for HTTP signatures now use @request-target instead of the # old (request-target). We'll now support both for incoming signatures. conn = @@ -106,8 +106,8 @@ def validate_signature(conn, request_target) do @http_signatures_impl.validate_conn(conn) end - @spec validate_signature(map()) :: boolean() - def validate_signature(conn) do + @spec validate_signature(Plug.Conn.t()) :: boolean() + def validate_signature(%Plug.Conn{} = conn) do # This (request-target) is non-standard, but many implementations do it # this way due to a misinterpretation of # https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures-06 From c19d55cabb4932b9786fa8a4571d7b92e3925e00 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 25 Jul 2024 16:18:45 -0400 Subject: [PATCH 5/6] Safer string concatenation --- lib/pleroma/signature.ex | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/pleroma/signature.ex b/lib/pleroma/signature.ex index 0f3362ebe..195513478 100644 --- a/lib/pleroma/signature.ex +++ b/lib/pleroma/signature.ex @@ -115,13 +115,13 @@ def validate_signature(%Plug.Conn{} = conn) do # show that it must be the absolute path + query. This behavior is kept to # make sure most software (Pleroma itself, Mastodon, and probably others) # do not break. - request_target = String.downcase("#{conn.method}") <> " #{conn.request_path}" + request_target = Enum.join([String.downcase(conn.method), conn.request_path], " ") # This is the proper way to build the @request-target, as expected by # many HTTP signature libraries, clarified in the following draft: # https://www.ietf.org/archive/id/draft-ietf-httpbis-message-signatures-11.html#section-2.2.6 # It is the same as before, but containing the query part as well. - proper_target = request_target <> "?#{conn.query_string}" + proper_target = Enum.join([request_target, "?", conn.query_string], "") cond do # Normal, non-standard behavior but expected by Pleroma and more. From 21cf321f7454edc1a7e00436f66f470edc222775 Mon Sep 17 00:00:00 2001 From: Mark Felder Date: Thu, 25 Jul 2024 16:36:34 -0400 Subject: [PATCH 6/6] Quiet Dialyzer It is angry we are making a fake %Plug.Conn{} to pass through Signature.validate_signature/1. We can work around it by making the code support a map, but then we lose the benefit of being able to use put_req_header/3 --- .dialyzer_ignore.exs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs index 865e7d782..692cac923 100644 --- a/.dialyzer_ignore.exs +++ b/.dialyzer_ignore.exs @@ -2,5 +2,8 @@ {"lib/cachex.ex", "Unknown type: Spec.cache/0."}, {"lib/pleroma/web/plugs/rate_limiter.ex", "The pattern can never match the type {:commit, _} | {:ignore, _}."}, {"lib/pleroma/web/plugs/rate_limiter.ex", "Function get_scale/2 will never be called."}, -{"lib/pleroma/web/plugs/rate_limiter.ex", "Function initialize_buckets!/1 will never be called."} +{"lib/pleroma/web/plugs/rate_limiter.ex", "Function initialize_buckets!/1 will never be called."}, +{"lib/pleroma/workers/receiver_worker.ex", :call}, +{"lib/pleroma/workers/receiver_worker.ex", :pattern_match}, +{"lib/pleroma/workers/receiver_worker.ex", :pattern_match_cov}, ]