Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

add options to require an access_token to GET /profile and /publicRooms on CS API #5083

Merged
merged 31 commits into from
May 8, 2019

Conversation

ara4n
Copy link
Member

@ara4n ara4n commented Apr 21, 2019

This deliberately doesn't try to restrict access to profile or publicRooms data over SS API, and as such is only useful on trusted private federations.

A more generic solution would be MSC1301 or https://github.com/matrix-org/matrix-doc/issues/612, but this focuses on restricting the CS API as an immediate fix.

@ara4n ara4n requested a review from a team April 21, 2019 00:12
@codecov
Copy link

codecov bot commented Apr 21, 2019

Codecov Report

Merging #5083 into dinsic will increase coverage by 15.53%.
The diff coverage is 42.85%.

@@             Coverage Diff             @@
##           dinsic    #5083       +/-   ##
===========================================
+ Coverage   45.19%   60.72%   +15.53%     
===========================================
  Files         351      335       -16     
  Lines       35845    34695     -1150     
  Branches        0     5733     +5733     
===========================================
+ Hits        16199    21069     +4870     
+ Misses      19646    12063     -7583     
- Partials        0     1563     +1563

@codecov
Copy link

codecov bot commented Apr 21, 2019

Codecov Report

Merging #5083 into develop will increase coverage by 0.06%.
The diff coverage is 78.04%.

@@             Coverage Diff             @@
##           develop    #5083      +/-   ##
===========================================
+ Coverage    61.67%   61.73%   +0.06%     
===========================================
  Files          334      334              
  Lines        34512    34549      +37     
  Branches      5680     5688       +8     
===========================================
+ Hits         21284    21328      +44     
+ Misses       11699    11692       -7     
  Partials      1529     1529

babolivier
babolivier previously approved these changes Apr 21, 2019
Copy link
Contributor

@babolivier babolivier left a comment

Choose a reason for hiding this comment

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

lgtm

docs/sample_config.yaml Outdated Show resolved Hide resolved
@ara4n ara4n changed the title add option to require an access_token to GET /profile on CS API add options to require an access_token to GET /profile and /publicRooms on CS API Apr 21, 2019
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Why would you want to lock down these APIs in the CS API whilst still leaving the federation API wide-open? It sounds like a massive footgun to me.

synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/config/server.py Outdated Show resolved Hide resolved
synapse/rest/client/v1/room.py Outdated Show resolved Hide resolved
synapse/rest/client/v1/room.py Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Apr 23, 2019

also, the CI is failing

synapse/config/server.py Outdated Show resolved Hide resolved
@richvdh
Copy link
Member

richvdh commented Apr 23, 2019

ok I now see

as such is only useful on trusted private federations.

Which is fine, but don't really understand why you are taking this approach. Is the idea that we follow-up with another PR to lock down the SS API, or is this PR meant to stand alone? If the former, I'm concerned that by only considering the CS API here, we will end up with two options which are hard to use correctly and easy to screw up. If the latter, the config setting needs big health warnings.

@ara4n
Copy link
Member Author

ara4n commented Apr 23, 2019

but don't really understand why you are taking this approach.

as an immediate fix for private federations (i.e. DINSIC, hence this PR being against the DINSIC branch).

Is the idea that we follow-up with another PR to lock down the SS API,

yes, as per the original comment.

also, the CI is failing

yes, looks like CI being broken, given the failures relate to ASes.

@ara4n
Copy link
Member Author

ara4n commented Apr 23, 2019

so, i wrote this PR as an immediate fix to an operational problem. I think there is value in locking down the CS API independently to the SS API for people running private federations, but it's a bit of an edge case so would be better to fix the SS API too.

please can someone in @matrix-org/synapse-core take it and handle @richvdh's feedback and make it work sensibly with SS API.

My suggestion would be:

  • Add a config option to make the room directory only visible to locally registered users (i.e. restrict access via SS API)
  • Implement MSC1301 to lock down visibility of profiles.

@babolivier
Copy link
Contributor

babolivier commented Apr 23, 2019

please can someone in @matrix-org/synapse-core take it and handle @richvdh's feedback and make it work sensibly with SS API.

/me does

Implement MSC1301 to lock down visibility of profiles.

Shouldn't we wait for MSC1301 to be approved before implementing it? The config option that's already in the PR (i.e. requesting valid access tokens on the CS API for profile queries) sounds good enough to me in the meantime. Though I'm unsure about what to do with the matching federation endpoints in this case.

@babolivier babolivier dismissed their stale review April 23, 2019 16:05

useless now

@richvdh
Copy link
Member

richvdh commented Apr 23, 2019

hence this PR being against the DINSIC branch

I'm afraid I missed that. It would be helpful to call that sort of thing out explicitly, though given I evidently failed to read the PR description anyway, I'm not entirely sure what more you could have done :/. Generally I think it's the sort of problem that happens when you have special branches.

@ara4n
Copy link
Member Author

ara4n commented Apr 23, 2019

Shouldn't we wait for MSC1301 to be approved before implementing it?

I wouldn't (as per #synapse-dev). MSC1301 is a suggestion for an implementation behaviour (i.e. additional validation of profile endpoints) to be standardised into the spec. There's nothing stopping an implementation to independently provide an impl-specific option to apply additional validation to its endpoints if it chooses - just like this PR already does. MSC1301 just says "hey, why don't we all do it like that".

@ara4n
Copy link
Member Author

ara4n commented Apr 23, 2019

The config option that's already in the PR (i.e. requesting valid access tokens on the CS API for profile queries) sounds good enough to me in the meantime. Though I'm unsure about what to do with the matching federation endpoints in this case.

AIUI, the idea is to expand this PR to cover both CS & SS API behaviour to mitigate @richvdh's concern about it only covering CS API being a footgun.

@babolivier
Copy link
Contributor

babolivier commented Apr 25, 2019

So here are the config options this PR adds:

  • restrict_public_rooms_to_local_users

Requires auth to fetch the public rooms directory through the CS API and disables fetching it through the federation API.

  • require_auth_for_profile_requests

When set to true, requires that requests to /profile over the CS API are authenticated, and only returns the user's profile if the requester shares a room with the profile's owner, as per MSC1301.

MSC1301 also specifies a behaviour for federation (only returning the profile if the server asking for it shares a room with the profile's owner), but that's currently really non-trivial to do in a not too expensive way. When discussing that with @erikjohnston, we concluded that a good way to solve the issue would be to propose a MSC that adds the user ID of the profile's requester to profile queries over federation, which I plan on doing. In the current state of this PR, Synapse won't send a profile query over federation if it doesn't believe it already shares a room with the profile's owner, which should be good enough in the meantime?

I've also omitted groups from this PR (for context, a group's server sends profiles of members when previewing it), because I don't think we can realistically expect the server owning a group to be aware of whether every server asking for info on the group share a room with each member (and afaik the groups implementation isn't in its final form yet).

@babolivier
Copy link
Contributor

babolivier commented Apr 25, 2019

Also, no idea why CI is failing, despite spending some time looking into it. Possibly a race (given it only happens to one sytest on unrelated tests), but can't find the source. If someone could have another look at it to see if they can find something obviously wrong with my PR that would explain it, that'd be amazing.

Edit: Well, now that I rebased the PR against develop, all of the sytest runs succeed...

@babolivier babolivier requested a review from a team April 25, 2019 15:16
@babolivier babolivier changed the base branch from dinsic to develop April 25, 2019 15:37
@babolivier babolivier changed the base branch from develop to dinsic April 25, 2019 15:38
@richvdh
Copy link
Member

richvdh commented Apr 30, 2019

I'm going to merge develop into this branch in an attempt to get github to give me a sensible diff

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Generally looks good! A few suggestions.

synapse/config/server.py Show resolved Hide resolved
synapse/federation/transport/server.py Outdated Show resolved Hide resolved
synapse/handlers/profile.py Outdated Show resolved Hide resolved
synapse/handlers/profile.py Outdated Show resolved Hide resolved
synapse/handlers/profile.py Outdated Show resolved Hide resolved
@babolivier babolivier requested a review from a team May 1, 2019 09:57
richvdh
richvdh previously requested changes May 8, 2019
synapse/federation/transport/server.py Outdated Show resolved Hide resolved
synapse/handlers/profile.py Outdated Show resolved Hide resolved
richvdh and others added 2 commits May 8, 2019 17:03
Co-Authored-By: babolivier <contact@brendanabolivier.com>
@babolivier babolivier requested a review from a team May 8, 2019 16:24
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

lgtm!

@defer.inlineCallbacks
def on_GET(self, origin, content, query):
if self.deny_access:
Copy link
Member

Choose a reason for hiding this comment

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

feels like maybe we should have a completely different Servlet class, but <shrug>. Let's go with this!

@babolivier babolivier merged commit c0e0740 into develop May 8, 2019
anoadragon453 added a commit that referenced this pull request May 10, 2019
* develop: (45 commits)
  URL preview blacklisting fixes (#5155)
  Revert 085ae34
  Add a DUMMY stage to captcha-only registration flow
  Make Prometheus snippet less confusing on the metrics collection doc (#4288)
  Set syslog identifiers in systemd units (#5023)
  Run Black on the tests again (#5170)
  Add AllowEncodedSlashes to apache (#5068)
  remove instructions for jessie installation (#5164)
  Run `black` on per_destination_queue
  Limit the number of EDUs in transactions to 100 as expected by receiver (#5138)
  Fix bogus imports in tests (#5154)
  add options to require an access_token to GET /profile and /publicRooms on CS API (#5083)
  Do checks on aliases for incoming m.room.aliases events (#5128)
  Remove the requirement to authenticate for /admin/server_version. (#5122)
  Fix spelling in server notices admin API docs (#5142)
  Fix sample config
  0.99.3.2
  include disco in deb build target list
  changelog
  Debian: we now need libpq-dev.
  ...
@babolivier babolivier deleted the matthew/auth_profile_reqs branch September 27, 2019 09:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants