From 632e3844b9c2b0dd1f67567b9c4411bf6b022a2e Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Mon, 10 Apr 2023 17:32:49 +1000 Subject: [PATCH 01/15] Don't assume user id is key id minus fragment Fixes #2801 Related to #2794 It is legitimate to use any url for the user's key id. We have been assuming this id is the user id plus a fragment (#key-id) but this is not always the case, notably in the case of GoToSocial it is at /key-id. This commit instead checks the remote user's information to see if the key id listed matches the key id of the message allegedly received from them. Whilst troubleshooting this it also became apparent that there is a mismatch between Bookwyrm users' keyId and the KeyId we claim to be using in signed requests (there is a forward slash missing). Since everything after the slash is a fragment, this usually slips through but we should be consistent so I updated that. --- bookwyrm/activitypub/__init__.py | 2 +- bookwyrm/signatures.py | 3 ++- bookwyrm/views/inbox.py | 10 ++++------ 3 files changed, 7 insertions(+), 8 deletions(-) diff --git a/bookwyrm/activitypub/__init__.py b/bookwyrm/activitypub/__init__.py index 05ca44476..8449b4f50 100644 --- a/bookwyrm/activitypub/__init__.py +++ b/bookwyrm/activitypub/__init__.py @@ -4,7 +4,7 @@ import sys from .base_activity import ActivityEncoder, Signature, naive_parse from .base_activity import Link, Mention, Hashtag -from .base_activity import ActivitySerializerError, resolve_remote_id +from .base_activity import ActivitySerializerError, resolve_remote_id, get_activitypub_data from .image import Document, Image from .note import Note, GeneratedNote, Article, Comment, Quotation from .note import Review, Rating diff --git a/bookwyrm/signatures.py b/bookwyrm/signatures.py index 3102f8da2..10d500f1c 100644 --- a/bookwyrm/signatures.py +++ b/bookwyrm/signatures.py @@ -38,8 +38,9 @@ def make_signature(method, sender, destination, date, digest=None): message_to_sign = "\n".join(signature_headers) signer = pkcs1_15.new(RSA.import_key(sender.key_pair.private_key)) signed_message = signer.sign(SHA256.new(message_to_sign.encode("utf8"))) + # TODO: this should be /#main-key to match our user key ids, even though that was probably a mistake in hindsight. signature = { - "keyId": f"{sender.remote_id}#main-key", + "keyId": f"{sender.remote_id}/#main-key", "algorithm": "rsa-sha256", "headers": headers, "signature": b64encode(signed_message).decode("utf8"), diff --git a/bookwyrm/views/inbox.py b/bookwyrm/views/inbox.py index 89e644db8..867b2b971 100644 --- a/bookwyrm/views/inbox.py +++ b/bookwyrm/views/inbox.py @@ -130,15 +130,13 @@ def has_valid_signature(request, activity): """verify incoming signature""" try: signature = Signature.parse(request) - - key_actor = urldefrag(signature.key_id).url - if key_actor != activity.get("actor"): - raise ValueError("Wrong actor created signature.") - - remote_user = activitypub.resolve_remote_id(key_actor, model=models.User) + remote_user = activitypub.resolve_remote_id(activity.get("actor"), model=models.User) if not remote_user: return False + if signature.key_id != remote_user.key_pair.remote_id: + raise ValueError("Wrong actor created signature.") + try: signature.verify(remote_user.key_pair.public_key, request) except ValueError: From 49758f2383a675ebedac29fd21e8d43b2d13751d Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Mon, 10 Apr 2023 17:50:25 +1000 Subject: [PATCH 02/15] formatting fixes --- bookwyrm/activitypub/__init__.py | 2 +- bookwyrm/views/inbox.py | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/bookwyrm/activitypub/__init__.py b/bookwyrm/activitypub/__init__.py index 8449b4f50..05ca44476 100644 --- a/bookwyrm/activitypub/__init__.py +++ b/bookwyrm/activitypub/__init__.py @@ -4,7 +4,7 @@ import sys from .base_activity import ActivityEncoder, Signature, naive_parse from .base_activity import Link, Mention, Hashtag -from .base_activity import ActivitySerializerError, resolve_remote_id, get_activitypub_data +from .base_activity import ActivitySerializerError, resolve_remote_id from .image import Document, Image from .note import Note, GeneratedNote, Article, Comment, Quotation from .note import Review, Rating diff --git a/bookwyrm/views/inbox.py b/bookwyrm/views/inbox.py index 867b2b971..da32ebdb0 100644 --- a/bookwyrm/views/inbox.py +++ b/bookwyrm/views/inbox.py @@ -130,7 +130,9 @@ def has_valid_signature(request, activity): """verify incoming signature""" try: signature = Signature.parse(request) - remote_user = activitypub.resolve_remote_id(activity.get("actor"), model=models.User) + remote_user = activitypub.resolve_remote_id( + activity.get("actor"), model=models.User + ) if not remote_user: return False From e112718d2d08369581dc14f0f12fef1525d0be74 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Mon, 10 Apr 2023 20:34:45 +1000 Subject: [PATCH 03/15] clean up troubleshooting comment --- bookwyrm/signatures.py | 1 - 1 file changed, 1 deletion(-) diff --git a/bookwyrm/signatures.py b/bookwyrm/signatures.py index 10d500f1c..3dfde4cb1 100644 --- a/bookwyrm/signatures.py +++ b/bookwyrm/signatures.py @@ -38,7 +38,6 @@ def make_signature(method, sender, destination, date, digest=None): message_to_sign = "\n".join(signature_headers) signer = pkcs1_15.new(RSA.import_key(sender.key_pair.private_key)) signed_message = signer.sign(SHA256.new(message_to_sign.encode("utf8"))) - # TODO: this should be /#main-key to match our user key ids, even though that was probably a mistake in hindsight. signature = { "keyId": f"{sender.remote_id}/#main-key", "algorithm": "rsa-sha256", From ef85394a1633fb599df2ec31b29707c26042ce30 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Mon, 10 Apr 2023 20:35:13 +1000 Subject: [PATCH 04/15] Allow for tag value to be object Previously the 'tag' value in an activitypub object was assumed to be a List (array). Some AP software sends 'tag' as a Dict (object) if there is only a single tag value. It's somewhat debatable whether this is spec compliant but we should aim to be robust. This commit puts an individual mention tag inside a list if necessary. --- bookwyrm/models/status.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 1fcc9ee75..bd2a98f35 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -136,10 +136,16 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): # keep notes if they mention local users if activity.tag == MISSING or activity.tag is None: return True - tags = [l["href"] for l in activity.tag if l["type"] == "Mention"] + + tags = activity.tag if type(activity.tag) == list else [activity.tag] user_model = apps.get_model("bookwyrm.User", require_ready=True) for tag in tags: - if user_model.objects.filter(remote_id=tag, local=True).exists(): + if ( + tag["type"] == "Mention" + and user_model.objects.filter( + remote_id=tag["href"], local=True + ).exists() + ): # we found a mention of a known use boost return False return True From c9dcd4f7add416601464167f0d8a599ea44cff99 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Mon, 10 Apr 2023 20:38:20 +1000 Subject: [PATCH 05/15] Include initial '@' in mention tag name GoToSocial expects the 'name' value of a mention tag to have an initial '@' symbol. Mastodon doesn't seem to mind either way. --- bookwyrm/models/fields.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookwyrm/models/fields.py b/bookwyrm/models/fields.py index 6cfe4c10c..c11a24630 100644 --- a/bookwyrm/models/fields.py +++ b/bookwyrm/models/fields.py @@ -371,7 +371,7 @@ class TagField(ManyToManyField): tags.append( activitypub.Link( href=item.remote_id, - name=getattr(item, item.name_field), + name=f"@{getattr(item, item.name_field)}", type=activity_type, ) ) From 03f21b0f35609c07fedfc71e6b32979dbbda927f Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Tue, 11 Apr 2023 15:45:06 +1000 Subject: [PATCH 06/15] Use correct keyId with legacy fallback Bookwyrm keyIds are at `userpath/#main-key`, however when signing AP objects we have claimed in the headers that the keyId is at `userpath#main-key`. This is incorrect, and makes GoToSocial's strict checking break. Simply updating the signatures to use the correct KeyId breaks legacy Bookwyrm's signature checks, becuase it assumes that the keyId path is the same as the user path plus a fragment. This commit allows for either option, by sending the request a second time with the incorrect keyId if sending with the correct one causes an error. --- bookwyrm/models/activitypub_mixin.py | 13 ++++++++++++- bookwyrm/signatures.py | 6 ++++-- bookwyrm/views/inbox.py | 3 ++- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 83ca90b0a..83a7dd998 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -540,10 +540,11 @@ async def sign_and_send( digest = make_digest(data) + headers = { "Date": now, "Digest": digest, - "Signature": make_signature("post", sender, destination, now, digest), + "Signature": make_signature("post", sender, destination, now, digest, False), "Content-Type": "application/activity+json; charset=utf-8", "User-Agent": USER_AGENT, } @@ -554,6 +555,16 @@ async def sign_and_send( logger.exception( "Failed to send broadcast to %s: %s", destination, response.reason ) + logger.info("Trying again with legacy keyId") + # try with incorrect keyId to enable communication with legacy Bookwyrm servers + legacy_signature = make_signature("post", sender, destination, now, digest, True) + headers["Signature"] = legacy_signature + async with session.post(destination, data=data, headers=headers) as response: + if not response.ok: + logger.exception( + "Failed to send broadcast with legacy keyId to %s: %s", destination, response.reason + ) + return response except asyncio.TimeoutError: logger.info("Connection timed out for url: %s", destination) diff --git a/bookwyrm/signatures.py b/bookwyrm/signatures.py index 3dfde4cb1..2a1f2e72a 100644 --- a/bookwyrm/signatures.py +++ b/bookwyrm/signatures.py @@ -22,7 +22,7 @@ def create_key_pair(): return private_key, public_key -def make_signature(method, sender, destination, date, digest=None): +def make_signature(method, sender, destination, date, digest=None, use_legacy_key=False): """uses a private key to sign an outgoing message""" inbox_parts = urlparse(destination) signature_headers = [ @@ -38,8 +38,10 @@ def make_signature(method, sender, destination, date, digest=None): message_to_sign = "\n".join(signature_headers) signer = pkcs1_15.new(RSA.import_key(sender.key_pair.private_key)) signed_message = signer.sign(SHA256.new(message_to_sign.encode("utf8"))) + # For legacy reasons we need to use an incorrect keyId for older Bookwyrm versions + key_id = f"{sender.remote_id}#main-key" if use_legacy_key else f"{sender.remote_id}/#main-key" signature = { - "keyId": f"{sender.remote_id}/#main-key", + "keyId": key_id, "algorithm": "rsa-sha256", "headers": headers, "signature": b64encode(signed_message).decode("utf8"), diff --git a/bookwyrm/views/inbox.py b/bookwyrm/views/inbox.py index da32ebdb0..9eef6792f 100644 --- a/bookwyrm/views/inbox.py +++ b/bookwyrm/views/inbox.py @@ -137,7 +137,8 @@ def has_valid_signature(request, activity): return False if signature.key_id != remote_user.key_pair.remote_id: - raise ValueError("Wrong actor created signature.") + if signature.key_id != f"{remote_user.remote_id}#main-key": # legacy Bookwyrm + raise ValueError("Wrong actor created signature.") try: signature.verify(remote_user.key_pair.public_key, request) From 279fa3851b6d043410804b789954f5e42a049348 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Tue, 11 Apr 2023 16:49:11 +1000 Subject: [PATCH 07/15] add comment --- bookwyrm/models/status.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index bd2a98f35..cabf2cf8d 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -137,6 +137,8 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): if activity.tag == MISSING or activity.tag is None: return True + # BUG: this fixes the TypeError but we still don't get any notifs + # and DMs are dropped tags = activity.tag if type(activity.tag) == list else [activity.tag] user_model = apps.get_model("bookwyrm.User", require_ready=True) for tag in tags: From c450947eeee26727dd48a5dd63c42d6ed45bc889 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Tue, 11 Apr 2023 18:57:55 +1000 Subject: [PATCH 08/15] update comment to identify bug --- bookwyrm/models/status.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index cabf2cf8d..1561295a7 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -137,9 +137,9 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): if activity.tag == MISSING or activity.tag is None: return True - # BUG: this fixes the TypeError but we still don't get any notifs - # and DMs are dropped - tags = activity.tag if type(activity.tag) == list else [activity.tag] + # BUG: this fixes the TypeError but if there is only one user mentioned + # we still don't get any notifs and DMs are dropped. + tags = activity.tag if type(activity.tag) == list else [ activity.tag ] user_model = apps.get_model("bookwyrm.User", require_ready=True) for tag in tags: if ( From e3261c6b88c8683bd50f4be94541bae1a134c1eb Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Thu, 13 Apr 2023 13:21:05 +1000 Subject: [PATCH 09/15] fix incoming GTS mentions and DMs GoToSocial sends 'tag' values as a single object if there is only one user mentioned, rather than an array with an object inside it. This causes Bookwyrm to reject the tag since it comes through as a dict rather than a list. This commit fixes this at the point the incoming AP object is transformed so that "mention" tags are turned into a mention_user. --- bookwyrm/models/fields.py | 7 ++++++- bookwyrm/models/status.py | 7 +++---- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/bookwyrm/models/fields.py b/bookwyrm/models/fields.py index c11a24630..168270973 100644 --- a/bookwyrm/models/fields.py +++ b/bookwyrm/models/fields.py @@ -379,7 +379,12 @@ class TagField(ManyToManyField): def field_from_activity(self, value, allow_external_connections=True): if not isinstance(value, list): - return None + # GoToSocial DMs and single-user mentions are + # sent as objects, not as an array of objects + if isinstance(value, dict): + value = [value] + else: + return None items = [] for link_json in value: link = activitypub.Link(**link_json) diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index 1561295a7..a620f09b1 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -136,10 +136,9 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): # keep notes if they mention local users if activity.tag == MISSING or activity.tag is None: return True - - # BUG: this fixes the TypeError but if there is only one user mentioned - # we still don't get any notifs and DMs are dropped. - tags = activity.tag if type(activity.tag) == list else [ activity.tag ] + # GoToSocial sends single tags as objects + # not wrapped in a list + tags = activity.tag if isinstance(activity.tag, list) else [ activity.tag ] user_model = apps.get_model("bookwyrm.User", require_ready=True) for tag in tags: if ( From a6676718cbd64cc2d259e81cf7cff339f533743a Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Thu, 13 Apr 2023 13:27:51 +1000 Subject: [PATCH 10/15] formatting --- bookwyrm/models/activitypub_mixin.py | 17 +++++++++++------ bookwyrm/models/fields.py | 2 +- bookwyrm/models/status.py | 2 +- bookwyrm/signatures.py | 10 ++++++++-- bookwyrm/views/inbox.py | 4 +++- 5 files changed, 24 insertions(+), 11 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 83a7dd998..5e52576aa 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -540,7 +540,6 @@ async def sign_and_send( digest = make_digest(data) - headers = { "Date": now, "Digest": digest, @@ -557,14 +556,20 @@ async def sign_and_send( ) logger.info("Trying again with legacy keyId") # try with incorrect keyId to enable communication with legacy Bookwyrm servers - legacy_signature = make_signature("post", sender, destination, now, digest, True) + legacy_signature = make_signature( + "post", sender, destination, now, digest, True + ) headers["Signature"] = legacy_signature - async with session.post(destination, data=data, headers=headers) as response: + async with session.post( + destination, data=data, headers=headers + ) as response: if not response.ok: logger.exception( - "Failed to send broadcast with legacy keyId to %s: %s", destination, response.reason - ) - + "Failed to send broadcast with legacy keyId to %s: %s", + destination, + response.reason, + ) + return response except asyncio.TimeoutError: logger.info("Connection timed out for url: %s", destination) diff --git a/bookwyrm/models/fields.py b/bookwyrm/models/fields.py index 168270973..62dc8f0d9 100644 --- a/bookwyrm/models/fields.py +++ b/bookwyrm/models/fields.py @@ -383,7 +383,7 @@ class TagField(ManyToManyField): # sent as objects, not as an array of objects if isinstance(value, dict): value = [value] - else: + else: return None items = [] for link_json in value: diff --git a/bookwyrm/models/status.py b/bookwyrm/models/status.py index a620f09b1..2f1999bf2 100644 --- a/bookwyrm/models/status.py +++ b/bookwyrm/models/status.py @@ -138,7 +138,7 @@ class Status(OrderedCollectionPageMixin, BookWyrmModel): return True # GoToSocial sends single tags as objects # not wrapped in a list - tags = activity.tag if isinstance(activity.tag, list) else [ activity.tag ] + tags = activity.tag if isinstance(activity.tag, list) else [activity.tag] user_model = apps.get_model("bookwyrm.User", require_ready=True) for tag in tags: if ( diff --git a/bookwyrm/signatures.py b/bookwyrm/signatures.py index 2a1f2e72a..d3475edf6 100644 --- a/bookwyrm/signatures.py +++ b/bookwyrm/signatures.py @@ -22,7 +22,9 @@ def create_key_pair(): return private_key, public_key -def make_signature(method, sender, destination, date, digest=None, use_legacy_key=False): +def make_signature( + method, sender, destination, date, digest=None, use_legacy_key=False +): """uses a private key to sign an outgoing message""" inbox_parts = urlparse(destination) signature_headers = [ @@ -39,7 +41,11 @@ def make_signature(method, sender, destination, date, digest=None, use_legacy_ke signer = pkcs1_15.new(RSA.import_key(sender.key_pair.private_key)) signed_message = signer.sign(SHA256.new(message_to_sign.encode("utf8"))) # For legacy reasons we need to use an incorrect keyId for older Bookwyrm versions - key_id = f"{sender.remote_id}#main-key" if use_legacy_key else f"{sender.remote_id}/#main-key" + key_id = ( + f"{sender.remote_id}#main-key" + if use_legacy_key + else f"{sender.remote_id}/#main-key" + ) signature = { "keyId": key_id, "algorithm": "rsa-sha256", diff --git a/bookwyrm/views/inbox.py b/bookwyrm/views/inbox.py index 9eef6792f..5670f35b9 100644 --- a/bookwyrm/views/inbox.py +++ b/bookwyrm/views/inbox.py @@ -137,7 +137,9 @@ def has_valid_signature(request, activity): return False if signature.key_id != remote_user.key_pair.remote_id: - if signature.key_id != f"{remote_user.remote_id}#main-key": # legacy Bookwyrm + if ( + signature.key_id != f"{remote_user.remote_id}#main-key" + ): # legacy Bookwyrm raise ValueError("Wrong actor created signature.") try: From c7adb62831f1fac673645bba67369955ffd00a39 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Thu, 13 Apr 2023 19:48:20 +1000 Subject: [PATCH 11/15] make get_legacy_key more DRY --- bookwyrm/models/activitypub_mixin.py | 29 +++++++++++++--------------- 1 file changed, 13 insertions(+), 16 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 5e52576aa..d8103c9c3 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -529,7 +529,7 @@ async def async_broadcast(recipients: List[str], sender, data: str): async def sign_and_send( - session: aiohttp.ClientSession, sender, data: str, destination: str + session: aiohttp.ClientSession, sender, data: str, destination: str, **kwargs ): """Sign the messages and send them in an asynchronous bundle""" now = http_date() @@ -539,11 +539,14 @@ async def sign_and_send( raise ValueError("No private key found for sender") digest = make_digest(data) + signature = make_signature( + "post", sender, destination, now, digest, kwargs.get("use_legacy_key") + ) headers = { "Date": now, "Digest": digest, - "Signature": make_signature("post", sender, destination, now, digest, False), + "Signature": signature, "Content-Type": "application/activity+json; charset=utf-8", "User-Agent": USER_AGENT, } @@ -554,21 +557,15 @@ async def sign_and_send( logger.exception( "Failed to send broadcast to %s: %s", destination, response.reason ) - logger.info("Trying again with legacy keyId") - # try with incorrect keyId to enable communication with legacy Bookwyrm servers - legacy_signature = make_signature( - "post", sender, destination, now, digest, True - ) - headers["Signature"] = legacy_signature - async with session.post( - destination, data=data, headers=headers - ) as response: - if not response.ok: - logger.exception( - "Failed to send broadcast with legacy keyId to %s: %s", - destination, - response.reason, + if kwargs.get("use_legacy_key") is not True: + # try with incorrect keyId to enable communication with legacy Bookwyrm servers + logger.info("Trying again with legacy keyId header value") + + asyncio.ensure_future( + sign_and_send( + session, sender, data, destination, use_legacy_key=True ) + ) return response except asyncio.TimeoutError: From 56a062d01f58be9088de22774f09a17ab08f32b3 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Thu, 13 Apr 2023 20:21:35 +1000 Subject: [PATCH 12/15] pylint fixes --- bookwyrm/models/activitypub_mixin.py | 2 +- bookwyrm/signatures.py | 5 +++-- bookwyrm/views/inbox.py | 1 - 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index d8103c9c3..3e04fa408 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -540,7 +540,7 @@ async def sign_and_send( digest = make_digest(data) signature = make_signature( - "post", sender, destination, now, digest, kwargs.get("use_legacy_key") + "post", sender, destination, now, digest=digest, use_legacy_key=kwargs.get("use_legacy_key") ) headers = { diff --git a/bookwyrm/signatures.py b/bookwyrm/signatures.py index d3475edf6..9730ec6a1 100644 --- a/bookwyrm/signatures.py +++ b/bookwyrm/signatures.py @@ -23,7 +23,7 @@ def create_key_pair(): def make_signature( - method, sender, destination, date, digest=None, use_legacy_key=False + method, sender, destination, date, **kwargs ): """uses a private key to sign an outgoing message""" inbox_parts = urlparse(destination) @@ -33,6 +33,7 @@ def make_signature( f"date: {date}", ] headers = "(request-target) host date" + digest = kwargs.get("digest") if digest is not None: signature_headers.append(f"digest: {digest}") headers = "(request-target) host date digest" @@ -43,7 +44,7 @@ def make_signature( # For legacy reasons we need to use an incorrect keyId for older Bookwyrm versions key_id = ( f"{sender.remote_id}#main-key" - if use_legacy_key + if kwargs.get("use_legacy_key") else f"{sender.remote_id}/#main-key" ) signature = { diff --git a/bookwyrm/views/inbox.py b/bookwyrm/views/inbox.py index 5670f35b9..1c6c64228 100644 --- a/bookwyrm/views/inbox.py +++ b/bookwyrm/views/inbox.py @@ -3,7 +3,6 @@ import json import re import logging -from urllib.parse import urldefrag import requests from django.http import HttpResponse, Http404 From 123628c66a2f332c48b26e261200ad81e3041d45 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Thu, 13 Apr 2023 22:33:54 +1000 Subject: [PATCH 13/15] fix tests and formatting --- bookwyrm/models/activitypub_mixin.py | 7 ++++++- bookwyrm/signatures.py | 4 +--- bookwyrm/tests/models/test_fields.py | 2 +- bookwyrm/tests/test_signing.py | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index 3e04fa408..d9942746a 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -540,7 +540,12 @@ async def sign_and_send( digest = make_digest(data) signature = make_signature( - "post", sender, destination, now, digest=digest, use_legacy_key=kwargs.get("use_legacy_key") + "post", + sender, + destination, + now, + digest=digest, + use_legacy_key=kwargs.get("use_legacy_key"), ) headers = { diff --git a/bookwyrm/signatures.py b/bookwyrm/signatures.py index 9730ec6a1..08780b731 100644 --- a/bookwyrm/signatures.py +++ b/bookwyrm/signatures.py @@ -22,9 +22,7 @@ def create_key_pair(): return private_key, public_key -def make_signature( - method, sender, destination, date, **kwargs -): +def make_signature(method, sender, destination, date, **kwargs): """uses a private key to sign an outgoing message""" inbox_parts = urlparse(destination) signature_headers = [ diff --git a/bookwyrm/tests/models/test_fields.py b/bookwyrm/tests/models/test_fields.py index 961fbd522..fc8d7387d 100644 --- a/bookwyrm/tests/models/test_fields.py +++ b/bookwyrm/tests/models/test_fields.py @@ -404,7 +404,7 @@ class ModelFields(TestCase): self.assertIsInstance(result, list) self.assertEqual(len(result), 1) self.assertEqual(result[0].href, "https://e.b/c") - self.assertEqual(result[0].name, "Name") + self.assertEqual(result[0].name, "@Name") self.assertEqual(result[0].type, "Serializable") def test_tag_field_from_activity(self, *_): diff --git a/bookwyrm/tests/test_signing.py b/bookwyrm/tests/test_signing.py index cde193f08..12a164eb5 100644 --- a/bookwyrm/tests/test_signing.py +++ b/bookwyrm/tests/test_signing.py @@ -87,7 +87,7 @@ class Signature(TestCase): data = json.dumps(get_follow_activity(sender, self.rat)) digest = digest or make_digest(data) signature = make_signature( - "post", signer or sender, self.rat.inbox, now, digest + "post", signer or sender, self.rat.inbox, now, digest=digest ) with patch("bookwyrm.views.inbox.activity_task.apply_async"): with patch("bookwyrm.models.user.set_remote_server.delay"): From 8a8af4e90927540077908b4474a10b427f9d7672 Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Apr 2023 18:03:51 +1000 Subject: [PATCH 14/15] fix tests and make pylint happier --- bookwyrm/models/activitypub_mixin.py | 2 -- bookwyrm/tests/test_signing.py | 4 +++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bookwyrm/models/activitypub_mixin.py b/bookwyrm/models/activitypub_mixin.py index d9942746a..ee8b0d26a 100644 --- a/bookwyrm/models/activitypub_mixin.py +++ b/bookwyrm/models/activitypub_mixin.py @@ -563,9 +563,7 @@ async def sign_and_send( "Failed to send broadcast to %s: %s", destination, response.reason ) if kwargs.get("use_legacy_key") is not True: - # try with incorrect keyId to enable communication with legacy Bookwyrm servers logger.info("Trying again with legacy keyId header value") - asyncio.ensure_future( sign_and_send( session, sender, data, destination, use_legacy_key=True diff --git a/bookwyrm/tests/test_signing.py b/bookwyrm/tests/test_signing.py index 12a164eb5..ca68d3fd6 100644 --- a/bookwyrm/tests/test_signing.py +++ b/bookwyrm/tests/test_signing.py @@ -111,6 +111,7 @@ class Signature(TestCase): datafile = pathlib.Path(__file__).parent.joinpath("data/ap_user.json") data = json.loads(datafile.read_bytes()) data["id"] = self.fake_remote.remote_id + data["publicKey"]["id"] = f"{self.fake_remote.remote_id}/#main-key" data["publicKey"]["publicKeyPem"] = self.fake_remote.key_pair.public_key del data["icon"] # Avoid having to return an avatar. responses.add(responses.GET, self.fake_remote.remote_id, json=data, status=200) @@ -138,6 +139,7 @@ class Signature(TestCase): datafile = pathlib.Path(__file__).parent.joinpath("data/ap_user.json") data = json.loads(datafile.read_bytes()) data["id"] = self.fake_remote.remote_id + data["publicKey"]["id"] = f"{self.fake_remote.remote_id}/#main-key" data["publicKey"]["publicKeyPem"] = self.fake_remote.key_pair.public_key del data["icon"] # Avoid having to return an avatar. responses.add(responses.GET, self.fake_remote.remote_id, json=data, status=200) @@ -157,7 +159,7 @@ class Signature(TestCase): "bookwyrm.models.relationship.UserFollowRequest.accept" ) as accept_mock: response = self.send_test_request(sender=self.fake_remote) - self.assertEqual(response.status_code, 200) + self.assertEqual(response.status_code, 200) #BUG this is 401 self.assertTrue(accept_mock.called) # Old key is cached, so still works: From 98726585f6dc8a36447519c630d093f81ef0adea Mon Sep 17 00:00:00 2001 From: Hugh Rundle Date: Fri, 14 Apr 2023 18:20:06 +1000 Subject: [PATCH 15/15] oops black --- bookwyrm/tests/test_signing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bookwyrm/tests/test_signing.py b/bookwyrm/tests/test_signing.py index ca68d3fd6..37d841b33 100644 --- a/bookwyrm/tests/test_signing.py +++ b/bookwyrm/tests/test_signing.py @@ -159,7 +159,7 @@ class Signature(TestCase): "bookwyrm.models.relationship.UserFollowRequest.accept" ) as accept_mock: response = self.send_test_request(sender=self.fake_remote) - self.assertEqual(response.status_code, 200) #BUG this is 401 + self.assertEqual(response.status_code, 200) # BUG this is 401 self.assertTrue(accept_mock.called) # Old key is cached, so still works: