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 ActivityPub secure mode #11269

Merged
merged 4 commits into from Jul 11, 2019

Conversation

Projects
None yet
8 participants
@Gargron
Copy link
Member

commented Jul 8, 2019

  • 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 Gargron force-pushed the feature-require-activitypub-fetch-authentication branch from f005e1d to 4ef76e6 Jul 9, 2019

@Gargron Gargron force-pushed the feature-require-activitypub-fetch-authentication branch from 4ef76e6 to 683984a Jul 9, 2019

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

@Gargron Gargron force-pushed the feature-require-activitypub-fetch-authentication branch 2 times, most recently from c87c869 to 7fa5e8a Jul 9, 2019

@Gargron Gargron marked this pull request as ready for review Jul 9, 2019

@Gargron Gargron force-pushed the feature-require-activitypub-fetch-authentication branch from 7fa5e8a to a4cd6a0 Jul 9, 2019

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

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

This comment has been minimized.

Copy link

commented Jul 9, 2019

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

This comment has been minimized.

Copy link

commented Jul 9, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

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:

  1. Current servers not signing requests
  2. 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

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

  1. 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

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

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

@BenLubar

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2019

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.

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Jul 10, 2019

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 Gargron force-pushed the feature-require-activitypub-fetch-authentication branch from bae03d9 to b635964 Jul 10, 2019

@Gargron

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

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

@kaniini

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 10, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 10, 2019

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
Copy link
Collaborator

left a comment

  • 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?
@@ -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 comment has been minimized.

Copy link
@nightpool

nightpool Jul 11, 2019

Collaborator

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)

This comment has been minimized.

Copy link
@nightpool

nightpool Jul 11, 2019

Collaborator

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

This comment has been minimized.

Copy link
@Gargron

Gargron Jul 11, 2019

Author Member

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.

This comment has been minimized.

Copy link
@nightpool

nightpool Jul 11, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@Gargron

Gargron Jul 11, 2019

Author Member

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.

Show resolved Hide resolved app/controllers/accounts_controller.rb Outdated
Show resolved Hide resolved app/controllers/accounts_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?

This comment has been minimized.

Copy link
@nightpool

nightpool Jul 11, 2019

Collaborator

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

This comment has been minimized.

Copy link
@Gargron

Gargron Jul 11, 2019

Author Member

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

Show resolved Hide resolved app/controllers/activitypub/outboxes_controller.rb Outdated
@@ -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 comment has been minimized.

Copy link
@nightpool

nightpool Jul 11, 2019

Collaborator

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 comment has been minimized.

Copy link
@Gargron

Gargron Jul 11, 2019

Author Member

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


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

This comment has been minimized.

Copy link
@nightpool

nightpool Jul 11, 2019

Collaborator

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

This comment has been minimized.

Copy link
@Gargron

Gargron Jul 11, 2019

Author Member

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.


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

This comment has been minimized.

Copy link
@nightpool

nightpool Jul 11, 2019

Collaborator

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.

This comment has been minimized.

Copy link
@Gargron

Gargron Jul 11, 2019

Author Member

Stuff doesn't federate over HTML pages.

This comment has been minimized.

Copy link
@BenLubar

BenLubar Jul 11, 2019

Contributor

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.

@@ -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 comment has been minimized.

Copy link
@kaniini

kaniini Jul 11, 2019

Contributor

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?

This comment has been minimized.

Copy link
@Gargron

Gargron Jul 11, 2019

Author Member

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.

This comment has been minimized.

Copy link
@BenLubar

BenLubar Jul 11, 2019

Contributor

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

This comment has been minimized.

Copy link
@trwnh

trwnh Jul 12, 2019

Contributor

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.

This comment has been minimized.

Copy link
@Gargron

Gargron Jul 12, 2019

Author Member

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

This comment has been minimized.

Copy link
@kaniini

kaniini Jul 12, 2019

Contributor

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) :)

This comment has been minimized.

Copy link
@kaniini

kaniini Jul 12, 2019

Contributor

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.

@Gargron Gargron merged commit 5bf67ca into master Jul 11, 2019

2 checks passed

build-and-test Workflow: build-and-test
Details
codeclimate 1 fixed issue
Details

@Gargron Gargron deleted the feature-require-activitypub-fetch-authentication branch Jul 11, 2019

@ThibG

This comment has been minimized.

Copy link
Collaborator

commented Jul 12, 2019

@kaniini

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

(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

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.