server: require signed requests #4

ayumi · 2016-12-06T07:29:16Z

Adds request-verifier which verifies signed timestamp in request bodies.
Uses request-verifier to verify requests to POST /{userId}/credentials.

diracdeltas · 2016-12-06T07:48:36Z

travis is error'ing at /home/travis/build/brave/sync/test/server/users.js:32

diracdeltas · 2016-12-06T07:50:42Z

server/lib/request-verifier.js

+      return response.status(400).end('Unable to verify the signed request. Please check the signing private key matches the pubkey.')
+    }
+
+    const result = serializer.serializer.byteArrayToString(verifiedBytes)


this needs to handle the case where serializer has not finished initializing; it should probably wait 1s and try again

is there a legit way to do this synchronously

not easily; i guess you could return a 503 and tell the client to retry the request after some time

diracdeltas · 2016-12-06T07:58:27Z

server/lib/request-verifier.js

+    if (!body || body.length === 0) {
+      return response.status(400).end('Signed request body is required.')
+    }
+    const publicKey = Buffer.from(userId, 'base64')


does this need to be urlsafe base64?

i think it's done automagically – if the corresponding test passes consistently it's probably okay?

https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_string_encoding indicates its prolly fine

diracdeltas · 2016-12-06T08:03:50Z

server/lib/request-verifier.js

+      return response.status(400).end('Signed request body is required.')
+    }
+    const publicKey = Buffer.from(userId, 'base64')
+    const verifiedBytes = crypto.verify(Buffer.from(body), publicKey)


body is a Buffer, so Buffer.from seems unnecessary here

diracdeltas · 2016-12-06T08:07:14Z

test/server/lib/request-verifier.js

+    function signedTimestamp (secretKey, timestamp) {
+      if (!timestamp) { timestamp = Math.floor(Date.now() / 1000) }
+      const message = timestamp.toString()
+      return crypto.sign(serializer.serializer.stringToByteArray(message), secretKey)


this needs to ensure that serializer is initialized; i handled this by putting the tests in the serializer.init callback in test/serializer.js

diracdeltas · 2016-12-06T08:07:37Z

test/server/users.js

+    function signedTimestamp (secretKey, timestamp) {
+      if (!timestamp) { timestamp = Math.floor(Date.now() / 1000) }
+      const message = timestamp.toString()
+      return crypto.sign(serializer.serializer.stringToByteArray(message), secretKey)


also needs to ensure serializer is initialized

ayumi

thanks, have updated serializer and tests to wait until it's been initialized.

ayumi · 2016-12-06T08:21:09Z

lib/serializer.js

+
+module.exports.serializer = null
+
+module.exports.initSerializer = function (apiFile/* : string */) {


Updated this to return a Promise

maybe init() above should just do this?

i made initSerializer() so a whole app might share a single serializer instance, since it's kind of #effort to reinit the serializer each time we need it.

yeah seems like this should just be a modification to init instead of its own method

ayumi · 2016-12-06T08:21:09Z

server/lib/request-verifier.js

+      return response.status(400).end('Unable to verify the signed request. Please check the signing private key matches the pubkey.')
+    }
+
+    const result = serializer.serializer.byteArrayToString(verifiedBytes)


is there a legit way to do this synchronously

diracdeltas · 2016-12-06T08:23:04Z

test/server/users.js

-            } else {
-              t.fail(`${t.name} (${response.statusCode}) (${body})`)
-            }
+          // TODO: Use protobuf?


++ protobuf here

diracdeltas · 2016-12-06T08:39:08Z

test/server/lib/request-verifier.js

+const serializer = require('../../../lib/serializer.js')
+const Express = require('express')
+
+test('users router', (t) => {


for async tests, i think it is best practice to use t.plan so that the test doesn't abort early if the callback with t.end gets run before some of the other callbacks

diracdeltas · 2016-12-06T08:40:25Z

test/server/users.js

@@ -3,90 +3,122 @@ const awsSdk = require('aws-sdk')
 const config = require('config')
 const crypto = require('../../lib/crypto')
 const request = require('request')
-const supertest = require('supertest')
+const serializer = require('../../lib/serializer.js')
 const usersRouter = require('../../server/users.js')
 const util = require('../../server/lib/util.js')
 const Express = require('express')

 test('users router', (t) => {


same as above re: t.plan

diracdeltas · 2016-12-06T08:45:22Z

sweet tests 🍭


        server: require signed requests


        upgrade tape; fix scripts: coverage, test


        refactor serializer.initSerializer() into .init()

ayumi · 2016-12-07T00:07:47Z

have updated with:

aws credentials protobuf
tape test.plan(n)

@diracdeltas can u pls take another look 🎁

diracdeltas · 2016-12-07T01:04:30Z

lib/serializer.js

+/**
+ * Deserializes client sync credentials for accessing AWS and S3.
+ * @param {Uint8Array} bytes
+ * @returns {{aws: <Uint8Array>, s3Post: <Uint8Array>}}


surprised that this deserializes to Uint8Array in the fields instead of the credentials object

diracdeltas · 2016-12-07T01:14:11Z

lib/serializer.js

+Serializer.prototype.credentialsToByteArray = function (credentials) {
+  return this.api.Credentials.encode({
+    aws: this.api.Credentials.lookup('Aws').create(credentials.aws),
+    s3Post: this.api.Credentials.lookup('S3Post').create(credentials.s3Post)


does this.api.Credentials.encode(credentials) work here? i think that would avoid the fields being Uint8Arrays instead of the original object types

see: typo comment above regarding return of decode not being UInt8Array

I think encode() for a type with nested types only works when the nested types are legit instances
{
aws: {Credentials.Aws},
s3Post: {Credentials.S3Post}
}

so the fields are the actual types instead of Uint8Arrays with this the way it is now? would be good to have a test to prove that

if so, lgtm for merging 🎆

diracdeltas · 2016-12-07T01:18:37Z

lib/serializer.js

@@ -5,16 +5,22 @@ const pb = require('protobufjs')

 module.exports.init = function (apiFile/* : string */) {
  return pb.load(apiFile || './lib/api.proto').then((r) => {
-    return new Serializer({
+    if (this.serializer) { return this.serializer }


i'm not sure this line helps actually. https://nodejs.org/api/modules.html#modules_caching suggests that every module import gets the same object, so calling 'init' won't help future imports.

yes, but we've been calling init() which always instantiates new Serializers.

with this return, it caches a single Serializer instance onto the shared cached serializer module and returns it next time we try to init()

i see; i didn't intend multiple calls of init in the same module to be used outside of tests. the intended pattern is module calls init, saves the serializer in its own scope, calls the saved serializer in the future.

not a big deal either way


        Server: serialize AWS credentials with protobuf


        server tests: use tape test plan()


        Try to fix test with expired s3 post params

ayumi force-pushed the feature/verified-requests branch from a28e89c to dff58e2 Dec 6, 2016

diracdeltas reviewed Dec 6, 2016

View changes

ayumi force-pushed the feature/verified-requests branch 2 times, most recently from 2e529ab to 76084e2 Dec 6, 2016

ayumi reviewed Dec 6, 2016

View changes

diracdeltas reviewed Dec 6, 2016

View changes

test/server/users.js Outdated

} else {

t.fail(`${t.name} (${response.statusCode}) (${body})`)

}

// TODO: Use protobuf?

This comment has been minimized.

Sign in to view

Copy link

diracdeltas Dec 6, 2016
Member

++ protobuf here

diracdeltas reviewed Dec 6, 2016

View changes

ayumi added 2 commits Dec 6, 2016

server: require signed requests

988be03

upgrade tape; fix scripts: coverage, test

37a5e6d

ayumi force-pushed the feature/verified-requests branch from c437afe to 1cf97e4 Dec 6, 2016

refactor serializer.initSerializer() into .init()

23869fb

ayumi force-pushed the feature/verified-requests branch from 1cf97e4 to 2f07193 Dec 6, 2016

diracdeltas reviewed Dec 7, 2016

View changes

ayumi force-pushed the feature/verified-requests branch from 7f317f4 to afd4f62 Dec 7, 2016

ayumi added 3 commits Dec 6, 2016

Server: serialize AWS credentials with protobuf

299e662

server tests: use tape test plan()

7c89b25

Try to fix test with expired s3 post params

Loading status checks…

ab2e5c1

ayumi force-pushed the feature/verified-requests branch from afd4f62 to ab2e5c1 Dec 7, 2016

ayumi merged commit e2dd84b into master Dec 7, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

continuous-integration/travis-ci/push The Travis CI build passed
Details

ayumi deleted the feature/verified-requests branch Dec 7, 2016

brave / sync

server: require signed requests #4

server: require signed requests #4

ayumi commented Dec 6, 2016

diracdeltas commented Dec 6, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ayumi left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

diracdeltas commented Dec 6, 2016

ayumi commented Dec 7, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.


		module.exports.serializer = null

		module.exports.initSerializer = function (apiFile/* : string */) {

brave / sync

Join GitHub today

server: require signed requests #4

server: require signed requests #4

Conversation

ayumi commented Dec 6, 2016

diracdeltas commented Dec 6, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ayumi left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

diracdeltas commented Dec 6, 2016

ayumi commented Dec 7, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

diracdeltas Dec 7, 2016 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

diracdeltas Dec 7, 2016 •

edited