Add encrypted http pusher (MSC3013) #11512

Sorunome · 2021-12-06T12:58:24Z

Pull Request Checklist

Server-side implementation of MSC3013 Encrypted Push matrix-org/matrix-spec-proposals#3013

Pull request is based on the develop branch
Pull request includes a changelog file. The entry should:
- Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
- Use markdown where necessary, mostly for code blocks.
- End with either a period (.) or an exclamation mark (!).
- Start with a capital letter.
Pull request includes a sign off
Code style is correct
(run the linters)

Signed-Off-By: Sorunome [email protected]

DMRobertson

A few thoughts.

I'm not a cryptography expert, but AFAICS this implements the process described in the MSC. I don't suppose you've had a chance to test it in action?

synapse/push/httppusher.py

tests/push/test_http.py

DMRobertson

Where possible, please try to avoid a force push. It makes it harder to see what's changed between review rounds!

I think the case of a missing public_key needs handling, and the mypy config change to get rid of the type-ignore in the code would be nice to have (but not crucial).

Otherwise LGTM. Would appreciate another pair of eyes from @matrix-org/synapse-core though---sticking it into the review queue.

Sorunome · 2021-12-10T09:35:06Z

I don't suppose you've had a chance to test it in action?

oh, totally missed this, yes this is tested in action

reivilibre

Looks sensible, will wait for the discussed changes about which library to use for the Curve25519 exchange.

Thanks!

synapse/push/httppusher.py

callahad · 2021-12-13T15:21:04Z

matrix-org/olm#53 may be relevant to the discussion about curve25519; we may have boxed ourselves into a corner. This is being looked at now.

Sorunome · 2021-12-13T15:37:10Z

this is completely irrelevant to us. zhat issue is about how libolm stores and restores its ed25519 keys, and we never use libolm so it doesn't matter. we just take a public key and using it encrypt stuffs. 13. 12. 2021 16:21:25 Dan Callahan ***@***.***>:

…

matrix-org/olm#53[matrix-org/olm#53] may be relevant to the discussion about curve25519; we may have boxed ourselves into a corner. This is being looked at now. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub[#11512 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AASSEVAGXGHH3CV3AJU2ZWTUQYFOZANCNFSM5JOR23SA]. Triage notifications on the go with GitHub Mobile for iOS[https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675] or Android[https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub]. [data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAEgAAABICAYAAABV7bNHAAAAAXNSR0IArs4c6QAAAARzQklUCAgICHwIZIgAAAArSURBVHic7cEBDQAAAMKg909tDjegAAAAAAAAAAAAAAAAAAAAAAAAAAA+DFFIAAEctgHwAAAAAElFTkSuQmCC###24x24:true###][Sledovací obrázek][https://github.com/notifications/beacon/AASSEVHQNGXQWRFORAPWPYDUQYFOZA5CNFSM5JOR23SKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHMU2RBY.gif]

DMRobertson

Glad to see the dependency stuff was sorted out.

I don't think we want to merge it as-is unless:

the MSC is accepted, or
these changes were guarded by an experimental config flag

because this change presently enables an unstandardised feature for all Synapse deployments.

synapse/python_dependencies.py

changes made

Sorunome · 2021-12-23T11:00:35Z

Because of stuff that came up around encrypted push and iOS needing some slight adjustments soru marked this PR as WIP again, the main part of the PR will still the same, though. Will un-mark as WIP (should soru ping someone, then?) once those are resolved.

EDIT: Oh, github doesn't allow re-marking a PR as WIP

richvdh · 2021-12-23T11:24:58Z

EDIT: Oh, github doesn't allow re-marking a PR as WIP

yes it does! have done so.

(should soru ping someone, then?)

no need, it should automatically come up on our review queue.

Sorunome · 2021-12-31T10:42:02Z

This PR is ready for review again. There are merge conflicts in this PR by now, should soru merge in master or rebase onto master?

reivilibre · 2021-12-31T14:42:51Z

This PR is ready for review again. There are merge conflicts in this PR by now, should soru merge in master or rebase onto master?

:) After there's already been discussion/review, prefer merge rather than rebase so that the conversations stay in place in the history of the PR.

DMRobertson · 2021-12-31T21:48:45Z

And note: please merge in develop, rather than master.

…soru/encrypted-push

DMRobertson · 2022-01-04T11:23:10Z

synapse/push/httppusher.py

+            devices = cleartext_notif["devices"]
+            del cleartext_notif["devices"]


Could use

Suggested change

devices = cleartext_notif["devices"]

del cleartext_notif["devices"]

devices = cleartext_notif.pop("devices")

As in https://docs.python.org/3/library/stdtypes.html?highlight=dict%20pop#dict.pop

DMRobertson · 2022-01-04T11:31:43Z

synapse/push/httppusher.py

+            # create the mac
+            mac = hmac.new(mac_key, ciphertext, hashlib.sha256).digest()[0:8]
+
+            d = {


Can we have a more descriptive name than d here? encrypted_notification or payload_with_encryption?

DMRobertson · 2022-01-04T11:47:26Z

synapse/push/httppusher.py

+            # now, if the push frame is a counts only frame, we have to respect the counts_only_type setting
+            if counts_only:
+                if self.counts_only_type == "boolean":
+                    d["is_counts_only"] = True
+                elif self.counts_only_type == "full":
+                    d["counts"] = cleartext_notif["counts"]


I'm not sure this logic belongs in a function called _encrypt_notification_dict---this isn't doing any encryption. I take the point that this is needed to make iOS E2EE work, but doesn't this setting make sense even if we don't want to use encryption?

Put differently: why is this tied to the use of curve25519-aes-sha2 specifically?

See the MSC https://github.com/Sorunome/matrix-doc/blob/soru/encrypted-push/proposals/3013-encrypted-push.md#ios

Why curve25519-aes-sha2 specifically? Because only plain and curve25519-aes-sha2 exist in this PR, and it is not in plain

so maybe call the method _process_notification_dict_for_sending instead?

DMRobertson · 2022-01-04T11:58:17Z

tests/push/test_http.py

+        # do the exchange
+        exchanged = X25519PrivateKey.from_private_bytes(
+            unpaddedbase64.decode_base64(private_key)
+        ).exchange(X25519PublicKey.from_public_bytes(ephemeral))
+        # expand with HKDF
+        zerosalt = bytes([0] * 32)
+        prk = hmac.new(zerosalt, exchanged, hashlib.sha256).digest()
+        aes_key = hmac.new(prk, bytes([1]), hashlib.sha256).digest()
+        mac_key = hmac.new(prk, aes_key + bytes([2]), hashlib.sha256).digest()
+        aes_iv = hmac.new(prk, mac_key + bytes([3]), hashlib.sha256).digest()[0:16]
+        # create the cleartext with AES-CBC-256
+        decryptor = Cipher(algorithms.AES(aes_key), modes.CBC(aes_iv)).decryptor()
+        # AES blocksize is always 128 bits
+        unpadder = padding.PKCS7(128).unpadder()
+        cleartext = json.loads(
+            (
+                unpadder.update(decryptor.update(ciphertext) + decryptor.finalize())
+                + unpadder.finalize()
+            ).decode("utf-8")
+        )
+        # create the mac
+        calculated_mac = hmac.new(mac_key, ciphertext, hashlib.sha256).digest()[0:8]


This look suspiciously like a test which re-implements the function under test. What's going on here?

we are testing if the encrypted push frame we created is decryptable. Soru was pondering between this and just testing if the mac/cipher/ephemeral we created are strings, however this seemed to at least have a bit more integrity. Due to the nature of the push frame being encrypted, we can't test its contents without decrypting them again

We aren't 100% re-implementing it, this is a decrypt implementation while in synapse is an encrypt implementation. But yeah, soru couldn't figure out a better way to test this, sadly :/

DMRobertson · 2022-01-04T11:59:25Z

tests/push/test_http.py

+    def tests_encrypted_push_counts_only_none(self):
+        """
+        The HTTP pusher will not add any extra fields to encrypted push frames if the counts_only_type
+        is none.


Suggested change

is none.

is none.

is 'none'.

To avoid confusion with the Python object None.

DMRobertson · 2022-01-04T12:06:44Z

tests/push/test_http.py

@@ -591,7 +793,7 @@ def test_push_unread_count_message_count(self):
            self.push_attempts[5][2]["notification"]["counts"]["unread"], 4
        )

-    def _test_push_unread_count(self):
+    def _test_push_unread_count(self, pusher_data=None, perform_checks=True):


Type annotations here, please.

That said, I'm not so keen on a method called _test_... which might not actually enforce any assertions, i.e. if perform_checks is falsey. What is the intention here? My first thought was that it was an optimisation to not run repeated checks that don't test anything new. But I think you're specifically opting out of the the checks which interrogate the unread count---because they need to be handled differently under the proposed scheme.

I think it would make more sense to create a new helper function

which does setup only, making no assertions

only does the minimum required to generate a counts-only and normal push notification (the present test helper seems to make many normal notifications?)

reivilibre · 2022-08-05T09:44:42Z

Hi, are you still interested in working on this?

Sorunome · 2022-08-05T09:51:52Z

In general yes, but sorus health does not allow working lately :/ 5. 8. 2022 11:44:55 reivilibre ***@***.***>:

…

Hi, are you still interested in working on this? — Reply to this email directly, view it on GitHub[#11512 (comment)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AASSEVBUG6R3AVVQLZLVWJDVXTPBLANCNFSM5JOR23SA]. You are receiving this because you authored the thread.[Sledovací obrázek][https://github.com/notifications/beacon/AASSEVFUGVRTRE5KRTVLNMLVXTPBLA5CNFSM5JOR23SKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOI7TAWII.gif]

Sorunome requested a review from a team as a code owner December 6, 2021 12:58

Sorunome mentioned this pull request Dec 6, 2021

MSC3013: Encrypted Push matrix-org/matrix-spec-proposals#3013

Open

reivilibre changed the title ~~Add encrypted http pusher~~ Add encrypted http pusher (MSC3013) Dec 6, 2021

Sorunome force-pushed the soru/encrypted-push branch 4 times, most recently from 0ffcbcb to dc70952 Compare December 8, 2021 10:56

DMRobertson self-assigned this Dec 8, 2021

DMRobertson reviewed Dec 8, 2021

View reviewed changes

Add encrypted http pusher

848aed2

Sorunome force-pushed the soru/encrypted-push branch from dc70952 to 848aed2 Compare December 9, 2021 10:29

DMRobertson previously requested changes Dec 9, 2021

View reviewed changes

DMRobertson requested a review from a team December 9, 2021 19:26

properly type-ignore donna25519 and fix an absent public_key

d4833c2

reivilibre self-requested a review December 13, 2021 12:01

reivilibre suggested changes Dec 13, 2021

View reviewed changes

synapse/push/httppusher.py Show resolved Hide resolved

synapse/push/httppusher.py Show resolved Hide resolved

synapse/push/httppusher.py Outdated Show resolved Hide resolved

Sorunome added 2 commits December 14, 2021 09:33

use cryptography in favour of donna25519

251f304

rename _process_notification_dict to _encrypt_notification_dict

bbf3baa

reivilibre requested review from reivilibre, callahad and DMRobertson December 14, 2021 11:23

DMRobertson reviewed Dec 14, 2021

View reviewed changes

synapse/python_dependencies.py Outdated Show resolved Hide resolved

Sorunome added 3 commits December 15, 2021 14:03

remove dependency of pynacl

e129b01

Hide encrypted push behind a feature flag

ba1d216

forgot to advertise encrypted push properly

2f32b28

DMRobertson requested a review from a team December 16, 2021 12:37

richvdh marked this pull request as draft December 23, 2021 11:20

richvdh removed request for a team, callahad and reivilibre December 23, 2021 11:23

add handeling of counts_only_type pusher data

d38ed73

Sorunome marked this pull request as ready for review December 31, 2021 10:41

Merge branch 'develop' of https://github.com/matrix-org/synapse into …

4e75838

…soru/encrypted-push

DMRobertson suggested changes Jan 4, 2022

View reviewed changes

DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Jan 14, 2022

DMRobertson removed their assignment Apr 27, 2022

matrixbot mentioned this pull request Dec 21, 2023

Add encrypted http pusher (MSC3013) element-hq/synapse#11512

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add encrypted http pusher (MSC3013) #11512

Add encrypted http pusher (MSC3013) #11512

Sorunome commented Dec 6, 2021 •

edited

Loading

DMRobertson left a comment

DMRobertson left a comment

Sorunome commented Dec 10, 2021

reivilibre left a comment

callahad commented Dec 13, 2021

Sorunome commented Dec 13, 2021 via email

DMRobertson left a comment

Sorunome commented Dec 23, 2021 •

edited

Loading

richvdh commented Dec 23, 2021

Sorunome commented Dec 31, 2021

reivilibre commented Dec 31, 2021

DMRobertson commented Dec 31, 2021

DMRobertson Jan 4, 2022

DMRobertson Jan 4, 2022

DMRobertson Jan 4, 2022

Sorunome Jan 5, 2022

Sorunome Jan 5, 2022

DMRobertson Jan 4, 2022

Sorunome Jan 5, 2022

Sorunome Jan 5, 2022

DMRobertson Jan 4, 2022

DMRobertson Jan 4, 2022

reivilibre commented Aug 5, 2022

Sorunome commented Aug 5, 2022 via email

		devices = cleartext_notif["devices"]
		del cleartext_notif["devices"]

	devices = cleartext_notif["devices"]
	del cleartext_notif["devices"]
	devices = cleartext_notif.pop("devices")

Add encrypted http pusher (MSC3013) #11512

Are you sure you want to change the base?

Add encrypted http pusher (MSC3013) #11512

Conversation

Sorunome commented Dec 6, 2021 • edited Loading

Pull Request Checklist

DMRobertson left a comment

Choose a reason for hiding this comment

DMRobertson left a comment

Choose a reason for hiding this comment

Sorunome commented Dec 10, 2021

reivilibre left a comment

Choose a reason for hiding this comment

callahad commented Dec 13, 2021

Sorunome commented Dec 13, 2021 via email

DMRobertson left a comment

Choose a reason for hiding this comment

Sorunome commented Dec 23, 2021 • edited Loading

richvdh commented Dec 23, 2021

Sorunome commented Dec 31, 2021

reivilibre commented Dec 31, 2021

DMRobertson commented Dec 31, 2021

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

reivilibre commented Aug 5, 2022

Sorunome commented Aug 5, 2022 via email

Sorunome commented Dec 6, 2021 •

edited

Loading

Sorunome commented Dec 23, 2021 •

edited

Loading