-
Notifications
You must be signed in to change notification settings - Fork 415
MSC3916: Authentication for media #3916
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
Changes from 17 commits
7606e53
3076de0
55303b5
d601637
c2ae25e
8351ebe
106ce55
a14c4af
92eba2c
a76d97f
8bb5159
e1e8a6a
71b8db4
1c864a3
e5c9316
41d2aa2
d73025b
656dfb8
87c08e0
1fe8d71
aac1909
0b4f2c9
f1777c2
4fe23e5
db90b92
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reviewers: Please compare this MSC against #2461 , which is proposed for rejection/closure. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @turt2live I think we can resolve this since #2461 is closed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it's still useful to compare the MSCs, given the ideas and concepts of 2461 are meant to be incorporated here. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,367 @@ | ||||||
| # MSC3916: Authentication for media access, and new endpoint names | ||||||
|
|
||||||
| Currently, access to media in Matrix has a number of problems including the following: | ||||||
|
|
||||||
| * The only protection for media is the obscurity of the URL, and URLs are | ||||||
| easily leaked (eg accidental sharing, access | ||||||
| logs). [synapse#2150](https://github.com/matrix-org/synapse/issues/2150) | ||||||
| * Anybody (including non-matrix users) can cause a homeserver to copy media | ||||||
| into its local | ||||||
| store. [synapse#2133](https://github.com/matrix-org/synapse/issues/2133) | ||||||
| * When a media event is redacted, the media it used remains visible to all. | ||||||
| [synapse#1263](https://github.com/matrix-org/synapse/issues/1263) | ||||||
| * There is currently no way to delete | ||||||
| media. [matrix-spec#226](https://github.com/matrix-org/matrix-spec/issues/226) | ||||||
| * If a user requests GDPR erasure, their media remains visible to all. | ||||||
| * When all users leave a room, their media is not deleted from the server. | ||||||
|
|
||||||
| These problems are all difficult to address currently, because access to media | ||||||
| is entirely unauthenticated. The first step for a solution is to require user | ||||||
| authentication. Once that is done, it will be possible to impose authorization | ||||||
| requirements to address the problems mentioned above. (See, for example, | ||||||
| [MSC3911](https://github.com/matrix-org/matrix-spec-proposals/pull/3911) which | ||||||
| builds on top of this MSC.) | ||||||
|
|
||||||
| This proposal supersedes [MSC1902](https://github.com/matrix-org/matrix-spec-proposals/pull/1902). | ||||||
|
|
||||||
| ## Proposal | ||||||
|
|
||||||
| 1. New endpoints | ||||||
|
|
||||||
| The existing `/_matrix/media/v3/` endpoints become deprecated, and new | ||||||
| endpoints under the `/_matrix/client` and `/_matrix/federation` | ||||||
| hierarchies are introduced. Removal of the deprecated endpoints would be a | ||||||
| later MSC under [the deprecation policy](https://spec.matrix.org/v1.10/#deprecation-policy). | ||||||
|
|
||||||
| The following table below shows a mapping between deprecated and new endpoint: | ||||||
|
|
||||||
| | Deprecated | Client-Server | Federation | | ||||||
| | ---------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------------------------------- | ------------------------------------------------------------------- | | ||||||
| | [`GET /_matrix/media/v3/preview_url`](https://spec.matrix.org/v1.6/client-server-api/#get_matrixmediav3preview_url) | `GET /_matrix/client/v1/media/preview_url` | - | | ||||||
| | [`GET /_matrix/media/v3/config`](https://spec.matrix.org/v1.6/client-server-api/#get_matrixmediav3config) | `GET /_matrix/client/v1/media/config` | - | | ||||||
| | [`GET /_matrix/media/v3/download/{serverName}/{mediaId}`](https://spec.matrix.org/v1.6/client-server-api/#get_matrixmediav3downloadservernamemediaid) | `GET /_matrix/client/v1/media/download/{serverName}/{mediaId}` | `GET /_matrix/federation/v1/media/download/{mediaId}` | | ||||||
| | [`GET /_matrix/media/v3/download/{serverName}/{mediaId}/{fileName}`](https://spec.matrix.org/v1.6/client-server-api/#get_matrixmediav3downloadservernamemediaidfilename) | `GET /_matrix/client/v1/media/download/{serverName}/{mediaId}/{fileName}` | - | | ||||||
| | [`GET /_matrix/media/v3/thumbnail/{serverName}/{mediaId}`](https://spec.matrix.org/v1.6/client-server-api/#get_matrixmediav3thumbnailservernamemediaid) | `GET /_matrix/client/v1/media/thumbnail/{serverName}/{mediaId}` | - | | ||||||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| **Note**: [`POST /_matrix/media/v3/upload`](https://spec.matrix.org/v1.6/client-server-api/#post_matrixmediav3upload) | ||||||
| and [`GET /_matrix/media/v1/create`](https://spec.matrix.org/v1.10/client-server-api/#post_matrixmediav1create) | ||||||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| are **not** modified or deprecated by this MSC: it is intended that they be brought into line with the other | ||||||
| endpoints by a future MSC, such as [MSC3911](https://github.com/matrix-org/matrix-spec-proposals/pull/3911). | ||||||
|
|
||||||
| **Note**: `/thumbnail` does not have a federation endpoint. It appears as though | ||||||
| no servers request thumbnails over federation, and so it is not supported here. | ||||||
| A later MSC may introduce such an endpoint. | ||||||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| The new `/download` and `/thumbnail` endpoints additionally drop the `?allow_redirect` | ||||||
| query parameters. Instead, the endpoints behave as though `allow_redirect=true` was | ||||||
| set, regardless of actual value. See [this comment on MSC3860](https://github.com/matrix-org/matrix-spec-proposals/pull/3860/files#r1005176480) | ||||||
| for details. | ||||||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
|
|
||||||
| After this proposal is released in a stable version of the specification, servers | ||||||
| which support the new `download` and `thumbnail` endpoints SHOULD cease to serve | ||||||
| newly uploaded media from the unauthenticated versions. This includes media | ||||||
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||
| uploaded by local users and requests for not-yet-cached remote media. This is | ||||||
| done with a 404 `M_NOT_FOUND` error, as though the media doesn't exist. Servers | ||||||
| SHOULD consider their local ecosystem impact before freezing the endpoints. For | ||||||
| example, ensuring that common bridges and clients will continue to work, and | ||||||
| encouraging updates to those affected projects as needed. | ||||||
|
|
||||||
| 2. Removal of `allow_remote` parameter from `/download` | ||||||
|
|
||||||
| The current | ||||||
| [`/download`](https://spec.matrix.org/v1.6/client-server-api/#get_matrixmediav3downloadservernamemediaid) | ||||||
| and | ||||||
| [`/thumbnail`](https://spec.matrix.org/v1.6/client-server-api/#get_matrixmediav3thumbnailservernamemediaid) | ||||||
| endpoints take an `allow_remote` query parameter, indicating whether the | ||||||
| server should request remote media from other servers. This is redundant | ||||||
| with the new endpoints, so will not be supported. | ||||||
|
|
||||||
| Servers MUST NOT return remote media from `GET /_matrix/federation/v1/media/download`. The | ||||||
| `serverName` is omitted from the endpoint's path to strongly enforce this - the `mediaId` in | ||||||
| a request is assumed to be scoped to the target server. | ||||||
|
|
||||||
| `/_matrix/client/v1/media/download` and | ||||||
| `/_matrix/client/v1/media/thumbnail` return remote media as normal. | ||||||
|
|
||||||
| 3. Authentication on all endpoints | ||||||
|
|
||||||
| Currently, the `/download` and `/thumbnail` endpoints have no authentication | ||||||
| requirements. Under this proposal, the new endpoints will be authenticated | ||||||
| the same way as other endpoints: they will require an `Authorization` header | ||||||
| which must be `Bearer {accessToken}` for `/_matrix/client`, or the signature | ||||||
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| for `/_matrix/federation`. | ||||||
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
|
|
||||||
| The query string `?access_token` approach is not supported on the new endpoints, | ||||||
| as it is [deprecated](https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/4126-deprecate-query-string-auth.md) | ||||||
|
||||||
| as it is [deprecated](https://github.com/matrix-org/matrix-spec-proposals/blob/main/proposals/4126-deprecate-query-string-auth.md) | |
| as it is [deprecated](https://github.com/matrix-org/matrix-spec-proposals/pull/4126) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR doesn't show any changes to accepted MSCs, unfortunately. Ideally this would be a link to the spec, but unstable has the same longevity problems.
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
tulir marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really true? What data do you have to back this up? It seems like a fairly elegant solution.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not hard data, but for the last 6 years it's felt like every third user for Element Web has disabled these things and more :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mmm, the latter seems spurious since cookies and localstorage will almost always be enabled/disabled as a pair, and Element has been relying on localstorage for years. The former is true in that setting a cookie for the homeserver would be a little weird. In my mind, the pros / cons are:
- Cookie would make all media requests magically work on that browser session, even if the user opened something (non-encrypted) in a tab manually.
- The above could cause them to think the link will work if sent to someone else, which it won't.
- It would be a slightly odd way of using cookies since this is a federated world and the HS is not necessarily the same endpoint as your client.
- It would cause every other C/S API request to also get the cookie sent, confusing auth.
- Simpler if all C/S API auth works the same way.
I'm not going to make this a concern as I don't think it's blocking, but it might be worth a quick sanity check to make sure we really are committing to the right thing.
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Show resolved
Hide resolved
turt2live marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are still planning to fast-follow with MSC3911, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully, yes. It won't be in the same spec release though.
Uh oh!
There was an error while loading. Please reload this page.