Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Refetch user keys when HTTP Signature validation fails #12051

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

erincandescent
Copy link

What

If a user has had a key rotation, and nobody on this server follows that user, we will not receive the Update activity with the new key

Therefore, when we encounter key validation errors we should check for an up-to-date key.

Why

Currently, federation will break with accounts on remote servers which have no local followers as the key update activity will not be received

This can happen if, for example, a Mastodon admin runs tootctl accounts rotate

Additional info (optional)

Behaviour mirrors other implementations:

This is a port of Iceshrimp PR 327, and has been verified as resolving federation issues there

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@github-actions github-actions bot added the packages/backend Server side specific issue/PR label Oct 16, 2023
@codecov
Copy link

codecov bot commented Oct 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (9338ab4) 79.26% compared to head (5641b0b) 79.09%.
Report is 3 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12051      +/-   ##
===========================================
- Coverage    79.26%   79.09%   -0.18%     
===========================================
  Files          930      174     -756     
  Lines        99219    22717   -76502     
  Branches      8065      485    -7580     
===========================================
- Hits         78646    17968   -60678     
+ Misses       20573     4749   -15824     

see 758 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@erincandescent erincandescent force-pushed the refetch-key branch 2 times, most recently from 98867b3 to 3ea5a8a Compare October 16, 2023 23:48
@u1-liquid
Copy link
Contributor

In my opinion, we should not automatically update the remote user's key when we fail to verify. This change would undermine the last line of defense in the ActivityPub design that protects whether the remote user is genuine. Perhaps, just like in Mastodon, it would be better to only update it when explicitly specified by the administrator.

If a user has had a key rotation, and nobody on this server follows
that user, we will not receive the Update activity with the new key

Therefore, when we encounter key validation errors we should check
for an up-to-date key.

References (other implementations):

 * [Mastodon](https://github.com/mastodon/mastodon/blob/fc9ab61448af94513524f14f5fd287d2a26ceeab/app/controllers/concerns/signature_verification.rb#L96)
 * [Akkoma](https://akkoma.dev/AkkomaGang/http_signatures/src/branch/main/lib/http_signatures/http_signatures.ex#L46)
@erincandescent
Copy link
Author

Perhaps, just like in Mastodon, it would be better to only update it when explicitly specified by the administrator.

This is not Mastodon behaviour - you can follow the code I linked

@erincandescent
Copy link
Author

I agree it would be nice if there were a better way of validating key authenticity, but there isn't for users which don't have an active following relation

(If there is a following relation, then an Update activity will be received with the new key, signed with the old)

@Mar0xy
Copy link
Contributor

Mar0xy commented Oct 19, 2023

Is this actually usable if so I would go and merge it into our fork

@erincandescent
Copy link
Author

I haven't yet gotten to the bottom of why there are lint issues (I'm not a TypeScript programmer at all), but it should work.

Its already merged into the FireFish and IceShrimp forks

Mar0xy pushed a commit to transfem-org/Sharkey that referenced this pull request Oct 20, 2023
@Mar0xy
Copy link
Contributor

Mar0xy commented Oct 20, 2023

I have implemented it now in a bit of a different way than in this PR as can be checked via this search query

Mainly due to the fact that the first info if it errors would throw a error causing bull to stop processing it all together also blocking the second if statement from ever running.

@mei23
Copy link
Contributor

mei23 commented Dec 2, 2023

In the first place, the process that involves updating the remote user's key is triggered once every 24 hours when something comes to the inbox or using the users/show API.
Therefore, there is no need to worry about security when updating the key, and there is no need to update it in a hurry.
Of course, I understand that the sooner the better.

There are security and performance concerns about always making a request every time an inbox fails.
It might be a good idea to have a cool down time.
In the case of Misskey, recovery may not be possible just by updating the key.
remoteUserResolveService.resolveUser might meet both needs.

I made some changes, but I'm sorry, I haven't tried it in a real environment.
mei23@12eec09

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants