From a06c18bb562749db1a175a6295e995ec877f1c92 Mon Sep 17 00:00:00 2001 From: Neil Alexander Date: Mon, 21 Sep 2020 14:55:46 +0100 Subject: [PATCH] Soft-fail (#1364) * Initial work on soft-fail * Fix state block retrieval * Copy-pasta QueryLatestEventsAndState code * Fix state lookup * Clean up * Fix up failing sytest * Linting * Update previous events SQLite insert query * Update SQLite InsertPreviousEvent properly * Hopefully fix the event references updates Co-authored-by: Kegan Dougal --- roomserver/api/wrapper.go | 10 +-- roomserver/internal/helpers/auth.go | 65 +++++++++++++++++++ roomserver/internal/input/input_events.go | 25 +++++-- .../storage/sqlite3/previous_events_table.go | 41 ++++++++++-- sytest-blacklist | 5 +- sytest-whitelist | 4 +- 6 files changed, 128 insertions(+), 22 deletions(-) diff --git a/roomserver/api/wrapper.go b/roomserver/api/wrapper.go index cc048ddd..24949fc6 100644 --- a/roomserver/api/wrapper.go +++ b/roomserver/api/wrapper.go @@ -122,15 +122,7 @@ func SendEventWithRewrite( // We will handle an event as if it's an outlier if one of the // following conditions is true: storeAsOutlier := false - if authOrStateEvent.Type() == event.Type() && *authOrStateEvent.StateKey() == *event.StateKey() { - // The event is a state event but the input event is going to - // replace it, therefore it can't be added to the state or we'll - // get duplicate state keys in the state block. We'll send it - // as an outlier because we don't know if something will be - // referring to it as an auth event, but need it to be stored - // just in case. - storeAsOutlier = true - } else if _, ok := isCurrentState[authOrStateEvent.EventID()]; !ok { + if _, ok := isCurrentState[authOrStateEvent.EventID()]; !ok { // The event is an auth event and isn't a part of the state set. // We'll send it as an outlier because we need it to be stored // in case something is referring to it as an auth event. diff --git a/roomserver/internal/helpers/auth.go b/roomserver/internal/helpers/auth.go index 524a5451..834bc0c6 100644 --- a/roomserver/internal/helpers/auth.go +++ b/roomserver/internal/helpers/auth.go @@ -16,13 +16,78 @@ package helpers import ( "context" + "fmt" "sort" + "github.com/matrix-org/dendrite/roomserver/state" "github.com/matrix-org/dendrite/roomserver/storage" "github.com/matrix-org/dendrite/roomserver/types" "github.com/matrix-org/gomatrixserverlib" ) +// CheckForSoftFail returns true if the event should be soft-failed +// and false otherwise. The return error value should be checked before +// the soft-fail bool. +func CheckForSoftFail( + ctx context.Context, + db storage.Database, + event gomatrixserverlib.HeaderedEvent, + stateEventIDs []string, +) (bool, error) { + rewritesState := len(stateEventIDs) > 1 + + var authStateEntries []types.StateEntry + var err error + if rewritesState { + authStateEntries, err = db.StateEntriesForEventIDs(ctx, stateEventIDs) + if err != nil { + return true, fmt.Errorf("StateEntriesForEventIDs failed: %w", err) + } + } else { + // Work out if the room exists. + var roomInfo *types.RoomInfo + roomInfo, err = db.RoomInfo(ctx, event.RoomID()) + if err != nil { + return false, fmt.Errorf("db.RoomNID: %w", err) + } + if roomInfo == nil || roomInfo.IsStub { + return false, nil + } + + // Then get the state entries for the current state snapshot. + // We'll use this to check if the event is allowed right now. + roomState := state.NewStateResolution(db, *roomInfo) + authStateEntries, err = roomState.LoadStateAtSnapshot(ctx, roomInfo.StateSnapshotNID) + if err != nil { + return true, fmt.Errorf("roomState.LoadStateAtSnapshot: %w", err) + } + } + + // As a special case, it's possible that the room will have no + // state because we haven't received a m.room.create event yet. + // If we're now processing the first create event then never + // soft-fail it. + if len(authStateEntries) == 0 && event.Type() == gomatrixserverlib.MRoomCreate { + return false, nil + } + + // Work out which of the state events we actually need. + stateNeeded := gomatrixserverlib.StateNeededForAuth([]gomatrixserverlib.Event{event.Unwrap()}) + + // Load the actual auth events from the database. + authEvents, err := loadAuthEvents(ctx, db, stateNeeded, authStateEntries) + if err != nil { + return true, fmt.Errorf("loadAuthEvents: %w", err) + } + + // Check if the event is allowed. + if err = gomatrixserverlib.Allowed(event.Event, &authEvents); err != nil { + // return true, nil + return true, fmt.Errorf("gomatrixserverlib.Allowed: %w", err) + } + return false, nil +} + // CheckAuthEvents checks that the event passes authentication checks // Returns the numeric IDs for the auth events. func CheckAuthEvents( diff --git a/roomserver/internal/input/input_events.go b/roomserver/internal/input/input_events.go index 0558cd76..f953a925 100644 --- a/roomserver/internal/input/input_events.go +++ b/roomserver/internal/input/input_events.go @@ -53,6 +53,20 @@ func (r *Inputer) processRoomEvent( isRejected = true } + var softfail bool + if input.Kind == api.KindBackfill || input.Kind == api.KindNew { + // Check that the event passes authentication checks based on the + // current room state. + softfail, err = helpers.CheckForSoftFail(ctx, r.DB, headered, input.StateEventIDs) + if err != nil { + logrus.WithFields(logrus.Fields{ + "event_id": event.EventID(), + "type": event.Type(), + "room": event.RoomID(), + }).WithError(err).Info("Error authing soft-failed event") + } + } + // If we don't have a transaction ID then get one. if input.TransactionID != nil { tdID := input.TransactionID @@ -88,6 +102,7 @@ func (r *Inputer) processRoomEvent( "event_id": event.EventID(), "type": event.Type(), "room": event.RoomID(), + "sender": event.Sender(), }).Debug("Stored outlier") return event.EventID(), nil } @@ -110,11 +125,13 @@ func (r *Inputer) processRoomEvent( } // We stop here if the event is rejected: We've stored it but won't update forward extremities or notify anyone about it. - if isRejected { + if isRejected || softfail { logrus.WithFields(logrus.Fields{ - "event_id": event.EventID(), - "type": event.Type(), - "room": event.RoomID(), + "event_id": event.EventID(), + "type": event.Type(), + "room": event.RoomID(), + "soft_fail": softfail, + "sender": event.Sender(), }).Debug("Stored rejected event") return event.EventID(), rejectionErr } diff --git a/roomserver/storage/sqlite3/previous_events_table.go b/roomserver/storage/sqlite3/previous_events_table.go index d28a42c6..222b53b9 100644 --- a/roomserver/storage/sqlite3/previous_events_table.go +++ b/roomserver/storage/sqlite3/previous_events_table.go @@ -18,6 +18,8 @@ package sqlite3 import ( "context" "database/sql" + "fmt" + "strings" "github.com/matrix-org/dendrite/internal/sqlutil" "github.com/matrix-org/dendrite/roomserver/storage/shared" @@ -25,10 +27,15 @@ import ( "github.com/matrix-org/dendrite/roomserver/types" ) +// TODO: previous_reference_sha256 was NOT NULL before but it broke sytest because +// sytest sends no SHA256 sums in the prev_events references in the soft-fail tests. +// In Postgres an empty BYTEA field is not NULL so it's fine there. In SQLite it +// seems to care that it's empty and therefore hits a NOT NULL constraint on insert. +// We should really work out what the right thing to do here is. const previousEventSchema = ` CREATE TABLE IF NOT EXISTS roomserver_previous_events ( previous_event_id TEXT NOT NULL, - previous_reference_sha256 BLOB NOT NULL, + previous_reference_sha256 BLOB, event_nids TEXT NOT NULL, UNIQUE (previous_event_id, previous_reference_sha256) ); @@ -45,6 +52,11 @@ const insertPreviousEventSQL = ` VALUES ($1, $2, $3) ` +const selectPreviousEventNIDsSQL = ` + SELECT event_nids FROM roomserver_previous_events + WHERE previous_event_id = $1 AND previous_reference_sha256 = $2 +` + // Check if the event is referenced by another event in the table. // This should only be done while holding a "FOR UPDATE" lock on the row in the rooms table for this room. const selectPreviousEventExistsSQL = ` @@ -55,6 +67,7 @@ const selectPreviousEventExistsSQL = ` type previousEventStatements struct { db *sql.DB insertPreviousEventStmt *sql.Stmt + selectPreviousEventNIDsStmt *sql.Stmt selectPreviousEventExistsStmt *sql.Stmt } @@ -69,6 +82,7 @@ func NewSqlitePrevEventsTable(db *sql.DB) (tables.PreviousEvents, error) { return s, shared.StatementList{ {&s.insertPreviousEventStmt, insertPreviousEventSQL}, + {&s.selectPreviousEventNIDsStmt, selectPreviousEventNIDsSQL}, {&s.selectPreviousEventExistsStmt, selectPreviousEventExistsSQL}, }.Prepare(db) } @@ -80,9 +94,28 @@ func (s *previousEventStatements) InsertPreviousEvent( previousEventReferenceSHA256 []byte, eventNID types.EventNID, ) error { - stmt := sqlutil.TxStmt(txn, s.insertPreviousEventStmt) - _, err := stmt.ExecContext( - ctx, previousEventID, previousEventReferenceSHA256, int64(eventNID), + var eventNIDs string + eventNIDAsString := fmt.Sprintf("%d", eventNID) + selectStmt := sqlutil.TxStmt(txn, s.selectPreviousEventExistsStmt) + err := selectStmt.QueryRowContext(ctx, previousEventID, previousEventReferenceSHA256).Scan(&eventNIDs) + if err != sql.ErrNoRows { + return fmt.Errorf("selectStmt.QueryRowContext.Scan: %w", err) + } + var nids []string + if eventNIDs != "" { + nids = strings.Split(eventNIDs, ",") + for _, nid := range nids { + if nid == eventNIDAsString { + return nil + } + } + eventNIDs = strings.Join(append(nids, eventNIDAsString), ",") + } else { + eventNIDs = eventNIDAsString + } + insertStmt := sqlutil.TxStmt(txn, s.insertPreviousEventStmt) + _, err = insertStmt.ExecContext( + ctx, previousEventID, previousEventReferenceSHA256, eventNIDs, ) return err } diff --git a/sytest-blacklist b/sytest-blacklist index 246e6830..2f80fc78 100644 --- a/sytest-blacklist +++ b/sytest-blacklist @@ -52,7 +52,4 @@ Inbound federation accepts a second soft-failed event Outbound federation requests missing prev_events and then asks for /state_ids and resolves the state # We don't implement lazy membership loading yet. -The only membership state included in a gapped incremental sync is for senders in the timeline - -# flakey since implementing rejected events -Inbound federation correctly soft fails events \ No newline at end of file +The only membership state included in a gapped incremental sync is for senders in the timeline \ No newline at end of file diff --git a/sytest-whitelist b/sytest-whitelist index 91516428..553df1f1 100644 --- a/sytest-whitelist +++ b/sytest-whitelist @@ -472,4 +472,6 @@ We can't peek into rooms with joined history_visibility Local users can peek by room alias Peeked rooms only turn up in the sync for the device who peeked them Room state at a rejected message event is the same as its predecessor -Room state at a rejected state event is the same as its predecessor \ No newline at end of file +Room state at a rejected state event is the same as its predecessor +Inbound federation correctly soft fails events +Inbound federation accepts a second soft-failed event \ No newline at end of file