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

Add encrypted access tokens #143

Merged
merged 1 commit into from
Jul 2, 2020
Merged

Conversation

cammellos
Copy link
Contributor

@cammellos cammellos commented Jun 25, 2020

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
Copy link
Contributor Author

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

@cammellos cammellos force-pushed the feature/push-notifications-feedback branch from 48a03fe to 771be99 Compare June 26, 2020 10:21
@cammellos cammellos changed the title address feedback Add encrypted access tokens Jun 29, 2020
@cammellos cammellos force-pushed the feature/push-notifications-feedback branch from 89c86dd to f685453 Compare June 29, 2020 07:17
@cammellos cammellos requested review from Samyoul, decanus, 0kok0, andremedeiros, oskarth and rajeevgopalakrishna and removed request for Samyoul June 29, 2020 07:18
@oskarth
Copy link
Contributor

oskarth commented Jul 1, 2020

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 oskarth removed request for decanus and oskarth July 1, 2020 03:53
@oskarth
Copy link
Contributor

oskarth commented Jul 1, 2020

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

Copy link

@rajeevgopalakrishna rajeevgopalakrishna left a comment

Choose a reason for hiding this comment

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

More privacy now with encrypted tokens - nice work! Just a few minor (formatting) suggestions to address for now.

publicly accessible server for sending the actual notification. This cannot be community run as it requires sharing private credentials that only status has access to.

## Components

### Gorush instance

A [gorush](https://github.com/appleboy/gorush) instance MUST be publicly
A [gorush](https://github.com/appleboy/gorush) instance MUST be publicly
available, this will be used only by push notification servers.

### Push notification server

Choose a reason for hiding this comment

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

Can we rename this to "Push Notification Node" to start avoiding further usage of the term server?

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 curious, what is the benefit of avoiding the term "server"?

Copy link
Contributor Author

@cammellos cammellos Jul 2, 2020

Choose a reason for hiding this comment

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

I don't mind either, technically the lines are a bit blurred, as they both operate as waku clients, but there's definitely a client/server relationship, so happy to go with any that we decide

Choose a reason for hiding this comment

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

Ok, just that it's confusing/misleading to see a client/server relationship in a p2p network (e.g. mailserver vs history node). We can cast this as a special type of node that is queried by others today with the potential to merge this functionality into other nodes in network later.


```protobuf
message PushNotificationQueryInfo {
string access_token = 1;
string installation_id = 2;
bytes public_key = 3;

Choose a reason for hiding this comment

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

Replace "public key" with "chat key"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a bit tricky,
I'd say, let's park the renaming for now and we will do in a different PR if it that's ok with you.
The thing I am not sure is that not always this will be chat keys, for example in anonymous mode this are just keys that a user creates specifically for push notifications, but they are not chat keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, so I have updated the references where we refer 100% to a chat key

uint version = 2;
boolean unregister = 3;
string access_token = 4;

Choose a reason for hiding this comment

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

access_token should perhaps not be bundled under PushNotificationPreferences. Consider renaming or splitting message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I might do that in a separate PR if it's ok, I will be getting rid of a few messages in order to simplify and we can shuffle things around at that point if that's ok.

Copy link
Member

@Samyoul Samyoul left a comment

Choose a reason for hiding this comment

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

Great work. Just a few points of clarification that I feel need addressing.

Also sorry for the late review @cammellos , I actually read through this whole PR on Monday, but got sidetracked with Nim 😬

publicly accessible server for sending the actual notification. This cannot be community run as it requires sharing private credentials that only status has access to.

## Components

### Gorush instance

A [gorush](https://github.com/appleboy/gorush) instance MUST be publicly
A [gorush](https://github.com/appleboy/gorush) instance MUST be publicly
available, this will be used only by push notification servers.

### Push notification server
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 curious, what is the benefit of avoiding the term "server"?

in place at the server level and they MAY retry the request using their real identity.
If `encrypted_access_tokens` is returned, the client SHOULD decrypt each
token by generating a `AES` symmetric key from the Diffie–Hellman between the
target client and the querying client.
Copy link
Member

Choose a reason for hiding this comment

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

Does this sentence describe communication between a Push Notification Server and a client node? Or are we talking about p2p comms between clients?

The section seems to be about server / client querying, but this sentence seems to be about p2p comms. Perhaps add a sentence or subsection header to describe the difference between directly querying the server and client to client querying. Though I may be misunderstanding / missing the part where this distinction is made

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This describe a client querying a server,
although the definition of a server is a bit lose, any client can act as a push notification node(server) if it wishes to do so, but that's probably a different matter.

Basically you pull the encrypted data from the server, and then you decrypt it taking into consideration your pk and the pk of the user who has registered with the server, but no direct communication between the two clients.

Choose a reason for hiding this comment

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

We could think of these as capabilities (history, relaying, notifications etc.) that can be implemented/enabled by various nodes without enforcing a strict client-server relationship.

`device_tokens`: a list of `PushNotificationPreferences`, one for each device owned by the user.
`version`: an atomically increasing number identifying the current `PushNotificationPreferences`. Any time anything is changed in the record it MUST be increased by the client, otherwise the request will not be accepted.
`options`: a list of `PushNotificationOptions`, one for each device owned by the user.
`version`: an monotonically increasing number identifying the current `PushNotificationPreferences`. Any time anything is changed in the record it MUST be increased by the client, otherwise the request will not be accepted.
Copy link
Member

Choose a reason for hiding this comment

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

I has to Google that "monotonically" term.

Comment on lines 651 to 652
- The fact that is interested in sending push notification to a given client,
but their public key is not disclosed
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- The fact that is interested in sending push notification to a given client,
but their public key is not disclosed
- That it is interested in sending push notification to another given client,
but the querying client's public key is not disclosed

@cammellos cammellos force-pushed the feature/push-notifications-feedback branch 3 times, most recently from 3b93924 to 64808eb Compare July 2, 2020 07:36
@cammellos
Copy link
Contributor Author

@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

@cammellos cammellos merged commit 7decaea into master Jul 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants