Initial APIs for storing E2E room keys #2731

ara4n · 2017-12-05T23:09:46Z

Implementing the proposal in https://docs.google.com/document/d/1MOoIA9qEKIhUQ3UmKZG-loqA8e0BzgWKKlKRUGMynVc/edit#

Now ready for final review

erikjohnston

Also, doc comments are a thing that you should do

erikjohnston · 2017-12-06T10:54:14Z

synapse/api/errors.py

+    """A client has tried to upload to a non-current version of the room_keys store
+    """
+    def __init__(self, code=403, msg="Wrong room_keys version", current_version=None,
+                 errcode=Codes.WRONG_ROOM_KEYS_VERSION):


Do we really want the code, msg and errcode to be configurable?

Do we not want to force the caller to include a current_version?

have made code, msg & errcode unconfigurable, and de-optionalised current_version.

erikjohnston · 2017-12-06T10:55:07Z

synapse/handlers/e2e_room_keys.py

+
+
+class E2eRoomKeysHandler(object):
+    def __init__(self, hs):


Would be good to have a quick comment on the handler to say what its responsible for. A two line description would probably be good enough,

erikjohnston · 2017-12-06T10:56:49Z

synapse/handlers/e2e_room_keys.py

+
+    @defer.inlineCallbacks
+    def delete_room_keys(self, user_id, version, room_id, session_id):
+        yield self.store.delete_e2e_room_keys(user_id, version, room_id, session_id)


oops; added for consistency with uploads now

erikjohnston · 2017-12-06T10:59:54Z

synapse/handlers/e2e_room_keys.py

+            # go through the room_keys
+            for room_id in room_keys['rooms']:
+                for session_id in room_keys['rooms'][room_id]['sessions']:
+                    room_key = room_keys['rooms'][room_id]['sessions'][session_id]


I'd do the loop as something like:

for room_id, entry in room_keys['rooms'].iteritems(): for session_id in entry['sessions']: etc...

So that we don't have to constantly look the key up

erikjohnston · 2017-12-06T11:00:02Z

synapse/handlers/e2e_room_keys.py

+
+                    yield self._upload_room_key(
+                        user_id, version, room_id, session_id, room_key
+                    )


I'd be tempted to do some of the different rooms concurrently, given its in a lock.

i think this is preemie optimisation; we don't even have a flow yet where the bulk upload is used. so have added a comment instead.

erikjohnston · 2017-12-06T11:23:46Z

synapse/storage/e2e_room_keys.py

+
+        sessions = AutoVivification()
+        for row in rows:
+            sessions['rooms'][row['room_id']]['sessions'][row['session_id']] = {


I think this can be written as:

sessions['rooms'].setdefault(row['room_id'], {"sessions":{}})['sessions'][row['session_id']] = { ... }

Or if you don't mind that we use an extra line:

room_entry = sessions['rooms'].setdefault(row['room_id'], {"sessions":{}}) room_entry['sessions'][row['session_id']] = { ... }

thanks; done

erikjohnston · 2017-12-06T11:24:46Z

synapse/storage/e2e_room_keys.py

+        if room_id:
+            keyvalues['room_id'] = room_id
+        if session_id:
+            keyvalues['session_id'] = session_id


If room_id and session_id aren't required I think it would be polite to make them optional arguments, even if that isn't used. It makes it a lot clearer just from the signature line that the params are optional.

Also, does it make sense to have a session_id without a room_id?

erikjohnston · 2017-12-06T11:26:25Z

synapse/storage/schema/delta/46/e2e_room_keys.sql

+
+CREATE UNIQUE INDEX e2e_room_keys_user_idx ON e2e_room_keys(user_id);
+CREATE UNIQUE INDEX e2e_room_keys_room_idx ON e2e_room_keys(room_id);
+CREATE UNIQUE INDEX e2e_room_keys_session_idx ON e2e_room_keys(session_id);


Ermh, are you sure that you want these to be unique indices? What if you have multiple users uploading keys for the same room?

Looking at the code I think we can get away with a single index of (user_id, room_id, session_id), which will be much faster.

oops, fixed.

erikjohnston · 2017-12-06T11:28:42Z

synapse/storage/schema/delta/46/e2e_room_keys.sql

+    auth_data TEXT NOT NULL
+);
+
+CREATE UNIQUE INDEX e2e_room_key_user_idx ON e2e_room_keys(user_id);


