fix(pruning): Avoid accidental full-table scans when pruning session tokens. (#295); r=philbooth

Author: rfk

lib/db/mysql.js

@@ -1110,7 +1110,7 @@ module.exports = function (log, error) {
   // exposed for testing only
   MySql.prototype.retryable_ = retryable
 
-  const PRUNE = 'CALL prune_6(?)'
+  const PRUNE = 'CALL prune_7(?)'
   MySql.prototype.pruneTokens = function () {
     log.info('MySql.pruneTokens')
 

lib/db/patch.js

@@ -4,4 +4,4 @@
 
 // The expected patch level of the database. Update if you add a new
 // patch in the ./schema/ directory.
-module.exports.level = 68
+module.exports.level = 69

lib/db/schema/patch-068-069.sql

@@ -0,0 +1,83 @@
+SET NAMES utf8mb4 COLLATE utf8mb4_bin;
+
+-- We might have gotten ourselves into a state where 'sessionTokensPrunedUntil'
+-- was set to the empty string.  Start it over from zero.
+UPDATE dbMetadata SET value = '0' WHERE name = 'sessionTokensPrunedUntil' AND value = '';
+
+-- Update prune to limit total number of sessionTokens examined,
+-- and avoid producing the above empty-string bug.
+CREATE PROCEDURE `prune_7` (IN `olderThan` BIGINT UNSIGNED)
+BEGIN
+  DECLARE EXIT HANDLER FOR SQLEXCEPTION
+  BEGIN
+    ROLLBACK;
+    RESIGNAL;
+  END;
+
+  SELECT @lockAcquired := GET_LOCK('fxa-auth-server.prune-lock', 3);
+
+  IF @lockAcquired THEN
+    DELETE FROM accountResetTokens WHERE createdAt < olderThan ORDER BY createdAt LIMIT 10000;
+    DELETE FROM passwordForgotTokens WHERE createdAt < olderThan ORDER BY createdAt LIMIT 10000;
+    DELETE FROM passwordChangeTokens WHERE createdAt < olderThan ORDER BY createdAt LIMIT 10000;
+    DELETE FROM unblockCodes WHERE createdAt < olderThan ORDER BY createdAt LIMIT 10000;
+    DELETE FROM signinCodes WHERE createdAt < olderThan ORDER BY createdAt LIMIT 10000;
+
+    -- Pruning session tokens is complicated because:
+    --   * we can't prune them if there is an associated device record, and
+    --   * we have to delete from both sessionTokens and unverifiedTokens tables, and
+    --   * MySQL won't allow `LIMIT` to be used in a multi-table delete.
+    -- To achieve all this in an efficient manner, we prune tokens within a specific
+    -- time window rather than using a `LIMIT` clause.  At the end of each run we
+    -- record the new lower-bound on creation time for tokens that might have expired.
+    START TRANSACTION;
+
+    -- Step 1: Find out how far we got on previous iterations.
+    SELECT @pruneFrom := value FROM dbMetadata WHERE name = 'sessionTokensPrunedUntil';
+
+    -- Step 2: Calculate what timestamp we will reach on this iteration
+    -- if we purge a sensibly-sized batch of tokens.
+    -- N.B. We deliberately do not filter on whether the token has
+    -- a device here.  We want to limit the number of tokens that we
+    -- *examine*, regardless of whether it actually delete them.
+    SELECT @pruneUntil := MAX(createdAt) FROM (
+      SELECT createdAt FROM sessionTokens
+      WHERE createdAt >= @pruneFrom AND createdAt < olderThan
+      ORDER BY createdAt
+      LIMIT 10000
+    ) AS candidatesForPruning;
+
+    -- This will be NULL if there are no expired tokens,
+    -- in which case we have nothing to do.
+    IF @pruneUntil IS NOT NULL THEN 
+
+      -- Step 3: Prune sessionTokens and unverifiedTokens tables.
+      -- Here we *do* filter on whether a device record exists.
+      -- We might not actually delete any tokens, but we will definitely
+      -- be able to increase 'sessionTokensPrunedUntil' for the next run.
+      DELETE st, ut
+      FROM sessionTokens AS st
+      LEFT JOIN unverifiedTokens AS ut
+      ON st.tokenId = ut.tokenId
+      WHERE st.createdAt > @pruneFrom
+      AND st.createdAt <= @pruneUntil
+      AND NOT EXISTS (
+        SELECT sessionTokenId FROM devices
+        WHERE uid = st.uid AND sessionTokenId = st.tokenId
+      );
+
+      -- Step 4: Tell following iterations how far we got.
+      UPDATE dbMetadata
+      SET value = @pruneUntil
+      WHERE name = 'sessionTokensPrunedUntil';
+
+    END IF;
+
+    COMMIT;
+
+    SELECT RELEASE_LOCK('fxa-auth-server.prune-lock');
+  END IF;
+END;
+
+UPDATE dbMetadata SET value = '69' WHERE name = 'schema-patch-level';
+

lib/db/schema/patch-069-068.sql

@@ -0,0 +1,6 @@
+-- SET NAMES utf8mb4 COLLATE utf8mb4_bin;
+
+-- DROP PROCEDURE `prune_7`;
+
+-- UPDATE dbMetadata SET value = '68' WHERE name = 'schema-patch-level';
+

test/local/prune_tokens.js

@@ -39,6 +39,7 @@ describe('prune tokens', () => {
       const unprunableSessionTokenId = crypto.randomBytes(16).toString('hex')
       const tokenVerificationId = crypto.randomBytes(8).toString('hex')
       const unverifiedKeyFetchToken = Object.assign({}, user.keyFetchToken, { tokenVerificationId })
+      const initialPrunedUntilValue = Date.now() - TOKEN_PRUNE_AGE * 2 + 1
       return db.createAccount(user.accountId, user.account)
         .then(function() {
           return db.createPasswordForgotToken(user.passwordForgotTokenId, user.passwordForgotToken)
@@ -55,7 +56,7 @@ describe('prune tokens', () => {
             db.createSessionToken(unprunableSessionTokenId, user.sessionToken),
             db.createSigninCode(signinCode, user.accountId, Date.now() - TOKEN_PRUNE_AGE),
             db.write('UPDATE dbMetadata SET value = ? WHERE name = \'sessionTokensPrunedUntil\'', [
-              Date.now() - TOKEN_PRUNE_AGE * 2 + 1
+              initialPrunedUntilValue
             ])
           ])
         })
@@ -160,6 +161,27 @@ describe('prune tokens', () => {
           // unverifiedTokens must not be pruned if they belong to keyFetchTokens
           assert.equal(keyFetchToken.tokenVerificationId, tokenVerificationId)
         })
+        .then(() => {
+          // 'sessionTokensPrunedUntil' should have increased after pruning
+          const sql = 'SELECT value FROM dbMetadata WHERE name = \'sessionTokensPrunedUntil\''
+          return db.read(sql)
+        })
+        .then((res) => {
+          assert.equal(res.length, 1, 'sessionTokensPrunedUntil still exists')
+          assert.ok(res[0].value, 'sessionTokensPrunedUntil is not falsy')
+          const updatedPrunedUntilValue = parseInt(res[0].value, 10)
+          assert.ok(updatedPrunedUntilValue >  initialPrunedUntilValue, 'sessionTokensPrunedUntil increased')
+          // Prune again, so we can check that it gracefully handles
+          // the case when there's nothing to prune.
+          return db.pruneTokens().then(() => {
+            const sql = 'SELECT value FROM dbMetadata WHERE name = \'sessionTokensPrunedUntil\''
+            return db.read(sql)
+          }).then((res) => {
+            assert.equal(res.length, 1, 'sessionTokensPrunedUntil still exists')
+            assert.ok(res[0].value, 'sessionTokensPrunedUntil is not falsy')
+            assert.equal(parseInt(res[0].value, 10), updatedPrunedUntilValue, 'sessionTokensPrunedUntil did not change')
+          })
+        })
     }
   )