From 27555adfdcf95ba53a951677b97e63da346d4a30 Mon Sep 17 00:00:00 2001 From: Till Faelligen Date: Tue, 10 May 2022 21:25:41 +0200 Subject: [PATCH 1/5] Fix OTK spam --- syncapi/sync/requestpool.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/syncapi/sync/requestpool.go b/syncapi/sync/requestpool.go index 8ab130911a..e81904c4d7 100644 --- a/syncapi/sync/requestpool.go +++ b/syncapi/sync/requestpool.go @@ -248,7 +248,15 @@ func (rp *RequestPool) OnIncomingSyncRequest(req *http.Request, device *userapi. defer userStreamListener.Close() giveup := func() util.JSONResponse { + syncReq.Log.Debugln("Responding to sync since client gave up or timeout was reached") syncReq.Response.NextBatch = syncReq.Since + // NOTSPEC: We should always include OTKs in sync responses, otherwise clients might upload keys + // even if that's not required. See also: + // https://github.com/matrix-org/synapse/blob/29f06704b8871a44926f7c99e73cf4a978fb8e81/synapse/rest/client/sync.py#L276-L281 + err = internal.DeviceOTKCounts(syncReq.Context, rp.keyAPI, syncReq.Device.UserID, syncReq.Device.ID, syncReq.Response) + if err != nil { + syncReq.Log.WithError(err).Error("failed to get OTK counts") + } return util.JSONResponse{ Code: http.StatusOK, JSON: syncReq.Response, From 1a438718aa19f25efae7b2759791a88812535575 Mon Sep 17 00:00:00 2001 From: Till Faelligen Date: Tue, 10 May 2022 22:09:55 +0200 Subject: [PATCH 2/5] Update comment --- syncapi/sync/requestpool.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/syncapi/sync/requestpool.go b/syncapi/sync/requestpool.go index e81904c4d7..30c490df05 100644 --- a/syncapi/sync/requestpool.go +++ b/syncapi/sync/requestpool.go @@ -250,7 +250,7 @@ func (rp *RequestPool) OnIncomingSyncRequest(req *http.Request, device *userapi. giveup := func() util.JSONResponse { syncReq.Log.Debugln("Responding to sync since client gave up or timeout was reached") syncReq.Response.NextBatch = syncReq.Since - // NOTSPEC: We should always include OTKs in sync responses, otherwise clients might upload keys + // We should always try to include OTKs in sync responses, otherwise clients might upload keys // even if that's not required. See also: // https://github.com/matrix-org/synapse/blob/29f06704b8871a44926f7c99e73cf4a978fb8e81/synapse/rest/client/sync.py#L276-L281 err = internal.DeviceOTKCounts(syncReq.Context, rp.keyAPI, syncReq.Device.UserID, syncReq.Device.ID, syncReq.Response) From 4081d11a17cf6b5e01fceb12b71ab601043004ec Mon Sep 17 00:00:00 2001 From: Till Faelligen Date: Wed, 11 May 2022 08:48:27 +0200 Subject: [PATCH 3/5] Optimize selectKeysCountSQL to only return max 100 keys --- keyserver/storage/postgres/one_time_keys_table.go | 4 +++- keyserver/storage/sqlite3/one_time_keys_table.go | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/keyserver/storage/postgres/one_time_keys_table.go b/keyserver/storage/postgres/one_time_keys_table.go index d8c76b49b9..2117efcae6 100644 --- a/keyserver/storage/postgres/one_time_keys_table.go +++ b/keyserver/storage/postgres/one_time_keys_table.go @@ -53,7 +53,9 @@ const selectKeysSQL = "" + "SELECT concat(algorithm, ':', key_id) as algorithmwithid, key_json FROM keyserver_one_time_keys WHERE user_id=$1 AND device_id=$2 AND concat(algorithm, ':', key_id) = ANY($3);" const selectKeysCountSQL = "" + - "SELECT algorithm, COUNT(key_id) FROM keyserver_one_time_keys WHERE user_id=$1 AND device_id=$2 GROUP BY algorithm" + "SELECT algorithm, COUNT(key_id) FROM " + + " (SELECT algorithm, key_id FROM keyserver_one_time_keys WHERE user_id = $1 AND device_id = $2 LIMIT 100)" + + " x GROUP BY algorithm" const deleteOneTimeKeySQL = "" + "DELETE FROM keyserver_one_time_keys WHERE user_id = $1 AND device_id = $2 AND algorithm = $3 AND key_id = $4" diff --git a/keyserver/storage/sqlite3/one_time_keys_table.go b/keyserver/storage/sqlite3/one_time_keys_table.go index d2c0b7b200..7a923d0e52 100644 --- a/keyserver/storage/sqlite3/one_time_keys_table.go +++ b/keyserver/storage/sqlite3/one_time_keys_table.go @@ -52,7 +52,9 @@ const selectKeysSQL = "" + "SELECT key_id, algorithm, key_json FROM keyserver_one_time_keys WHERE user_id=$1 AND device_id=$2" const selectKeysCountSQL = "" + - "SELECT algorithm, COUNT(key_id) FROM keyserver_one_time_keys WHERE user_id=$1 AND device_id=$2 GROUP BY algorithm" + "SELECT algorithm, COUNT(key_id) FROM " + + " (SELECT algorithm, key_id FROM keyserver_one_time_keys WHERE user_id = $1 AND device_id = $2 LIMIT 100)" + + " x GROUP BY algorithm" const deleteOneTimeKeySQL = "" + "DELETE FROM keyserver_one_time_keys WHERE user_id = $1 AND device_id = $2 AND algorithm = $3 AND key_id = $4" From 7dbdda964189f5542048c06ce5ffc6d4da1814e6 Mon Sep 17 00:00:00 2001 From: Till Faelligen Date: Wed, 11 May 2022 08:48:52 +0200 Subject: [PATCH 4/5] Return CurrentPosition if the request timed out --- syncapi/sync/requestpool.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/syncapi/sync/requestpool.go b/syncapi/sync/requestpool.go index 30c490df05..048b57b9ba 100644 --- a/syncapi/sync/requestpool.go +++ b/syncapi/sync/requestpool.go @@ -250,6 +250,13 @@ func (rp *RequestPool) OnIncomingSyncRequest(req *http.Request, device *userapi. giveup := func() util.JSONResponse { syncReq.Log.Debugln("Responding to sync since client gave up or timeout was reached") syncReq.Response.NextBatch = syncReq.Since + + // The request timed out and wasn't woken up by the streams, advance NextBatch to avoid hitting + // /sync and returning immediately + if !timer.Stop() { + syncReq.Log.Traceln("Advancing next batch, as request wasn't woken by streams") + syncReq.Response.NextBatch = rp.Notifier.CurrentPosition() + } // We should always try to include OTKs in sync responses, otherwise clients might upload keys // even if that's not required. See also: // https://github.com/matrix-org/synapse/blob/29f06704b8871a44926f7c99e73cf4a978fb8e81/synapse/rest/client/sync.py#L276-L281 From eb866ef47f250acab55062f06ef403f71830e8a0 Mon Sep 17 00:00:00 2001 From: Till Faelligen Date: Wed, 11 May 2022 09:04:14 +0200 Subject: [PATCH 5/5] Revert "Return CurrentPosition if the request timed out" This reverts commit 7dbdda964189f5542048c06ce5ffc6d4da1814e6. --- syncapi/sync/requestpool.go | 7 ------- 1 file changed, 7 deletions(-) diff --git a/syncapi/sync/requestpool.go b/syncapi/sync/requestpool.go index 048b57b9ba..30c490df05 100644 --- a/syncapi/sync/requestpool.go +++ b/syncapi/sync/requestpool.go @@ -250,13 +250,6 @@ func (rp *RequestPool) OnIncomingSyncRequest(req *http.Request, device *userapi. giveup := func() util.JSONResponse { syncReq.Log.Debugln("Responding to sync since client gave up or timeout was reached") syncReq.Response.NextBatch = syncReq.Since - - // The request timed out and wasn't woken up by the streams, advance NextBatch to avoid hitting - // /sync and returning immediately - if !timer.Stop() { - syncReq.Log.Traceln("Advancing next batch, as request wasn't woken by streams") - syncReq.Response.NextBatch = rp.Notifier.CurrentPosition() - } // We should always try to include OTKs in sync responses, otherwise clients might upload keys // even if that's not required. See also: // https://github.com/matrix-org/synapse/blob/29f06704b8871a44926f7c99e73cf4a978fb8e81/synapse/rest/client/sync.py#L276-L281