This is not what you mean, I assume:

CREATE UNIQUE INDEX e2e_room_key_versions_idx ON e2e_room_key_versions(user_id);

oops, fixed

erikjohnston · 2017-12-06T11:29:27Z

synapse/storage/schema/delta/46/e2e_room_keys.sql

+CREATE UNIQUE INDEX e2e_room_keys_session_idx ON e2e_room_keys(session_id);
+
+-- the metadata for each generation of encrypted e2e session backups
+CREATE TABLE e2e_room_key_versions (


Room keys? There isn't a room_id in the table.

fixed to be called e2e_room_keys_versions which is more grammatically correct, given we are talking about versions of sets of backup of multiple room_keys here.

erikjohnston

Generally we don't have a new line at the start of functions, which you seem to be doing a lot for some reason.

erikjohnston · 2017-12-18T17:01:00Z

synapse/rest/client/v2_alpha/room_keys.py

+
+        info = yield self.e2e_room_keys_handler.get_version_info(
+            user_id, version
+        )


I didn't bother checking that version was supplied; i think it will explode fairly sensibly if it isn't?

My hunch is that it'd 500 in a fairly non obvious way tbh, or possibly succeed as the database ends up executing SELECT * FROM versions WHERE version = null. Not hugely friendly.

Not sure what the point of splitting into POST and GET/DELETE would be?

So you have a GET and DELETE that only work if a version is supplied, and a POST that only works if you don't supply a version. To save having to do checks everywhere, and to make it clearer when a version is required or disallowed, having this as separate classes seems much clearer.

And shouldn't POST have the same path anyway?

They don't really share a path as it is. If you think that POST /room_keys/version is clearer than POST /room_keys/create_version then fair enough, twas but a thought.

And yes, turns out naming APIs is hard.

erikjohnston · 2017-12-18T17:03:22Z

synapse/storage/e2e_room_keys.py

+
+    # XXX: this isn't currently used and isn't tested anywhere
+    # it could be used in future for bulk-uploading new versions of room_keys
+    # for a user or something though.


I'd rather not have to review code that isn't used, and dead code just adds bloat to the code base and makes understanding things more annoying.

Also, I think I would ask you to rewrite.

erikjohnston · 2017-12-18T17:04:11Z

synapse/storage/e2e_room_keys.py

+    def get_e2e_room_keys(
+        self, user_id, version, room_id=None, session_id=None
+    ):
+


Its probably worth documenting what happens in the presence of a null room_id and null session_id

erikjohnston · 2017-12-18T17:04:59Z

synapse/storage/e2e_room_keys.py

+    def delete_e2e_room_keys(
+        self, user_id, version, room_id=None, session_id=None
+    ):
+


Ditto with docstrings

ara4n · 2017-12-31T18:09:15Z

@erikjohnston I've:

added full docstring
added UTs
addressed all your PR feedback (i think), to date
If you can give a final check it'd be much appreciated; merry new year...

In other news (cc @richvdh @erikjohnston), this felt really painful to write - and not just because I'm a bit rusty.

In the end, this is an incredibly trivial API, for simple CRUD of some JSON objects. The only subtlety is the merge semantics when updating, but that's a whole 5 lines of code. But meanwhile, the overall PR has ended up being a 1,511 line diff (including tests), which feels insanely heavyweight for a relatively non-verbose language like Python. Stuff which felt particularly "gah we're doing this wrong" included:

The triple-layering of rest, handler & storage. It feels like the rest layer is spectacularly premature optimisation, given rest is the only transport synapse supports, and it doubles all the docstring and threading the API signatures through the stack - lots of overhead and boilerplate for pretty much zero value.
The duplication of docstring doc between all three layers.
The fact we don't have any generic ways for storing JSON in DB tables (a la glow) which this could have utilised in the first place.
The fact that docstring is completely freeform, and nothing provides any help in ensuring its correctness (let alone the pep-484 type hinting)
The fact our linting seems particularly insanely stringent - although this may be my fault for not using a pep-8 aware editor. It's very frustrating for the build to fail due to having trailing whitespace on a blank line.

Anyway, though I'd mention this, as if it's painful for me, I imagine it's also painful for other contributors...

erikjohnston · 2018-01-03T11:34:07Z

synapse/storage/e2e_room_keys.py

+                    "SELECT MAX(version) FROM e2e_room_keys_versions WHERE user_id=?",
+                    (user_id,)
+                )
+                this_version = txn.fetchone()[0]


What if there isn't a version for this userID

erikjohnston · 2018-01-03T11:36:53Z

synapse/rest/client/v2_alpha/room_keys.py

+
+        info = yield self.e2e_room_keys_handler.get_version_info(
+            user_id, version
+        )


Honestly, the thought of changing the API shape was simply a tangential suggestion. The actual suggestion was to change the code layout to better avoid having to manually handle null values of version.

Having GET /version that returns the most recent version works, but what does it return if there isn't a version for that user? What does DELETE /version do? I note that we still don't seem to be checking if the version is null in that case.

erikjohnston · 2018-01-03T11:37:31Z

synapse/storage/e2e_room_keys.py

+        keyvalues = {
+            "user_id": user_id,
+            "version": version,
+        }


Why is this a separate variable?

no idea. fixed.

erikjohnston · 2018-01-03T11:42:05Z

synapse/rest/client/v2_alpha/room_keys.py

+
+        requester = yield self.auth.get_user_by_req(request, allow_guest=False)
+        user_id = requester.user.to_string()
+        version = request.args.get("version")[0]


For query params its probably better to use parse_string and co

erikjohnston · 2018-01-03T12:05:45Z

TBH I wouldn't classify this as simple CRUD, given the merging semantics and linearization that is going on.

The triple-layering of rest, handler & storage. It feels like the rest layer is spectacularly premature optimisation, given rest is the only transport synapse supports, and it doubles all the docstring and threading the API signatures through the stack - lots of overhead and boilerplate for pretty much zero value.

The separation has been really quite useful to split out the parsing of the request versus the actual functionality. Merging rest/handlers quickly become unwieldy in most cases, and it makes skimming the rest layer to figure out what the APIs are really easy, which is something I do a lot to get back up to speed with how some bit of synapse works.

The duplication of docstring doc between all three layers.

This is a nuisance, yes, though its perhaps worth noting that the documentation on the rest layer is describing the APIs in detail, while the handler functions are describing exactly what each bit is doing under the hood.

In general I've found that even without explicit layering you end up with duplicated documentation, unless you have a single mammoth function that does everything.

The fact we don't have any generic ways for storing JSON in DB tables (a la glow) which this could have utilised in the first place.

The structure of the glow database was quite complicated, iirc, as you need to somehow magically have all the correct indices in place. It also didn't support common queries, e.g. MAX(..) which you are using.

If you simply want to store JSON in the database then that's easy─and we do it all over the place─just use json.dumps(..) and json.loads(..) and have a json column in the table.

Other than not requiring you to define a table and what indices you want, I'm not sure that it would actually buy you very much here, since you'd still have all the merging and validation logic.

The fact our linting seems particularly insanely stringent - although this may be my fault for not using a pep-8 aware editor. It's very frustrating for the build to fail due to having trailing whitespace on a blank line.

The flip side is that it makes the code so much nicer to work on. Most developers nowadays are so used to having linters that its second nature.

richvdh · 2018-01-05T07:35:58Z

In other news (cc @richvdh @erikjohnston), this felt really painful to write - and not just because I'm a bit rusty.

Not much to add to your pain points beyond what erik has already written here, all of which I agree with completely.

FWIW I see a 1366 line diff rather than 1511, of which a quarter is tests (for which yay), and, eyeballing it, it looks like fully half of it is either doc-comments (for which yay again) or copyright notices (meh).

I think that one of the reasons that synapse has been so hard to work on in the past is that we have not done well with doc-comments, which leads to a lot of spelunking if you want to change existing code. The fact that this code is now clearly documented is something to be proud of, not be sad about.

ara4n · 2018-01-07T23:50:07Z

@erikjohnston ptal.

ara4n · 2018-01-15T02:34:39Z

Ironically, there are some problems which have turned up with this whilst implementing the clientside, so I'm going to avoid merging this until I'm sure it's on the right track. Need to:

Need to include ephemeral public key alongside session_data in order to calculate the AES key
Need to Include backup public key alongside the version info, so it can be passed to new devices

richvdh · 2018-04-09T23:33:34Z

let's reopen this when it's ready, then

uhoreg · 2018-08-21T18:51:12Z

synapse/storage/e2e_room_keys.py

+class EndToEndRoomKeyStore(SQLBaseStore):
+
+    @defer.inlineCallbacks
+    def get_e2e_room_key(self, user_id, version, room_id, session_id):


This function is only used when checking to see if we should overwrite a key. Would it be better to just convert that bit of code to use get_e2e_room_keys instead, and get rid of this function?

ara4n added 4 commits December 5, 2017 01:29

total WIP skeleton for /room_keys API

824b339

interim WIP checkin; doesn't build yet

e558f21

make it work and fix pep8

7983840

document the API

602b647

ara4n assigned erikjohnston Dec 5, 2017

ara4n added 2 commits December 6, 2017 01:02

implement /room_keys/version too (untested)

552934e

make /room_keys/version work

124efee

erikjohnston suggested changes Dec 6, 2017

View reviewed changes

erikjohnston assigned ara4n and unassigned erikjohnston Dec 6, 2017

ara4n added 2 commits December 18, 2017 01:52

blindly incorporate PR review - needs testing & fixing

3e98a00

rename room_key_version table correctly, and fix opt args

356829c

erikjohnston reviewed Dec 18, 2017

View reviewed changes

ara4n added 17 commits December 24, 2017 14:19

Merge branch 'develop' into matthew/e2e_backups

59aa35b

actually spell example right

ace298f

fix factoring out of _should_replace_room_key

a658f4e

add storage docstring; remove unused set_e2e_room_keys

dee6849

more docstring for the e2e_room_keys rest

a22968d

add a tonne of docstring; make upload_room_keys properly assert version

98fcffd

add a tonne of docstring; make upload_room_keys properly assert version

fecfa63

fix typos

1aa87fc

fix flakes

68e9cfe

switch get_current_version_info back to being get_version_info

eb1a429

improve docstring

ef75daf

don't needlessly return user_id

3a8d9ab

first cut at a UT

7ab77f6

fix idiocies and so make tests pass

843ce19

linting

4fdd02f

implement remaining tests and make them work

10cec6f

flake8

ebfaeac

ara4n assigned erikjohnston and unassigned ara4n Dec 31, 2017

erikjohnston reviewed Jan 3, 2018

View reviewed changes

ara4n added 3 commits January 7, 2018 23:45

support DELETE /version with no args

1976540

404 nicely if you try to interact with a missing current version

4cd381d

use parse_string

e3bc9f2

ara4n added 2 commits January 7, 2018 23:58

missing import

c199970

missing import

f27afe1

erikjohnston approved these changes Jan 8, 2018

View reviewed changes

erikjohnston assigned ara4n and unassigned erikjohnston Jan 9, 2018

ara4n mentioned this pull request Jan 15, 2018

WIP: online incremental megolm backups matrix-org/matrix-react-sdk#1703

Closed

8 tasks

richvdh closed this Apr 9, 2018

uhoreg mentioned this pull request Aug 17, 2018

[WIP] initial tests for key backup matrix-org/sytest#486

Closed

uhoreg reviewed Aug 21, 2018

View reviewed changes

uhoreg mentioned this pull request Aug 24, 2018

E2e backups #3757

Closed

dbkr mentioned this pull request Oct 8, 2018

E2E backups #4019

Merged

Initial APIs for storing E2E room keys #2731

Initial APIs for storing E2E room keys #2731

Conversation

ara4n commented Dec 5, 2017 • edited Loading

erikjohnston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ara4n commented Dec 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erikjohnston commented Jan 3, 2018

richvdh commented Jan 5, 2018

ara4n commented Jan 7, 2018

ara4n commented Jan 15, 2018

richvdh commented Apr 9, 2018

Choose a reason for hiding this comment

ara4n commented Dec 5, 2017 •

edited

Loading