Merge pull request #18344 from mozilla/fxa-11019

Add custom rate limiting for sms confirm and setup

Author: vbudhram

packages/fxa-auth-server/lib/routes/recovery-phone.ts

@@ -31,6 +31,11 @@ enum RecoveryPhoneStatus {
 
 export type Customs = {
   check: (req: AuthRequest, email: string, action: string) => Promise<void>;
+  checkAuthenticated: (
+    req: AuthRequest,
+    uid: string,
+    action: string
+  ) => Promise<void>;
 };
 
 class RecoveryPhoneHandler {
@@ -102,7 +107,7 @@ class RecoveryPhoneHandler {
     if (!email) {
       throw AppError.invalidToken();
     }
-    await this.customs.check(request, email, 'recoveryPhoneCreate');
+    await this.customs.checkAuthenticated(request, uid, 'recoveryPhoneCreate');
 
     try {
       const result = await this.recoveryPhoneService.setupPhoneNumber(
@@ -158,7 +163,11 @@ class RecoveryPhoneHandler {
       throw AppError.invalidToken();
     }
 
-    await this.customs.check(request, email, 'recoveryPhoneConfirmCode');
+    await this.customs.checkAuthenticated(
+      request,
+      uid,
+      'recoveryPhoneConfirmCode'
+    );
 
     let success = false;
     try {

packages/fxa-auth-server/test/local/routes/recovery-phone.js

@@ -33,6 +33,7 @@ describe('/recovery_phone', () => {
   let mockMailer;
   const mockCustoms = {
     check: sandbox.fake(),
+    checkAuthenticated: sandbox.fake(),
   };
   const mockGlean = {
     twoStepAuthPhoneCode: {
@@ -177,9 +178,12 @@ describe('/recovery_phone', () => {
       assert.equal(mockGlean.twoStepAuthPhoneCode.sent.callCount, 1);
       assert.equal(mockGlean.twoStepAuthPhoneCode.sendError.callCount, 0);
 
-      assert.equal(mockCustoms.check.callCount, 1);
-      assert.equal(mockCustoms.check.getCall(0).args[1], email);
-      assert.equal(mockCustoms.check.getCall(0).args[2], 'recoveryPhoneCreate');
+      assert.equal(mockCustoms.checkAuthenticated.callCount, 1);
+      assert.equal(mockCustoms.checkAuthenticated.getCall(0).args[1], uid);
+      assert.equal(
+        mockCustoms.checkAuthenticated.getCall(0).args[2],
+        'recoveryPhoneCreate'
+      );
     });
 
     it('indicates failure sending sms', async () => {

packages/fxa-customs-server/lib/config/config.js

@@ -475,6 +475,64 @@ module.exports = function (fs, path, url, convict) {
           },
         },
       },
+      recoveryPhoneCreateCodeRules: {
+        actions: {
+          doc:
+            'Array of actions that this rule should be applied to. A user should only be able to initiate ' +
+            'a recovery phone setup X times a day.',
+          default: ['recoveryPhoneCreate'],
+          format: Array,
+        },
+        limits: {
+          max: {
+            doc: 'max actions during `period` that can occur before rate limit is applied',
+            format: 'nat',
+            default: 9,
+            env: 'RECOVERY_PHONE_SETUP_CODE_RULE_MAX',
+          },
+          periodMs: {
+            doc: 'period needed before rate limit is reset',
+            format: 'duration',
+            default: '1 day',
+            env: 'RECOVERY_PHONE_SETUP_CODE_RULE_PERIOD_MS',
+          },
+          rateLimitIntervalMs: {
+            doc: 'how long rate limit is applied',
+            format: 'duration',
+            default: '2 hours',
+            env: 'RECOVERY_PHONE_SETUP_CODE_RULE_LIMIT_INTERVAL_MS',
+          },
+        },
+      },
+      recoveryPhoneConfirmCodeRules: {
+        actions: {
+          doc:
+            'Array of actions that this rule should be applied to. A user should only be able to initiate ' +
+            'a recovery phone confirm X times a day.',
+          default: ['recoveryPhoneConfirmCode'],
+          format: Array,
+        },
+        limits: {
+          max: {
+            doc: 'max actions during `period` that can occur before rate limit is applied',
+            format: 'nat',
+            default: 6,
+            env: 'RECOVERY_PHONE_CONFIRM_CODE_RULE_MAX',
+          },
+          periodMs: {
+            doc: 'period needed before rate limit is reset',
+            format: 'duration',
+            default: '1 day',
+            env: 'RECOVERY_PHONE_CONFIRM_CODE_RULE_PERIOD_MS',
+          },
+          rateLimitIntervalMs: {
+            doc: 'how long rate limit is applied',
+            format: 'duration',
+            default: '2 hours',
+            env: 'RECOVERY_PHONE_CONFIRM_CODE_RULE_LIMIT_INTERVAL_MS',
+          },
+        },
+      },
     },
     dataflow: {
       enabled: {

packages/fxa-customs-server/lib/server.js

@@ -78,11 +78,10 @@ module.exports = async function createServer(config, log) {
       statsd
     );
 
-  const checkUserDefinedRateLimitRules = require('./user_defined_rules')(
-    config,
-    fetchRecord,
-    setRecords
-  );
+  const {
+    checkUserDefinedRateLimitRulesByIpEmail,
+    checkUserDefinedRateLimitRulesByUid,
+  } = require('./user_defined_rules')(config, fetchRecord, setRecords);
 
   dataflow(config, log, fetchRecords, setRecord);
 
@@ -418,7 +417,12 @@ module.exports = async function createServer(config, log) {
       return fetchRecords({ ip, email, phoneNumber })
         .then(checkRecords)
         .then((result) => {
-          return checkUserDefinedRateLimitRules(result, action, email, ip);
+          return checkUserDefinedRateLimitRulesByIpEmail(
+            result,
+            action,
+            email,
+            ip
+          );
         })
         .then((result) => {
           return result;
@@ -454,8 +458,15 @@ module.exports = async function createServer(config, log) {
       }
 
       return fetchRecords({ uid })
-        .then(({ uidRecord }) => {
-          var retryAfter = uidRecord.addCount(action, uid);
+        .then(async ({ uidRecord }) => {
+          var result = uidRecord.addCount(action, uid);
+          const { retryAfter } = await checkUserDefinedRateLimitRulesByUid(
+            {
+              retryAfter: result,
+            },
+            action,
+            uid
+          );
           return setRecords(uidRecord).then(function () {
             return {
               block: retryAfter > 0,

packages/fxa-customs-server/lib/user_defined_rules.js

@@ -21,12 +21,7 @@ module.exports = function (config, fetchRecord, setRecords) {
     });
   });
 
-  return async function checkUserDefinedRateLimitRules(
-    result,
-    action,
-    email,
-    ip
-  ) {
+  async function checkUserDefinedRateLimitRulesByKey(result, action) {
     // Get all the user defined rules that might apply to this action
     const checkRules = computedRules.get(action);
 
@@ -35,10 +30,12 @@ module.exports = function (config, fetchRecord, setRecords) {
       return result;
     }
 
+    const keys = [...arguments].slice(2);
+
     const retries = [];
     await Promise.all(
       checkRules.map(async (ruleName) => {
-        const recordKey = ruleName + ':' + utils.createHashHex(email, ip);
+        const recordKey = ruleName + ':' + utils.createHashHex(...keys);
         const record = await fetchRecord(
           recordKey,
           (object) => new Record(object, configuredRules[ruleName])
@@ -60,5 +57,23 @@ module.exports = function (config, fetchRecord, setRecords) {
     }
 
     return result;
+  }
+
+  async function checkUserDefinedRateLimitRulesByIpEmail(
+    result,
+    action,
+    ip,
+    email
+  ) {
+    return checkUserDefinedRateLimitRulesByKey(result, action, ip, email);
+  }
+
+  async function checkUserDefinedRateLimitRulesByUid(result, action, uid) {
+    return checkUserDefinedRateLimitRulesByKey(result, action, uid);
+  }
+
+  return {
+    checkUserDefinedRateLimitRulesByIpEmail,
+    checkUserDefinedRateLimitRulesByUid,
   };
 };

packages/fxa-customs-server/test/remote/user_defined_rules.js

@@ -6,6 +6,7 @@ const TestServer = require('../test_server');
 const Promise = require('bluebird');
 const restifyClients = Promise.promisifyAll(require('restify-clients'));
 const mcHelper = require('../cache-helper');
+Promise.promisifyAll(mcHelper, { multiArgs: true });
 
 function randomEmail() {
   return Math.floor(Math.random() * 10000) + '@email.com';
@@ -18,13 +19,25 @@ function randomIp() {
   return [getSubnet(), getSubnet(), getSubnet(), getSubnet()].join('.');
 }
 
+function randomUid() {
+  return Math.floor(Math.random() * 10000) + '';
+}
+
 const config = require('../../lib/config').getProperties();
 config.userDefinedRateLimitRules.totpCodeRules.limits.periodMs = 1000;
 config.userDefinedRateLimitRules.totpCodeRules.limits.rateLimitIntervalMs = 1000;
 config.userDefinedRateLimitRules.tokenCodeRules.limits.max = 2;
 config.userDefinedRateLimitRules.tokenCodeRules.limits.periodMs = 1000;
 config.userDefinedRateLimitRules.tokenCodeRules.limits.rateLimitIntervalMs = 1000;
 
+config.userDefinedRateLimitRules.recoveryPhoneCreateCodeRules.limits.max = 5;
+config.userDefinedRateLimitRules.recoveryPhoneCreateCodeRules.limits.periodMs = 5000;
+config.userDefinedRateLimitRules.recoveryPhoneCreateCodeRules.limits.rateLimitIntervalMs = 1000;
+
+config.userDefinedRateLimitRules.recoveryPhoneConfirmCodeRules.limits.max = 5;
+config.userDefinedRateLimitRules.recoveryPhoneConfirmCodeRules.limits.periodMs = 5000;
+config.userDefinedRateLimitRules.recoveryPhoneConfirmCodeRules.limits.rateLimitIntervalMs = 1000;
+
 const ACTIONS = ['verifyTotpCode', 'verifyTokenCode'];
 
 const testServer = new TestServer(config);
@@ -136,6 +149,91 @@ ACTIONS.forEach((action) => {
   });
 });
 
+const recoveryPhoneActions = [
+  'recoveryPhoneCreate',
+  'recoveryPhoneConfirmCode',
+];
+recoveryPhoneActions.forEach((action) => {
+  test(`clear everything for ${action}`, async (t) => {
+    try {
+      await mcHelper.clearEverythingAsync();
+      t.pass('cleared redis');
+    } catch (err) {
+      t.fail(err);
+    } finally {
+      t.end();
+    }
+  });
+
+  test('/checkAuthenticated `' + action + '` by uid', async (t) => {
+    const uid = randomUid();
+    const ip = randomIp();
+    // Send requests until throttled
+    let [req, res, obj] = await client.postAsync('/checkAuthenticated', {
+      ip,
+      action,
+      uid,
+    });
+    t.equal(res.statusCode, 200, 'returns a 200');
+    t.equal(obj.block, false, 'not rate limited');
+
+    [req, res, obj] = await client.postAsync('/checkAuthenticated', {
+      ip,
+      action,
+      uid,
+    });
+    t.equal(res.statusCode, 200, 'returns a 200');
+    t.equal(obj.block, false, 'not rate limited');
+
+    [req, res, obj] = await client.postAsync('/checkAuthenticated', {
+      ip,
+      action,
+      uid,
+    });
+    t.equal(res.statusCode, 200, 'returns a 200');
+    t.equal(obj.block, false, 'not rate limited');
+
+    [req, res, obj] = await client.postAsync('/checkAuthenticated', {
+      ip,
+      action,
+      uid,
+    });
+    t.equal(res.statusCode, 200, 'returns a 200');
+    t.equal(obj.block, false, 'not rate limited');
+
+    [req, res, obj] = await client.postAsync('/checkAuthenticated', {
+      ip,
+      action,
+      uid,
+    });
+    t.equal(res.statusCode, 200, 'returns a 200');
+    t.equal(obj.block, false, 'not rate limited');
+
+    [req, res, obj] = await client.postAsync('/checkAuthenticated', {
+      ip,
+      action,
+      uid,
+    });
+    t.equal(res.statusCode, 200, 'returns a 200');
+    t.equal(obj.block, true, 'rate limited');
+    t.equal(obj.retryAfter, 1, 'rate limit retry amount');
+
+    // Wait for limit to expire
+    await Promise.delay(1010);
+
+    [req, res, obj] = await client.postAsync('/checkAuthenticated', {
+      ip,
+      action,
+      uid,
+    });
+    t.equal(res.statusCode, 200, 'returns a 200');
+    t.equal(obj.block, false, 'not rate limited');
+
+    t.pass(req);
+    t.end();
+  });
+});
+
 test('teardown', async function (t) {
   await testServer.stop();
   t.end();

packages/fxa-customs-server/test/test_reputation_server.js

@@ -69,7 +69,6 @@ async function init(config) {
     path: '/mostRecentViolation/{ip}',
     handler: async (req) => {
       var ip = req.params.ip;
-      console.log('delete req', req.url);
       delete mostRecentViolationByIp[ip];
       return {};
     },

packages/fxa-customs-server/test/test_reputation_server_stub.js

@@ -38,7 +38,6 @@ server.get('/mostRecentViolation/:ip', function (req, res, next) {
 // This is not a real route in iprepd, and is only used by the tests
 server.del('/mostRecentViolation/:ip', function (req, res, next) {
   var ip = req.params.ip;
-  console.log('delete req', req.url);
   delete mostRecentViolationByIp[ip];
   res.send(200);