Add ActivityPub secure mode #11269

Gargron · 2019-07-08T23:41:29Z

Controlled by SECURE_MODE=true
Requires incoming GET requests to ActivityPub resources to be signed
Actors can still be fetched without signature, but return only technical attributes
Bug fix: Handle inbox requests without request body gracefully

Gargron · 2019-07-09T04:11:08Z

One thing I am not sure about is the naming of the environment variable and derivative methods. Secure mode sounds too generic. On the other hand, "require http signatures for activitypub resources" sounds too specific (and long).

kaniini · 2019-07-09T07:59:53Z

cwebber and i have been working on integrating OCAP into ActivityPub for the past several months, and we are getting close. i think it makes better sense to use OCAP to authorize the fetches instead of signatures. no objection to allowing signatures as a temporary bandaid though.

thebaer · 2019-07-09T11:27:52Z

So I understand what this is for, is the idea just to restrict data access? I.e. a signature requirement means an instance can choose whether or not to grant accounts read privileges?

Either way, is there a way to discover whether this is enabled or not from the outside world? Or should platforms like WriteFreely always send GET requests with a signature now?

schmittlauch · 2019-07-09T12:10:29Z

One thing I am not sure about is the naming of the environment variable and derivative methods. Secure mode sounds too generic. On the other hand, "require http signatures for activitypub resources" sounds too specific (and long).

What about AUTHORIZED_FETCH?

Gargron · 2019-07-09T15:31:01Z

Either way, is there a way to discover whether this is enabled or not from the outside world? Or should platforms like WriteFreely always send GET requests with a signature now?

I'm not sure on the first part, and on the second, even Mastodon is not currently signing all outgoing requests, so there are definitely considerations about how to proceed without breaking stuff, which is why it's going to be behind an off-by-default environment variable for now. There are actually two problems here:

Current servers not signing requests
High-performance proxy caching not being available for authorized fetches

