calling policy servers #857
No reviewers
Labels
No labels
Bug
Cherry-picking
Database
Dependencies
Difficulty
Easy
Difficulty
Hard
Difficulty
Medium
Documentation
Enhancement
Good first issue
Help wanted
Inherited
Matrix/Administration
Matrix/Appservices
Matrix/Auth
Matrix/Client
Matrix/Core
Matrix/Federation
Matrix/MSC
Matrix/Media
Meta
Meta/Packaging
Priority
Blocking
Priority
High
Priority
Low
Security
Status
Confirmed
Status
Duplicate
Status
Invalid
Status
Needs Investigation
Wont fix
old/ci/cd
old/rust
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: continuwuation/continuwuity#857
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "nex/serving-policy"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Allows us to call MSC4284 policy servers to determine if an event should be soft failed or not.
There's some artifacts of some other branches in here, I'll want to filter those out before merge. Truly am a git mastermind
It looks mostly good aside from what happened overnight in offtopic. If you want I can handle some of these changes for you.
@ -0,0 +59,4 @@
return Ok(());
},
};
if response.recommendation == "spam" {
Are there any other recommendations we should handle there? Or is it just spam so far?
The MSC says
spam
andok
are the only two defined responses, however namespaced ones are possible. Synapse only actions if the recommendation isspam
, so we do the same@ -45,3 +45,3 @@
}
debug!("Upgrading to timeline pdu");
debug!("Upgrading pdu {} from outlier to timeline pdu", incoming_pdu.event_id);
Use structured logging parameters at the start of the log macro rather than interpolating the dynamic value into the string.
Could probably add this as a code style guide.
Would be a good idea given I've been doing this everywhere else too lol
@ -217,1 +219,4 @@
// 14-pre. If the event is not a state event, ask the policy server about it
if incoming_pdu.state_key.is_none()
&& incoming_pdu.sender().server_name() != self.services.globals.server_name()
Aren't we supposed to use the policy server to check our own events?
We aren't supposed to send federation requests to ourselves (or at least Synapse said they didn't do this, intentionally, but that was probably something to do with "unexpected behaviours")
That's not what this is doing though, you're checking the sender of the event against our server name.
To avoid sending federation requests to ourselves, compare in src/service/rooms/event_handler/call_policyserv.rs:25 the via vs our server name.
call_policyserv will be used to check ourselves later, so I can't put the check there. Upgrading an outlier PDU should only happen for remote events anyway afaik, so comparing the sender's homeserver to ours before calling the check is just a guard rail
That's the function that has the federation request, so the guard should be in that function anyway.
Are we misunderstanding each other?
Policy server != our server as self-federation isn't allowed
Current check is event sender != our server - but that check should never be false as upgrading an outlier PDU should only happen for remote events ??
I haven't checked, but are you sure our own events can never be outliers? I'm inclined to keep the check just as a safety net since it doesn't exactly cost anything to compare, but if you're sure we've never got own-outliers I can remove it
@ -218,0 +222,4 @@
&& incoming_pdu.sender().server_name() != self.services.globals.server_name()
{
debug!("Checking policy server for event {}", incoming_pdu.event_id);
let policy = self.policyserv_check(&incoming_pdu, room_id);
We're missing a timeout here
@ -218,0 +223,4 @@
{
debug!("Checking policy server for event {}", incoming_pdu.event_id);
let policy = self.policyserv_check(&incoming_pdu, room_id);
if let Err(e) = policy.await {
Can we send off the policy server check at an earlier point so that it's done in parallel to another check? Might require moving a part of this function into its own async function that can be async-joined
not really sure if doing it in parallel is a good use of resources - if an event fails the auth checks because of its own auth events, there's no poing making an additional network request to see if it's spam, for example. It only needs to call the policy server if it'll potentially be sent to the client, so if it's not going to be sent to the client there's not much point checking.
Unless you think it's worth it anyway? I'm not actually sure how heavy the cost of checking is yet
My intuition is that it is time-slow but resource-cheap, so we want to start it as early as possible. However rooting through the code here again, during normal operation the only thing in there that would be slow is state events, and this doesn't check those anyway, so ignore this for now
State events should be checked eventually, but I want to battle test it on timeline events before I start soft-failing state events, since the latter is quite dangerous in comparison
@ -701,0 +701,4 @@
if state_key.is_none() {
if prev_events.is_empty() {
warn!("Timeline event had zero prev_events, something broke.");
return Err!(Request(Unknown("Timeline event had zero prev_events.")));
Does this block belong in this PR? Should probably be tested separately
No, this is from the fix-create-auth branch, got caught up in the cherry pick
Note, the aforementioned offtopic explosion was a Meowlnir bug, not bug with this impl.
67cfd5fd99
to5ec8e378c2
5ec8e378c2
toce94e241b3
cb4a7b5b2e
to977fddf4c5
WIP: calling policy serversto calling policy servers@ -0,0 +10,4 @@
/// Returns Ok if the policy server allows the event
#[implement(super::Service)]
#[tracing::instrument(skip_all, level = "debug")]
pub async fn policyserv_check(&self, pdu: &PduEvent, room_id: &RoomId) -> Result {
I get the feeling that this should actually be a result(bool) - fail due to an error is different from rejection, and I don't think we're supposed to fail closed? For clarity at least.
And a comment more like
@ -0,0 +25,4 @@
return Ok(());
};
let via = match policyserver.via {
via? This is not a via server for routing!
4f90379ac1
todd3ace92e9
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.