From 17734940accceb96aa3bd3a73f7fe4aff8f5f25f Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Thu, 21 May 2020 16:32:28 +0100 Subject: [PATCH 1/4] Use get_or_create_remote_user from get_public_key. --- fedireads/incoming.py | 18 +++--------------- fedireads/remote_user.py | 3 ++- fedireads/tests/test_signing.py | 20 +++++++++++++++++--- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/fedireads/incoming.py b/fedireads/incoming.py index 6ca51d0e..00fea094 100644 --- a/fedireads/incoming.py +++ b/fedireads/incoming.py @@ -54,7 +54,7 @@ def shared_inbox(request): key = get_public_key(key_actor) signature.verify(key, request) - except ValueError: + except (ValueError, requests.exceptions.HTTPError): return HttpResponse(status=401) handlers = { @@ -91,20 +91,8 @@ def shared_inbox(request): def get_public_key(key_actor): ''' try a stored key or load it from remote ''' - try: - user = models.User.objects.get(remote_id=key_actor) - public_key = user.public_key - except models.User.DoesNotExist: - response = requests.get( - key_actor, - headers={'Accept': 'application/activity+json'} - ) - if not response.ok: - raise ValueError('Could not load public key') - user_data = response.json() - public_key = user_data['publicKey']['publicKeyPem'] - - return public_key + user = get_or_create_remote_user(key_actor) + return user.public_key @app.task def handle_follow(activity): diff --git a/fedireads/remote_user.py b/fedireads/remote_user.py index 81c8c34c..72a0391c 100644 --- a/fedireads/remote_user.py +++ b/fedireads/remote_user.py @@ -36,7 +36,8 @@ def get_or_create_remote_user(actor): user.save() avatar = get_avatar(data) - user.avatar.save(*avatar) + if avatar: + user.avatar.save(*avatar) if user.fedireads_user: get_remote_reviews(user) diff --git a/fedireads/tests/test_signing.py b/fedireads/tests/test_signing.py index b93d263e..62bdc761 100644 --- a/fedireads/tests/test_signing.py +++ b/fedireads/tests/test_signing.py @@ -1,6 +1,7 @@ import time from collections import namedtuple from urllib.parse import urlsplit +import pathlib import json import responses @@ -73,13 +74,26 @@ class Signature(TestCase): @responses.activate def test_remote_signer(self): + 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']['publicKeyPem'] = self.fake_remote.public_key + del data['icon'] # Avoid having to return an avatar. responses.add( responses.GET, self.fake_remote.remote_id, - json={'publicKey': { - 'publicKeyPem': self.fake_remote.public_key - }}, + json=data, status=200) + responses.add( + responses.GET, + 'https://localhost/.well-known/nodeinfo', + status=404) + responses.add( + responses.GET, + 'https://example.com/user/mouse/outbox?page=true', + json={'orderedItems': []}, + status=200 + ) response = self.send_test_request(sender=self.fake_remote) self.assertEqual(response.status_code, 200) From 5cfc9aa8defaafeef9744829ff3bc44e60c38e2a Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Fri, 22 May 2020 13:49:56 +0100 Subject: [PATCH 2/4] Fetch updated key if old key is invalid. --- fedireads/incoming.py | 18 +++++++----- fedireads/remote_user.py | 45 +++++++++++++++++++++------- fedireads/tests/test_signing.py | 52 +++++++++++++++++++++++++++++++++ 3 files changed, 96 insertions(+), 19 deletions(-) diff --git a/fedireads/incoming.py b/fedireads/incoming.py index 00fea094..d2025892 100644 --- a/fedireads/incoming.py +++ b/fedireads/incoming.py @@ -10,7 +10,7 @@ from django.views.decorators.csrf import csrf_exempt from fedireads import books_manager, models, outgoing from fedireads import status as status_builder -from fedireads.remote_user import get_or_create_remote_user +from fedireads.remote_user import get_or_create_remote_user, refresh_remote_user from fedireads.tasks import app from fedireads.signatures import Signature @@ -51,9 +51,16 @@ def shared_inbox(request): if key_actor != activity.get('actor'): raise ValueError("Wrong actor created signature.") - key = get_public_key(key_actor) + remote_user = get_or_create_remote_user(key_actor) - signature.verify(key, request) + try: + signature.verify(remote_user.public_key, request) + except ValueError: + old_key = remote_user.public_key + refresh_remote_user(remote_user) + if remote_user.public_key == old_key: + raise # Key unchanged. + signature.verify(remote_user.public_key, request) except (ValueError, requests.exceptions.HTTPError): return HttpResponse(status=401) @@ -89,11 +96,6 @@ def shared_inbox(request): return HttpResponse() -def get_public_key(key_actor): - ''' try a stored key or load it from remote ''' - user = get_or_create_remote_user(key_actor) - return user.public_key - @app.task def handle_follow(activity): ''' someone wants to follow a local user ''' diff --git a/fedireads/remote_user.py b/fedireads/remote_user.py index 72a0391c..9cf11889 100644 --- a/fedireads/remote_user.py +++ b/fedireads/remote_user.py @@ -17,17 +17,7 @@ def get_or_create_remote_user(actor): except models.User.DoesNotExist: pass - # load the user's info from the actor url - response = requests.get( - actor, - headers={'Accept': 'application/activity+json'} - ) - if not response.ok: - response.raise_for_status() - data = response.json() - - # make sure our actor is who they say they are - assert actor == data['id'] + data = fetch_user_data(actor) actor_parts = urlparse(actor) with transaction.atomic(): @@ -43,6 +33,21 @@ def get_or_create_remote_user(actor): get_remote_reviews(user) return user +def fetch_user_data(actor): + # load the user's info from the actor url + response = requests.get( + actor, + headers={'Accept': 'application/activity+json'} + ) + if not response.ok: + response.raise_for_status() + data = response.json() + + # make sure our actor is who they say they are + if actor != data['id']: + raise ValueError("Remote actor id must match url.") + return data + def create_remote_user(data): ''' parse the activitypub actor data into a user ''' @@ -72,6 +77,24 @@ def create_remote_user(data): 'manuallyApprovesFollowers', False), ) +def refresh_remote_user(user): + data = fetch_user_data(user.remote_id) + + shared_inbox = data.get('endpoints').get('sharedInbox') if \ + data.get('endpoints') else None + + # TODO - I think dataclasses change will mean less repetition here later. + user.name = data.get('name') + user.summary = data.get('summary') + user.inbox = data['inbox'] #fail if there's no inbox + user.outbox = data['outbox'] # fail if there's no outbox + user.shared_inbox = shared_inbox + user.public_key = data.get('publicKey').get('publicKeyPem') + user.local = False + user.fedireads_user = data.get('fedireadsUser', False) + user.manually_approves_followers = data.get( + 'manuallyApprovesFollowers', False) + user.save() def get_avatar(data): ''' find the icon attachment and load the image from the remote sever ''' diff --git a/fedireads/tests/test_signing.py b/fedireads/tests/test_signing.py index 62bdc761..0293a2d3 100644 --- a/fedireads/tests/test_signing.py +++ b/fedireads/tests/test_signing.py @@ -98,6 +98,58 @@ class Signature(TestCase): response = self.send_test_request(sender=self.fake_remote) self.assertEqual(response.status_code, 200) + @responses.activate + def test_key_needs_refresh(self): + 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']['publicKeyPem'] = self.fake_remote.public_key + del data['icon'] # Avoid having to return an avatar. + responses.add( + responses.GET, + self.fake_remote.remote_id, + json=data, + status=200) + responses.add( + responses.GET, + 'https://localhost/.well-known/nodeinfo', + status=404) + responses.add( + responses.GET, + 'https://example.com/user/mouse/outbox?page=true', + json={'orderedItems': []}, + status=200 + ) + + # Second and subsequent fetches get a different key: + new_private_key, new_public_key = create_key_pair() + new_sender = Sender( + self.fake_remote.remote_id, new_private_key, new_public_key) + data['publicKey']['publicKeyPem'] = new_public_key + responses.add( + responses.GET, + self.fake_remote.remote_id, + json=data, + status=200) + + + # Key correct: + response = self.send_test_request(sender=self.fake_remote) + self.assertEqual(response.status_code, 200) + + # Old key is cached, so still works: + response = self.send_test_request(sender=self.fake_remote) + self.assertEqual(response.status_code, 200) + + # Try with new key: + response = self.send_test_request(sender=new_sender) + self.assertEqual(response.status_code, 200) + + # Now the old key will fail: + response = self.send_test_request(sender=self.fake_remote) + self.assertEqual(response.status_code, 401) + + @responses.activate def test_nonexistent_signer(self): responses.add( From ae7339928c621d80c18acc036e5b101ce1f9ae49 Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Fri, 22 May 2020 13:53:56 +0100 Subject: [PATCH 3/4] Move signature checking logic out of shared_inbox. --- fedireads/incoming.py | 42 ++++++++++++++++++++++++------------------ 1 file changed, 24 insertions(+), 18 deletions(-) diff --git a/fedireads/incoming.py b/fedireads/incoming.py index d2025892..ceefebc9 100644 --- a/fedireads/incoming.py +++ b/fedireads/incoming.py @@ -44,24 +44,7 @@ def shared_inbox(request): if not activity.get('object'): return HttpResponseBadRequest() - 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 = get_or_create_remote_user(key_actor) - - try: - signature.verify(remote_user.public_key, request) - except ValueError: - old_key = remote_user.public_key - refresh_remote_user(remote_user) - if remote_user.public_key == old_key: - raise # Key unchanged. - signature.verify(remote_user.public_key, request) - except (ValueError, requests.exceptions.HTTPError): + if not has_valid_signature(request, activity): return HttpResponse(status=401) handlers = { @@ -96,6 +79,29 @@ def shared_inbox(request): return HttpResponse() +def has_valid_signature(request, activity): + 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 = get_or_create_remote_user(key_actor) + + try: + signature.verify(remote_user.public_key, request) + except ValueError: + old_key = remote_user.public_key + refresh_remote_user(remote_user) + if remote_user.public_key == old_key: + raise # Key unchanged. + signature.verify(remote_user.public_key, request) + except (ValueError, requests.exceptions.HTTPError): + return False + return True + + @app.task def handle_follow(activity): ''' someone wants to follow a local user ''' From 9af410f921f1458ba02f0ca767b0af30f0a8e9c0 Mon Sep 17 00:00:00 2001 From: Adam Kelly Date: Fri, 22 May 2020 14:20:27 +0100 Subject: [PATCH 4/4] Silently ignore unauthorised deletes. Fixes: #166 --- fedireads/incoming.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/fedireads/incoming.py b/fedireads/incoming.py index ceefebc9..58da9292 100644 --- a/fedireads/incoming.py +++ b/fedireads/incoming.py @@ -45,6 +45,10 @@ def shared_inbox(request): return HttpResponseBadRequest() if not has_valid_signature(request, activity): + if activity['type'] == 'Delete': + # Pretend that unauth'd deletes succeed. Auth may be failing because + # the resource or owner of the resource might have been deleted. + return HttpResponse() return HttpResponse(status=401) handlers = {