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

MSC2211: Identity Servers Storing Threepid Hashes at Rest #2211

Open
wants to merge 3 commits into
base: master
from

Conversation

@anoadragon453
Copy link
Member

commented Aug 1, 2019

Rendered

@anoadragon453 anoadragon453 requested a review from matrix-org/spec-core-team Aug 1, 2019

@ara4n

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

this looks very very promising :) thanks for splitting it out from #2134

* [GET /_matrix/identity/api/v1/3pid/getValidated3pid](https://matrix.org/docs/spec/identity_service/unstable#get-matrix-identity-api-v1-3pid-getvalidated3pid)
This endpoint needs to be changed to return a hash instead of `medium` and

This comment has been minimized.

Copy link
@uhoreg

uhoreg Aug 1, 2019

Member

Do we need to specify how this is hashed? Do callers of this endpoint depend on it in order to retrieve the 3pid info?

* [POST /_matrix/identity/api/v1/3pid/unbind](https://matrix.org/docs/spec/identity_service/unstable#post-matrix-identity-api-v1-3pid-unbind)
This endpoint needs to be changed to have `threepid` be a hash instead.

This comment has been minimized.

Copy link
@uhoreg

uhoreg Aug 1, 2019

Member

This certainly needs the hash method to be specified, so that the client knows how to hash the threepid to send the correct information. Though I'm not sure that it does need to be changed, since the client can send the information in the clear, and the server can just apply the hash.

* [POST /_matrix/identity/api/v1/store-invite](https://matrix.org/docs/spec/identity_service/unstable#post-matrix-identity-api-v1-store-invite)
This endpoint needs to be changed to remove parameters `medium`, and
`address`, and instead just have a new field containing a hash value.

This comment has been minimized.

Copy link
@uhoreg

uhoreg Aug 1, 2019

Member

I don't think that we can replace medium and address with a hash here, because this endpoint is supposed to send an email to the invited address, and if the address is hashed, then the server doesn't have access to the address.

@turt2live turt2live self-requested a review Aug 1, 2019

@turt2live
Copy link
Member

left a comment

the approach largely seems in the right direction. I have mostly stylistic concerns (diffs should be legible), but would like some of the limitations raised earlier in the proposal text.

plaintext addresses. Due to protocol endpoints requiring plaintext addresses,
major implementations have always stored 3PID data as plaintext at rest. An
example is the [GET
/_matrix/identity/api/v1/3pid/getValidated3pid](https://matrix.org/docs/spec/identity_service/unstable#get-matrix-identity-api-v1-3pid-getvalidated3pid)

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 1, 2019

Member

stick this endpoint in a code tag to prevent italics for the rest of the doc please. This is a theme throughout the proposal.

information.
Storing 3PIDs as hashes at rest can be accomplished with a few protocol
changes. As recently done with [GET

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 1, 2019

Member

A link to the MSC would be great here.

what was done for `/_matrix/identity/v2/lookup` in
[MSC2134](https://github.com/matrix-org/matrix-doc/pull/2134).
Thus, the new endpoints should be:

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 1, 2019

Member

while we're here, can we fix the casing to match up with our style doc?

  • request_token
  • store_invite
  • /3pid/validated
## Potential issues
Another sticking point to consider is identity servers that hook into
third-party data sources, such as LDAP, may have trouble answering requests

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 1, 2019

Member

I feel like supporting LDAP is a hard requirement for us to be able to achieve this. I'm not sure how much of this API LDAP-enabled identity servers actually use though - I can imagine they'd just abuse /lookup.

## Conclusion
With a few endpoint changes, we can enable identity servers to store user
contact information in a hashed format, thereby reducing the impact of a

This comment has been minimized.

Copy link
@turt2live

turt2live Aug 1, 2019

Member

There should be a mention somewhere early in this proposal that we want to encourage identity servers to store hashed identifiers, but cannot enforce it. We can't define how implementations store data, but we sure can make it obvious how we want them to work.

These endpoints just take token/session information, so no changes are
needed. All other endpoints would not need to be changed.
## Tradeoffs

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Aug 2, 2019

Member

As mentioned in the other PR, how do we rotate hashes?

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Aug 2, 2019

Author Member

That should be an implementation detail, no? I can mention it in the doc as such.

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Aug 2, 2019

Member

I don't see how an IS can rotate hashes if it doesn't have the original 3PID, so I think this would require some funky stuff to happen to make it possible (or to just drop all existing values).

This comment has been minimized.

Copy link
@anoadragon453

anoadragon453 Aug 2, 2019

Author Member

Ah right. This isn't spelled out anywhere but I think the idea was that the server would have a lifetime salt (probably just generated on first-run), and then rotated peppers for lookups.

So clients would have to hash with a salt then with a pepper.

I think the only benefit there would be in the case of MITM, or for storing invites where the sent hash is stored temporarily.

This comment has been minimized.

Copy link
@erikjohnston

erikjohnston Aug 2, 2019

Member

I'm more thinking of the case where we store hashes as SHA-1 then discover that its vulnerable and we need to use SHA-256 instead. What do we do?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.