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

MSC2461: Proposal for Authenticated Content Repository API #2461

Open
wants to merge 3 commits into
base: master
from

Conversation

@SyrupThinker
Copy link

SyrupThinker commented Mar 15, 2020

Signed-off-by: Valentin Anger <syrupthinker@gryphno.de>
@SyrupThinker SyrupThinker changed the title Proposal for Authenticated Content Repository API MSC2461: Proposal for Authenticated Content Repository API Mar 15, 2020
@turt2live turt2live self-requested a review Mar 15, 2020
Copy link
Contributor

Sorunome left a comment

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 |

This comment has been minimized.

Copy link
@Sorunome

Sorunome Mar 15, 2020

Contributor

What does this mean? Basically all users in the room can see it, but noone else?

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 15, 2020

Author

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 |

This comment has been minimized.

Copy link
@Sorunome

Sorunome Mar 15, 2020

Contributor

Is this even needed? It sounds odd - someone in the room accesses the content and then suddenly everyone can as it is cached

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 15, 2020

Author

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.

This comment has been minimized.

Copy link
@Sorunome

Sorunome Mar 15, 2020

Contributor

Added where? To which endpoint? Down there you have that as a reply and not as sending, so something is off

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 15, 2020

Author

Added endpoint in new revision

}
```

Clients can decide based on this

This comment has been minimized.

Copy link
@Sorunome

Sorunome Mar 15, 2020

Contributor

why not add the authentification type to m.file etc.? So like

{
  "url": "blah",
  "info": { ... },
  "authenticated": "m.local"
}

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 15, 2020

Author

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>
@helaan

This comment has been minimized.

Copy link

helaan commented Mar 21, 2020

I've read this MSC and I think I've found a few issues related to the modes and interactions between them.

  • If the mode used is m.local, authentication is needed for cached content. This does not restrict access, as they are still available from the origin homeserver. It only prevents others sapping bandwidth.
  • If the mode used is m.cached, local media needs authentication. This means that everyone who can authenticate with that server can get the media. So if another server can authenticate with this server, it can cache the media and then possibly send it out unauthenticated unless it has mode m.local or m.none.
  • If the mode used is m.none, servers are not allowed to cache the content and as I don't see a way for users to authenticate with a different homeserver (nor do I think that is desireable), media sent by users of that homeserver cannot be viewed by users on other homeservers.
  • m.unspecified is unclear to me, null/missing is the status quo.

Please correct me if I'm wrong. I'm not well versed in the Matrix protocol, so I'm probably wrong.

`GET /_matrix/media/r0/config`.
It specifies what content can be accessed unauthenticated.

The following behaviours are defined:

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 21, 2020

Author

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. Maybe m.stored is less confusing?
  • m.none and null / 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".

This comment has been minimized.

Copy link
@helaan

helaan Mar 21, 2020

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 on m.cached being comparable to m.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?

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 21, 2020

Author

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

Copy link
Member

turt2live left a comment

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

This comment has been minimized.

Copy link
@turt2live

turt2live Mar 27, 2020

Member

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

This comment has been minimized.

Copy link
@turt2live

turt2live Mar 27, 2020

Member

generally we should be naming things in the positive:

Suggested change
the field m.media.unauthenticated is added to
the field m.media.authenticated is added to

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 28, 2020

Author

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

This comment has been minimized.

Copy link
@turt2live

turt2live Mar 28, 2020

Member

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.

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 28, 2020

Author

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.

This comment has been minimized.

Copy link
@turt2live

turt2live Mar 27, 2020

Member

There is no federation API for this.

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 28, 2020

Author

I figured that because servers use the client API for media downloads they could also use it for the media config endpoint

This comment has been minimized.

Copy link
@turt2live

turt2live Mar 28, 2020

Member

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.

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 28, 2020

Author

Then this proposal is the time to specify that I'd say. Sharing media will be hard.


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

This comment has been minimized.

Copy link
@turt2live

turt2live Mar 27, 2020

Member

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:

This comment has been minimized.

Copy link
@turt2live

turt2live Mar 27, 2020

Member

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.

}
```

## Potential issues

This comment has been minimized.

Copy link
@turt2live

turt2live Mar 27, 2020

Member

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.

This comment has been minimized.

Copy link
@SyrupThinker

SyrupThinker Mar 28, 2020

Author

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.