Merge pull request #18864 from mozilla/support-rate-limit-allow-lists

Support rate limit allow lists

Author: dschom

libs/accounts/rate-limit/README.md

@@ -34,3 +34,18 @@ would replicate the pre-existing behavior:
    passwordForgotVerifyOtp    : email    : 10 attempts  : 24 hours       : 24 hours
 
 ```
+
+### Testing & Development Considerations
+
+When developing new features, running functional test suites locally, or running smoke tests remotely, rate-limiting behavior can be annoying — or even downright disruptive. To work around this, there are a few options:
+
+#### Disable the rules:
+You can simply remove the rate-limiting rules. Our servers typically hold the rules as a string in the environment, e.g., RATE_LIMIT__RULES=['some','rules']. Override this value with an empty string, and rate limiting will effectively be disabled. Want to test a single rate limit? Just specify that one rule. It doesn’t get much simpler than that.
+
+#### Exclude specific attributes like ip, email, or UID
+When running smoke tests against a remote server, the first approach may not work — so we also support excluding specific emails, user IDs (UIDs), or IP addresses from being checked by the rate limiter. To do this, define these values in your server's config file and pass them to the rate limiter.
+
+ - Email ignore values can use regex filters.
+ - IP addresses and UIDs must be exact string matches.
+
+Ideally, the IP filter should be used. This is the preferred method because all requests contain an IP address, making it the lowest common denominator for rate-limiting attributes. Other advantages of using the IP filter include its consistency and ease of configuration.

libs/accounts/rate-limit/src/lib/rate-limit.in.spec.ts

@@ -24,7 +24,7 @@ describe('rate-limit', () => {
 
   beforeEach(async () => {
     await redis.flushall();
-    rateLimit = new RateLimit({}, redis, statsd);
+    rateLimit = new RateLimit({ rules: {} }, redis, statsd);
     mockIncrement.mockReset();
   });
 
@@ -35,7 +35,7 @@ describe('rate-limit', () => {
   for (const blockOn of ['ip', 'email', 'uid']) {
     it('should block on ' + blockOn, async () => {
       rateLimit = new RateLimit(
-        parseConfigRules(['testBlock:ip:1:1s:1s']),
+        { rules: parseConfigRules(['testBlock:ip:1:1s:1s']) },
         redis,
         statsd
       );
@@ -57,7 +57,7 @@ describe('rate-limit', () => {
 
   it(`should not block after window clears`, async () => {
     rateLimit = new RateLimit(
-      parseConfigRules(['testWindowCleared:ip:1:1s:1s']),
+      { rules: parseConfigRules(['testWindowCleared:ip:1:1s:1s']) },
       redis,
       statsd
     );
@@ -76,7 +76,12 @@ describe('rate-limit', () => {
 
   it('can block multiple rules on a single action', async () => {
     rateLimit = new RateLimit(
-      parseConfigRules(['testMulti:ip:2:10s:2s', 'testMulti:email:2:10s:3s']),
+      {
+        rules: parseConfigRules([
+          'testMulti:ip:2:10s:2s',
+          'testMulti:email:2:10s:3s',
+        ]),
+      },
       redis,
       statsd
     );
@@ -120,10 +125,12 @@ describe('rate-limit', () => {
 
   it('can block a doubled up rule', async () => {
     rateLimit = new RateLimit(
-      parseConfigRules([
-        'testDouble:email:2:10s:2s',
-        'testDouble:email:3:10s:4s',
-      ]),
+      {
+        rules: parseConfigRules([
+          'testDouble:email:2:10s:2s',
+          'testDouble:email:3:10s:4s',
+        ]),
+      },
       redis,
       statsd
     );
@@ -153,7 +160,9 @@ describe('rate-limit', () => {
      * Note that once the ban kicks in the window disappears. So despite
      */
     rateLimit = new RateLimit(
-      parseConfigRules(['test:ip:1:20s:1s']),
+      {
+        rules: parseConfigRules(['test:ip:1:20s:1s']),
+      },
       redis,
       statsd
     );
@@ -174,7 +183,9 @@ describe('rate-limit', () => {
 
   it('can unblock', async () => {
     rateLimit = new RateLimit(
-      parseConfigRules(['testBlock:ip:1:1s:1s']),
+      {
+        rules: parseConfigRules(['testBlock:ip:1:1s:1s']),
+      },
       redis,
       statsd
     );

libs/accounts/rate-limit/src/lib/rate-limit.spec.ts

@@ -29,13 +29,13 @@ describe('rate-limit', () => {
   });
 
   it('creates rate limiter', () => {
-    rateLimit = new RateLimit({}, redis, statsd);
+    rateLimit = new RateLimit({ rules: {} }, redis, statsd);
     expect(rateLimit).toBeDefined();
   });
 
   it('can determine if action is supported', () => {
     rateLimit = new RateLimit(
-      parseConfigRules(['test:ip:1:1s:1s']),
+      { rules: parseConfigRules(['test:ip:1:1s:1s']) },
       redis,
       statsd
     );
@@ -45,15 +45,15 @@ describe('rate-limit', () => {
   });
 
   it('throws if no action can be found', async () => {
-    rateLimit = new RateLimit({}, redis, statsd);
+    rateLimit = new RateLimit({ rules: {} }, redis, statsd);
     expect(rateLimit.check('foo', {})).rejects.toThrow(
       `Could not locate the 'foo' action.`
     );
   });
 
   it('throws error if option is missing', async () => {
     rateLimit = new RateLimit(
-      parseConfigRules(['test:ip:1:1s:1s']),
+      { rules: parseConfigRules(['test:ip:1:1s:1s']) },
       redis,
       statsd
     );
@@ -98,6 +98,57 @@ describe('rate-limit', () => {
     );
   });
 
+  it('can ignore certain emails', () => {
+    rateLimit = new RateLimit(
+      {
+        rules: {},
+        ignoreEmails: ['@mozilla.com$', '[email protected]'],
+      },
+      redis,
+      statsd
+    );
+
+    expect(rateLimit.skip({ email: '[email protected]' })).toBeTruthy();
+    expect(rateLimit.skip({ email: '[email protected]' })).toBeTruthy();
+    expect(rateLimit.skip({ email: '[email protected] ' })).toBeFalsy();
+    expect(rateLimit.skip({ email: '[email protected]' })).toBeTruthy();
+    expect(rateLimit.skip({ email: '[email protected] ' })).toBeFalsy();
+    expect(rateLimit.skip({})).toBeFalsy();
+    expect(statsd.increment).toBeCalledWith('rate_limit.ignore.email');
+  });
+
+  it('can ignore certain ips', () => {
+    rateLimit = new RateLimit(
+      {
+        rules: {},
+        ignoreIPs: ['127.0.0.1'],
+      },
+      redis,
+      statsd
+    );
+
+    expect(rateLimit.skip({ ip: '127.0.0.1' })).toBeTruthy();
+    expect(rateLimit.skip({ ip: '0.0.0.0' })).toBeFalsy();
+    expect(rateLimit.skip({})).toBeFalsy();
+    expect(statsd.increment).toBeCalledWith('rate_limit.ignore.ip');
+  });
+
+  it('can ignore certain uids', () => {
+    rateLimit = new RateLimit(
+      {
+        rules: {},
+        ignoreUIDs: ['000-000-000'],
+      },
+      redis,
+      statsd
+    );
+
+    expect(rateLimit.skip({ uid: '000-000-000' })).toBeTruthy();
+    expect(rateLimit.skip({ uid: '000-000-001' })).toBeFalsy();
+    expect(rateLimit.skip({})).toBeFalsy();
+    expect(statsd.increment).toBeCalledWith('rate_limit.ignore.uid');
+  });
+
   describe('configuration rules', () => {
     it('parses rule set', () => {
       const ruleSet = parseConfigRules(`

libs/accounts/rate-limit/src/lib/rate-limit.ts

@@ -21,7 +21,12 @@ export class RateLimit {
    * @param redis A Redis client
    */
   constructor(
-    public readonly rules: Record<string, Rule[]>,
+    public readonly config: {
+      rules: Record<string, Rule[]>;
+      ignoreIPs?: Array<string>;
+      ignoreEmails?: Array<string>;
+      ignoreUIDs?: Array<string>;
+    },
     private readonly redis: Redis,
     private readonly statsd?: StatsD
   ) {}
@@ -32,8 +37,8 @@ export class RateLimit {
    * @returns Null if no block is found. Or a status containing info about the block.
    */
   async unblock(opts: { ip?: string; email?: string; uid?: string }) {
-    for (const action in this.rules) {
-      for (const rule of this.rules[action]) {
+    for (const action in this.config.rules) {
+      for (const rule of this.config.rules[action]) {
         const blockedValue = opts[rule.blockingOn];
         if (blockedValue) {
           const attemptsKey = getKey('attempts', rule, blockedValue);
@@ -57,8 +62,34 @@ export class RateLimit {
    * @param action - Name of action
    * @returns - True if action has rules, otherwise false.
    */
-  supportsAction(action:string) {
-    return this.rules[action] != null;
+  supportsAction(action: string) {
+    return this.config.rules[action] != null;
+  }
+
+  /**
+   * Determines if a check can be skipped, due to an ignored ip, email, or uid.
+   * When testing and developing it's often helpful to disable customs rules for
+   * certain users.
+   * @param opts - The current properties being checked.
+   * @returns
+   */
+  skip(opts: { ip?: string; email?: string; uid?: string }) {
+    if (opts.ip != null && this.config.ignoreIPs?.some((x) => opts.ip === x)) {
+      this.statsd?.increment('rate_limit.ignore.ip');
+      return true;
+    }
+
+    if (opts.uid != null && this.config.ignoreUIDs?.some((x) => opts.uid === x)) {
+      this.statsd?.increment('rate_limit.ignore.uid');
+      return true;
+    }
+
+    if (opts.email != null && this.config.ignoreEmails?.some((x) => opts.email?.match(x))) {
+      this.statsd?.increment('rate_limit.ignore.email');
+      return true;
+    }
+
+    return false
   }
 
   /**
@@ -77,7 +108,7 @@ export class RateLimit {
     }
   ): Promise<BlockStatus | null> {
     // Make sure action actually exists
-    const rules = this.rules[action];
+    const rules = this.config.rules[action];
     if (!rules) {
       throw new ActionNotFound(action);
     }

packages/fxa-auth-server/bin/key_server.js

@@ -227,7 +227,16 @@ async function run(config) {
     ...config.redis,
     ...config.redis.customs,
   });
-  const rateLimit = new RateLimit(rules, rateLimitRedis, statsd);
+  const rateLimit = new RateLimit(
+    {
+      rules,
+      ignoreIPs: config.rateLimit.ignoreIPs,
+      ignoreEmails: config.rateLimit.ignoreEmails,
+      ignoreUIDs: config.rateLimit.ignoreUIDs,
+    },
+    rateLimitRedis,
+    statsd
+  );
   Container.set(RateLimit, rateLimit);
 
   // Create Recovery Phone Service

packages/fxa-auth-server/config/index.ts

@@ -2161,6 +2161,24 @@ const convictConf = convict({
     },
   },
   rateLimit: {
+    ignoreIPs: {
+      default: undefined,
+      doc: 'Set of IPs to ignore while rate limiting. Rules will not apply to these values.',
+      env: 'RATE_LIMIT__IGNORE_IPS',
+      format: Array,
+    },
+    ignoreUIDs: {
+      default: undefined,
+      doc: 'Set of UIDs to ignore while rate limiting. Rules will not apply to these values.',
+      env: 'RATE_LIMIT__IGNORE_UIDS',
+      format: Array,
+    },
+    ignoreEmails: {
+      default: undefined,
+      doc: 'Set of emails to ignore while rate limiting. Rules will not apply to these values. Values can be a string or parsable regex.',
+      env: 'RATE_LIMIT__IGNORE_EMAILS',
+      format: Array,
+    },
     rules: {
       default: [
         'unblockEmail          : email  : 10   : 24 hours    : 24 hours    ',

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

@@ -306,6 +306,18 @@ class CustomsClient {
       return false;
     }
 
+    // The config can specify that certain ips, emails, or uids should be excluded
+    // from rate limit checks.
+    const skip = this.rateLimit.skip(opts);
+    if (skip) {
+      this.statsd.increment(`${serviceName}.check.v2.skip`, [
+        opts.ip ? 'ip':'',
+        opts.email ? 'email':'',
+        opts.uid ? 'uid':'',
+      ]);
+      return true;
+    }
+
     // Otherwise, call the new nx lib instead of the legacy service
     this.statsd?.increment(`${serviceName}.check.v2`, [`action:${action}`]);
     const result = await this.rateLimit.check(action, opts);

packages/fxa-auth-server/test/local/customs.js

@@ -721,9 +721,11 @@ describe('Customs', () => {
 
   describe('customs v2', () => {
     const mockRateLimit = {
-      supportsAction: sinon.spy(() => true),
       check: sinon.spy(),
+      skip: sinon.spy(),
+      supportsAction: sinon.spy(),
     };
+
     const customs = new Customs(
       CUSTOMS_URL_REAL,
       log,
@@ -732,6 +734,12 @@ describe('Customs', () => {
       mockRateLimit
     );
 
+    beforeEach(() => {
+      mockRateLimit.check = sinon.spy();
+      mockRateLimit.skip = sinon.spy(() => false);
+      mockRateLimit.supportsAction = sinon.spy(() => true);
+    });
+
     it('can allow checkAccountStatus with rate-limit lib', async () => {
       mockRateLimit.check = sandbox.spy(async () => {
         return await Promise.resolve(null);
@@ -763,6 +771,24 @@ describe('Customs', () => {
         'Client has sent too many requests'
       );
     });
+
+    it('can skip certain emails, ips, and uids', async () => {
+      mockRateLimit.skip = sandbox.spy(() => true);
+      mockRateLimit.check = sandbox.spy(async () => {
+        return await Promise.resolve({
+          retryAfter: 1000,
+          reason: 'too-many-attempts',
+        });
+      });
+
+      // Should not throw despite, check being mocked to return an error. The
+      // skip check should be called first, and since it indicates a skip should
+      // occur, the actual customs check shouldn't ever happen.
+      await customs.check(request, email, 'accountStatusCheck');
+
+      assert.calledWith(mockRateLimit.skip, { ip, email });
+      assert.callCount(mockRateLimit.check, 0);
+    });
   });
 
   describe('statsd metrics', () => {