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

MSC3593: Safety Controls through a generic Administration API #3593

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

ShadowJonathan
Copy link
Contributor

@ShadowJonathan ShadowJonathan commented Dec 25, 2021

@ShadowJonathan ShadowJonathan changed the title MSCXXXX: Safety Controls through a generic Administration API MSC3593: Safety Controls through a generic Administration API Dec 25, 2021
proposals/3593-admin-safety-controls.md Outdated Show resolved Hide resolved
proposals/3593-admin-safety-controls.md Outdated Show resolved Hide resolved
proposals/3593-admin-safety-controls.md Outdated Show resolved Hide resolved
@uhoreg uhoreg added client-server Client-Server API needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal proposal-in-review labels Dec 25, 2021
@turt2live turt2live added the kind:feature MSC for not-core and not-maintenance stuff label Dec 25, 2021
Comment on lines +305 to +306
All of these APIs work off of a user's normal access token, any compromise would enable the attacker
access to this API surface. (Note: This was already the case with synapse's Admin API)
Copy link
Member

Choose a reason for hiding this comment

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

This is the reason that it is recommended to not expose this to the Internet via a reverse proxy (see documentation), and related to why these endpoints are on /_synapse/admin -- this allows easily configuring access controls for any endpoints under that.

I wonder if it would make more sense to place all of these under /_matrix/admin instead? That seems to have obvious downsides though (e.g. if you wanted admin endpoints for a push server or something).

Anyway, this probably needs some words about whether this is meant to be exposed to clients or if it is meant for use in other ways and is just about standardization across homeserver ecosystems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is primarily meant to be exposed to clients, to make matrix administration simple and straight-forward. I think that security by obscurity (by exposing it on a different domain), or not exposing this at all (which defeats the purpose of these endpoints in the first place), is not an option.

However, I think that a large part of these problems could potentially be mitigated by introducing a mechanism which forces the admin API to be secure even if an attacker has an access token, and for this, I was indeed thinking that the Capable User would have to re-assess their password, just as they would to change their password in the normal endpoints.

For this, maybe an additional endpoint could be introduced which enables "sudo mode", just as github would request when you change or look at sensitive settings, this'd either enable the access token that "sudo mode", or returns an additional token which is valid for such a period. I don't know if this additional complexity in this way is reasonable for the spec, i'm open to finding more ergonomic ways of doing it.

One problem i already see, for example, is that some accounts might not have passwords at all, and attest their identity via third-party SSO, so such a "sudo mode" endpoint would have to be generic across multiple solutions.


The main takeaway is that i'd like for matrix administration to be simple, yet secure, as simple as it can be to be implementable in all clients, accessible via the same CSAPI domain it is pointed to. I get that for larger servers, of course, security has to be more professional and more strict, but that is what this API would allow; modular access which is then set up in a server-specific way.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it is good to standardise the administration and to organise and build up the URLs conceptually.
IMO the API to not expose is not "security by obscurity". Security by obscurity is when you don't document the API or the urls are random long strings similar to the media files in Matrix.
In this case it is an out of band management.
There is no such thing as 100% security. Not even with a web application firewall. Therefore, the consistent separation of admin and user traffic greatly increases security. In this way, each administrator can decide for themselves whether users should administer via app or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would also just like the APIs to be under /_matrix/admin for visibility. Those APIs need a separate set of permissions and shouldn't be required for normal client applications, but are rather used to manage a server. As such all such endpoints should be under /admin to separate them conceptually. Not from a security or anything stand point, just because I think it belongs there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wonder if it would make more sense to place all of these under /_matrix/admin instead? That seems to have obvious downsides though (e.g. if you wanted admin endpoints for a push server or something).

Apologies, i didn't realise what you said here earlier.

So, essentially, you're proposing that this is a whole new branch of API, on-par with CS and SS APIs? That could make sense imo, but it would also raise the bar of scrutiny on this proposal.

I'll think about it, the idea is sound, but i also don't know if this specifically resolves the concern of user compromise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's been a few months now, and I've had time to think about this.

I think that adding these endpoints under a separate API prefix can help with some security aspects.

Additionally, I think that I will add a section to this proposal outlining a compromise between ease and security; Allowing the admin API bearer token to be and-or the user's access token, or a specific token retrieved via another out-of-band means.

This would allow server administration to become easy for simple servers, such as conduit servers, where the server would listen on the same hostname for authentication through bearer tokens.

However, on a large server (such as matrix.org), the endpoints could be bundled together under a different domain, by a different token process, not bundled to someone's access token.

It'd probably also be possible to "bless" a specific (refresh/access) token - also via out-of-band. These are all possible implementation details, however it should be clear that a simple discovery mechanism is to call /capabilities and observe a return. Then within clients there could be a menu section to input this admin API, both hostname and token, for the user's client.

I hope this could be an acceptable resolution, at the cost of making this proposal a bit more complex. I'll submit these changes and ask y'all to make another round of comments on this new approach afterwards.

@ShadowJonathan
Copy link
Contributor Author

I've discovered matrix-org/matrix-spec#725 and matrix-org/synapse#5323, which touch on related subjects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client-server Client-Server API kind:feature MSC for not-core and not-maintenance stuff needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants