Support HS unbinding 3pids #67

dbkr · 2018-05-24T10:17:08Z

As per spec draft matrix-org/matrix-spec-proposals#1194

Related synapse PR: matrix-org/synapse#3276

Adds autoincrement to local and global associations table so we know IDs will never be re-used if we start deleting associations. Also some bits of the unbind servlet.

Written in theory - comitting now before going to write the synapse part

So I can test federation easily

richvdh

well, it's missing some important things.

I'm also very concerned that we appear to be ignoring feedback from the community in how this API is implemented.

richvdh · 2018-06-01T10:10:36Z

sydent/db/threepid_associations.sql

@@ -14,10 +14,10 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */

-CREATE TABLE IF NOT EXISTS local_threepid_associations (id integer primary key, medium varchar(16) not null, address varchar(256) not null, mxid varchar(256) not null, ts integer not null, notBefore bigint not null, notAfter bigint not null);
-CREATE UNIQUE INDEX IF NOT EXISTS medium_address on local_threepid_associations(medium, address);
+CREATE TABLE IF NOT EXISTS local_threepid_associations (id integer primary key autoincrement, medium varchar(16) not null, address varchar(256) not null, mxid varchar(256) not null, ts integer, notBefore bigint, notAfter bigint);


could you write a comment which explains what it means for the nullable fields to be null?

This is no longer in this schema file but I've commented it in the upgrade script

richvdh · 2018-06-01T10:30:01Z

sydent/db/version.py

+    def upgradeSchema(self):
+        curVer = self._getSchemaVersion()
+
+        if curVer < 1:


should we do something to make new installations have schema v1 ?

Yeah, this wasn't thought through very well. Hopefully this is more sensible now.

richvdh · 2018-06-01T10:32:31Z

sydent/db/version.py

+            )
+            cur.execute("DROP TABLE old_local_threepid_associations")
+
+            # same for global_threepid_associations


well, except we're keeping the not null constraints.

good point: fixed

richvdh · 2018-06-01T10:36:36Z

sydent/threepid/bind.py

@@ -22,7 +23,7 @@
 from sydent.db.invite_tokens import JoinTokenStore

 from sydent.db.valsession import ThreePidValSessionStore
-from sydent.db.threepid_associations import LocalAssociationStore
+from sydent.db.threepid_associations import LocalAssociationStore, GlobalAssociationStore


looks unused? don't you have a PEP8 checker which catches this?

oops. No, we probably should do if sydent were a better maintained project.

richvdh · 2018-06-01T10:38:12Z

sydent/sydent.py

 from http.servlets.replication import ReplicationPushServlet
 from http.servlets.getvalidated3pidservlet import GetValidated3pidServlet
 from http.servlets.store_invite_servlet import StoreInviteServlet

+from db.version import VersionStore


shouldn't this be up with "from db.sqlitedb import SqliteDatabase" ?

It's gone now anyway

richvdh · 2018-06-01T10:40:47Z

sydent/replication/peer.py

@@ -100,6 +102,9 @@ def pushUpdates(self, sgAssocs):
                                                                  '/_matrix/identity/replicate/v1/push',
                                                                  body)

+        # XXX: We'll also need to prune the deleted associations out of the


I thought this was what https://github.com/matrix-org/sydent/pull/67/files#diff-8db67a35998f38ac70c2d2fa7d2d8200R90 was doing?

Have tried to clarify in the comment

richvdh · 2018-06-01T10:49:51Z

sydent/http/httpclient.py

+    def creatorForNetloc(self, hostname, port):
+        context = SSL.Context(SSL.SSLv23_METHOD)
+        try:
+            _ecCurve = _OpenSSLECCurve(_defaultCurveName)


can you do matrix-org/synapse#3157 to this please

richvdh · 2018-06-01T10:52:11Z

sydent/http/servlets/threepidunbindservlet.py

+
+from sydent.http.servlets import get_args, jsonwrap
+from sydent.hs_federation.verifier import NoAuthenticationError
+from signedjson.sign import SignatureVerifyException


before twisted, please

richvdh · 2018-06-01T10:54:16Z

sydent/sydent.py

@@ -31,6 +32,7 @@
 from http.servlets.pubkeyservlets import EphemeralPubkeyIsValidServlet, PubkeyIsValidServlet
 from validators.emailvalidator import EmailValidator
 from validators.msisdnvalidator import MsisdnValidator
+from hs_federation.verifier import Verifier


this appears to be missing from the PR

oops, fixed

* Keep v0 schema in .sql scripts * Run schema scripts if version == 0 * Do schema upgrades if v < current, so new db will get v0 schema then upgrade

as per matrix-org/synapse#3157

dbkr · 2018-06-04T15:16:03Z

Hopefully missing fewer things now.

richvdh · 2018-06-05T17:08:11Z

sydent/db/sqlitedb.py

+        # We always run the schema files if the version is zero: either the db is
+        # completely empty and schema-less or it has the v0 schema, which is safe to
+        # replay the schema files.
+        if curVer == 0:


ok, so the files on disk are the v0 schema, and new installations are made to be v1 by first running the v0 schema and then doing the upgrades?

that's fine, but saying so in the comment would help

richvdh · 2018-06-05T17:11:26Z

sydent/db/sqlitedb.py

+            cur.execute("DROP INDEX IF EXISTS local_threepid_medium_address")
+            cur.execute("ALTER TABLE local_threepid_associations RENAME TO old_local_threepid_associations");
+            cur.execute(
+                "CREATE TABLE local_threepid_associations (id integer primary key autoincrement, "


triple-quotes are totally a thing btw

(for reference, doesn't need changing now)

oh true, although istr synapse used separate single line quotes. whatever.

richvdh · 2018-06-05T17:13:01Z

sydent/db/threepid_associations.py

+    def removeAssociation(self, threepid, mxid):
+        cur = self.sydent.db.cursor()
+
+        cur.execute(


why do we bother with this, ooi?

Because it's a REPLACE INTO, so we don't need to insert an empty record if there's nothing there to start with

richvdh · 2018-06-05T17:13:53Z

sydent/hs_federation/verifier.py

+
+
+"""
+Verifies signed json blobs from Matrix Homeservers by finding the


is this intended for the file or the class? if the file it goes at the top, if the class it goes inside the class

richvdh · 2018-06-05T17:14:10Z

sydent/hs_federation/verifier.py

+logger = logging.getLogger(__name__)
+
+
+class NoAuthenticationError(Exception):


could this have a docstring describing what it represents please

richvdh · 2018-06-05T17:33:16Z

sydent/hs_federation/verifier.py

+        client = FederationHttpClient(self.sydent)
+        result = yield client.get_json("https://%s:%s/_matrix/key/v2/server/" % host_port)
+        if 'verify_keys' not in result:
+            raise Exception("No key found in response")


shouldn't this be a SignatureVerifyException ?

yes, it probably ought to

richvdh · 2018-06-05T17:37:58Z

sydent/hs_federation/verifier.py

+        XXX: This contains a very noddy version of the home server
+        SRV lookup and signature verification. It forms HTTPS URLs
+        from the result of the SRV lookup which will mean the Host:
+        parameter in the request will be wrong. It only looks at


isn't the wrong Host header going to mess things up? People will have servers behind proxies which rely on it being right.

Yeah, this is taken from the impl I did for user directory searching. I've also realised we actually have another impl of this in sydent for the onbind callbacks (https://github.com/matrix-org/sydent/blob/master/sydent/threepid/bind.py#L91) which has the same shortcoming. I guess now is the time to fix both of them as this presumably is going to start impacting people.

richvdh · 2018-06-05T17:40:26Z

sydent/hs_federation/verifier.py

+        auth_headers = request.requestHeaders.getRawHeaders(b"Authorization")
+
+        if not auth_headers:
+            raise NoAuthenticationError("Missing Authorization headers")


what is the logic behind when we raise a SignatureVerifyException and when a NoAuthenticationError?

Mostly just me thinking that the you might want separate errors for them to be slightly more helpful (eg. for if you spelt Authorization with an s or something). Granted, with the way we're using it here we could just have the same exception type with a different message.

richvdh · 2018-06-05T17:40:58Z

sydent/hs_federation/verifier.py

+    @defer.inlineCallbacks
+    def authenticate_request(self, request, content):
+        """Authenticates a Matrix federation request based on the X-Matrix header
+        XXX: Copied largely from synapse


could do with some docstring regardless...

richvdh · 2018-06-05T17:44:46Z

sydent/hs_federation/verifier.py

+        signature each time and does not contact any other servers
+        to do perspectives checks.
+
+        :param acceptable_server_names If provided and not None,


of course we use different docstring conventions in different projects. Anyway there should be a colon after the param name aiui. And ideally documentation of a type.

https://pythonhosted.org/an_example_pypi_project/sphinx.html#function-definitions

So the Host header will be correct in requests to homeservers.

dbkr · 2018-06-08T11:05:08Z

OK, this now does more sensible SRV lookup - ptal!

richvdh

lgtm modulo nits

richvdh · 2018-06-08T12:07:25Z

sydent/hs_federation/verifier.py

+                json_request["signatures"].setdefault(origin, {})[key] = sig
+
+        if not json_request["signatures"]:
+            raise NoAuthenticationError("Missing Authorization headers")


this is misleading

oops, thanks

richvdh · 2018-06-08T12:07:55Z

sydent/hs_federation/verifier.py

+        :param request: The request object to authenticate
+        :param content: The content of the request, if any
+        :type content: bytes or None
+        """


could you document the return val please

dbkr added 7 commits May 22, 2018 10:42

Update db schema

2a303aa

Adds autoincrement to local and global associations table so we know IDs will never be re-used if we start deleting associations. Also some bits of the unbind servlet.

Missed the version store

af03054

Remove association store functions

10c9a47

Threepid unbinding

d018a08

Written in theory - comitting now before going to write the synapse part

Make threepid unbind work

389e41f

Add env var for config file

9ab64a5

So I can test federation easily

Make federation of deletes work

e007df6

dbkr mentioned this pull request May 24, 2018

Remove email addresses / phone numbers from ID servers when they're removed from synapse matrix-org/synapse#3276

Merged

dbkr assigned richvdh May 24, 2018

richvdh requested changes Jun 1, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Jun 1, 2018

dbkr added 8 commits June 4, 2018 13:42

Add missing hs_federation files

b5213fb

Do schema migration more sensibly

d7e55e6

* Keep v0 schema in .sql scripts * Run schema scripts if version == 0 * Do schema upgrades if v < current, so new db will get v0 schema then upgrade

Comment why fields are nullable

5da85e4

Fix comment

59ae3d3

Unused import

bb23b8c

Clarify comment

be74377

Import ordering

844d3d6

Fix elliptic curve setup

acc0d14

as per matrix-org/synapse#3157

dbkr assigned richvdh and unassigned dbkr Jun 4, 2018

richvdh assigned dbkr and richvdh and unassigned richvdh and dbkr Jun 5, 2018

richvdh requested changes Jun 5, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Jun 5, 2018

dbkr added 10 commits June 6, 2018 10:45

Comment on how db schema works

58259dc

comment replace into voodoo

b96f0ee

put class comment in the right place

a6ba1c4

doc NoAuthenticationError

fac721c

capitalisation

3328685

Wrap sync handler in try block

94b49f9

Use sensible exception type

9a3bda5

Fix doc

8b0b866

Doc authenticate_request

33f91ae

Do proper SRV lookup

a90ec3a

So the Host header will be correct in requests to homeservers.

dbkr assigned richvdh and unassigned dbkr Jun 8, 2018

richvdh approved these changes Jun 8, 2018

View reviewed changes

richvdh assigned dbkr and unassigned richvdh Jun 8, 2018

dbkr added 2 commits June 8, 2018 13:41

Fix error

5adef48

doc return type

14226e7

dbkr merged commit 95d92d7 into master Jun 8, 2018



		"""
		Verifies signed json blobs from Matrix Homeservers by finding the

		logger = logging.getLogger(__name__)


		class NoAuthenticationError(Exception):

Support HS unbinding 3pids #67

Support HS unbinding 3pids #67

Conversation

dbkr commented May 24, 2018 • edited Loading

richvdh 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

dbkr commented Jun 4, 2018

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

dbkr commented Jun 8, 2018

richvdh 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

dbkr commented May 24, 2018 •

edited

Loading