status-im / specs

Specifications for Status clients.
https://specs.status.im/
MIT License
14 stars 14 forks source link

Add encrypted access tokens #143

Closed cammellos closed 4 years ago

cammellos commented 4 years ago

This commit modifies the spec so that instead of passing a list of SHA3 public_keys to the server for filtering, the user will pass instead a list of access_token encrypted with the allowed contact DH PK(bob) PK(alice).

It also removes the blocked_user_list, so all the filtering is done based on the chat_id field.

Effectively both were used a sort of a spam prevention mechanism, but the cost of keeping blocked_user_list is that a user will need to disclose their identity to the server, and is trivial to circumvent for any determined attacker, so keeping only filtering on chat_id achieves a similar naive prevention of spam without compromising the privacy of other users. Spam prevention is something that we don't have a solution in status app and this reflects the same behavior.

These changes effectively means that:

1) The server has no way to know which users are in allowed_user_list. 2) The client can query the server anonymously, so at no point they have to disclose their identity to the server.

Addressed some leftover feedback @rajeevgopalakrishna

cammellos commented 4 years ago

@rajeevgopalakrishna I have addressed the feedback in the closed PR here, we can probably keep the conversation here.

I have changed the protocol so that the access_token is generated client side and passed to the server.

I want to change how allowed_user_list works, and instead of passing a list of hashed public keys, the user can encrypt the token with all the public keys of the allowed users in a blob and push that to the server.

When querying users can just pull this blob and look for a token by decrypting the message, so that we don't have to communicate to the server which users are actually allowed, and conversely the users don't have to communicate their real identity to the server. But likely I will address that in a different PR.

oskarth commented 4 years ago

Generally seems fine to me, but I'd rather see core and security take ownership of these reviews and getting them merged. One review from core and one from security seems reasonable to me

oskarth commented 4 years ago

Removing reviewers to avoid diffusion of responsibility, pinging @andremedeiros @Samyoul @0kok0 @rajeevgopalakrishna for reviews

cammellos commented 4 years ago

@rajeevgopalakrishna @Samyoul I should have addressed all the outstanding feedback. There's still some considerable changes that I need to make, but I will do in a different PR (getting rid of some of the messages, specify encryption etc), so there will be still time for feedback at a later round. thanks