From 37ea1464b03231b2819e05667a25640b54f7b567 Mon Sep 17 00:00:00 2001 From: dschom Date: Wed, 5 Mar 2025 17:52:39 -0800 Subject: [PATCH] task(recovery-phone): Support twilio api key Because: - We want to use twilio API keys - We don't want to use the twilio Auth Token This Commit: - Switches over to using api keys - Adds method for signing webhook urls, so we can validate webhook calls originated from us. - Adds ability to configure webhook url, keys for signing, and api keys. - Still supports authToken so it's backwards compatible - Creates custom mechanism for validating webhook calls, since the authToken is used by twilio to validate the signed payload. - Removes the twilio-signature auth strategy and moves validation inline due to hapi limitations accessing the payload in the authorization. - Adds lots of comments and some doc about how to manually test this with ngrok. - Updates functional tests so they work with api keys - Updates functional tests so they can work for smoke tests as well as ci pipeline tests. - Updates functional tests so that twilio client can be configured independently of auth servers twilio client. --- libs/accounts/recovery-phone/README.md | 20 ++ .../src/lib/recovery-phone.service.spec.ts | 176 ++++++++++++++---- .../src/lib/recovery-phone.service.ts | 118 ++++++++++-- .../recovery-phone/src/lib/sms.manager.ts | 8 +- .../recovery-phone/src/lib/twilio.config.ts | 42 ++++- .../src/lib/twilio.provider.spec.ts | 3 +- .../recovery-phone/src/lib/twilio.provider.ts | 9 +- .../recovery-phone/src/lib/util.spec.ts | 41 ++++ libs/accounts/recovery-phone/src/lib/util.ts | 70 +++++++ packages/functional-tests/README.md | 35 ++++ packages/functional-tests/lib/sms.ts | 56 ++++-- packages/functional-tests/lib/targets/base.ts | 19 +- .../functional-tests/lib/targets/index.ts | 33 ++++ .../tests/settings/recoveryPhone.spec.ts | 81 +++++--- packages/fxa-auth-server/bin/key_server.js | 1 + packages/fxa-auth-server/config/index.ts | 24 ++- .../routes/auth-schemes/twilio-signature.ts | 42 ----- .../lib/routes/recovery-phone.ts | 85 ++++++++- packages/fxa-auth-server/lib/server.js | 5 - .../routes/auth-schemes/twilio-signature.js | 90 --------- .../test/local/routes/recovery-phone.js | 90 ++++++++- 21 files changed, 805 insertions(+), 243 deletions(-) create mode 100644 libs/accounts/recovery-phone/src/lib/util.spec.ts create mode 100644 libs/accounts/recovery-phone/src/lib/util.ts delete mode 100644 packages/fxa-auth-server/lib/routes/auth-schemes/twilio-signature.ts delete mode 100644 packages/fxa-auth-server/test/local/routes/auth-schemes/twilio-signature.js diff --git a/libs/accounts/recovery-phone/README.md b/libs/accounts/recovery-phone/README.md index c3f75d6eb34..475ccceaf3e 100644 --- a/libs/accounts/recovery-phone/README.md +++ b/libs/accounts/recovery-phone/README.md @@ -13,3 +13,23 @@ Run `nx test-unit accounts-recovery-phone` to execute the unit tests via [Jest]( ## Running integration tests Run `nx test-integration accounts-recovery-phone` to execute the integration tests via [Jest](https://jestjs.io). + +## Testing webhook callbacks for message status updates + +Important Tip! To manually test webhook callbacks isn't super straight forward. Per Twilio's docs the best way to manually test is as follows. + +1. Use ngrok to reverse proxy: ngrok http 9000 +2. Whatever url ngrok spits out, update the auth server configuration to use this value plus `/v1/recovery_phone/status` as the twilio webHook url value. eg. + `RECOVERY_PHONE__TWILIO__WEBHOOK_URL=https://YOUR_NGROK_SUBDOMAIN.ngrok-free.app/v1/recovery_phone/status` +3. Start up the server again `dotenv -- yarn restart auth --update-env`. Or the whole stack if you are starting from scratch `dotenv -- yarn start`. +4. Create an account +5. Enable 2FA +6. Register a recovery phone +7. Watch the logs, you'll see twilio making the call back to the `/v1/recovery_phone/status` end point. + +(Based on, https://www.twilio.com/en-us/blog/test-your-webhooks-locally-with-ngrok-html) + +Also it's good to note that there are some caveats about configuration for webhooks. If you have an twilio authToken in used, the webhook will validate +the `X-Twilio-Signature` header. If you are using twilio API keys, then we need the fxaPublicKey/fxaPrivateKey pair set in the config. Reach +out to a teammate for this value, or generate one yourself. There's a method that will do this in the `util.ts`. And make sure this key pair +is in the config! diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts index efb5a78902b..21af72cbc90 100644 --- a/libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts +++ b/libs/accounts/recovery-phone/src/lib/recovery-phone.service.spec.ts @@ -21,6 +21,11 @@ import { MessageStatus } from 'twilio/lib/rest/api/v2010/account/message'; import { TwilioConfig } from './twilio.config'; import { getExpectedTwilioSignature } from 'twilio/lib/webhooks/webhooks'; import { SegmentedMessage } from 'sms-segments-calculator'; +import { + createNewFxaKeyPair, + createRandomFxaMessage, + signFxaMessage, +} from './util'; describe('RecoveryPhoneService', () => { const phoneNumber = '+15005551234'; @@ -70,11 +75,21 @@ describe('RecoveryPhoneService', () => { }, } satisfies RecoveryPhoneConfig; + const mockAccountSid = 'AC00000000000000000000000000000000'; + const mockAuthToken = '00000000000000000000000000000000'; + const mockValidateWebhookCalls = true; + const mockWebhookUrl = + 'https://accounts.firefox.com/v1/recovery_phone/message_status'; + + const mockKeys = createNewFxaKeyPair(); + const mockTwilioConfig: TwilioConfig = { - accountSid: 'AC00000000000000000000000000000000', - authToken: '00000000000000000000000000000000', - webhookUrl: 'http://accounts.firefox.com/recovery-phone/message-status', - validateWebhookCalls: true, + accountSid: mockAccountSid, + authToken: undefined, + webhookUrl: mockWebhookUrl, + validateWebhookCalls: mockValidateWebhookCalls, + fxaPublicKey: mockKeys.publicKey, + fxaPrivateKey: mockKeys.privateKey, } satisfies TwilioConfig; const mockGetFormattedMessage = async (code: string) => { @@ -151,6 +166,7 @@ describe('RecoveryPhoneService', () => { expect(mockSmsManager.sendSMS).toBeCalledWith({ to: phoneNumber, body: (await mockGetFormattedMessage(code)).msg, + statusCallback: expect.stringContaining(mockWebhookUrl), }); expect(mockRecoveryPhoneManager.storeUnconfirmed).toBeCalledWith( uid, @@ -182,6 +198,7 @@ describe('RecoveryPhoneService', () => { expect(mockSmsManager.sendSMS).toBeCalledWith({ to: phoneNumber, body: (await mockGetFormattedMessage(code)).msg, + statusCallback: expect.stringContaining(mockWebhookUrl), }); expect(mockRecoveryPhoneManager.storeUnconfirmed).toBeCalledWith( uid, @@ -206,6 +223,7 @@ describe('RecoveryPhoneService', () => { expect(mockSmsManager.sendSMS).toBeCalledWith({ to: phoneNumber, body: (await mockGetFormattedMessage(code)).msg, + statusCallback: expect.stringContaining(mockWebhookUrl), }); expect(mockRecoveryPhoneManager.storeUnconfirmed).toBeCalledWith( uid, @@ -500,6 +518,7 @@ describe('RecoveryPhoneService', () => { expect(mockSmsManager.sendSMS).toBeCalledWith({ to: phoneNumber, body: (await mockGetFormattedMessage(code)).msg, + statusCallback: expect.stringContaining(mockWebhookUrl), }); }); @@ -520,6 +539,7 @@ describe('RecoveryPhoneService', () => { expect(mockSmsManager.sendSMS).toBeCalledWith({ to: phoneNumber, body: `Your Mozilla Account code is ${code}`, + statusCallback: expect.stringContaining(mockWebhookUrl), }); }); @@ -540,6 +560,7 @@ describe('RecoveryPhoneService', () => { expect(mockSmsManager.sendSMS).toBeCalledWith({ to: phoneNumber, body: `Failsafe: ${code}`, + statusCallback: expect.stringContaining(mockWebhookUrl), }); }); @@ -592,6 +613,7 @@ describe('RecoveryPhoneService', () => { expect(mockSmsManager.sendSMS).toBeCalledWith({ to: phoneNumber, body: (await mockGetFormattedMessage(code)).msg, + statusCallback: expect.stringContaining(mockWebhookUrl), }); expect(mockRecoveryPhoneManager.removeCode).toBeCalledWith( @@ -716,49 +738,135 @@ describe('RecoveryPhoneService', () => { }); }); - describe('verify twilio signature', () => { - // This is how Twilio generates the signature, see following doc for more info: - // https://www.twilio.com/docs/usage/security#test-the-validity-of-your-webhook-signature - const signature = getExpectedTwilioSignature( - mockTwilioConfig.authToken, - mockTwilioConfig.webhookUrl, - { - foo: 'bar', - } - ); + describe('verify twilio webhook signature', () => { + const AccountSid = 'AC123'; + const From = '+123456789'; afterEach(() => { + mockTwilioConfig.authToken = undefined; mockTwilioConfig.validateWebhookCalls = true; }); - it('can validate twilio signature', () => { - const valid = service.validateTwilioSignature(signature, { - foo: 'bar', + it('will always validate if validateWebhookCalls is false', () => { + mockTwilioConfig.validateWebhookCalls = false; + expect(service.validateTwilioWebhookCallback({})).toBeTruthy(); + }); + + describe('twilio signature', () => { + /** + * This is how Twilio generates the signature, see following doc for more info: + * https://www.twilio.com/docs/usage/security#test-the-validity-of-your-webhook-signature + * + * Note that when using twilio API keys instead of the main twilio auth token, this approach won't work! + * Unfortunately twilio doesn't offer any way to validate the signature with api keys. + * + */ + const twilioSignature = getExpectedTwilioSignature( + mockAuthToken, + mockWebhookUrl, + { + AccountSid, + From, + } + ); + + it('can validate twilio signature', () => { + mockTwilioConfig.authToken = mockAuthToken; + expect( + service.validateTwilioWebhookCallback({ + twilio: { + signature: twilioSignature, + params: { + AccountSid, + From, + }, + }, + }) + ).toBeTruthy(); }); - expect(valid).toBeTruthy(); - }); - it('can invalidate twilio signature due to bad payload', () => { - const valid = service.validateTwilioSignature(signature, { - foo: 'bar', - bar: 'baz', + it('can invalidate twilio signature due to bad payload', () => { + mockTwilioConfig.authToken = mockAuthToken; + const valid = service.validateTwilioWebhookCallback({ + twilio: { + signature: twilioSignature, + params: { + AccountSid: AccountSid + '0', + From: From + '0', + }, + }, + }); + expect(valid).toBeFalsy(); }); - expect(valid).toBeFalsy(); - }); - it('can invalidate twilio signature due to bad signature', () => { - const valid = service.validateTwilioSignature(signature + '0', { - foo: 'bar', + it('can invalidate twilio signature due to bad signature', () => { + mockTwilioConfig.authToken = mockAuthToken; + const valid = service.validateTwilioWebhookCallback({ + twilio: { + signature: twilioSignature + '0', + params: { + AccountSid, + From, + }, + }, + }); + expect(valid).toBeFalsy(); + }); + + it('can create twilio webhook callback url', () => { + mockTwilioConfig.authToken = mockAuthToken; + mockTwilioConfig.webhookUrl = mockWebhookUrl; + + expect(service.createMessageStatusCallback()).toEqual(mockWebhookUrl); }); - expect(valid).toBeFalsy(); }); - it('will always validate if validateWebhookCalls is false', () => { - mockTwilioConfig.validateWebhookCalls = false; - const valid = service.validateTwilioSignature(signature + '0', { - foo: 'bar', + describe('fxa signature', () => { + const { privateKey, publicKey } = createNewFxaKeyPair(); + const fxaMessage = createRandomFxaMessage(); + const fxaSignature = signFxaMessage(privateKey, fxaMessage); + + it('can validate fxa signature', () => { + const valid = service.validateTwilioWebhookCallback({ + fxa: { + signature: fxaSignature, + message: fxaMessage, + }, + }); + expect(valid).toBeFalsy(); + }); + + it('can invalidate fxa-signature due to bad payload', () => { + const valid = service.validateTwilioWebhookCallback({ + fxa: { + signature: fxaSignature, + message: '00', + }, + }); + expect(valid).toBeFalsy(); + }); + + it('can validate fxa-signature due to bad signature', () => { + const valid = service.validateTwilioWebhookCallback({ + fxa: { + signature: '00', + message: fxaMessage, + }, + }); + expect(valid).toBeFalsy(); + }); + + it('can create fxa webhook callback url', () => { + mockTwilioConfig.authToken = undefined; + mockTwilioConfig.webhookUrl = mockWebhookUrl; + mockTwilioConfig.fxaPrivateKey = privateKey; + mockTwilioConfig.fxaPublicKey = publicKey; + + const url = service.createMessageStatusCallback(); + expect(url).toContain(mockWebhookUrl); + expect(url).toContain('?fxaSignature='); + expect(url).toContain('&fxaMessage='); }); - expect(valid).toBeTruthy(); }); }); }); diff --git a/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts b/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts index 5441b3e27f9..f834e56d3c1 100644 --- a/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts +++ b/libs/accounts/recovery-phone/src/lib/recovery-phone.service.ts @@ -22,6 +22,11 @@ import { LOGGER_PROVIDER } from '@fxa/shared/log'; import { StatsD, StatsDService } from '@fxa/shared/metrics/statsd'; import { TwilioConfig } from './twilio.config'; import { validateRequest } from 'twilio'; +import { + createRandomFxaMessage, + signFxaMessage, + validateFxaSignature, +} from './util'; /** SMS message with different fallbacks for varying message sizes. */ export type FormattedMessages = { @@ -128,15 +133,61 @@ export class RecoveryPhoneService { // Pick the message with the best length and send it const formattedMessages = await getFormattedMessages(code); const body = this.getSafeSmsBody(formattedMessages); + const statusCallback = this.createMessageStatusCallback(); const msg = await this.smsManager.sendSMS({ to: phoneNumber, body, + statusCallback, }); // Relay status return this.isSuccessfulSmsSend(msg); } + /** + * Specifies the callback url to get status updates about the message delivery. + * + * IMPORTANT! Twilio signs messages sent to this callback url with a X-Twilio-Signature + * header. Unfortunately, they decided to use the auth token for signature validation. + * This means that if we decide to use api keys instead the auth token, which is best + * practice, we can't validate the X-Twilio-Signature header. + * + * As a work around, we will sign the callback url ourselves. This will stop unauthorized + * requests from being handled by our webhook. It will not, however, guard against + * message tampering in the event of a man in the middle attack on TLS. Luckily we don't + * use these message status updates in a critical way, so this is probably super good + * enough. + * @returns A webhook url + */ + public createMessageStatusCallback() { + let url: string | undefined; + if (this.twilioConfig.webhookUrl) { + // This is the way we'd like to do it. Unfortunately, this won't work with API keys... + // We will leave it here just incase we need to support it. + if (this.twilioConfig.authToken) { + url = this.twilioConfig.webhookUrl; + } + + // Here we will use our own key pair, to sign sign the callback url. + // Not perfect, but good enough, and better than using our authToken. + else if (this.twilioConfig.fxaPrivateKey) { + const message = createRandomFxaMessage(); + const signature = signFxaMessage( + this.twilioConfig.fxaPrivateKey, + message + ); + url = + this.twilioConfig.webhookUrl + + `?fxaSignature=${encodeURIComponent(signature)}` + + `&fxaMessage=${encodeURIComponent(message)}`; + } + } + + // We don't have to provide a callback. If nothing is specified, we drop + // some metrics on message delivery status ¯\_(ツ)_/¯ + return url; + } + public async getNationalFormat(phoneNumber: string) { // When the user _confirms_ their OTP code we also call the lookup endpoint to // store the full data returned in our DB, but we need the national format on the @@ -395,9 +446,11 @@ export class RecoveryPhoneService { // Pick the message with the best length and send it const formattedMessages = await getFormattedMessages(code); const body = this.getSafeSmsBody(formattedMessages); + const statusCallback = this.createMessageStatusCallback(); const msg = await this.smsManager.sendSMS({ to: phoneNumber, body, + statusCallback, }); // Relay status @@ -413,24 +466,65 @@ export class RecoveryPhoneService { } /** - * Produces the signature used to sign requests sent to twilio webhooks - * @param params + * Validates webhook calls coming from twilio * @returns */ - public validateTwilioSignature( - twilioSignature: string, - params: Record - ) { + public validateTwilioWebhookCallback({ + twilio, + fxa, + }: { + twilio?: { signature: string; params: Record }; + fxa?: { signature: string; message: string }; + }) { + // Check flag that toggles validation of webhook calls if (this.twilioConfig.validateWebhookCalls === false) { return true; } - return validateRequest( - this.twilioConfig.authToken, - twilioSignature, - this.twilioConfig.webhookUrl, - params - ); + /** + * IMPORTANT! This is the best way to validate the web hook, + * but the worst way to authenticate the client. We typically + * do not want to rely on the authToken... This is being kept + * around just in case it's needed. + */ + if (twilio && this.twilioConfig.authToken) { + return validateRequest( + this.twilioConfig.authToken, + twilio.signature, + this.twilioConfig.webhookUrl, + twilio.params + ); + } + + /** + * IMPORTANT! When using twilio api keys, we will fallback + * to this approach. As mentioned above. It prevents bogus + * requests, but doesn't validate the actual payload / prevent + * message tampering. At the moment, we don't use this call back + * for anything other metrics, so this is probably good enough. + */ + if (fxa && this.twilioConfig.fxaPublicKey) { + // Check the fxa signature. This validates that the signature was generated + // using our private key. + return validateFxaSignature( + this.twilioConfig.fxaPublicKey, + fxa.signature, + fxa.message + ); + } + + // Unless something is misconfigured, this typically won't happen. Add a log + // just incase... + this.log?.warn('validateTwilioCallback', { + msg: 'Potentially invalid config or args.', + hasFxaPublicKey: !!this.twilioConfig.fxaPublicKey, + hasFxaSignature: !!fxa?.signature, + hasFxaMessage: !!fxa?.message, + hasTwilioAuthToken: !!this.twilioConfig.authToken, + hasTwilioSignature: !!twilio?.signature, + hasTwilioParams: !!twilio?.params, + }); + return false; } /** diff --git a/libs/accounts/recovery-phone/src/lib/sms.manager.ts b/libs/accounts/recovery-phone/src/lib/sms.manager.ts index 8e496baf9dd..259ca944b68 100644 --- a/libs/accounts/recovery-phone/src/lib/sms.manager.ts +++ b/libs/accounts/recovery-phone/src/lib/sms.manager.ts @@ -100,10 +100,12 @@ export class SmsManager { public async sendSMS({ to, body, + statusCallback, uid, }: { to: string; body: string; + statusCallback?: string; uid?: string; }) { // Calling code should try to avoid this from happening though, but we @@ -126,12 +128,13 @@ export class SmsManager { } } - return await this._sendSMS(to, body, uid, 0); + return await this._sendSMS(to, body, statusCallback, uid, 0); } private async _sendSMS( to: string, body: string, + statusCallback: string | undefined, uid: string | undefined, retryCount: number ): Promise { @@ -143,6 +146,7 @@ export class SmsManager { to, from, body, + statusCallback, }); // Typically the message will be in queued status. The following metric and log // can help track or debug send problems. @@ -159,7 +163,7 @@ export class SmsManager { await new Promise((r) => setTimeout(r, Math.pow(2, retryCount++) * 1000) ); - return await this._sendSMS(to, body, uid, retryCount); + return await this._sendSMS(to, body, statusCallback, uid, retryCount); } } diff --git a/libs/accounts/recovery-phone/src/lib/twilio.config.ts b/libs/accounts/recovery-phone/src/lib/twilio.config.ts index f2452be706b..453cb38a6d6 100644 --- a/libs/accounts/recovery-phone/src/lib/twilio.config.ts +++ b/libs/accounts/recovery-phone/src/lib/twilio.config.ts @@ -8,12 +8,52 @@ import { IsBoolean, IsString } from 'class-validator'; * Configuration for twilio client. See twilio SDK docs for more details. */ export class TwilioConfig { + /** + * The twilio account sid + */ @IsString() accountSid!: string; + + /** + * The twilio auth token. Note that this, or an apiKey/apiSecret must be set! + * Using the auth token is not preferred. + */ @IsString() - authToken!: string; + authToken?: string; + + /** + * The webhook url that twilio will deliver status updates about messages to. + */ @IsString() webhookUrl!: string; + + /** + * Flag that toggles on / off webhook validation. + */ @IsBoolean() validateWebhookCalls!: boolean; + + /** + * A twilio apiKey. Works in conjunction with apiSecret. This is the preferred path for client authentication. + */ + @IsString() + apiKey?: string; + + /** + * A twilio apiSecret. Works in conjunction with apiKey. This is the preferred path for client authentication. + */ + @IsString() + apiSecret?: string; + + /** + * A public key for validating webhook messages. Works in conjunction with fxaPrivateKey. + */ + @IsString() + fxaPublicKey?: string; + + /** + * A private key for signing webhook message callback urls. Works in conjunction with fxaPublicKey. + */ + @IsString() + fxaPrivateKey?: string; } diff --git a/libs/accounts/recovery-phone/src/lib/twilio.provider.spec.ts b/libs/accounts/recovery-phone/src/lib/twilio.provider.spec.ts index 6d77eeeda7d..89d19cf8c0b 100644 --- a/libs/accounts/recovery-phone/src/lib/twilio.provider.spec.ts +++ b/libs/accounts/recovery-phone/src/lib/twilio.provider.spec.ts @@ -14,7 +14,8 @@ describe('TwilioFactory', () => { const MockTwilioConfig = { accountSid: 'AC', - authToken: '', + apiKey: 'SK', + apiSecret: 'SHHH', webhookUrl: 'https://accounts.firefox.com/v1/recovery_phone/message_status', validateWebhookCalls: true, } satisfies TwilioConfig; diff --git a/libs/accounts/recovery-phone/src/lib/twilio.provider.ts b/libs/accounts/recovery-phone/src/lib/twilio.provider.ts index 6ccf05ac50e..b78b6876c3a 100644 --- a/libs/accounts/recovery-phone/src/lib/twilio.provider.ts +++ b/libs/accounts/recovery-phone/src/lib/twilio.provider.ts @@ -22,7 +22,14 @@ export const TwilioProvider = Symbol('TwilioProvider'); export const TwilioFactory: Provider = { provide: TwilioProvider, useFactory: (config: TwilioConfig) => { - const { accountSid, authToken } = config; + const { accountSid, authToken, apiKey, apiSecret } = config; + + // Preferred way! + if (accountSid && apiKey && apiSecret) { + return new Twilio(apiKey, apiSecret, { accountSid }); + } + + // Deprecated way! return new Twilio(accountSid, authToken); }, inject: [TwilioConfig], diff --git a/libs/accounts/recovery-phone/src/lib/util.spec.ts b/libs/accounts/recovery-phone/src/lib/util.spec.ts new file mode 100644 index 00000000000..7f43ca0c35d --- /dev/null +++ b/libs/accounts/recovery-phone/src/lib/util.spec.ts @@ -0,0 +1,41 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import { + createNewFxaKeyPair, + createRandomFxaMessage, + signFxaMessage, + validateFxaSignature, +} from './util'; + +describe('Message Signing', () => { + /** Note, same method that auth-server uses. */ + const { privateKey, publicKey } = createNewFxaKeyPair(); + + afterEach(() => { + jest.resetAllMocks(); + }); + + it('Creates message', async () => { + const message = createRandomFxaMessage(); + expect(message).toBeDefined(); + expect(message.length).toBeGreaterThan(0); + }); + + it('signs message', () => { + const message = createRandomFxaMessage(); + const signature = signFxaMessage(privateKey, message); + expect(signature).toBeDefined(); + expect(signature.length).toBeGreaterThan(0); + }); + + it('validates signature', () => { + const message = createRandomFxaMessage(); + const signature = signFxaMessage(privateKey, message); + expect(validateFxaSignature(publicKey, signature, message)).toBeTruthy(); + expect(validateFxaSignature(publicKey, signature, '00')).toBeFalsy(); + expect(validateFxaSignature(publicKey, '00', message)).toBeFalsy(); + expect(validateFxaSignature(publicKey, '00', '00')).toBeFalsy(); + }); +}); diff --git a/libs/accounts/recovery-phone/src/lib/util.ts b/libs/accounts/recovery-phone/src/lib/util.ts new file mode 100644 index 00000000000..c7156ab5eec --- /dev/null +++ b/libs/accounts/recovery-phone/src/lib/util.ts @@ -0,0 +1,70 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +import { + createSign, + createVerify, + generateKeyPairSync, + randomBytes, +} from 'crypto'; + +const algorithm = 'RSA-SHA256'; + +/** + * Convenience method to create a public/private key pair. + * @returns + */ +export function createNewFxaKeyPair() { + return generateKeyPairSync('rsa' as any, { + modulusLength: 256 * 8, + publicKeyEncoding: { + type: 'pkcs1', + format: 'pem', + }, + privateKeyEncoding: { + type: 'pkcs1', + format: 'pem', + }, + }); +} + +/** + * Creates a random body, for signing. + */ +export function createRandomFxaMessage() { + return randomBytes(128).toString('hex'); +} + +/** + * Signs message using the private key. + * @param message + * @param privateKey + * @param twilioApiSecret + * @returns A signature for the message + */ +export function signFxaMessage(privateKey: string, message: string) { + const sign = createSign(algorithm); + sign.update(message); + sign.end(); + return sign.sign(privateKey, 'base64'); +} + +/** + * Given a message, a public key, and a signature, determines if the signature is + * in fact valid. ie The signature was generated with our private key. + * @param message + * @param publicKey + * @param signature + * @returns + */ +export function validateFxaSignature( + publicKey: string, + signature: string, + message: string +) { + const verify = createVerify(algorithm); + verify.update(message); + verify.end(); + return verify.verify(publicKey, signature, 'base64'); +} diff --git a/packages/functional-tests/README.md b/packages/functional-tests/README.md index cc41bc76326..2b5c2d978b4 100644 --- a/packages/functional-tests/README.md +++ b/packages/functional-tests/README.md @@ -129,3 +129,38 @@ We record traces for failed tests locally and in CI. On CircleCI they are in the ## Avoiding Race condition while writing tests See related [Ecosystem Docs](https://mozilla.github.io/ecosystem-platform/reference/functional-testing#avoiding-race-condition-while-writing-tests) + +## Configuration for Recovery Phone testing + +Recovery phone testing presents a special challenge. Typically when testing locally, or in the CI, we can simply look at redis to validate the state of the messages we send. i.e. If we want to provide the code sent to the end user, we just look at redis. This approach is also convenient because it works with twilio magic test numbers and twilio client testing credentials, which incur no messaging fees. Unfortunately, this approach does not work during smoke testing for stage & production. In this scenario, we don't (and shouldn't) have access to the redis instance. Furthermore, we aren't using test credentials in stage/prod, we are using real credentials, which means we cannot send messages to 'magic' test numbers. Our solution for smoke testing prod/stage case is to ask Twilio for the last message which was just sent out, for our testing user's phone number, which is actually a twilio number we have procured just for testing purposes. + +TL;DR, There are two ways to configure functional tests, we can either use twilio test numbers, and peek at outgoing codes via the redis client, or we can have real phone numbers and peak at outgoing codes via the twilio api. + +For day to day local/CI pipeline testing, we can just use the redis with magic test number approach. This incurs no cost and requires no extra configuration. + +For smoke testing scenarios, or validating this works with a real phone number, we can use the twilio client with a real test phone number approach. To enable this approach, simply add the following environment variables. This will allow us to use the twilio client to peek at codes, and to use a twilio test number to receive messages. + +``` +FUNCTIONAL_TESTS__TWILIO__ACCOUNT_SID=XXX +FUNCTIONAL_TESTS__TWILIO__API_KEY=XXX +FUNCTIONAL_TESTS__TWILIO__API_SECRET=XXX +FUNCTIONAL_TESTS__TWILIO__TEST_NUMBER=XXX +``` + +One final note about CI configuration. It might be necessary to have different settings per testing environment. For example we might want to use different credentials for stage smoke tests typical CI pipeline tests. Or perhaps stage, and production need different api keys. In either case, you can override the default env values by appending an environment name. + +For example, let’s say we wanted stage and production to use unique API keys, and have production use a unique test phone number. Apply the following configuration to our CI environment should do the trick: + +``` +FUNCTIONAL_TESTS__TWILIO__ACCOUNT_SID=XXX +FUNCTIONAL_TESTS__TWILIO__API_KEY=XXX +FUNCTIONAL_TESTS__TWILIO__API_SECRET=XXX +FUNCTIONAL_TESTS__TWILIO__TEST_NUMBER=XXX + +FUNCTIONAL_TESTS__TWILIO__API_KEY__STAGE=XXX +FUNCTIONAL_TESTS__TWILIO__API_SECRET__STAGE=XXX + +FUNCTIONAL_TESTS__TWILIO__API_KEY__PRODUCTION=XXX +FUNCTIONAL_TESTS__TWILIO__API_SECRET__PRODUCTION=XXX +FUNCTIONAL_TESTS__TWILIO__TEST_NUMBER__PRODUCTION=XXX +``` diff --git a/packages/functional-tests/lib/sms.ts b/packages/functional-tests/lib/sms.ts index 5d074e64e8c..d6ef8d2a89d 100644 --- a/packages/functional-tests/lib/sms.ts +++ b/packages/functional-tests/lib/sms.ts @@ -2,30 +2,58 @@ * License, v. 2.0. If a copy of the MPL was not distributed with this * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ -import TwilioSDK from 'twilio'; +import { Twilio } from 'twilio'; import Redis from 'ioredis'; import type { Redis as RedisType } from 'ioredis'; +import { TargetName, getFromEnv, getFromEnvWithFallback } from './targets'; function wait() { return new Promise((r) => setTimeout(r, 500)); } -const accountSid = process.env.RECOVERY_PHONE__TWILIO__ACCOUNT_SID; -const authToken = process.env.RECOVERY_PHONE__TWILIO__AUTH_TOKEN; -const testPhoneNumber = process.env.RECOVERY_PHONE__TWILIO__TEST_NUMBER; - export class SmsClient { - private twilioClient?: TwilioSDK.Twilio; - private redisClient?: RedisType; - private uidCodes: Map; + private readonly twilioClient?: Twilio; + private readonly redisClient?: RedisType; + private readonly uidCodes: Map; private lastCode: string | undefined; private redisClientConnected = false; private hasLoggedRedisConnectionError = false; - constructor() { - if (accountSid && authToken && testPhoneNumber) { - this.twilioClient = new TwilioSDK.Twilio(accountSid, authToken); - } else { + constructor(public readonly targetName: TargetName) { + const accountSid = getFromEnv( + 'FUNCTIONAL_TESTS__SMS_CLIENT__TWILIO__ACCOUNT_SID', + targetName + ); + const apiKey = getFromEnv( + 'FUNCTIONAL_TESTS__SMS_CLIENT__TWILIO__ACCOUNT_API_KEY', + targetName + ); + const apiSecret = getFromEnv( + 'FUNCTIONAL_TESTS__SMS_CLIENT__TWILIO__ACCOUNT_API_SECRET', + targetName + ); + const authToken = getFromEnv( + 'FUNCTIONAL_TESTS__SMS_CLIENT__TWILIO__ACCOUNT_AUTH_TOKEN', + targetName + ); + const enableRedis = getFromEnvWithFallback( + 'FUNCTIONAL_TESTS__SMS_CLIENT__REDIS__ENABLED', + targetName, + targetName === 'local' ? 'true' : 'false' + ); + + if (accountSid && apiKey && apiSecret) { + this.twilioClient = new Twilio(apiKey, apiSecret, { + accountSid, + }); + } else if (accountSid && authToken) { + this.twilioClient = new Twilio(apiKey, apiSecret, { + accountSid, + }); + } + + // When testing local or in CI pipe, we should enable redis. + if (enableRedis === 'true') { this.redisClient = new Redis(); this.redisClient.on('ready', () => { this.redisClientConnected = true; @@ -46,6 +74,10 @@ export class SmsClient { return !!this.twilioClient; } + isRedisEnabled() { + return !!this.redisClient; + } + async getCode(recipientNumber: string, uid: string, timeout = 10000) { if (this.isTwilioEnabled()) { return this._getCodeTwilio(recipientNumber); diff --git a/packages/functional-tests/lib/targets/base.ts b/packages/functional-tests/lib/targets/base.ts index 39baa2ab9fb..5c5d48a2905 100644 --- a/packages/functional-tests/lib/targets/base.ts +++ b/packages/functional-tests/lib/targets/base.ts @@ -24,7 +24,6 @@ interface SubConfig { export abstract class BaseTarget { readonly authClient: AuthClient; readonly emailClient: EmailClient; - readonly smsClient: SmsClient; abstract readonly contentServerUrl: string; abstract readonly paymentsServerUrl: string; abstract readonly relierUrl: string; @@ -32,17 +31,25 @@ export abstract class BaseTarget { abstract readonly name: TargetName; abstract readonly subscriptionConfig: SubConfig; + // Must be lazy loaded, because it depends on abstract field, 'name'. + get smsClient() { + if (this._smsClient == null) { + this._smsClient = new SmsClient(this.name); + } + return this._smsClient; + } + private _smsClient: SmsClient | undefined; + + get baseUrl() { + return this.contentServerUrl; + } + constructor(readonly authServerUrl: string, emailUrl?: string) { const keyStretchVersion = parseInt( process.env.AUTH_CLIENT_KEY_STRETCH_VERSION || '1' ); this.authClient = this.createAuthClient(keyStretchVersion); this.emailClient = new EmailClient(emailUrl); - this.smsClient = new SmsClient(); - } - - get baseUrl() { - return this.contentServerUrl; } createAuthClient(keyStretchVersion = 1): AuthClient { diff --git a/packages/functional-tests/lib/targets/index.ts b/packages/functional-tests/lib/targets/index.ts index 1d73a4f632a..aaaaefce333 100644 --- a/packages/functional-tests/lib/targets/index.ts +++ b/packages/functional-tests/lib/targets/index.ts @@ -26,3 +26,36 @@ export function create(name: TargetName): BaseTarget { export { BaseTarget as ServerTarget }; export { Credentials } from './base'; + +/** + * Helper function to get a value from the environment. If the key + __ + targetName exists, this + * will be returned. Otherwise, if $key exists it will be returned. Otherwise undefined will be + * returned. + * @param key The environment variable name + * @param targetName The current target, eg local, stage, production. + * @returns the env value, given deference to the env with key + __ + targetName. + */ +export function getFromEnv(key: string, targetName: TargetName) { + return ( + process.env[`${key}__${targetName.toUpperCase()}`] || process.env[`${key}`] + ); +} + +/** + * Same as getFromEnv, expect supports a fall back value if the environment variable is missing. + * @param key The environment variable name + * @param targetName The current target. eg local, stage, production. + * @param fallbackValue A default value to return if no env value can be resolved. + * @returns The env value, given deference to the env with key + __ + targetName. + */ +export function getFromEnvWithFallback( + key: string, + targetName: TargetName, + fallbackValue: string +) { + return ( + process.env[`${key}__${targetName.toUpperCase()}`] || + process.env[`${key}`] || + fallbackValue + ); +} diff --git a/packages/functional-tests/tests/settings/recoveryPhone.spec.ts b/packages/functional-tests/tests/settings/recoveryPhone.spec.ts index 0f85f8e7227..78cfceca3d2 100644 --- a/packages/functional-tests/tests/settings/recoveryPhone.spec.ts +++ b/packages/functional-tests/tests/settings/recoveryPhone.spec.ts @@ -15,46 +15,67 @@ import { RecoveryPhoneSetupPage } from '../../pages/settings/recoveryPhone'; import { FirefoxCommand } from '../../lib/channels'; import { syncDesktopOAuthQueryParams } from '../../lib/query-params'; import { getCode } from 'fxa-settings/src/lib/totp'; +import { TargetName, getFromEnvWithFallback } from '../../lib/targets'; + +// Default test number, see Twilio test credentials phone numbers: +// https://www.twilio.com/docs/iam/test-credentials +const TEST_NUMBER = '4159929960'; + +/** + * Checks the process env for a configured twilio test phone number. Defaults + * to generic magic test number if one is not provided. + * @param targetName The test target name. eg local, stage, prod. + * @returns + */ +function getPhoneNumber(targetName: TargetName) { + return getFromEnvWithFallback( + 'FUNCTIONAL_TESTS__TWILIO__TEST_NUMBER', + targetName, + TEST_NUMBER + ); +} -const realTestPhoneNumber = process.env.RECOVERY_PHONE__TWILIO__TEST_NUMBER; - -function getPhoneNumber(env: string) { - if (env !== 'local' && realTestPhoneNumber) { - return realTestPhoneNumber; - } - // See Twilio test credentials phone numbers: https://www.twilio.com/docs/iam/test-credentials - return '4159929960'; +function usingRealTestPhoneNumber(targetName: TargetName) { + return getPhoneNumber(targetName) !== TEST_NUMBER; } test.describe('severity-1 #smoke', () => { test.describe('recovery phone', () => { - // Run these tests sequentially when using the Twilio API because they rely on the same test phone number. - // When using the Twilio API, we cannot determine the order in which the messages were received. - if (realTestPhoneNumber) { - test.describe.configure({ mode: 'serial' }); - } else { - test.describe.configure({ mode: 'parallel' }); - } - + // Run these tests sequentially. This must be done when using the Twilio API, because they rely on + // the same test phone number, and we cannot determine the order in which the messages were received. + test.describe.configure({ mode: 'serial' }); + + test.beforeAll(async ({ target }) => { + /** + * Important! Twilio does not allow you to fetch messages when using test + * credentials. Twilio also does not allow you to send messages to magic + * test numbers with real credentials. + * + * Therefore, if a 'magic' test number is configured, then we to look + * use redis to peek at codes sent out, and if a 'real' testing phone + * number is being being used, then we need to check the Twilio API for + * the message sent out and look at the code within. + */ + if ( + usingRealTestPhoneNumber(target.name) && + !target.smsClient.isTwilioEnabled() + ) { + throw new Error( + 'Twilio must be enabled when using a real test number.' + ); + } + if ( + !usingRealTestPhoneNumber(target.name) && + !target.smsClient.isRedisEnabled() + ) { + throw new Error('Redis must be enabled when using a real test number.'); + } + }); test.beforeEach(async ({ pages: { configPage }, target }) => { // Ensure that the feature flag is enabled const config = await configPage.getConfig(); test.skip(config.featureFlags.enableAdding2FABackupPhone !== true); test.skip(config.featureFlags.enableUsing2FABackupPhone !== true); - - // Twilio does not allow you to fetch messages when using test credentials. - // Therefore, we fallback to peeking at Redis to get confirmation codes. - if (target.name === 'local') { - expect( - target.smsClient.isTwilioEnabled(), - 'Local env found, use redis and Twilio test creds' - ).toBeFalsy(); - } else { - expect( - target.smsClient.isTwilioEnabled(), - 'Stage/Prod env, use Twilio API' - ).toBeTruthy(); - } }); test('setup fails with invalid number', async ({ diff --git a/packages/fxa-auth-server/bin/key_server.js b/packages/fxa-auth-server/bin/key_server.js index fcc20634717..004862c811b 100755 --- a/packages/fxa-auth-server/bin/key_server.js +++ b/packages/fxa-auth-server/bin/key_server.js @@ -247,6 +247,7 @@ async function run(config) { smsManager, otpCodeManager, config.recoveryPhone, + config.twilio, statsd, log ); diff --git a/packages/fxa-auth-server/config/index.ts b/packages/fxa-auth-server/config/index.ts index 336da3ea136..97eb3a18f50 100644 --- a/packages/fxa-auth-server/config/index.ts +++ b/packages/fxa-auth-server/config/index.ts @@ -2241,8 +2241,8 @@ const convictConf = convict({ format: String, }, authToken: { - default: '?', - doc: 'Twilio Auth Token, required to access api', + default: '', + doc: 'Twilio Auth Token to access api. Note, using apiKey/apiSecret is preferred.', env: 'RECOVERY_PHONE__TWILIO__AUTH_TOKEN', }, webhookUrl: { @@ -2255,6 +2255,26 @@ const convictConf = convict({ doc: 'Controls if twilio signature is validated during webhook calls from twilio', env: 'RECOVERY_PHONE__TWILIO__VALIDATE_WEBHOOK_CALLS', }, + apiKey: { + default: '', + doc: 'An api key used to access the twilio rest api. Note, when provided the authToken is no longer needed.', + env: 'RECOVERY_PHONE__TWILIO__API_KEY', + }, + apiSecret: { + default: '', + doc: 'A secret used in conjunction with the apiKey to access the twilio rest api.', + env: 'RECOVERY_PHONE__TWILIO__API_SECRET', + }, + fxaPublicKey: { + default: '', + doc: 'A key used to to for validating signature in webhook calls.', + env: 'RECOVERY_PHONE__TWILIO__FXA_PUBLIC_KEY', + }, + fxaPrivateKey: { + default: '', + doc: 'A private key used for signing messages provided to twilio webhook calls.', + env: 'RECOVERY_PHONE__TWILIO__FXA_PRIVATE_KEY', + }, }, }); diff --git a/packages/fxa-auth-server/lib/routes/auth-schemes/twilio-signature.ts b/packages/fxa-auth-server/lib/routes/auth-schemes/twilio-signature.ts deleted file mode 100644 index 51148a41835..00000000000 --- a/packages/fxa-auth-server/lib/routes/auth-schemes/twilio-signature.ts +++ /dev/null @@ -1,42 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -import { Container } from 'typedi'; -import { Request, ResponseToolkit } from '@hapi/hapi'; -import { RecoveryPhoneService } from '@fxa/accounts/recovery-phone'; -import AppError from '../../error'; - -export const strategy = function () { - // Resolve the recovery phone service from typedi. This service - // is a wrapper around the calls to the twilio sdk needed to - // authenticate incoming requests. - const recoveryPhoneService = Container.get(RecoveryPhoneService); - if (!recoveryPhoneService) { - throw new Error('RecoveryPhoneService not registered with typedi'); - } - - return () => ({ - authenticate: async function (req: Request, h: ResponseToolkit) { - const signature = req.headers['X-Twilio-Signature']; - if (!signature) { - throw AppError.unauthorized('X-Twilio-Signature header missing'); - } - - if (typeof req.payload !== 'object') { - throw AppError.unauthorized('Invalid payload'); - } - - const valid = recoveryPhoneService.validateTwilioSignature( - req.headers['X-Twilio-Signature'], - req.payload - ); - - // Signature invalid. Deny request - if (!valid) { - throw AppError.unauthorized('X-Twilio-Signature header invalid'); - } - return { signature }; - }, - }); -}; diff --git a/packages/fxa-auth-server/lib/routes/recovery-phone.ts b/packages/fxa-auth-server/lib/routes/recovery-phone.ts index 80acf3567d5..48808b3ed3c 100644 --- a/packages/fxa-auth-server/lib/routes/recovery-phone.ts +++ b/packages/fxa-auth-server/lib/routes/recovery-phone.ts @@ -572,7 +572,85 @@ class RecoveryPhoneHandler { } } + /** + * Validates if the request is a legitimate request from Twilio. Throws an unauthorized + * error if validation fails. + * + * Important notes! + * + * 1. We are doing this inline, because it could require the request payload and + * this is not available during the authentication lifecycle without jumping + * through some weird hoops. + * + * 2. We have two ways of validating requests. The first way is by using a signature + * we generate. This will be used when twilio is configured with api keys and the + * authToken isn't used, which is considered best practice. The downside to this + * approach is that while we can validate the incoming call was signed by us, we + * can't validate the message body. There is very unlikely chance that a man in + * the middle attack on TLS could result in a bogus payload state. We aren't doing + * anything critical with message status updates, so this is probably good enough. + * + * 3. The second way of authenticating is the default twilio approach. Unfortunately + * this requires the authToken to be known and we don't to set this in the env. + * If at some point, validating the request payload becomes super important, we + * might consider this approach, despite the authToken requirement. + * + * @param request A typical hapi request. + */ + validateWebhookCall(request: Request) { + const fxaSignature = request.query?.fxaSignature; + const fxaMessage = request.query?.fxaMessage; + const twilioSignature = request.headers['X-Twilio-Signature']; + const twilioPayload = request.payload; + + this.log?.debug('validateWebhookCall', { + fxaSignature, + fxaMessage, + twilioSignature, + twilioPayload, + }); + + let valid = false; + if (fxaSignature && fxaMessage) { + valid = this.recoveryPhoneService.validateTwilioWebhookCallback({ + fxa: { + signature: fxaSignature, + message: fxaMessage, + }, + }); + } else if (twilioSignature && typeof twilioPayload === 'object') { + valid = this.recoveryPhoneService.validateTwilioWebhookCallback({ + twilio: { + signature: twilioSignature, + params: twilioPayload, + }, + }); + } + + this.log?.debug('validateWebhookCall', { + valid, + }); + + if (valid) { + this.statsd.increment('account.recoveryPhone.validateWebhookCall.valid'); + } else { + this.statsd.increment( + 'account.recoveryPhone.validateWebhookCall.invalid' + ); + throw AppError.unauthorized(`Signature Invalid`); + } + } + + /** + * Takes a request, and processes the message status provided by twilio. This + * is u + * @param request + * @returns + */ async messageStatus(request: Request) { + this.validateWebhookCall(request); + + // We can now continue. await this.recoveryPhoneService.onMessageStatusUpdate( request.payload as TwilioMessageStatus ); @@ -713,10 +791,9 @@ export const recoveryPhoneRoutes = ( method: 'POST', path: '/recovery_phone/message_status', options: { - pre: [{ method: featureEnabledCheck }], - auth: { - strategy: 'twilioSignature', - payload: false, + payload: { + parse: true, + allow: 'application/x-www-form-urlencoded', }, }, handler: function (request: Request) { diff --git a/packages/fxa-auth-server/lib/server.js b/packages/fxa-auth-server/lib/server.js index 020d03b3d21..56aaa1ea025 100644 --- a/packages/fxa-auth-server/lib/server.js +++ b/packages/fxa-auth-server/lib/server.js @@ -13,7 +13,6 @@ const userAgent = require('fxa-shared/lib/user-agent').parseToScalars; const schemeRefreshToken = require('./routes/auth-schemes/refresh-token'); const authOauth = require('./routes/auth-schemes/auth-oauth'); const sharedSecretAuth = require('./routes/auth-schemes/shared-secret'); -const twilioSignatureAuth = require('./routes/auth-schemes/twilio-signature'); const pubsubAuth = require('./routes/auth-schemes/pubsub'); const googleOIDC = require('./routes/auth-schemes/google-oidc'); const { HEX_STRING } = require('./routes/validators'); @@ -477,10 +476,6 @@ async function create(log, error, config, routes, db, statsd, glean) { ); server.auth.strategy('cloudSchedulerOIDC', 'cloudSchedulerOIDC'); - // Authorizes incoming webhook calls from Twilio - server.auth.scheme('twilioSignature', twilioSignatureAuth.strategy()); - server.auth.strategy('twilioSignature', 'twilioSignature'); - // register all plugins and Swagger configuration await server.register([ { diff --git a/packages/fxa-auth-server/test/local/routes/auth-schemes/twilio-signature.js b/packages/fxa-auth-server/test/local/routes/auth-schemes/twilio-signature.js deleted file mode 100644 index e84725a3e09..00000000000 --- a/packages/fxa-auth-server/test/local/routes/auth-schemes/twilio-signature.js +++ /dev/null @@ -1,90 +0,0 @@ -/* This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ - -const { Container } = require('typedi'); -const { assert } = require('chai'); -const AppError = require('../../../../lib/error'); -const { - strategy, -} = require('../../../../lib/routes/auth-schemes/twilio-signature'); -const { RecoveryPhoneService } = require('@fxa/accounts/recovery-phone'); -const sinon = require('sinon'); - -describe('lib/routes/auth-schemes/twilio-signature', () => { - let mockRecoveryPhoneService; - let tempRecoveryPhoneService; - - function getTwilioAuth() { - return strategy()(); - } - - before(() => { - tempRecoveryPhoneService = Container.get(RecoveryPhoneService); - mockRecoveryPhoneService = { - // There is already test coverage that signature validation works - // in the recovery-phone library, so using a simplified mock here. - validateTwilioSignature: sinon.fake((signature) => { - return signature === 'VALID_SIGNATURE'; - }), - }; - Container.set(RecoveryPhoneService, mockRecoveryPhoneService); - }); - - after(() => { - Container.set(RecoveryPhoneService, tempRecoveryPhoneService); - }); - - it('should return valid signature', async () => { - const request = { - headers: { 'X-Twilio-Signature': 'VALID_SIGNATURE' }, - payload: { foo: 'bar' }, - }; - const result = await getTwilioAuth().authenticate(request, { foo: 'bar' }); - assert.equal(result.signature, 'VALID_SIGNATURE'); - }); - - it('throw on missing header', async () => { - const request = { - headers: {}, - payload: { foo: 'bar' }, - }; - try { - await getTwilioAuth().authenticate(request, {}); - assert.fail('Missing X-Twilio-Signature header should have thrown'); - } catch (err) { - assert.deepEqual( - err, - AppError.unauthorized('X-Twilio-Signature header missing') - ); - } - }); - - it('should throw on missing payload', async () => { - const request = { - headers: { 'X-Twilio-Signature': 'VALID_SIGNATURE' }, - }; - try { - await getTwilioAuth().authenticate(request, {}); - assert.fail('Missing payload should have thrown'); - } catch (err) { - assert.deepEqual(err, AppError.unauthorized('Invalid payload')); - } - }); - - it('should throw on invalid signature', async () => { - const request = { - headers: { 'X-Twilio-Signature': 'INVALID_SIGNATURE' }, - payload: { foo: 'bar' }, - }; - try { - await getTwilioAuth().authenticate(request); - assert.fail('Invalid X-Twilio-Signature header should have thrown'); - } catch (err) { - assert.deepEqual( - err, - AppError.unauthorized('X-Twilio-Signature header invalid') - ); - } - }); -}); diff --git a/packages/fxa-auth-server/test/local/routes/recovery-phone.js b/packages/fxa-auth-server/test/local/routes/recovery-phone.js index 2f17926d94d..307e3345814 100644 --- a/packages/fxa-auth-server/test/local/routes/recovery-phone.js +++ b/packages/fxa-auth-server/test/local/routes/recovery-phone.js @@ -3,6 +3,7 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ const { AccountEventsManager } = require('../../../lib/account-events'); +const AppError = require('../../../lib/error'); const chai = require('chai'); const chaiAsPromised = require('chai-as-promised'); @@ -62,6 +63,7 @@ describe('/recovery_phone', () => { stripPhoneNumber: sandbox.fake(), hasConfirmed: sandbox.fake(), onMessageStatusUpdate: sandbox.fake(), + validateTwilioWebhookCallback: sandbox.fake(), }; const mockAccountManager = { verifySession: sandbox.fake(), @@ -657,9 +659,11 @@ describe('/recovery_phone', () => { }); describe('POST /recovery_phone/message_status', async () => { - it('handles a message status update from twilio', async () => { + it('handles a message status update from twilio using X-Twilio-Signature header', async () => { mockRecoveryPhoneService.onMessageStatusUpdate = sinon.fake.resolves(undefined); + mockRecoveryPhoneService.validateTwilioWebhookCallback = + sinon.fake.returns(true); const payload = { AccountSid: 'AC123', @@ -679,11 +683,95 @@ describe('/recovery_phone', () => { }); assert.isDefined(resp); + + assert.equal( + mockRecoveryPhoneService.validateTwilioWebhookCallback.callCount, + 1 + ); + assert.deepEqual( + mockRecoveryPhoneService.validateTwilioWebhookCallback.getCall(0) + .args[0], + { + twilio: { + signature: 'VALID_SIGNATURE', + params: payload, + }, + } + ); + assert.equal(mockRecoveryPhoneService.onMessageStatusUpdate.callCount, 1); assert.equal( mockRecoveryPhoneService.onMessageStatusUpdate.getCall(0).args[0], payload ); }); + + it('handles a message status update from twilio using fxaSignature query param', async () => { + mockRecoveryPhoneService.onMessageStatusUpdate = + sinon.fake.resolves(undefined); + mockRecoveryPhoneService.validateTwilioWebhookCallback = + sinon.fake.returns(true); + + const payload = { + AccountSid: 'AC123', + MessageSid: 'M123', + From: '+1234567890', + MessageStatus: 'delivered', + RawDlrDoneDate: 'TWILIO_DATE_FORMAT', + }; + + const resp = await makeRequest({ + method: 'POST', + path: '/recovery_phone/message_status', + credentials: {}, + headers: { + 'X-Twilio-Signature': 'VALID_SIGNATURE', + }, + query: { + fxaSignature: 'VALID_SIGNATURE', + fxaMessage: 'FXA_MESSAGE', + }, + payload, + }); + + assert.isDefined(resp); + + assert.equal( + mockRecoveryPhoneService.validateTwilioWebhookCallback.callCount, + 1 + ); + assert.deepEqual( + mockRecoveryPhoneService.validateTwilioWebhookCallback.getCall(0) + .args[0], + { + fxa: { + signature: 'VALID_SIGNATURE', + message: 'FXA_MESSAGE', + }, + } + ); + + assert.equal(mockRecoveryPhoneService.onMessageStatusUpdate.callCount, 1); + assert.equal( + mockRecoveryPhoneService.onMessageStatusUpdate.getCall(0).args[0], + payload + ); + }); + + it('throws on invalid / missing signatures', async () => { + mockRecoveryPhoneService.validateTwilioWebhookCallback = + sinon.fake.rejects(AppError.unauthorized('Signature Invalid')); + try { + await makeRequest({ + method: 'POST', + path: '/recovery_phone/message_status', + headers: {}, + payload: {}, + }); + assert.fail('Invalid Signature should have been thrown'); + } catch (err) { + assert.deepEqual(err, AppError.unauthorized('Signature Invalid')); + } + }); }); });