-
Notifications
You must be signed in to change notification settings - Fork 379
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
MSC2461: Proposal for Authenticated Content Repository API #2461
MSC2461: Proposal for Authenticated Content Repository API #2461
Conversation
Signed-off-by: Valentin Anger <syrupthinker@gryphno.de>
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.
Nice idea, a few things aren't clear to soru, though
| ---------- | ----------- | | ||
| null / missing | All content can be accessed unauthenticated | | ||
| m.cached | Only cached content can be accessed unauthenticated | | ||
| m.local | Only content with an authority the server is responsible for can be accessed unauthenticated | |
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.
What does this mean? Basically all users in the room can see it, but noone else?
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.
See other comment.
Does the new context clarify the meaning of this?
Because to me it is phrased very clearly.
| Enum value | Description | | ||
| ---------- | ----------- | | ||
| null / missing | All content can be accessed unauthenticated | | ||
| m.cached | Only cached content can be accessed unauthenticated | |
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 even needed? It sounds odd - someone in the room accesses the content and then suddenly everyone can as it is cached
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.
Replying to the first part of the question, as the latter is unrelated as clarified elsewhere.
This is based on Mathew's comment in synapse#2133.
One possible use case for m.cached
is that a server admin knows that all users are responsible and allows content that they accessed (and then for exampled didn't report) to be accessed by older clients and to be directly linked to for bridges or download links.
|
||
### Configuration | ||
To allow clients to predetermine whether authentication is required, | ||
the configuration field m.media.unauthenticated is added. |
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.
Added where? To which endpoint? Down there you have that as a reply and not as sending, so something is off
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.
Added endpoint in new revision
} | ||
``` | ||
|
||
Clients can decide based on this |
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.
why not add the authentification type to m.file
etc.? So like
{
"url": "blah",
"info": { ... },
"authenticated": "m.local"
}
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.
I might have miscommunicated the intent of this proposal by adding synapse#2150 considering it is only tagentally related. I've read the conversation some time ago and added it from memory. Only checking whether I cought the issues I was thinking of.
I have clarified the intent in a new revision.
Signed-off-by: Valentin Anger <syrupthinker@gryphno.de>
This comment has been minimized.
This comment has been minimized.
`GET /_matrix/media/r0/config`. | ||
It specifies what content can be accessed unauthenticated. | ||
|
||
The following behaviours are defined: |
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.
Threaded discussions are preferred @helaan
m.local
Correct, it also allows compliance with local regulations regarding the possession and distribution of illegal content.m.cached
Not quite, if the server already fetched the content (or it originated on this server), unauthenticated users can access it. Maybem.stored
is less confusing?m.none
andnull / missing
Exactly.m.unspecified
The server could use different criteria to allow access to content, that are not covered by this document.
This is basically the fallback case of "you need to check to know whether you can access something".
One might also call it arbitrary, but I didn't want the connotation of
it never being consistent.
An unpublished version contained "Each entry in the following table is a subset of the preceding ones, with m.unspecified
not fitting into this hierarchy".
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.
Ah, I'll switch to threaded replies then.
An unpublished version contained "Each entry in the following table is a subset of the preceding ones, with m.unspecified not fitting into this hierarchy".
That makes it a lot clearer, I did not pick up onm.cached
being comparable tom.local
.
About m.none
: Maybe state more clearly that this is only possible on unfederated servers?
What do you think will be the mode that servers in the public federation will use when this MSC is implemented and it is worth it to have it flexible? Personally, I think federated servers will switch to m.local
and unfederated servers will just use m.none
. Wouldn't this be simpler and as effective to have this setting depend on whether or not you have federation enabled or am I underestimating the importance of old client support?
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.
In my opinion m.none
is not just possible on unfederated servers. Having m.none
as the default for unfederated servers seems sensible, but concluding that when a server federates it will always have an accessible content repo is wrong. An admin might just not want to distribute through the media endpoints regardless.
I agree m.local
would be the most common case for federated servers.
Especially m.cached
gives nice properties like explained in the MSC. Although that could be cut and signaled through m.unspecified
, loosing the benefit of clients knowing they can link to the content directly.
Having m.unspecified
forces client and server authors to think about the possibility of rejection, something that some current clients don't do. It empowers admins to have a different rule set without clients failing when they make assumptions for the other cases.
Maybe this should be replaced or amended with "When an unknown value is encountered a client should account for the possibility of rejection".
Preserving compatibility with older clients is also a choice admins should make.
Explicitly supporting the old behavior allows this.
If I were to create the simplest form of this proposal it would just state.
All unauthenticated accesses to media endpoints may be rejected for any reason
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.
Generally is a good idea, but the footgun on exploiting your own account will need solving.
Additionally older clients and servers might encounter an unexpected error code | ||
which may lead to unknown behaviour. | ||
|
||
## Alternatives |
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.
This will need a section comparing it to MSC701
|
||
### Configuration | ||
To allow clients to predetermine whether authentication is required, | ||
the field m.media.unauthenticated is added to |
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.
generally we should be naming things in the positive:
the field m.media.unauthenticated is added to | |
the field m.media.authenticated is added to |
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.
This would lead to the inverted enum values m.non-local
and m.uncached
which are in my opinion worse than having the field name in the negative.
I'm open to suggestions for better enum names
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.
tbh when I was leaving this comment I was expecting it to be a boolean, not an enum. Not sure there's a benefit for having it be an enum at all, or even really needing this flag is needed.
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.
Well, that depends on whether servers can actually make use of that value (according to you they should not) and whether its considered useful enough for clients (as noted in the MSC).
If those are not considered useful then all that stuff can be thrown out.
### Server to server | ||
To reduce the amount of server to server communication, | ||
when one homeserver tries to fetch content from another homeserver, | ||
the configuration should first be retrieved and cached. |
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.
There is no federation API for this.
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.
I figured that because servers use the client API for media downloads they could also use it for the media config endpoint
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.
They cannot. Also they aren't supposed to be using the client endpoints, they just haven't been defined correctly in the server-side endpoints.
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.
Then this proposal is the time to specify that I'd say. Sharing media will be hard.
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.
Maybe the "federation api" could be an extra query parameter on existing download/thumbnail endpoints, which can be as simple as authed=yes/no
? Of course, servers would need to also authenticate that it is another server that is requesting such media, and not someone else.
|
||
## Potential issues | ||
Once homeservers enable this behaviour with a m.media.unauthenticated | ||
value other than null, older clients will not be able to access some content. |
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 only older clients, but many desktop clients will have a hard time accessing images now. Some of the toolkits people use don't allow them to add headers before the request, so they'd need to add special code for this to buffer the media ahead of time.
This isn't a problem we need to fix in the MSC though, just something to be aware of.
`GET /_matrix/media/r0/config`. | ||
It specifies what content can be accessed unauthenticated. | ||
|
||
The following behaviours are defined: |
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.
I'd generally just recommend we have it turned on for the client-server API regardless. There's nothing stopping a server from accepting lack of auth, and realistically the spec should fix the general case of media being locked down.
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.
Such a change should only happen after some server-server API is in place though, right?
Otherwise media couldn't be fetched from a strictly conforming server.
} | ||
``` | ||
|
||
## Potential issues |
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.
Also mentioned in MSC701 (I think, I'm going off memory) is a way to solve the authentication issue without potentially leaking your access token. Clients which have 'download this file' or 'open in new tab' buttons will need to pass along the access token via the query string. In doing so, when someone copies the link and pastes it to someone else they've exposed their account.
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.
I really try to stay away from crypto, so this is just a naive simplification of MSC701:
The authentication token is created through HMAC(access_token, media_id).
This way only the media download can be replayed.
I'm not sure how the HMAC would be verified, but that'd need to be solved for MSC701 as well.
- Specify behaviour on unknown enum values - Include points brought up by turt2live - Draw a comparison to MSC701 - Formatting Signed-off-by: Valentin Anger <syrupthinker@gryphno.de>
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.
I like this idea, but I think it has a fatal flaw and confusion with m.none
that i'd like to see resolved first.
|
||
| Enum value | Description | | ||
| ---------- | ----------- | | ||
| null / missing | All content can be accessed unauthenticated | |
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.
I'm seeing m.none
mentioned below, should that one be added here? Or what's the status on that?
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.
null
should actually be m.none
here, thanks for pointing that out.
To reduce the amount of server to server communication, | ||
when one homeserver tries to fetch content from another homeserver, | ||
the configuration should first be retrieved and cached. | ||
When the value is m.none the server should not attempt to fetch the |
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.
Does m.none
effectively mean "All content uploaded on here is non-accessible to other servers"? that breaks quite a lot of (core) assumptions in matrix, where f.e. someone setting a room avatar in a federated room can suddenly "not display" that avatar simply because the server does not allow federated propagation of that file, per server-wide rules.
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.
I assume my other comment clears the first part up, but m.none
actually means no restrictions, as in the current behaviour. m.all
would actually be the behaviour you describe, as in everything is restricted.
For the latter, yes, that is an expected consequence of this proposal.
I'd expect a user to be made aware by server owners if such media restrictions are in place.
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.
"restricted" as in "can only be accessed once authenticated"? And that any server over federation can request such media in an authenticated user's stead?
@SyrupThinker Thanks for opening this MSC! We've been considering authentication for quite a while, and really appreciate efforts to fix this particular area of the protocol. Lately we've been working on #3916 as a way forward to authentication, and are currently gearing up for acceptance on it. If there's something we haven't yet considered in MSC3916 that you think we should, let us know. It's our current proposal to close this MSC in favour of MSC3916. |
Unknown disposition 'reject'. |
@mscbot fcp close |
This FCP proposal has been cancelled by #2461 (comment). Team member @mscbot has proposed to close this. The next step is review by the rest of the tagged people: Once at least 75% of reviewers approve (and there are no outstanding concerns), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for information about what commands tagged team members can give me. |
Apologies for the lack of reply, I have only now been aware of activity on this PR. Glancing at MSC3916 I believe the concerns bringing about this proposal are acknowledged and taken into account. I don't think I have further insight as I have not been following recent Matrix development. Thank you for working out a solution to this :) |
Great to hear we've addressed things on a cursory glance, and thanks for taking a look! This MSC is currently set on a closure path which requires fairly heavy review from the SCT. If you prefer, we can close this MSC in favour of MSC3916, shortcutting a lot of the feedback steps. |
Yes, let's close this one in favor of MSC3916. |
@mscbot fcp cancel |
Rendered
FCP tickyboxes (reject/close)