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

Add Update Actor preferredUsername support #30361

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

angusmcleod
Copy link

@renchap renchap added the activitypub Protocol-related changes, federation label May 18, 2024
@ClearlyClaire
Copy link
Contributor

Hi! Thank you for your PR!
Mastodon doesn't officially support renaming, but as you've seen, it should still handle things relatively gracefully (even though there's still room for improvement).

At a glance, the PR looks ok, but I will do a full review later.

It's still marked as a draft, so is there something in particular you want feedback on?

@angusmcleod angusmcleod marked this pull request as ready for review May 23, 2024 14:19
@angusmcleod
Copy link
Author

@ClearlyClaire Thanks for taking a look, it's much appreciated. I've added a few more tests for this case, so it's ready for review now.

@angusmcleod
Copy link
Author

angusmcleod commented May 23, 2024

@ClearlyClaire Thanks for the feedback. This is what I've done:

  • Made the formatting changes you suggested
  • Removed the Update Note and Follow specs as suggested
  • Added update username support for Update Actor. I think it makes more sense to directly update the username in this case instead of performing a migration, as it's a contained case (i.e. nothing else is going on).
  • Added more tests for the Search case (it occurred to me that some services may still return the actor for the old handle).

@ClearlyClaire
Copy link
Contributor

  • Added update username support for Update Actor. I think it makes more sense to directly update the username in this case instead of performing a migration, as it's a contained case (i.e. nothing else is going on).

I think we should do something like this eventually, but this is a much more complex endeavour. Indeed, we enforce uniqueness of (username, domain) tuples, so this introduces a new failure path (another account with the same username already exists) that needs extra consideration.

@angusmcleod
Copy link
Author

angusmcleod commented May 24, 2024

@ClearlyClaire Ensuring username uniqueness is something the remote domain has responsibility for in this case. We can protect against that within the context of Update Actor: angusmcleod@138fee1

@angusmcleod
Copy link
Author

angusmcleod commented Jun 11, 2024

Just a gentle bump on this one. If there's anything I can do to make this easier to review, let me know.

Copy link
Contributor

@ClearlyClaire ClearlyClaire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The proposed behavior changes have some issues, and since doing it right is pretty tricky, I think I'd much prefer if this PR focused on describing the current behavior, even if that is suboptimal.

Comment on lines 28 to 35
if @account.username != @object['preferredUsername']
account_proxy = @account.dup
account_proxy.username = @object['preferredUsername']
UniqueUsernameValidator.new.validate(account_proxy)
opts[:allow_username_update] = account_proxy.errors.blank?
end

ActivityPub::ProcessAccountService.new.call(@account.username, @account.domain, @object, opts)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the assumption that the preferredUsername has been verified to resolve to the right account through webfinger, which could be exploited in the following theoretical setting:

  1. identity provider example.com allows its users to point at arbitrary external ActivityPub servers
  2. eve creates an account eve@example.com pointing at her ActivityPub server eve.social and gets known as eve@example.com to Mastodon
  3. eve@example.com sends an Update to Mastodon where preferredUsername is set to bob
  4. eve@example.com is thus renamed to bob@example.com as far as Mastodon is concerned, even though bob@example.com is a different person using a different ActivityPub server

Copy link
Author

@angusmcleod angusmcleod Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If all of those premises were true that would be an exploit of example.com, not an exploit of Mastodon. To make the point clearer, flesh out the implicit premises:

2.a bob has an account bob@example.com and is not already known to Mastodon (otherwise 3 would not succeed).

2.b example.com allows, or is vulnerable to, actors setting preferredUsernames already used by other actors on the domain (otherwise 3 would not be possible).

Or, more simply, consider the scenario in which example.com does not enforce any kind of username uniqueness, in which case this scenario could already exist, i.e.

  1. eve creates an account bob@example.com pointing at her ActivityPub server eve.social and gets known as bob@example.com to Mastodon

In short, as long as Mastodon applies the same username uniqueness rules consistently there isn't really any difference between when an actor from a remote domain is created and when it is updated.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case I'm describing, example.com would delegate all ActivityPub logic to arbitrary third-party servers and only implement webfinger, so 2.b would be irrelevant, as example.com would not be involved with the preferredUsernames change at all.

Copy link
Author

@angusmcleod angusmcleod Jun 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would still be an exploit of example.com not Mastodon. You could do the same thing without this PR, just using the initial activity:

eve creates an account bob@example.com pointing at her ActivityPub server eve.social and gets known as bob@example.com to Mastodon

There is no difference between Create and Update in that scenario.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mastodon does not support Create for actors, and before processing actors, it requires the webfinger discovery to loop back to the actor, as described in https://docs.joinmastodon.org/spec/webfinger/#mastodons-requirements-for-webfinger

Your proposed change skips that test, and that is what I was pointing out.

Copy link
Author

@angusmcleod angusmcleod Jun 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not being clear. I simply mean that, given the scenario you've established, as far as Mastodon is concerned, I don't think there is any difference in the risk profile between this scenario (which is currently possible):

eve creates an account bob@example.com pointing at her ActivityPub server eve.social and gets known as bob@example.com to Mastodon

And this scenario:

eve@example.com sends an Update to Mastodon where preferredUsername is set to bob

In both scenarios, it doesn't matter what Mastodon does with Webfinger, or how many times it re-checks the Actor, as the same risk will be present. Given that, as you've required, this identity server is not performing any username uniqueness checks, assuming that sending a Webfinger request to example.com with bob@example.com is always returning the "right" Actor, or the same Actor it returned last time, is equally circumspect. Fundamentally, in both scenarios, Mastodon has no control over the cogency or consistency of external identities. All you can (possibly) control is spoofing (i.e. using HTTP signatures) and your own registration of those identities, e.g. enforcing unique preferredUsernames on a domain when storing the Actor.

Nevertheless, I think I understand what you're saying, i.e. that Mastodon makes this assumption (that Webfinger entails identity consistency) to an extent, and that this assumption should be followed here too. In that vein I've added in a Webfinger check of the updated username:

angusmcleod@60ba6d1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will require more careful review to make sure nothing can go wrong, but this looks good after a first review pass. The PR title needs a change though as this is no longer about adding tests, but changing the actual behavior.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated the title. Let me know if there's anything more I can do to help the review, whether it be adding more tests or staging this branch somewhere.

@angusmcleod angusmcleod changed the title Add integration test for when remote actor username changes Add Update Actor preferredUsername support Jun 14, 2024
@julianlam
Copy link

Hi there, just chiming in to provide another voice in support of this PR. NodeBB sends Update(Actor) and while it currently works "well enough", it'd be great to get this merged in 😄

Thank you for your hard work :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub Protocol-related changes, federation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants