[E2E] Upload keys #233

Zil0 · 2018-06-13T09:24:11Z

Second part of the crypto stuff.

Signed-off-by: Valentin Deniaud <valentin.deniaud@inpt.fr>

non-Jedi

Didn't have time to review tests, but in interest of getting things merged before your GSOC is over, I'm assuming those are correct. Tests in general have been excellent so far.

non-Jedi · 2018-06-29T18:10:55Z

matrix_client/crypto/olm_device.py

+        }
+        self.sign_json(device_keys)
+        ret = self.api.upload_keys(device_keys=device_keys)
+        self.one_time_key_counts = ret['one_time_key_counts']


Is there a situation where the server can return key counts for one type of key but not others? Seems like there could be.

This happens when the server has no more key of that type. So when a key is missing in self.one_time_key_counts, we infer that it means a count of 0 (hence we do self.one_time_key_counts.get(type, 0)). I'm not really sure keeping this dict is good, even though it is the solution with the less overhead.
Would you prefer if there was two interger attribute for the counts, and two more for the target_counts? This would allow to get rid of the weird dicts, but if there are new types of one-time keys later it could lead to a lot of attributes...

Didn't see your last comment about a lightweight class, that sure seems like an option. I'll see what I can do!

non-Jedi · 2018-06-29T18:13:48Z

matrix_client/crypto/olm_device.py

+        # used instantly, and we want them to stay in libolm, until the limit is reached
+        # and it starts discarding keys, starting by the oldest.
+        target_keys_number = self.olm_account.max_one_time_keys // 2
+        if not 0 <= signed_otk_proportion <= 1:


Let's put all input validation at the top of the method.

non-Jedi · 2018-06-29T18:14:50Z

matrix_client/crypto/olm_device.py

+        target_keys_number = self.olm_account.max_one_time_keys // 2
+        if not 0 <= signed_otk_proportion <= 1:
+            raise ValueError('signed_otk_proportion must be between 0 and 1.')
+        self.otk_target_counts = {}


Let's either use the "otk" abbreviation throughout the code or expand it everywhere throughout the code. In previous commit, you added the self.one_time_key_counts attribute. I'll try to resist any more bikeshedding. Sorry.

Can we not add both the attributes below during the dictionary instantiation for clarity?

self.otk_target_counts = { 'signed_curve25519': int(round(signed_otk_proportion * target_keys_number)), ... }

I like bikeshedding!

non-Jedi · 2018-06-29T18:18:26Z

matrix_client/crypto/olm_device.py

+        Args:
+            force_update (bool): Fetch the number of one-time keys currently on the HS
+                before uploading, even if we already know one. In most cases this should
+                not be necessary, as we get this value from sync responses.


Assuming the logic for pulling this from sync comes in later commit.

Yes it does.

non-Jedi · 2018-06-29T18:20:46Z

matrix_client/crypto/olm_device.py

+                uploaded, the corresponding key will not be present. Consequently, an
+                empty dict indicates that no keys were uploaded.
+        """
+        if not self.one_time_key_counts or force_update:


I don't remember enough about operator priority in python, but I'm assuming this is equivalent to (not self.one_time_key_counts) or force_update. Could we reverse that so people don't have to think about operator priority? force_update or not self.one_time_key_counts?

non-Jedi · 2018-07-01T04:58:41Z

matrix_client/crypto/olm_device.py

+            raise ValueError('signed_otk_proportion must be between 0 and 1.')
+        self.otk_target_counts = {}
+        self.otk_target_counts['signed_curve25519'] = \
+            int(round(signed_otk_proportion * target_keys_number))


isn't int(round(x)) redundant?

round(1.5) will give you 2.0 with py2. It led to trouble.

Does the same thing in py3 afaict. Isn't that the desired behavior?

My py3 tells me 2, not 2.0. It fails with py2 at the generate_one_time_keys call, which wants an int and not a float.

Gotcha. I understand now. 👍

non-Jedi · 2018-07-01T05:06:22Z

matrix_client/crypto/olm_device.py

        user_id (str): Matrix user ID. Must match the one used when logging in.
        device_id (str): Must match the one used when logging in.
+        signed_otk_proportion (float): Optional. The proportion of signed one-time keys we
+            should maintain on the HS compared to unsigned keys. The maximum value of


https://matrix.org/docs/guides/e2e_implementation.html#creating-and-registering-one-time-keys says that all one-time keys uploaded should be signed. I'm guessing this is one of the differences @richvdh mentioned in current E2E compared to that guide?

Mmmh, I think that Riot only uploads signed keys. However, there are benefits to uploading unsigned keys (see https://git.matrix.org/git/olm/about/docs/signing.rst), so it is a nice ability to have! Note that from the default parameters of __init__, the default behavior is to only upload signed keys.

non-Jedi · 2018-07-01T05:08:28Z

matrix_client/crypto/olm_device.py

+        if not 0 <= signed_otk_proportion <= 1:
+            raise ValueError('signed_otk_proportion must be between 0 and 1.')
+        self.otk_target_counts = {}
+        self.otk_target_counts['signed_curve25519'] = \


Can we encode the difference between signed/unsigned keys in the type system somehow instead of just having two arbitrary strings?

Now I see that these strings are actually used in the upload request. Still feels weird to encode the difference as string keys in a dict for some reason.

Probably the "proper" way of doing it would be to have lightweight classes for this, and move most of the logic for manipulating an individual key to the class methods.

Yes, this is for consistency with self.one_time_key_counts.

non-Jedi · 2018-07-01T05:13:26Z

matrix_client/crypto/olm_device.py

+            if not num_to_create:
+                continue
+            keys_uploaded[key_type] = num_to_create
+        self.olm_account.generate_one_time_keys(sum(keys_uploaded.values()))


Given that you explicitly name the keys as curve25519 in self.otk_target_counts, doesn't this do the wrong thing? Having "curve25519" included in name of dictionary keys implies there could be other algos in dictionary in the future, and this call shouldn't care about that.

non-Jedi · 2018-07-01T05:18:12Z

matrix_client/crypto/olm_device.py

+        self.otk_target_counts['signed_curve25519'] = \
+            int(round(signed_otk_proportion * target_keys_number))
+        self.otk_target_counts['curve25519'] = \
+            int(round((1 - signed_otk_proportion) * target_keys_number))


This is incorrect. In case of partial number, this leaves a single key unaccounted for. e.g. target_keys_number = 9 and signed_otk_proportion = 0.5.

Well, there was an arbitrary choice to make: add a signed key, add an unsigned key or "lose" a key. I don't see why randomly adding a key would be a better behavior?
Anyway you're right that this is problematic as is. How about I document the behavior in the docstring?

actually, I think I'm wrong here. round should give a pair of numbers that sums to target_keys_number.

You found the edge case where it can't. I really think it's not a big deal though.

Zil0 · 2018-07-02T10:37:30Z

Tried the lightweight class idea. Let me know how it looks! Needs squashing before merge obviously.

non-Jedi

I think the class for one-time keys is a pretty big win for readability. I would probably move all of the functionality related to otks (including the upload/download itself) into the class, but I think it's pretty reasonable/readable as is. Add some clarification to OneTimeKeysManager.counts, squash down, and we can get this merged.

non-Jedi · 2018-07-05T00:25:28Z

matrix_client/crypto/one_time_keys.py

+        self.to_upload = {}
+
+    @property
+    def counts(self):


More descriptive name here please or brief docstring (what is this count of?).

Changed to key_counts.

Sorry. Should've been more explicit. Was looking for something like server_counts or even server_key_counts.

Done, server_counts it is.

uhoreg

looks good to me

uhoreg · 2018-07-13T14:37:17Z

@non-Jedi it looks like this, #234, and #235 are ready to be merged now?

non-Jedi requested changes Jul 1, 2018

View reviewed changes

upload identity keys

e2e12d0

non-Jedi requested changes Jul 5, 2018

View reviewed changes

Zil0 added 2 commits July 5, 2018 15:04

upload one-time keys

4eabc6a

add upload identity/one-time keys tests

7153a61

uhoreg approved these changes Jul 6, 2018

View reviewed changes

non-Jedi approved these changes Jul 13, 2018

View reviewed changes

non-Jedi merged commit 7153a61 into matrix-org:master Jul 13, 2018

[E2E] Upload keys #233

[E2E] Upload keys #233

Uh oh!

Conversation

Zil0 commented Jun 13, 2018

Uh oh!

non-Jedi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zil0 Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Zil0 commented Jul 2, 2018

Uh oh!

non-Jedi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uhoreg left a comment

Choose a reason for hiding this comment

Uh oh!

uhoreg commented Jul 13, 2018

Zil0 Jul 1, 2018 •

edited

Loading