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: old_master
Choose a base branch
from

Conversation

anoadragon453
Copy link
Member

@anoadragon453 anoadragon453 commented Aug 1, 2019

@anoadragon453 anoadragon453 added proposal-in-review proposal A matrix spec change proposal labels Aug 1, 2019
@anoadragon453 anoadragon453 requested a review from a team August 1, 2019 15:11
@ara4n
Copy link
Member

ara4n 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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 August 1, 2019 16:39
Copy link
Member

@turt2live turt2live left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@turt2live turt2live added the kind:maintenance MSC which clarifies/updates existing spec label Apr 20, 2020
@turt2live turt2live added the needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. label Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:maintenance MSC which clarifies/updates existing spec needs-implementation This MSC does not have a qualifying implementation for the SCT to review. The MSC cannot enter FCP. proposal A matrix spec change proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants