From be8be535f73e51a29cfa30f1eac266a7a08b695b Mon Sep 17 00:00:00 2001 From: David Baker Date: Thu, 30 Jun 2016 17:51:28 +0100 Subject: [PATCH 01/17] requestToken update Don't send requestToken request to untrusted ID servers Also correct the THREEPID_IN_USE error to add the M_ prefix. This is a backwards incomaptible change, but the only thing using this is the angular client which is now unmaintained, so it's probably better to just do this now. --- synapse/api/errors.py | 3 ++- synapse/handlers/identity.py | 41 ++++++++++++++++++++++++------------ 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/synapse/api/errors.py b/synapse/api/errors.py index b106fbed6d..b219b46a4b 100644 --- a/synapse/api/errors.py +++ b/synapse/api/errors.py @@ -42,8 +42,9 @@ class Codes(object): TOO_LARGE = "M_TOO_LARGE" EXCLUSIVE = "M_EXCLUSIVE" THREEPID_AUTH_FAILED = "M_THREEPID_AUTH_FAILED" - THREEPID_IN_USE = "THREEPID_IN_USE" + THREEPID_IN_USE = "M_THREEPID_IN_USE" INVALID_USERNAME = "M_INVALID_USERNAME" + SERVER_NOT_TRUSTED = "M_SERVER_NOT_TRUSTED" class CodeMessageException(RuntimeError): diff --git a/synapse/handlers/identity.py b/synapse/handlers/identity.py index 656ce124f9..559e5d5a71 100644 --- a/synapse/handlers/identity.py +++ b/synapse/handlers/identity.py @@ -21,7 +21,7 @@ from synapse.api.errors import ( ) from ._base import BaseHandler from synapse.util.async import run_on_reactor -from synapse.api.errors import SynapseError +from synapse.api.errors import SynapseError, Codes import json import logging @@ -41,6 +41,20 @@ class IdentityHandler(BaseHandler): hs.config.use_insecure_ssl_client_just_for_testing_do_not_use ) + def _should_trust_id_server(self, id_server): + if id_server not in self.trusted_id_servers: + if self.trust_any_id_server_just_for_testing_do_not_use: + logger.warn( + "Trusting untrustworthy ID server %r even though it isn't" + " in the trusted id list for testing because" + " 'use_insecure_ssl_client_just_for_testing_do_not_use'" + " is set in the config", + id_server, + ) + else: + return False + return True + @defer.inlineCallbacks def threepid_from_creds(self, creds): yield run_on_reactor() @@ -59,19 +73,12 @@ class IdentityHandler(BaseHandler): else: raise SynapseError(400, "No client_secret in creds") - if id_server not in self.trusted_id_servers: - if self.trust_any_id_server_just_for_testing_do_not_use: - logger.warn( - "Trusting untrustworthy ID server %r even though it isn't" - " in the trusted id list for testing because" - " 'use_insecure_ssl_client_just_for_testing_do_not_use'" - " is set in the config", - id_server, - ) - else: - logger.warn('%s is not a trusted ID server: rejecting 3pid ' + - 'credentials', id_server) - defer.returnValue(None) + if not self._should_trust_id_server(id_server): + logger.warn( + '%s is not a trusted ID server: rejecting 3pid ' + + 'credentials', id_server + ) + defer.returnValue(None) data = {} try: @@ -129,6 +136,12 @@ class IdentityHandler(BaseHandler): def requestEmailToken(self, id_server, email, client_secret, send_attempt, **kwargs): yield run_on_reactor() + if not self._should_trust_id_server(id_server): + raise SynapseError( + 400, "Untrusted ID server '%s'" % id_server, + Codes.SERVER_NOT_TRUSTED + ) + params = { 'email': email, 'client_secret': client_secret, From f18d7546c63ae30c4058d1ec6ab2d5c3b001d257 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Mon, 4 Jul 2016 15:48:25 +0100 Subject: [PATCH 02/17] Use a query that postgresql optimises better for get_events_around --- synapse/storage/stream.py | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/synapse/storage/stream.py b/synapse/storage/stream.py index b9ad965fd6..4dd11284e5 100644 --- a/synapse/storage/stream.py +++ b/synapse/storage/stream.py @@ -591,25 +591,28 @@ class StreamStore(SQLBaseStore): query_before = ( "SELECT topological_ordering, stream_ordering, event_id FROM events" - " WHERE room_id = ? AND (topological_ordering < ?" - " OR (topological_ordering = ? AND stream_ordering < ?))" - " ORDER BY topological_ordering DESC, stream_ordering DESC" - " LIMIT ?" + " WHERE room_id = ? AND topological_ordering < ?" + " UNION ALL " + " SELECT topological_ordering, stream_ordering, event_id FROM events" + " WHERE room_id = ? AND topological_ordering = ? AND stream_ordering < ?" + " ORDER BY topological_ordering DESC, stream_ordering DESC LIMIT ?" ) query_after = ( "SELECT topological_ordering, stream_ordering, event_id FROM events" - " WHERE room_id = ? AND (topological_ordering > ?" - " OR (topological_ordering = ? AND stream_ordering > ?))" - " ORDER BY topological_ordering ASC, stream_ordering ASC" - " LIMIT ?" + " WHERE room_id = ? AND topological_ordering > ?" + " UNION ALL" + " SELECT topological_ordering, stream_ordering, event_id FROM events" + " WHERE room_id = ? AND topological_ordering = ? AND stream_ordering > ?" + " ORDER BY topological_ordering ASC, stream_ordering ASC LIMIT ?" ) txn.execute( query_before, ( - room_id, topological_ordering, topological_ordering, - stream_ordering, before_limit, + room_id, topological_ordering, + room_id, topological_ordering, stream_ordering, + before_limit, ) ) @@ -630,8 +633,9 @@ class StreamStore(SQLBaseStore): txn.execute( query_after, ( - room_id, topological_ordering, topological_ordering, - stream_ordering, after_limit, + room_id, topological_ordering, + room_id, topological_ordering, stream_ordering, + after_limit, ) ) From 8bdaf5f7afaee98a8cf25d2fb170fe4b2aa97f3d Mon Sep 17 00:00:00 2001 From: Kent Shikama Date: Tue, 5 Jul 2016 02:13:52 +0900 Subject: [PATCH 03/17] Add pepper to password hashing Signed-off-by: Kent Shikama --- synapse/config/password.py | 6 +++++- synapse/handlers/auth.py | 5 +++-- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/synapse/config/password.py b/synapse/config/password.py index dec801ef41..ea822f2bb5 100644 --- a/synapse/config/password.py +++ b/synapse/config/password.py @@ -23,10 +23,14 @@ class PasswordConfig(Config): def read_config(self, config): password_config = config.get("password_config", {}) self.password_enabled = password_config.get("enabled", True) + self.pepper = password_config.get("pepper", "") def default_config(self, config_dir_path, server_name, **kwargs): return """ # Enable password for login. password_config: enabled: true - """ + # Uncomment for extra security for your passwords. + # DO NOT CHANGE THIS AFTER INITIAL SETUP! + #pepper: "HR32t0xZcQnzn3O0ZkEVuetdFvH1W6TeEPw6JjH0Cl+qflVOseGyFJlJR7ACLnywjN9" + """ \ No newline at end of file diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index 968095c141..fd5fadf73d 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -750,7 +750,7 @@ class AuthHandler(BaseHandler): Returns: Hashed password (str). """ - return bcrypt.hashpw(password, bcrypt.gensalt(self.bcrypt_rounds)) + return bcrypt.hashpw(password + self.hs.config.password_config.pepper, bcrypt.gensalt(self.bcrypt_rounds)) def validate_hash(self, password, stored_hash): """Validates that self.hash(password) == stored_hash. @@ -763,6 +763,7 @@ class AuthHandler(BaseHandler): Whether self.hash(password) == stored_hash (bool). """ if stored_hash: - return bcrypt.hashpw(password, stored_hash.encode('utf-8')) == stored_hash + return bcrypt.hashpw(password + self.hs.config.password_config.pepper, + stored_hash.encode('utf-8')) == stored_hash else: return False From 0fb76c71ac4bdd00e7524cf11668c13754d29a08 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Mon, 4 Jul 2016 19:44:55 +0100 Subject: [PATCH 04/17] Use different SQL for postgres and sqlite3 for when using multicolumn indexes --- synapse/storage/event_push_actions.py | 18 ++--- synapse/storage/stream.py | 100 +++++++++++++------------- 2 files changed, 59 insertions(+), 59 deletions(-) diff --git a/synapse/storage/event_push_actions.py b/synapse/storage/event_push_actions.py index 5f1b6f63a9..e3e2e8083e 100644 --- a/synapse/storage/event_push_actions.py +++ b/synapse/storage/event_push_actions.py @@ -16,6 +16,8 @@ from ._base import SQLBaseStore from twisted.internet import defer from synapse.util.caches.descriptors import cachedInlineCallbacks +from synapse.types import RoomStreamToken +from .stream import lower_bound import logging import ujson as json @@ -73,6 +75,9 @@ class EventPushActionsStore(SQLBaseStore): stream_ordering = results[0][0] topological_ordering = results[0][1] + token = RoomStreamToken( + topological_ordering, stream_ordering + ) sql = ( "SELECT sum(notif), sum(highlight)" @@ -80,15 +85,10 @@ class EventPushActionsStore(SQLBaseStore): " WHERE" " user_id = ?" " AND room_id = ?" - " AND (" - " topological_ordering > ?" - " OR (topological_ordering = ? AND stream_ordering > ?)" - ")" - ) - txn.execute(sql, ( - user_id, room_id, - topological_ordering, topological_ordering, stream_ordering - )) + " AND %s" + ) % (lower_bound(token, self.database_engine, inclusive=""),) + + txn.execute(sql, (user_id, room_id)) row = txn.fetchone() if row: return { diff --git a/synapse/storage/stream.py b/synapse/storage/stream.py index 4dd11284e5..23b3a40aaf 100644 --- a/synapse/storage/stream.py +++ b/synapse/storage/stream.py @@ -40,6 +40,7 @@ from synapse.util.caches.descriptors import cached from synapse.api.constants import EventTypes from synapse.types import RoomStreamToken from synapse.util.logcontext import preserve_fn +from synapse.storage.engines import PostgresEngine import logging @@ -54,25 +55,41 @@ _STREAM_TOKEN = "stream" _TOPOLOGICAL_TOKEN = "topological" -def lower_bound(token): +def lower_bound(token, engine, inclusive=""): if token.topological is None: - return "(%d < %s)" % (token.stream, "stream_ordering") + return "(%d <%s %s)" % (token.stream, inclusive, "stream_ordering") else: - return "(%d < %s OR (%d = %s AND %d < %s))" % ( + if isinstance(engine, PostgresEngine): + # Postgres doesn't optimise ``(x < a) OR (x=a AND y= %s)" % (token.stream, "stream_ordering") + return "(%d >%s %s)" % (token.stream, inclusive, "stream_ordering") else: - return "(%d > %s OR (%d = %s AND %d >= %s))" % ( + if isinstance(engine, PostgresEngine): + # Postgres doesn't optimise ``(x > a) OR (x=a AND y>b)`` as well + # as it optimises ``(x,y) > (a,b)`` on multicolumn indexes. So we + # use the later form when running against postgres. + return "((%d,%d) >%s (%s,%s))" % ( + token.topological, token.stream, inclusive, + "topological_ordering", "stream_ordering", + ) + return "(%d > %s OR (%d = %s AND %d >%s %s))" % ( token.topological, "topological_ordering", token.topological, "topological_ordering", - token.stream, "stream_ordering", + token.stream, inclusive, "stream_ordering", ) @@ -308,18 +325,22 @@ class StreamStore(SQLBaseStore): args = [False, room_id] if direction == 'b': order = "DESC" - bounds = upper_bound(RoomStreamToken.parse(from_key)) + bounds = upper_bound( + RoomStreamToken.parse(from_key), self.database_engine + ) if to_key: - bounds = "%s AND %s" % ( - bounds, lower_bound(RoomStreamToken.parse(to_key)) - ) + bounds = "%s AND %s" % (bounds, lower_bound( + RoomStreamToken.parse(to_key), self.database_engine + )) else: order = "ASC" - bounds = lower_bound(RoomStreamToken.parse(from_key)) + bounds = lower_bound( + RoomStreamToken.parse(from_key), self.database_engine + ) if to_key: - bounds = "%s AND %s" % ( - bounds, upper_bound(RoomStreamToken.parse(to_key)) - ) + bounds = "%s AND %s" % (bounds, upper_bound( + RoomStreamToken.parse(to_key), self.database_engine + )) if int(limit) > 0: args.append(int(limit)) @@ -586,35 +607,24 @@ class StreamStore(SQLBaseStore): retcols=["stream_ordering", "topological_ordering"], ) - stream_ordering = results["stream_ordering"] - topological_ordering = results["topological_ordering"] + token = RoomStreamToken( + results["topological_ordering"], + results["stream_ordering"], + ) query_before = ( "SELECT topological_ordering, stream_ordering, event_id FROM events" - " WHERE room_id = ? AND topological_ordering < ?" - " UNION ALL " - " SELECT topological_ordering, stream_ordering, event_id FROM events" - " WHERE room_id = ? AND topological_ordering = ? AND stream_ordering < ?" + " WHERE room_id = ? AND %s" " ORDER BY topological_ordering DESC, stream_ordering DESC LIMIT ?" - ) + ) % (upper_bound(token, self.database_engine, inclusive=""),) query_after = ( "SELECT topological_ordering, stream_ordering, event_id FROM events" - " WHERE room_id = ? AND topological_ordering > ?" - " UNION ALL" - " SELECT topological_ordering, stream_ordering, event_id FROM events" - " WHERE room_id = ? AND topological_ordering = ? AND stream_ordering > ?" + " WHERE room_id = ? AND %s" " ORDER BY topological_ordering ASC, stream_ordering ASC LIMIT ?" - ) + ) % (lower_bound(token, self.database_engine, inclusive=""),) - txn.execute( - query_before, - ( - room_id, topological_ordering, - room_id, topological_ordering, stream_ordering, - before_limit, - ) - ) + txn.execute(query_before, (room_id, before_limit)) rows = self.cursor_to_dict(txn) events_before = [r["event_id"] for r in rows] @@ -626,18 +636,11 @@ class StreamStore(SQLBaseStore): )) else: start_token = str(RoomStreamToken( - topological_ordering, - stream_ordering - 1, + token.topological, + token.stream - 1, )) - txn.execute( - query_after, - ( - room_id, topological_ordering, - room_id, topological_ordering, stream_ordering, - after_limit, - ) - ) + txn.execute(query_after, (room_id, after_limit)) rows = self.cursor_to_dict(txn) events_after = [r["event_id"] for r in rows] @@ -648,10 +651,7 @@ class StreamStore(SQLBaseStore): rows[-1]["stream_ordering"], )) else: - end_token = str(RoomStreamToken( - topological_ordering, - stream_ordering, - )) + end_token = str(token) return { "before": { From d44d11d864714d4d99953bdae6625973519f120f Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Tue, 5 Jul 2016 10:39:13 +0100 Subject: [PATCH 05/17] Use true/false for boolean parameter inclusive to avoid potential for sqli, and possibly make the code clearer --- synapse/storage/event_push_actions.py | 2 +- synapse/storage/stream.py | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/synapse/storage/event_push_actions.py b/synapse/storage/event_push_actions.py index e3e2e8083e..3d93285f84 100644 --- a/synapse/storage/event_push_actions.py +++ b/synapse/storage/event_push_actions.py @@ -86,7 +86,7 @@ class EventPushActionsStore(SQLBaseStore): " user_id = ?" " AND room_id = ?" " AND %s" - ) % (lower_bound(token, self.database_engine, inclusive=""),) + ) % (lower_bound(token, self.database_engine, inclusive=False),) txn.execute(sql, (user_id, room_id)) row = txn.fetchone() diff --git a/synapse/storage/stream.py b/synapse/storage/stream.py index 23b3a40aaf..56304999dc 100644 --- a/synapse/storage/stream.py +++ b/synapse/storage/stream.py @@ -55,7 +55,8 @@ _STREAM_TOKEN = "stream" _TOPOLOGICAL_TOKEN = "topological" -def lower_bound(token, engine, inclusive=""): +def lower_bound(token, engine, inclusive=False): + inclusive = "=" if inclusive else "" if token.topological is None: return "(%d <%s %s)" % (token.stream, inclusive, "stream_ordering") else: @@ -74,7 +75,8 @@ def lower_bound(token, engine, inclusive=""): ) -def upper_bound(token, engine, inclusive="="): +def upper_bound(token, engine, inclusive=True): + inclusive = "=" if inclusive else "" if token.topological is None: return "(%d >%s %s)" % (token.stream, inclusive, "stream_ordering") else: @@ -616,13 +618,13 @@ class StreamStore(SQLBaseStore): "SELECT topological_ordering, stream_ordering, event_id FROM events" " WHERE room_id = ? AND %s" " ORDER BY topological_ordering DESC, stream_ordering DESC LIMIT ?" - ) % (upper_bound(token, self.database_engine, inclusive=""),) + ) % (upper_bound(token, self.database_engine, inclusive=False),) query_after = ( "SELECT topological_ordering, stream_ordering, event_id FROM events" " WHERE room_id = ? AND %s" " ORDER BY topological_ordering ASC, stream_ordering ASC LIMIT ?" - ) % (lower_bound(token, self.database_engine, inclusive=""),) + ) % (lower_bound(token, self.database_engine, inclusive=False),) txn.execute(query_before, (room_id, before_limit)) From 507b8bb0910ef6fae9c7d9cb1405a33c4e4b6e8e Mon Sep 17 00:00:00 2001 From: Kent Shikama Date: Tue, 5 Jul 2016 18:42:35 +0900 Subject: [PATCH 06/17] Add comment to prompt changing of pepper --- synapse/config/password.py | 1 + 1 file changed, 1 insertion(+) diff --git a/synapse/config/password.py b/synapse/config/password.py index ea822f2bb5..7c5cb5f0e1 100644 --- a/synapse/config/password.py +++ b/synapse/config/password.py @@ -31,6 +31,7 @@ class PasswordConfig(Config): password_config: enabled: true # Uncomment for extra security for your passwords. + # Change to a secret random string. # DO NOT CHANGE THIS AFTER INITIAL SETUP! #pepper: "HR32t0xZcQnzn3O0ZkEVuetdFvH1W6TeEPw6JjH0Cl+qflVOseGyFJlJR7ACLnywjN9" """ \ No newline at end of file From 1ee258430724618c7014bb176186c23b0b5b06f0 Mon Sep 17 00:00:00 2001 From: Kent Shikama Date: Tue, 5 Jul 2016 19:01:00 +0900 Subject: [PATCH 07/17] Fix pep8 --- synapse/config/password.py | 2 +- synapse/handlers/auth.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/config/password.py b/synapse/config/password.py index 7c5cb5f0e1..058a3a5346 100644 --- a/synapse/config/password.py +++ b/synapse/config/password.py @@ -34,4 +34,4 @@ class PasswordConfig(Config): # Change to a secret random string. # DO NOT CHANGE THIS AFTER INITIAL SETUP! #pepper: "HR32t0xZcQnzn3O0ZkEVuetdFvH1W6TeEPw6JjH0Cl+qflVOseGyFJlJR7ACLnywjN9" - """ \ No newline at end of file + """ diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index fd5fadf73d..be46681c64 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -750,7 +750,8 @@ class AuthHandler(BaseHandler): Returns: Hashed password (str). """ - return bcrypt.hashpw(password + self.hs.config.password_config.pepper, bcrypt.gensalt(self.bcrypt_rounds)) + return bcrypt.hashpw(password + self.hs.config.password_config.pepper, + bcrypt.gensalt(self.bcrypt_rounds)) def validate_hash(self, password, stored_hash): """Validates that self.hash(password) == stored_hash. From 14362bf3590eb95a50201a84c8e16d5626b86249 Mon Sep 17 00:00:00 2001 From: Kent Shikama Date: Tue, 5 Jul 2016 19:12:53 +0900 Subject: [PATCH 08/17] Fix password config --- synapse/config/password.py | 2 +- synapse/handlers/auth.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/synapse/config/password.py b/synapse/config/password.py index 058a3a5346..00b1ea3df9 100644 --- a/synapse/config/password.py +++ b/synapse/config/password.py @@ -23,7 +23,7 @@ class PasswordConfig(Config): def read_config(self, config): password_config = config.get("password_config", {}) self.password_enabled = password_config.get("enabled", True) - self.pepper = password_config.get("pepper", "") + self.password_pepper = password_config.get("pepper", "") def default_config(self, config_dir_path, server_name, **kwargs): return """ diff --git a/synapse/handlers/auth.py b/synapse/handlers/auth.py index be46681c64..e259213a36 100644 --- a/synapse/handlers/auth.py +++ b/synapse/handlers/auth.py @@ -750,7 +750,7 @@ class AuthHandler(BaseHandler): Returns: Hashed password (str). """ - return bcrypt.hashpw(password + self.hs.config.password_config.pepper, + return bcrypt.hashpw(password + self.hs.config.password_pepper, bcrypt.gensalt(self.bcrypt_rounds)) def validate_hash(self, password, stored_hash): @@ -764,7 +764,7 @@ class AuthHandler(BaseHandler): Whether self.hash(password) == stored_hash (bool). """ if stored_hash: - return bcrypt.hashpw(password + self.hs.config.password_config.pepper, + return bcrypt.hashpw(password + self.hs.config.password_pepper, stored_hash.encode('utf-8')) == stored_hash else: return False From 252ee2d979f8814ff5bd0f9acb76b9ba3ce86b52 Mon Sep 17 00:00:00 2001 From: Kent Shikama Date: Tue, 5 Jul 2016 19:15:51 +0900 Subject: [PATCH 09/17] Remove default password pepper string --- synapse/config/password.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/synapse/config/password.py b/synapse/config/password.py index 00b1ea3df9..66f0d93eea 100644 --- a/synapse/config/password.py +++ b/synapse/config/password.py @@ -30,8 +30,7 @@ class PasswordConfig(Config): # Enable password for login. password_config: enabled: true - # Uncomment for extra security for your passwords. # Change to a secret random string. # DO NOT CHANGE THIS AFTER INITIAL SETUP! - #pepper: "HR32t0xZcQnzn3O0ZkEVuetdFvH1W6TeEPw6JjH0Cl+qflVOseGyFJlJR7ACLnywjN9" + #pepper: "" """ From b6b0132ac7cac86e8cc5457783311b4db59e5870 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Tue, 5 Jul 2016 13:55:18 +0100 Subject: [PATCH 10/17] Make get_events_around more efficient on sqlite3 --- synapse/storage/stream.py | 62 +++++++++++++++++++++++++++++++-------- 1 file changed, 49 insertions(+), 13 deletions(-) diff --git a/synapse/storage/stream.py b/synapse/storage/stream.py index 56304999dc..f18fb63c5e 100644 --- a/synapse/storage/stream.py +++ b/synapse/storage/stream.py @@ -40,7 +40,7 @@ from synapse.util.caches.descriptors import cached from synapse.api.constants import EventTypes from synapse.types import RoomStreamToken from synapse.util.logcontext import preserve_fn -from synapse.storage.engines import PostgresEngine +from synapse.storage.engines import PostgresEngine, Sqlite3Engine import logging @@ -614,19 +614,55 @@ class StreamStore(SQLBaseStore): results["stream_ordering"], ) - query_before = ( - "SELECT topological_ordering, stream_ordering, event_id FROM events" - " WHERE room_id = ? AND %s" - " ORDER BY topological_ordering DESC, stream_ordering DESC LIMIT ?" - ) % (upper_bound(token, self.database_engine, inclusive=False),) + if isinstance(self.database_engine, Sqlite3Engine): + # SQLite3 doesn't optimise ``(x < a) OR (x = a AND y < b)`` + # So we give pass it to SQLite3 as the UNION ALL of the two queries. - query_after = ( - "SELECT topological_ordering, stream_ordering, event_id FROM events" - " WHERE room_id = ? AND %s" - " ORDER BY topological_ordering ASC, stream_ordering ASC LIMIT ?" - ) % (lower_bound(token, self.database_engine, inclusive=False),) + query_before = ( + "SELECT topological_ordering, stream_ordering, event_id FROM events" + " WHERE room_id = ? AND topological_ordering < ?" + " UNION ALL" + " SELECT topological_ordering, stream_ordering, event_id FROM events" + " WHERE room_id = ? AND topological_ordering = ? AND stream_ordering < ?" + " ORDER BY topological_ordering DESC, stream_ordering DESC LIMIT ?" + ) + before_args = ( + room_id, token.topological, + room_id, token.topological, token.stream, + before_limit, + ) - txn.execute(query_before, (room_id, before_limit)) + query_after = ( + "SELECT topological_ordering, stream_ordering, event_id FROM events" + " WHERE room_id = ? AND topological_ordering > ?" + " UNION ALL" + " SELECT topological_ordering, stream_ordering, event_id FROM events" + " WHERE room_id = ? AND topological_ordering = ? AND stream_ordering > ?" + " ORDER BY topological_ordering ASC, stream_ordering ASC LIMIT ?" + ) + after_args = ( + room_id, token.topological, + room_id, token.topological, token.stream, + after_limit, + ) + else: + query_before = ( + "SELECT topological_ordering, stream_ordering, event_id FROM events" + " WHERE room_id = ? AND %s" + " ORDER BY topological_ordering DESC, stream_ordering DESC LIMIT ?" + ) % (upper_bound(token, self.database_engine, inclusive=False),) + + before_args = (room_id, before_limit), + + query_after = ( + "SELECT topological_ordering, stream_ordering, event_id FROM events" + " WHERE room_id = ? AND %s" + " ORDER BY topological_ordering ASC, stream_ordering ASC LIMIT ?" + ) % (lower_bound(token, self.database_engine, inclusive=False),) + + after_args = (room_id, after_limit) + + txn.execute(query_before, before_args) rows = self.cursor_to_dict(txn) events_before = [r["event_id"] for r in rows] @@ -642,7 +678,7 @@ class StreamStore(SQLBaseStore): token.stream - 1, )) - txn.execute(query_after, (room_id, after_limit)) + txn.execute(query_after, after_args) rows = self.cursor_to_dict(txn) events_after = [r["event_id"] for r in rows] From dd2ccee27d834107e86cc18f46a5e4d4aa88d3c9 Mon Sep 17 00:00:00 2001 From: Mark Haines Date: Tue, 5 Jul 2016 14:06:07 +0100 Subject: [PATCH 11/17] Fix typo --- synapse/storage/stream.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/stream.py b/synapse/storage/stream.py index f18fb63c5e..c08c5b9979 100644 --- a/synapse/storage/stream.py +++ b/synapse/storage/stream.py @@ -652,7 +652,7 @@ class StreamStore(SQLBaseStore): " ORDER BY topological_ordering DESC, stream_ordering DESC LIMIT ?" ) % (upper_bound(token, self.database_engine, inclusive=False),) - before_args = (room_id, before_limit), + before_args = (room_id, before_limit) query_after = ( "SELECT topological_ordering, stream_ordering, event_id FROM events" From caf33b2d9be1b992098a00ee61cf4b4009ee3a09 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2016 17:18:19 +0100 Subject: [PATCH 12/17] Protect password when registering using shared secret --- scripts/register_new_matrix_user | 11 ++++++++--- synapse/rest/client/v1/register.py | 11 +++++++---- 2 files changed, 15 insertions(+), 7 deletions(-) diff --git a/scripts/register_new_matrix_user b/scripts/register_new_matrix_user index 27a6250b14..6d055fd012 100755 --- a/scripts/register_new_matrix_user +++ b/scripts/register_new_matrix_user @@ -25,12 +25,17 @@ import urllib2 import yaml -def request_registration(user, password, server_location, shared_secret): +def request_registration(user, password, server_location, shared_secret, admin=False): mac = hmac.new( key=shared_secret, - msg=user, digestmod=hashlib.sha1, - ).hexdigest() + ) + + mac.update(user) + mac.update(password) + mac.update("admin" if admin else "notadmin") + + mac = mac.hexdigest() data = { "user": user, diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py index d791d5e07e..0eb7490e5d 100644 --- a/synapse/rest/client/v1/register.py +++ b/synapse/rest/client/v1/register.py @@ -324,6 +324,8 @@ class RegisterRestServlet(ClientV1RestServlet): raise SynapseError(400, "Shared secret registration is not enabled") user = register_json["user"].encode("utf-8") + password = register_json["password"].encode("utf-8") + admin = register_json.get("admin", None) # str() because otherwise hmac complains that 'unicode' does not # have the buffer interface @@ -331,11 +333,12 @@ class RegisterRestServlet(ClientV1RestServlet): want_mac = hmac.new( key=self.hs.config.registration_shared_secret, - msg=user, digestmod=sha1, - ).hexdigest() - - password = register_json["password"].encode("utf-8") + ) + want_mac.update(user) + want_mac.update(password) + want_mac.update("admin" if admin else "notadmin") + want_mac = want_mac.hexdigest() if compare_digest(want_mac, got_mac): handler = self.handlers.registration_handler From 651faee698d5ff4806d1e0e7f5cd4c438bf434f1 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2016 17:30:22 +0100 Subject: [PATCH 13/17] Add an admin option to shared secret registration --- scripts/register_new_matrix_user | 19 ++++++++-- synapse/handlers/register.py | 4 +- synapse/rest/client/v1/register.py | 1 + synapse/storage/registration.py | 61 +++++++++++++++++++----------- 4 files changed, 58 insertions(+), 27 deletions(-) diff --git a/scripts/register_new_matrix_user b/scripts/register_new_matrix_user index 6d055fd012..987bf32d1c 100755 --- a/scripts/register_new_matrix_user +++ b/scripts/register_new_matrix_user @@ -42,6 +42,7 @@ def request_registration(user, password, server_location, shared_secret, admin=F "password": password, "mac": mac, "type": "org.matrix.login.shared_secret", + "admin": admin, } server_location = server_location.rstrip("/") @@ -73,7 +74,7 @@ def request_registration(user, password, server_location, shared_secret, admin=F sys.exit(1) -def register_new_user(user, password, server_location, shared_secret): +def register_new_user(user, password, server_location, shared_secret, admin): if not user: try: default_user = getpass.getuser() @@ -104,7 +105,14 @@ def register_new_user(user, password, server_location, shared_secret): print "Passwords do not match" sys.exit(1) - request_registration(user, password, server_location, shared_secret) + if not admin: + admin = raw_input("Make admin [no]: ") + if admin in ("y", "yes", "true"): + admin = True + else: + admin = False + + request_registration(user, password, server_location, shared_secret, bool(admin)) if __name__ == "__main__": @@ -124,6 +132,11 @@ if __name__ == "__main__": default=None, help="New password for user. Will prompt if omitted.", ) + parser.add_argument( + "-a", "--admin", + action="store_true", + help="Register new user as an admin. Will prompt if omitted.", + ) group = parser.add_mutually_exclusive_group(required=True) group.add_argument( @@ -156,4 +169,4 @@ if __name__ == "__main__": else: secret = args.shared_secret - register_new_user(args.user, args.password, args.server_url, secret) + register_new_user(args.user, args.password, args.server_url, secret, args.admin) diff --git a/synapse/handlers/register.py b/synapse/handlers/register.py index 88c82ba7d0..8c3381df8a 100644 --- a/synapse/handlers/register.py +++ b/synapse/handlers/register.py @@ -90,7 +90,8 @@ class RegistrationHandler(BaseHandler): password=None, generate_token=True, guest_access_token=None, - make_guest=False + make_guest=False, + admin=False, ): """Registers a new client on the server. @@ -141,6 +142,7 @@ class RegistrationHandler(BaseHandler): # If the user was a guest then they already have a profile None if was_guest else user.localpart ), + admin=admin, ) else: # autogen a sequential user ID diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py index 0eb7490e5d..25d63a0b0b 100644 --- a/synapse/rest/client/v1/register.py +++ b/synapse/rest/client/v1/register.py @@ -345,6 +345,7 @@ class RegisterRestServlet(ClientV1RestServlet): user_id, token = yield handler.register( localpart=user, password=password, + admin=bool(admin), ) self._remove_session(session) defer.returnValue({ diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 5c75dbab51..4999175ddb 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -77,7 +77,7 @@ class RegistrationStore(SQLBaseStore): @defer.inlineCallbacks def register(self, user_id, token, password_hash, was_guest=False, make_guest=False, appservice_id=None, - create_profile_with_localpart=None): + create_profile_with_localpart=None, admin=False): """Attempts to register an account. Args: @@ -104,6 +104,7 @@ class RegistrationStore(SQLBaseStore): make_guest, appservice_id, create_profile_with_localpart, + admin ) self.get_user_by_id.invalidate((user_id,)) self.is_guest.invalidate((user_id,)) @@ -118,6 +119,7 @@ class RegistrationStore(SQLBaseStore): make_guest, appservice_id, create_profile_with_localpart, + admin, ): now = int(self.clock.time()) @@ -125,29 +127,42 @@ class RegistrationStore(SQLBaseStore): try: if was_guest: - txn.execute("UPDATE users SET" - " password_hash = ?," - " upgrade_ts = ?," - " is_guest = ?" - " WHERE name = ?", - [password_hash, now, 1 if make_guest else 0, user_id]) + txn.execute( + "UPDATE users SET" + " password_hash = ?," + " upgrade_ts = ?," + " is_guest = ?," + " admin = ?" + " WHERE name = ?", + (password_hash, now, 1 if make_guest else 0, admin, user_id,) + ) + self._simple_update_one_txn( + txn, + "users", + keyvalues={ + "name": user_id, + }, + updatevalues={ + "password_hash": password_hash, + "upgrade_ts": now, + "is_guest": 1 if make_guest else 0, + "appservice_id": appservice_id, + "admin": admin, + } + ) else: - txn.execute("INSERT INTO users " - "(" - " name," - " password_hash," - " creation_ts," - " is_guest," - " appservice_id" - ") " - "VALUES (?,?,?,?,?)", - [ - user_id, - password_hash, - now, - 1 if make_guest else 0, - appservice_id, - ]) + self._simple_insert_txn( + txn, + "users", + values={ + "name": user_id, + "password_hash": password_hash, + "creation_ts": now, + "is_guest": 1 if make_guest else 0, + "appservice_id": appservice_id, + "admin": admin, + } + ) except self.database_engine.module.IntegrityError: raise StoreError( 400, "User ID already taken.", errcode=Codes.USER_IN_USE From 4adf93e0f743338c929860a1384beabeae9fded8 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2016 17:34:25 +0100 Subject: [PATCH 14/17] Fix for postgres --- synapse/storage/registration.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 4999175ddb..232dcfd9eb 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -147,7 +147,7 @@ class RegistrationStore(SQLBaseStore): "upgrade_ts": now, "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, - "admin": admin, + "admin": 1 if admin else 0, } ) else: @@ -160,7 +160,7 @@ class RegistrationStore(SQLBaseStore): "creation_ts": now, "is_guest": 1 if make_guest else 0, "appservice_id": appservice_id, - "admin": admin, + "admin": 1 if admin else 0, } ) except self.database_engine.module.IntegrityError: From be3548f7e14f411b0bb4d176ea0977672ed58252 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Tue, 5 Jul 2016 17:46:51 +0100 Subject: [PATCH 15/17] Remove spurious txn --- synapse/storage/registration.py | 9 --------- 1 file changed, 9 deletions(-) diff --git a/synapse/storage/registration.py b/synapse/storage/registration.py index 232dcfd9eb..0a68341494 100644 --- a/synapse/storage/registration.py +++ b/synapse/storage/registration.py @@ -127,15 +127,6 @@ class RegistrationStore(SQLBaseStore): try: if was_guest: - txn.execute( - "UPDATE users SET" - " password_hash = ?," - " upgrade_ts = ?," - " is_guest = ?," - " admin = ?" - " WHERE name = ?", - (password_hash, now, 1 if make_guest else 0, admin, user_id,) - ) self._simple_update_one_txn( txn, "users", From 0da24cac8bde47961396f7da774d8dc8ed847107 Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 6 Jul 2016 11:04:44 +0100 Subject: [PATCH 16/17] Add null separator to hmac --- scripts/register_new_matrix_user | 2 ++ synapse/rest/client/v1/register.py | 2 ++ 2 files changed, 4 insertions(+) diff --git a/scripts/register_new_matrix_user b/scripts/register_new_matrix_user index 987bf32d1c..12ed20d623 100755 --- a/scripts/register_new_matrix_user +++ b/scripts/register_new_matrix_user @@ -32,7 +32,9 @@ def request_registration(user, password, server_location, shared_secret, admin=F ) mac.update(user) + mac.update("\x00") mac.update(password) + mac.update("\x00") mac.update("admin" if admin else "notadmin") mac = mac.hexdigest() diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py index 25d63a0b0b..83872f5f60 100644 --- a/synapse/rest/client/v1/register.py +++ b/synapse/rest/client/v1/register.py @@ -336,7 +336,9 @@ class RegisterRestServlet(ClientV1RestServlet): digestmod=sha1, ) want_mac.update(user) + want_mac.update("\x00") want_mac.update(password) + want_mac.update("\x00") want_mac.update("admin" if admin else "notadmin") want_mac = want_mac.hexdigest() From 76b18df3d95cd881017a9aa5c8473409928faecd Mon Sep 17 00:00:00 2001 From: Erik Johnston Date: Wed, 6 Jul 2016 11:16:10 +0100 Subject: [PATCH 17/17] Check that there are no null bytes in user and passsword --- synapse/rest/client/v1/register.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/synapse/rest/client/v1/register.py b/synapse/rest/client/v1/register.py index 83872f5f60..ce7099b18f 100644 --- a/synapse/rest/client/v1/register.py +++ b/synapse/rest/client/v1/register.py @@ -327,6 +327,12 @@ class RegisterRestServlet(ClientV1RestServlet): password = register_json["password"].encode("utf-8") admin = register_json.get("admin", None) + # Its important to check as we use null bytes as HMAC field separators + if "\x00" in user: + raise SynapseError(400, "Invalid user") + if "\x00" in password: + raise SynapseError(400, "Invalid password") + # str() because otherwise hmac complains that 'unicode' does not # have the buffer interface got_mac = str(register_json["mac"])