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

Validate Attestation Requests #1468

Merged
merged 15 commits into from
Oct 30, 2019
2 changes: 1 addition & 1 deletion packages/attestation-service/config/.env.development
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
DB_URL=sqlite://db/dev.db
DATABASE_URL=sqlite://db/dev.db
CELO_PROVIDER=https://integration-infura.celo-testnet.org
ACCOUNT_ADDRESS=0xE6e53b5fc2e18F51781f14a3ce5E7FD468247a15
ATTESTATION_KEY=x
Expand Down
6 changes: 3 additions & 3 deletions packages/attestation-service/config/database.json
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
{
"development": {
"use_env_variable": "DB_URL"
"use_env_variable": "DATABASE_URL"
},
"staging": {
"use_env_variable": "DB_URL"
"use_env_variable": "DATABASE_URL"
},
"production": {
"use_env_variable": "DB_URL"
"use_env_variable": "DATABASE_URL"
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
'use strict'
module.exports = {
up: async (queryInterface, Sequelize) => {
const transaction = await queryInterface.sequelize.transaction()

try {
await queryInterface.createTable('Attestations', {
id: {
allowNull: false,
autoIncrement: true,
primaryKey: true,
type: Sequelize.INTEGER,
},
account: {
allowNull: false,
type: Sequelize.STRING,
},
phoneNumber: {
allowNull: false,
type: Sequelize.STRING,
},
issuer: {
allowNull: false,
type: Sequelize.STRING,
},
createdAt: {
allowNull: false,
type: Sequelize.DATE,
},
updatedAt: {
allowNull: false,
type: Sequelize.DATE,
},
})

await queryInterface.addIndex(
'Attestations',
['account', 'phoneNumber', 'issuer'],
{ fields: ['account', 'phoneNumber', 'issuer'], unique: true },
{ transaction }
)

await transaction.commit()
} catch (error) {
await transaction.rollback()
throw error
}
},
down: (queryInterface, Sequelize) => {
return queryInterface.dropTable('Attestations')
},
}
9 changes: 7 additions & 2 deletions packages/attestation-service/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,24 +17,29 @@
"clean": "tsc -b . --clean",
"clean:all": "yarn clean && rm -rf lib",
"prepublishOnly": "yarn build:gen && yarn build",
"start": "TS_NODE_FILES=true ts-node src/index.ts",
"start-ts": "TS_NODE_FILES=true ts-node src/index.ts",
"start": "node lib/index.js",
"db:create:dev": "mkdir -p db && touch db/dev.db",
"db:migrate": "sequelize db:migrate",
"db:migrate:undo": "sequelize db:migrate:undo",
"db:migrate:dev": "sequelize db:migrate",
"dev": "CONFIG=config/.env.development nodemon",
"lint": "tslint -c tslint.json --project ."
},
"dependencies": {
"@celo/contractkit": "0.1.1",
"@celo/contractkit": "0.1.6",
"@celo/utils": "^0.1.0",
"bignumber.js": "^7.2.0",
"body-parser": "1.19.0",
"debug": "^4.1.1",
"dotenv": "8.0.0",
"eth-lib": "^0.2.8",
"io-ts": "2.0.1",
"fp-ts":"2.1.1",
"nexmo": "2.4.2",
"web3": "1.0.0-beta.37",
"express": "4.17.1",
"mysql2": "2.0.0-alpha1",
"pg": "7.12.1",
"pg-hstore": "2.3.3",
"sequelize": "5.13.1",
Expand Down
115 changes: 101 additions & 14 deletions packages/attestation-service/src/attestation.ts
Original file line number Diff line number Diff line change
@@ -1,38 +1,125 @@
import { AttestationState } from '@celo/contractkit/lib/wrappers/Attestations'
import { attestToIdentifier, SignatureUtils } from '@celo/utils'
import { privateKeyToAddress } from '@celo/utils/lib/address'
import { retryAsyncWithBackOff } from '@celo/utils/lib/async'
import express from 'express'
import * as t from 'io-ts'
import { existingAttestationRequest, kit, persistAttestationRequest } from './db'
import { Address, AddressType, E164Number, E164PhoneNumberType } from './request'
import { sendSms } from './sms'
function signAttestation(phoneNumber: string, account: string) {

export const AttestationRequestType = t.type({
phoneNumber: E164PhoneNumberType,
account: AddressType,
issuer: AddressType,
})

export type AttestationRequest = t.TypeOf<typeof AttestationRequestType>

function getAttestationKey() {
if (process.env.ATTESTATION_KEY === undefined) {
console.error('Did not specify ATTESTATION_KEY')
throw new Error('Did not specify ATTESTATION_KEY')
}

const signature = attestToIdentifier(phoneNumber, account, process.env.ATTESTATION_KEY)
return process.env.ATTESTATION_KEY
}

async function validateAttestationRequest(request: AttestationRequest) {
// check if it exists in the database
if (
(await existingAttestationRequest(request.phoneNumber, request.account, request.issuer)) !==
null
) {
throw new Error('Attestation already sent')
Copy link
Contributor

Choose a reason for hiding this comment

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

So this implies users cannot request another delivery if the SMS did not arrive?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, not at this moment, though we could consider circumstances in which the user could re-request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Re-requesting is a pretty useful debugging tool for verification atm. I'd say (anecdotally) half the time I get 2/3, the third comes if I retry verification. So a better alternative here would be a retry threshold, which could be just 1 time for now. But I understand that's a fair bit more complexity here so open to deferring that to another PR if you prefer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would prefer to keep this out for another PR. Raised #1502 for it.

}
const key = getAttestationKey()
const address = privateKeyToAddress(key)

// TODO: Check with the new Accounts.sol
if (address.toLowerCase() !== request.issuer.toLowerCase()) {
throw new Error(`Mismatching issuer, I am ${address}`)
}

const attestations = await kit.contracts.getAttestations()
const state = await attestations.getAttestationState(
request.phoneNumber,
request.account,
request.issuer
)

console.info(state)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope (at least not in this form)

if (state.attestationState !== AttestationState.Incomplete) {
throw new Error('No incomplete attestation found')
}

// TODO: Check expiration
return
}

async function validateAttestation(
attestationRequest: AttestationRequest,
attestationCode: string
) {
const attestations = await kit.contracts.getAttestations()
const isValid = await attestations.validateAttestationCode(
attestationRequest.phoneNumber,
attestationRequest.account,
attestationRequest.issuer,
attestationCode
)
if (!isValid) {
throw new Error('Valid attestation could not be provided')
}
return
}

function signAttestation(phoneNumber: E164Number, account: Address) {
const signature = attestToIdentifier(phoneNumber, account, getAttestationKey())

return SignatureUtils.serializeSignature(signature)
}

function toBase64(str: string) {
return Buffer.from(str, 'hex').toString('base64')
return Buffer.from(str.slice(2), 'hex').toString('base64')
}

function createAttestationTextMessage(attestationCode: string) {
return `<#> ${toBase64(attestationCode)} ${process.env.APP_SIGNATURE}`
}

export async function handleAttestationRequest(req: express.Request, res: express.Response) {
// TODO: Should parse request appropriately

// TODO: Should validate request here
// const attestations = await kit.contracts.getAttestations()

// Produce attestation
const attestationCode = signAttestation(req.body.phoneNumber, req.body.account)
const textMessage = createAttestationTextMessage(attestationCode)
export async function handleAttestationRequest(
_req: express.Request,
Copy link
Contributor

Choose a reason for hiding this comment

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

why the underscore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In typescript (and many other languages), a leading underscore indicates the parameter not being used. Not prefixing is I believe a lint violation (or maybe even compiler?)

Copy link
Contributor

Choose a reason for hiding this comment

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

huh, didn't know that!

res: express.Response,
attestationRequest: AttestationRequest
) {
let attestationCode
try {
await validateAttestationRequest(attestationRequest)
attestationCode = signAttestation(attestationRequest.phoneNumber, attestationRequest.account)
await validateAttestation(attestationRequest, attestationCode)
} catch (error) {
console.error(error)
res.status(422).json({ success: false, error: error.toString() })
return
}

// Send the SMS
await retryAsyncWithBackOff(sendSms, 10, [req.body.phoneNumber, textMessage], 1000)
try {
const textMessage = createAttestationTextMessage(attestationCode)
await persistAttestationRequest(
attestationRequest.phoneNumber,
attestationRequest.account,
attestationRequest.issuer
)
await retryAsyncWithBackOff(sendSms, 10, [attestationRequest.phoneNumber, textMessage], 1000)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: wrapping the phone number and txt message in an array seems odd to me

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 our current interface into retryAsyncWithBackOff which I don't think we have much alternative to unless we convert all our functions to only take one argument?

Copy link
Contributor

Choose a reason for hiding this comment

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

if we make change to signature to move up the delay param and then have ...args for the rest, we won't need to wrap the params. But it's nbd

} catch (error) {
console.error(error)
res.status(500).json({
success: false,
error: 'Something went wrong while attempting to send SMS, try again later',
})
return
}

res.json({ success: true })
}
22 changes: 20 additions & 2 deletions packages/attestation-service/src/db.ts
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import { ContractKit, newKit } from '@celo/contractkit'
import { Sequelize } from 'sequelize'
import { fetchEnv } from './env'
import Attestation, { AttestationStatic } from './models/attestation'

export let sequelize: Sequelize | undefined

export function initializeDB() {
if (sequelize === undefined) {
sequelize = new Sequelize(fetchEnv('DB_URL'))
sequelize = new Sequelize(fetchEnv('DATABASE_URL'))
return sequelize.authenticate() as Promise<void>
}

return Promise.resolve()
}

Expand All @@ -20,3 +20,21 @@ export function initializeKit() {
kit = newKit(fetchEnv('CELO_PROVIDER'))
}
}

export async function existingAttestationRequest(
phoneNumber: string,
account: string,
issuer: string
): Promise<AttestationStatic | null> {
const AttestationTable = await Attestation(sequelize!)
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't know anything about sequelize, just wondering if it'd be better to cache this AttestationTable object instead of recreating it? maybe a non-issue

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 personally don't like global caching too much so eventually there should probably be a cache manager, but I figured for now it's no big deal

Copy link
Contributor

Choose a reason for hiding this comment

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

If you but it above in the global scope, it's not actually global caching, it's module level caching, which is totally fine. If you don't export the cache var, it won't pollute your global scope. I think the bias against is mostly a holdover from browser days when the polluting the global scope was a big issue.
But yeah really nbd, just a thought.

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 changed it to module level caching!

return AttestationTable.findOne({ where: { phoneNumber, account, issuer } })
}

export async function persistAttestationRequest(
phoneNumber: string,
account: string,
issuer: string
) {
const AttestationTable = await Attestation(sequelize!)
return AttestationTable.create({ phoneNumber, account, issuer })
}
5 changes: 3 additions & 2 deletions packages/attestation-service/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import * as dotenv from 'dotenv'
import express from 'express'
import { handleAttestationRequest } from './attestation'
import { AttestationRequestType, handleAttestationRequest } from './attestation'
import { initializeDB, initializeKit } from './db'
import { parseRequest } from './request'
import { initializeSmsProviders } from './sms'

async function init() {
Expand All @@ -18,7 +19,7 @@ async function init() {
const port = process.env.PORT || 3000
app.listen(port, () => console.log(`Server running on ${port}!`))

app.post('/attestations', handleAttestationRequest)
app.post('/attestations', parseRequest(AttestationRequestType, handleAttestationRequest))
}

init().catch((err) => {
Expand Down
18 changes: 18 additions & 0 deletions packages/attestation-service/src/models/attestation.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { BuildOptions, DataTypes, Model, Sequelize } from 'sequelize'

interface AttestationModel extends Model {
readonly id: number
account: string
phoneNumber: string
issuer: string
}

export type AttestationStatic = typeof Model &
(new (values?: object, options?: BuildOptions) => AttestationModel)

export default (sequelize: Sequelize) =>
sequelize.define('Attestations', {
account: DataTypes.STRING,
phoneNumber: DataTypes.STRING,
issuer: DataTypes.STRING,
}) as AttestationStatic
Loading