That being said... Including a signature header with a request is basically free (public resources that don't check HTTP signatures will just ignore it) so I don't see a disadvantage to WriteFreely doing that.

BenLubar · 2019-07-09T21:14:54Z

High-performance proxy caching not being available for authorized fetches

This is actually already the case for me, because Cloudflare doesn't support Vary headers other than Accept-Encoding. If I have public caching mode enabled, Cloudflare will send HTML responses to ActivityStreams requests and vice versa.

Gargron · 2019-07-09T21:20:23Z

@BenLubar However, nginx supports it, and you probably still have nginx between puma and cloudflare! This is what would no longer be possible

BenLubar · 2019-07-09T21:21:39Z

@BenLubar However, nginx supports it, and you probably still have nginx between puma and cloudflare! This is what would no longer be possible

sending Vary: Signature should be enough to make nginx handle this gracefully

Gargron · 2019-07-09T21:23:24Z

It would be pointless though, each signature is different. By definition you can't use the same one multiple times (past a threshold time window) to prevent replay attacks.

app/controllers/activitypub/collections_controller.rb

app/controllers/activitypub/inboxes_controller.rb

ClearlyClaire · 2019-07-10T10:04:59Z

Seems good to me overall, although I'm very interested with the token-based approach discussed by kaniini, as that would make it far easier for a caching reverse-proxy to cooperate.

Also, we should make sure to properly document the new setting in .env.production.sample at the very least.

Gargron · 2019-07-10T16:11:52Z

I see no point in adding it to .env.production.sample until signing is implemented

kaniini · 2019-07-10T19:27:35Z

In Pleroma, we plan on using an instance-specific actor to represent the fetches for this mitigation.

Of course, we also plan to use bearcaps for the long term, but that's something we talk about later :)

Point is, for now, will an instance-wide actor work?

Gargron · 2019-07-10T19:34:24Z

Point is, for now, will an instance-wide actor work?

Sure. I'm not sure why you think it wouldn't, if it's just a typical actor.

kaniini · 2019-07-10T19:38:48Z

My main question is will the access control be enforced directly (fetching an object requiring a signature of someone authorized to fetch the object) or indirectly (any actor on the same instance will work)?

Gargron · 2019-07-10T19:53:37Z

Ah. Well, if a post is public, then any actor can fetch it, provided the actor is not domain-blocked. If a post is private, then only an authorized follower can fetch it.

nightpool

consider allowing authenticated local users to view activitypub resources as well (in the browser or perhaps using an api client)
consider whether we're comfortable showing in the browser things we're not presenting in a machine readable format. Maybe this should affect unauthenticated viewing of public profile pages/status pages as well?

nightpool · 2019-07-11T02:13:35Z

app/controllers/accounts_controller.rb

@@ -132,4 +133,12 @@ def filtered_status_page(params)
      filtered_statuses.paginate_by_max_id(PAGE_SIZE, params[:max_id], params[:since_id]).to_a
    end
  end
+
+  def fields_selection
+    if signed_request_account.present? || !authorized_fetch_mode?


this might be more clear with early returns?

return unless authorized_fetch_mode? return if signed_request_account.present? %i(id type preferred_username inbox public_key endpoints)

also i'm unclear what the point of returning a limited object is... mind explaining?

Assuming that your goal is to prevent blocked instances from obtaining any data from yours, profile info is just as important as individual statuses. Bio text, header image, profile picture--all of that is quite personal. The attributes we do return here are the bare minimum, and technical in nature, to allow a two-way cryptography to be established.

ah, right, a chicken and an egg sort of problem. we need to return the limited object so that we can authenticate to people who don't know who we are. hmm.

Also no I would find two early returns harder to read in this case. "If we're signed in, or we don't require sign in, do this" reads more naturally to me.

app/controllers/accounts_controller.rb

nightpool · 2019-07-11T02:17:31Z

app/controllers/activitypub/collections_controller.rb

  before_action :set_size
  before_action :set_statuses
  before_action :set_cache_headers

  def show
-    expires_in 3.minutes, public: true
+    expires_in 3.minutes, public: !authorized_fetch_mode?


might be nice to have a readable helper method for !authorized_fetch_mode?

I'm not sure I agree a simple negation warrants that. not_authorized_fetch_mode? would just be longer.

app/controllers/activitypub/outboxes_controller.rb

nightpool · 2019-07-11T02:24:38Z

app/controllers/concerns/account_controller_concern.rb

@@ -11,7 +11,7 @@ module AccountControllerConcern
    layout 'public'

    before_action :set_instance_presenter
-    before_action :set_link_headers
+    before_action :set_link_headers, if: -> { request.format.nil? || request.format == :html }


this might be clearer/more maintainable if it was an after_action so it could read the actual response type? I don't think I've ever actually seen someone use an after action so I have no clue whether this is a good idea or not but it seems better

This is a minor detail and it works as it is though

nightpool · 2019-07-11T02:28:43Z

app/controllers/follower_accounts_controller.rb


-        expires_in 3.minutes, public: true if params[:page].blank?
+        expires_in page_requested? ? 0 : 3.minutes, public: !authorized_fetch_mode?


what's the point of changing the expires_in when a page is requested?

When no page is requested, what we return is essentially a counter--and these counters are requested when we're fetching actor info (so that endpoint can be hammered quite a bit). When a page is requested, it returns actual data, and I guess we don't want actual data to be outdated.

nightpool · 2019-07-11T02:29:41Z

app/controllers/follower_accounts_controller.rb


+  before_action :require_signature!, if: -> { request.format == :json && authorized_fetch_mode? }


feels deeply weird to me that we would allow people to scrape this info from the html but refuse to show it in a machine-readable format.

Stuff doesn't federate over HTML pages.

There's no way to have people able to access HTML pages without auth and also prevent scraping. Mastodon already supports not showing followers publicly, and unless federated followers lists becomes a thing in Mastodon, the HTML won't need a separate setting.

kaniini · 2019-07-11T14:41:57Z

app/controllers/accounts_controller.rb

@@ -132,4 +133,12 @@ def filtered_status_page(params)
      filtered_statuses.paginate_by_max_id(PAGE_SIZE, params[:max_id], params[:since_id]).to_a
    end
  end
+
+  def fields_selection
+    if signed_request_account.present? || !secure_mode_enabled?


This makes me nervous. Pleroma's Service actors aren't known via WebFinger. Will fetches have to be signed by an actor that Mastodon can WebFinger? If so, is it possible to change that to be pure AP?

Did you mean to comment on this particular line? It controls what JSON attributes we expose

Separately from this, we do require webfinger to process an actor/key, currently. We need to investigate what it would mean to not require that in terms of uniqueness/authority guarantees.

does Pleroma at least support webfinger for service actors via URLs (that is, https:, not acct:)?

we do require webfinger to process an actor/key, currently. We need to investigate what it would mean to not require that in terms of uniqueness/authority guarantees.

wouldn't it be the same? webfinger queries a domain for a resource, but having a direct url should effectively skip the webfinger step, since you already know the resource.

Webfinger is host-wide by spec while there is no guarantee the URL path space of a host is not shared by independent entities that could try to impersonate each other... At least I believe that was the original argumentation

the security model that makes most sense for the fediverse is that whatever software is running at the root of a domain has control over all keys associated with that domain. while it is possible that some implementations don't work like this, those implementations don't presently exist (at least not in the mainstream).

thusly, it does not really matter if entities in the path space can impersonate each other, obviously they can, because the software itself controls the keys anyway.

the reason why Pleroma does not publish it's service actors over WebFinger is because we do not want people interacting with them. they are meant to be used for internal transactions (like relaying presently), and for cases such as handling whatever authorization token is required for fetching remote objects (presently signatures, but ideally bearcaps in the near future) :)

as for why i selected this line, it's just the line that brought this thought up about how the signatures will be enforced.

it is pointless for mastodon to try to enforce access control based on what key belonging to a remote host is used to fetch the object: once the remote peer holds the object, it can either respect your wishes, or not. thusly, i believe it makes most sense to simply never have the metadata exposure in the first place and sign all fetches with a designated instance-wide actor (such as Account.representative). this makes the implementation simpler and more robust and doesn't make the implementation try to do things that are ultimately irrelevant anyway.

the question is not whether the software trusts a given user on an instance, but whether the software trusts the instance itself.

ClearlyClaire · 2019-07-12T09:44:55Z

That is true in practice, however: 1. I really dislike relying on this instance-centric view in non-obvious ways, especially as this is completely alien to the current specs 2. There *are* cases we want private toots to be fetchable, and trying to figure out whether the instance has an actor that is allowed to access it seems less obvious and natural than just checking the requesting author

kaniini · 2019-07-12T09:49:13Z

I'm only talking about public posts in this context.

But if you want to go there, those posts should also be fetched using a negotiated bearcap, which could be issued in the Accept Follow message sent back to the user when a follow gets ACKed.

I see this signature stuff strictly as a bandaid, so I don't think it really matters that the bandaid is alien to what is in the current spec. It's a mitigation, with a limited life cycle.

kaniini · 2019-07-12T09:49:54Z

(and to be clear if you want to fetch private posts from Pleroma, we can talk about doing that, but I only want to talk about doing that with a bearcap. again, I see this as a mitigation and only a mitigation.)

kaniini · 2019-07-12T09:51:23Z

a thought I had on removing the webfinger requirement for signatures would be to add an actor field to the signature itself. then the actor can be fetched and the signature can be checked against keys belonging to the actor. this solves impersonation nicely: actor A says he signed it, that attestation is provable with the signature.

anotak · 2019-08-29T03:49:10Z

hey, is this documented anywhere? what are the drawbacks of having this on? i assume there are some, right, because why is it not on by default?

* Add HTTP signature requirement for served ActivityPub resources * Change `SECURE_MODE` to `AUTHORIZED_FETCH` * Add 'Signature' to 'Vary' header and improve code style * Improve code style by adding `public_fetch_mode?` method

Gargron added work in progress Not to be merged, currently being worked on activitypub Protocol-related changes, federation labels Jul 8, 2019

Gargron force-pushed the feature-require-activitypub-fetch-authentication branch from f005e1d to 4ef76e6 Compare July 9, 2019 03:20

Gargron removed the work in progress Not to be merged, currently being worked on label Jul 9, 2019

Gargron force-pushed the feature-require-activitypub-fetch-authentication branch from 4ef76e6 to 683984a Compare July 9, 2019 03:22

Gargron changed the title ~~Add HTTP signature requirement for served ActivityPub resources~~ Add ActivityPub secure mode Jul 9, 2019

Gargron force-pushed the feature-require-activitypub-fetch-authentication branch 2 times, most recently from c87c869 to 7fa5e8a Compare July 9, 2019 03:59

Gargron marked this pull request as ready for review July 9, 2019 03:59

Add HTTP signature requirement for served ActivityPub resources

a4cd6a0

Gargron force-pushed the feature-require-activitypub-fetch-authentication branch from 7fa5e8a to a4cd6a0 Compare July 9, 2019 04:07

BenLubar reviewed Jul 9, 2019

View reviewed changes

app/controllers/activitypub/collections_controller.rb Outdated Show resolved Hide resolved

ClearlyClaire reviewed Jul 10, 2019

View reviewed changes

app/controllers/activitypub/inboxes_controller.rb Show resolved Hide resolved

Change SECURE_MODE to AUTHORIZED_FETCH

b635964

Gargron force-pushed the feature-require-activitypub-fetch-authentication branch from bae03d9 to b635964 Compare July 10, 2019 16:06

Gargron mentioned this pull request Jul 10, 2019

Add HTTP signatures to all outgoing ActivityPub GET requests #11284

Merged

nightpool requested changes Jul 11, 2019

View reviewed changes

Add 'Signature' to 'Vary' header and improve code style

6063f4f

nightpool approved these changes Jul 11, 2019

View reviewed changes

kaniini reviewed Jul 11, 2019

View reviewed changes

Improve code style by adding public_fetch_mode? method

9c81a0f

Gargron merged commit 5bf67ca into master Jul 11, 2019

Gargron deleted the feature-require-activitypub-fetch-authentication branch July 11, 2019 18:11

Gargron mentioned this pull request Oct 26, 2019

Authenticated object fetches #9849

Closed

rinsuki mentioned this pull request Jul 10, 2021

Unable to communicate with a specific Mastodon instance misskey-dev/misskey#7592

Closed

grishka added a commit to grishka/Smithereen that referenced this pull request Sep 13, 2021

Sign outgoing GET requests for AP objects (mastodon/mastodon#11269)

a3e9b0b

aciccarello mentioned this pull request Nov 17, 2022

Add signature when replying to posts snarfed/bridgy-fed#291

Closed


		expires_in 3.minutes, public: true if params[:page].blank?
		expires_in page_requested? ? 0 : 3.minutes, public: !authorized_fetch_mode?


		before_action :require_signature!, if: -> { request.format == :json && authorized_fetch_mode? }

Add ActivityPub secure mode #11269

Add ActivityPub secure mode #11269

Conversation

Gargron commented Jul 8, 2019 • edited Loading

Gargron commented Jul 9, 2019

kaniini commented Jul 9, 2019

thebaer commented Jul 9, 2019

schmittlauch commented Jul 9, 2019

Gargron commented Jul 9, 2019

BenLubar commented Jul 9, 2019

Gargron commented Jul 9, 2019

BenLubar commented Jul 9, 2019

Gargron commented Jul 9, 2019

ClearlyClaire commented Jul 10, 2019

Gargron commented Jul 10, 2019

kaniini commented Jul 10, 2019

Gargron commented Jul 10, 2019

kaniini commented Jul 10, 2019

Gargron commented Jul 10, 2019

nightpool left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gargron Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClearlyClaire commented Jul 12, 2019 via email

kaniini commented Jul 12, 2019

kaniini commented Jul 12, 2019

kaniini commented Jul 12, 2019

anotak commented Aug 29, 2019

Gargron commented Jul 8, 2019 •

edited

Loading

Gargron Jul 11, 2019 •

edited

Loading