From c83c77a73e66648f9a04f45e2b8fe20f133b0ce7 Mon Sep 17 00:00:00 2001 From: Ryan Barrett Date: Mon, 16 Oct 2023 11:13:38 -0700 Subject: [PATCH] authorization: log when authed user doesn't match activity's author/actor for #566. just logging for now, want to see if we're already hitting this at all. --- activitypub.py | 14 ++++++++++---- protocol.py | 8 ++++++-- tests/test_activitypub.py | 16 ++++++++++++++++ tests/test_protocol.py | 22 +++++++++++++++++++--- tests/test_web.py | 21 +++++++++++++++++++++ tests/testutil.py | 13 +++++++------ 6 files changed, 79 insertions(+), 15 deletions(-) diff --git a/activitypub.py b/activitypub.py index d064b42..6e6d584 100644 --- a/activitypub.py +++ b/activitypub.py @@ -328,13 +328,17 @@ class ActivityPub(User, Protocol): def verify_signature(cls, activity): """Verifies the current request's HTTP Signature. - Raises :class:`werkzeug.exceptions.HTTPError` if the - signature is missing or invalid, otherwise does nothing and returns None. + Raises :class:`werkzeug.exceptions.HTTPError` if the signature is + missing or invalid, otherwise does nothing and returns the id of the + actor whose key signed the request. Logs details of the result. Args: activity (dict): AS2 activity + + Returns: + str: signing AP actor id """ headers = dict(request.headers) # copy so we can modify below sig = headers.get('Signature') @@ -408,6 +412,8 @@ class ActivityPub(User, Protocol): else: error('HTTP Signature verification failed', status=401) + return keyId + def signed_get(url, **kwargs): return signed_request(util.requests_get, url, **kwargs) @@ -838,7 +844,7 @@ def inbox(protocol=None, id=None): actor_obj = ActivityPub.load(actor_id) ActivityPub.get_or_create(actor_id, direct=True, obj=actor_obj) - ActivityPub.verify_signature(activity) + authed_as = ActivityPub.verify_signature(activity) # check that this activity is public. only do this for creates, not likes, # follows, or other activity types, since Mastodon doesn't currently mark @@ -861,7 +867,7 @@ def inbox(protocol=None, id=None): activity.setdefault('url', f'{follower_url}#followed-{followee_url}') obj = Object(id=activity.get('id'), as2=redirect_unwrap(activity)) - return ActivityPub.receive(obj) + return ActivityPub.receive(obj, authed_as=authed_as) # protocol in subdomain diff --git a/protocol.py b/protocol.py index 550957c..4f4e962 100644 --- a/protocol.py +++ b/protocol.py @@ -479,7 +479,7 @@ class Protocol: DOMAIN_BLOCKLIST + DOMAINS) @classmethod - def receive(from_cls, obj): + def receive(from_cls, obj, authed_as=None): """Handles an incoming activity. If ``obj``'s key is unset, ``obj.as1``'s id field is used. If both are @@ -528,9 +528,13 @@ class Protocol: logger.info(msg) return msg, 204 + # authorization check + actor = as1.get_owner(obj.as1) + if authed_as and actor != authed_as: + logger.warning(f"actor {actor} isn't authed user {authed_as}") + # write Object to datastore orig = obj - actor = as1.get_owner(orig.as1) obj = Object.get_or_create(id, **orig.to_dict(), actor=actor) if orig.new is not None: obj.new = orig.new diff --git a/tests/test_activitypub.py b/tests/test_activitypub.py index f132c9f..aa6feff 100644 --- a/tests/test_activitypub.py +++ b/tests/test_activitypub.py @@ -1342,6 +1342,22 @@ class ActivityPubTest(TestCase): self.assertEqual(204, got.status_code) self.assertEqual(0, Follower.query().count()) + def test_inbox_http_sig_is_not_actor_author(self, mock_head, mock_get, mock_post): + mock_get.side_effect = [ + self.as2_resp(ACTOR), # author + ] + + with self.assertLogs() as logs: + got = self.post('/user.com/inbox', json={ + **NOTE_OBJECT, + 'author': 'https://alice', + }) + self.assertEqual(204, got.status_code, got.get_data(as_text=True)) + + self.assertIn( + "WARNING:protocol:actor https://alice isn't authed user http://my/key/id", + logs.output) + def test_followers_collection_unknown_user(self, *_): resp = self.client.get('/nope.com/followers') self.assertEqual(404, resp.status_code) diff --git a/tests/test_protocol.py b/tests/test_protocol.py index 841db6d..71f4a02 100644 --- a/tests/test_protocol.py +++ b/tests/test_protocol.py @@ -553,8 +553,6 @@ class ProtocolReceiveTest(TestCase): } self.store_object(id='fake:post', our_as1=post_as1) - orig_disable_level = logging.root.manager.disable - logging.disable(logging.NOTSET) with self.assertRaises(NoContent), self.assertLogs() as logs: Fake.receive_as1({ 'id': 'fake:update', @@ -563,7 +561,6 @@ class ProtocolReceiveTest(TestCase): 'actor': 'fake:other', 'object': post_as1, }) - logging.disable(orig_disable_level) self.assertIn( "WARNING:models:actor fake:other isn't fake:post's author or actor ['fake:user']", @@ -1415,3 +1412,22 @@ class ProtocolReceiveTest(TestCase): }) self.assertEqual([], Fake.sent) + + def test_like_not_authed_as_actor(self): + Fake.fetchable['fake:post'] = { + 'objectType': 'note', + 'author': 'fake:bob', + } + + with self.assertLogs() as logs: + Fake.receive_as1({ + 'id': 'fake:like', + 'objectType': 'activity', + 'verb': 'like', + 'actor': 'fake:user', + 'object': 'fake:post', + }, authed_as='fake:other') + + self.assertIn( + "WARNING:protocol:actor fake:user isn't authed user fake:other", + logs.output) diff --git a/tests/test_web.py b/tests/test_web.py index 9b3e9e5..aabd0fa 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -1637,6 +1637,27 @@ class WebTest(TestCase): labels=['user', 'activity'], ) + def test_like_actor_is_not_source_domain(self, mock_get, mock_post): + like_html = LIKE_HTML.replace( + 'class="p-author h-card" href="https://user.com/"', + 'class="p-author h-card" href="https://eve.com/"') + mock_get.side_effect = [ + requests_response(like_html, url='https://user.com/like'), + TOOT_AS2, + ACTOR, + ] + + with self.assertLogs() as logs: + got = self.client.post('/queue/webmention', data={ + 'source': 'https://user.com/like', + 'target': 'https://fed.brid.gy/', + }) + self.assertEqual(200, got.status_code) + + self.assertIn( + "WARNING:models:actor https://user.com/ isn't https://user.com/like's author or actor ['https://eve.com/']", + logs.output) + def _test_verify(self, redirects, hcard, actor, redirects_error=None): g.user.has_redirects = False g.user.put() diff --git a/tests/testutil.py b/tests/testutil.py index e2f4d6a..d288583 100644 --- a/tests/testutil.py +++ b/tests/testutil.py @@ -132,14 +132,14 @@ class Fake(User, protocol.Protocol): return 'shared:target' if shared else f'{obj.key.id()}:target' @classmethod - def receive(cls, obj): + def receive(cls, obj, **kwargs): assert isinstance(obj, Object) - return super().receive(obj=obj) + return super().receive(obj=obj, **kwargs) @classmethod - def receive_as1(cls, our_as1): + def receive_as1(cls, our_as1, **kwargs): assert isinstance(our_as1, dict) - return super().receive(Object(id=our_as1['id'], our_as1=our_as1)) + return super().receive(Object(id=our_as1['id'], our_as1=our_as1), **kwargs) class OtherFake(Fake): @@ -495,7 +495,8 @@ class TestCase(unittest.TestCase, testutil.Asserts): with super().assertLogs() as logs: yield logs finally: - logging.disable(orig_disable_level) # emit logs that were captured for record in logs.records: - logging.getLogger().handle(record) + if record.levelno >= orig_disable_level: + logging.root.handle(record) + logging.disable(orig_disable_level)