Fix federation with GoToSocial and inconsistent KeyId in headers #2812
Conversation
Fixes bookwyrm-social#2801 Related to bookwyrm-social#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.
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.
GoToSocial expects the 'name' value of a mention tag to have an initial '@' symbol. Mastodon doesn't seem to mind either way.
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/views/inbox.py
Outdated
| if not remote_user: | ||
| 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 |
There was a problem hiding this comment.
I'm a bit confused by this, isn't the legacy one the one with the /? And I thought remote_user.remote_id didn't have a /?
There was a problem hiding this comment.
It is incredibly confusing.
All Bookwyrm keyIds are at {username}/#main-key - this is set in models.User so it's impractical to change that because we can't retrospectively change people's keyIds.
However, the legacy signatures (falsely) assert that the keyId is at {username}#main-key. Both are equivalent if you want to hit a url, but are not if you want to compare strings.
So if the remote user is from another Bookwyrm instance that has not upgraded, it will send a signature asserting the remote user's keyId is at #main-key but when our updated instance goes and checks, it will see that the keyId is actually listed as /#main-key. This line is a bit of a hack where our instance says "hmm, maybe it's old Bookwyrm, what if I just pretend the last forward slash isn't there?".
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.
hughrun
changed the title
Haven’t looked too much into it yet but I think the same fix should also help with any interactions like boosting or liking BookWyrm posts from Takahē and those ActivityPub objects being properly handled on BookWyrm’s side. |
|
Cool, nice one! :) Thanks for making these changes, looking forward to hanging out with Bookwyrm peeps from GtS :) |
|
Is this ready to merge? |
|
I think so but I'd really like someone to review and test it because it has a lot of potential to break things. |
|
I brought this up to current main and ran it on a test instance for a bit. So far:
|
|
Thanks for testing!
This is intended behaviour - Bookwyrm has never displayed standard So it sounds like we're good to go @mouse-reeve ! |
|
Is this released? And if so, in 0.6.3? I have many messages like… … and even one… … in my GtS log and can’t follow my new Bookwyrm user from my GtS user (it stays on “following requested”, doesn’t get the accept). |
|
Former is not related to this, it's an issue with GtS perhaps too strictly enforcing POST content-type as given in the spec: https://www.w3.org/TR/activitypub/#server-to-server-interactions We could fix this by trimming Latter error message looks like an unsigned GET to a user profile. |
|
Ah, okay then, thanks. I can hold off further experiments until after the GtS vacation (and until the instance I’m using runs latest BookWyrm, too). I was just having an initial look at BookWyrm. |
Fixes #2801
Fixes #2794
This PR resolves a number of problems specific to GoToSocial federation but also potentially affecting other fediverse software.
Fix making assumptions about location of
main-keyIt 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 (
{userid}#main-key) but this is not always the case, notably in the case of GoToSocial it is at{userid}/main-key. 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.This resolves most of the
401errors when trying to communicate with GtS servers.Fix Bookwyrm using wrong keyId in signed request headers
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:
User:{userid}/#main-key{userid}#main-keyThis commit changes the keyId listed in request headers to align with that assigned to users on creation.
Fix GoToSOcial DMs not appearing and single-user mentions not notifying
This error was caused by GtS sending the
tagvalue for these posts as a single object (dict) rather than an array containing the object, and Bookwyrm assuming it would be an array (list).This commit wraps the dict in a list where necessary.
Maintain backwards compatibility with legacy Bookwyrm instances
Aligning the keyId in the signatures with the real keyId of Bookwyrm users would break compatibility with all Bookwyrm versions before this change. The PR adds some logic to re-send the request with the old (wrong) keyId in the header if the first request is rejected. As Bookwyrm instances are updated this will become less of a problem.