Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hash 3PID lookups #184

Merged
merged 27 commits into from Aug 20, 2019

Conversation

@anoadragon453
Copy link
Member

commented Jul 31, 2019

The implementation of matrix-org/matrix-doc#2134

TODO:

  • Support hashed lookups
  • Add column to the db for pre-computed hashes
  • Re-compute hashes for all 3PIDs on startup when pepper changes
  • Store previous pepper in the db so we know it changed
  • Hash addresses as they come in via /bind and replication servlets
  • Recompute lookup hashes if the algorithms in the config changed

We need to have a think about how this will work in the replication use case in general. Obviously we don't want to replicate lookup hashes as the receiving server may have a different pepper/algorithm in use than the sending server.

When we receive an association, we just rehash the address/medium combo with our own lookup pepper.

@anoadragon453 anoadragon453 added this to In progress in Homeserver Task Board via automation Jul 31, 2019
@anoadragon453 anoadragon453 marked this pull request as ready for review Aug 6, 2019
@anoadragon453 anoadragon453 requested a review from matrix-org/synapse-core Aug 6, 2019
@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 6, 2019

Heads up that CI is probably never going to pass on this.

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 8, 2019

Removed my assignment as this PR is currently waiting for review.

Copy link
Member

left a comment

Some preliminary thoughts.

Whats the deal with CI? This can't really get merged with it failing. Also, what about some unit tests?

sydent/db/hashing_metadata.py Outdated Show resolved Hide resolved
sydent/db/hashing_metadata.py Outdated Show resolved Hide resolved
sydent/db/hashing_metadata.py Outdated Show resolved Hide resolved
sydent/db/hashing_metadata.py Outdated Show resolved Hide resolved
sydent/db/hashing_metadata.py Outdated Show resolved Hide resolved
sydent/http/servlets/lookupservlet.py Show resolved Hide resolved
sydent/http/servlets/lookupv2servlet.py Outdated Show resolved Hide resolved
sydent/http/servlets/lookupv2servlet.py Outdated Show resolved Hide resolved
sydent/http/servlets/v2_servlet.py Show resolved Hide resolved
sydent/http/srvresolver.py Outdated Show resolved Hide resolved
@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 9, 2019

Whats the deal with CI? This can't really get merged with it failing.

CI has been turned on but without a pipeline, hence it failing.

Also, what about some unit tests?

Blocked on #171

anoadragon453 and others added 6 commits Aug 13, 2019
Co-Authored-By: Erik Johnston <erik@matrix.org>
…ent into anoa/hashing_3pid_lookups
Co-Authored-By: Erik Johnston <erik@matrix.org>
…ent into anoa/hashing_3pid_lookups
@anoadragon453 anoadragon453 requested a review from erikjohnston Aug 19, 2019
@erikjohnston

This comment has been minimized.

Copy link
Member

commented Aug 19, 2019

Does this all work correctly with canonicalised emails? I.e. things like case-insensitive etc.

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Does this all work correctly with canonicalised emails? I.e. things like case-insensitive etc.

Sydent continues to make use of the lower() function here: https://github.com/matrix-org/sydent/pull/184/files#diff-c5c56bd2a12d20e5d6f24879fd48d15eR154

@babolivier What is the status of updating this on master?

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

@babolivier says the current state using lower() is fine. We're using lower case emails everywhere.

Copy link
Member

left a comment

Just a couple of trivial nits now

sydent/http/servlets/lookupv2servlet.py Show resolved Hide resolved
sydent/http/servlets/lookupv2servlet.py Show resolved Hide resolved
@anoadragon453 anoadragon453 requested a review from erikjohnston Aug 20, 2019
sydent/db/hashing_metadata.sql Outdated Show resolved Hide resolved
@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@erikjohnston https://matrix.org/docs/spec/identity_service/unstable#get-matrix-identity-api-v1 oh, maybe we should keep the empty 200 return on GET .../v2.

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

@erikjohnston https://matrix.org/docs/spec/identity_service/unstable#get-matrix-identity-api-v1 oh, maybe we should keep the empty 200 return on GET .../v2.

Have done this. Simple to remove in the future if we want to clean up that part of the spec for any reason.

@anoadragon453 anoadragon453 requested a review from dbkr Aug 20, 2019
Copy link
Member

left a comment

Only other question is why the local threepid assocs have hashes? Wouldn't we only ever be matching against the global ones?

@anoadragon453

This comment has been minimized.

Copy link
Member Author

commented Aug 20, 2019

Only other question is why the local threepid assocs have hashes? Wouldn't we only ever be matching against the global ones?

Conclusion is it's not necessary but we'll remove it in a later PR. (issue #196)

@anoadragon453 anoadragon453 merged commit 81f0de7 into master Aug 20, 2019
1 check passed
1 check passed
buildkite/sydent Build #65 passed (35 seconds)
Details
Homeserver Task Board automation moved this from In progress to Done Aug 20, 2019
@anoadragon453 anoadragon453 deleted the anoa/hashing_3pid_lookups branch Aug 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants
You can’t perform that action at this time.