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

Initial implementation of sync-v2 server #1

Merged
merged 38 commits into from
Jun 1, 2020
Merged

Conversation

yrliou
Copy link
Member

@yrliou yrliou commented Apr 1, 2020

Spec of sync-v2 is currently an internal google document, link could be found in our internal issue: https://github.com/brave/security/issues/154.

Endpoints

  1. The GET /v2/timestamp endpoint returns a UNIX timestamp in milliseconds and expected time for a token to expire in JSON format. Sync clients are responsible to create valid access tokens using timestamps returned by the server. The response is defined in schema/json/timestamp.go.
  2. The POST /v2/command/ endpoint handles Commit and GetUpdates requests from sync clients and return corresponding responses both in protobuf format. Detailed of requests and their corresponding responses are defined in schema/protobuf/sync_pb/sync.proto. Previous granted access token should be passed in the request's Authorization header.

Endpoint 1 above is implementing what we specified in the Authenication section in the sync-v2 spec. Client will generate access token from received timestamp to use in requests to v2/command, and the verification of the token is implemented in auth/auth.go.

Endpoint2 is the main functionality of this sync server, which command/command.go could be a good starting point, it handles client's sync requests, processes the messages and calls out to datastore module for DB operations, then send the result responses to clients.
The protocol between client and server is specified in https://source.chromium.org/chromium/chromium/src/+/master:components/sync/protocol/sync.proto, see GetUpdatesMessage, GetUpdatesResponse, CommitMessage, and CommitResponse.

Datastore

Currently we use dynamoDB as the datastore, the schema could be found in schema/dynamodb/table.json, all DB operations are located in the database module.

Sync Protocol

The sync protocol in schema/protobuf/sync_pb folder is defined by chromium sync client, in this first implementation, we manually copied the proto files from chromium (*.proto under components/sync/protocol/), and do make protobuf to generate codes in golang.
This manually process should be feasible for the short term because the main protocol buf file we cared about in our server is sync.proto and currently what we processed are critical fields which should not be changed frequently and also chromium will ensure backward compatibility when updating the protocol.
We might want to setup a better process on updating the protocol at server side but it is out of the scope of our initial implementation.

@yrliou yrliou force-pushed the master branch 3 times, most recently from e02f90c to f4f4690 Compare May 5, 2020 04:28
@yrliou yrliou force-pushed the master branch 3 times, most recently from d932894 to 801f4cb Compare May 12, 2020 05:20
@yrliou yrliou force-pushed the master branch 2 times, most recently from 7671d1e to e8238fb Compare May 15, 2020 06:27
1. Timestamp handler will reply the current unix time in milliseconds to sync
   clients to used in auth requests.

2. Auth handler authenicates the client by verifying its signature and the
   freshness of the timestamp.

3. Authorize function queries the database to check if the access token in the
   client's request is valid or not and returns the client ID associated with
   the access token.
@yrliou yrliou force-pushed the master branch 2 times, most recently from 06ecb0c to 7b5d680 Compare May 18, 2020 21:19
@yrliou yrliou marked this pull request as ready for review May 18, 2020 23:14
@yrliou yrliou changed the title [WIP] Initial implementation of v2 sync-server [WIP] Initial implementation of sync-v2 server May 19, 2020
@yrliou yrliou changed the title [WIP] Initial implementation of sync-v2 server Initial implementation of sync-v2 server May 19, 2020
@yrliou yrliou requested a review from husobee May 19, 2020 20:15
Wrong assumption was made that Deleted and Folder values will not be nil in
client messages, this PR fixed the bug by setting default values for new
entities and don't update their values when client does not include the field
in the message.
auth/auth.go Outdated Show resolved Hide resolved
Copy link

@husobee husobee left a comment

Choose a reason for hiding this comment

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

Please let me know if further clarification is needed.

Dockerfile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
auth/auth.go Outdated Show resolved Hide resolved
dynamo_local/table.json Outdated Show resolved Hide resolved
server/export_test.go Show resolved Hide resolved
server/server.go Show resolved Hide resolved
server/server.go Outdated Show resolved Hide resolved
server/server.go Show resolved Hide resolved
yrliou added 10 commits May 27, 2020 14:32
…tly.

In this commit, auth endpoint is removed and sync clients are expected to
generate valid access tokens from the timestamp responses directly to use in
future sync requests, server no longer save the states of token and clientID.
It seems the bat-go setting of the RateLimiter is not applicable in our
case. It's easy to hit the limit bat-go is using for sync server case.
@sonarcloud
Copy link

sonarcloud bot commented May 29, 2020

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

server/server.go Outdated Show resolved Hide resolved
Copy link

@husobee husobee left a comment

Choose a reason for hiding this comment

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

Looks good. Glad we are going with the new authentication scheme.

@yrliou yrliou merged commit d95ae65 into brave:master Jun 1, 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.

4 participants