matrix-org / matrix-spec

The Matrix protocol specification
Apache License 2.0
171 stars 91 forks source link

Clarify how implementations of federation `/send` are to apply Server Access Control Lists on a "per PDU basis" #1784

Open Gnuxie opened 2 months ago

Gnuxie commented 2 months ago

Link to problem area: https://spec.matrix.org/v1.10/server-server-api/#server-access-control-lists-acls https://spec.matrix.org/v1.10/client-server-api/#mroomserver_acl https://spec.matrix.org/v1.10/server-server-api/#checks-performed-on-receipt-of-a-pdu

Issue The description of the m.room.server_acl event says the following

The ACLs are applied to servers when they make requests

Note: Server ACLs do not restrict the events relative to the room DAG via authorisation rules, but instead act purely at the network layer to determine which servers are allowed to connect and interact with a given room.

The specification is incorrect that Server ACLs are only applied to the network layer, because in the server-server spec describing which endpoints need to be protected, the spec says:

When a remote server makes a request, it MUST be verified to be allowed by the server ACLs. If the server is denied access to a room, the receiving server MUST reply with a 403 HTTP status code and an errcode of M_FORBIDDEN.

The following endpoint prefixes MUST be protected:

/_matrix/federation/v1/send (on a per-PDU basis)

Which implies that federation/v1/send cannot reply with 403 and M_FORBIDDEN. If we make a reasonable assumption of the specification's intent, then we can assume that implementations must fail PDUs but it is unclear how or when with regards to the section describing checks performed on PDUs.

It is also unclear whether events are failed only when the federation send request originates from a server that is denied by the server ACL or whether the PDU itself originates from a server denied by the server ACL. It is understood to me that implementations can only reasonably perform the former, because backfilling is unprotected from server ACL. The reason being you would otherwise cause all historical events from a denied server to fail a per-PDU server ACL check.

Possible solution

A possible solution could be adding a step after 2. Passes signature checks, otherwise it is dropped. that checks the event's origin against the current state for m.room.server_acl when the request origin also matches against the current state for m.room.server_acl.

Gnuxie commented 2 months ago

Related: https://github.com/matrix-org/matrix-spec/issues/1783

richvdh commented 2 months ago

The specification is incorrect that Server ACLs are only applied to the network layer,

I think it depends how you interpret "at the network layer". The point is that the ACLs are applied during the /send transaction, without particular regard to the events in question.

The only reason that /send is special here is because it can contain events for more than one room, some of which must be accepted, so we can't 403 the entire request. Instead, individual events are listed with errors in the /send response.

richvdh commented 2 months ago

... and the check is based on the server making the /send request, not the server mentioned in the sender of the event in question. (Indeed, we have no way of knowing if the sender is genuine or forged without doing a signature verification, which could involve requesting keys for the sender, which is all way too much activity if the /send request itself came from a server which is banned in the room in question.)

Gnuxie commented 2 months ago

Ok, with that in mind, would you be willing to accept a clarification to https://spec.matrix.org/v1.10/server-server-api/#transactions that specifies this behavior? Maybe it would be clearer as a subsection (4.1)? Where do you think is most appropriate?

richvdh commented 2 months ago

I would welcome a clarifying PR.

I'd suggest a paragraph or two within https://spec.matrix.org/v1.10/server-server-api/#server-access-control-lists-acls, and then give it a brief mention with a link "for more details" in https://spec.matrix.org/v1.10/server-server-api/#server-access-control-lists-acls.