Bug 1182740 - treat keypair and certificate as an atomic pair to avoid invalid assertions. r=stomlinson
authorMark Hammond <mhammond@skippinet.com.au>
Wed, 29 Jul 2015 16:06:29 +1000
changeset 286863 836ab5febfeef9b56cf704fb660c88fc5a3ad383
parent 286862 237122da53544f410a02c24ff52adc2fe0c04467
child 286864 89aa9083a960fafc88cee683a49ebb0856f7c0fe
push id5067
push userraliiev@mozilla.com
push dateMon, 21 Sep 2015 14:04:52 +0000
treeherdermozilla-beta@14221ffe5b2f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersstomlinson
bugs1182740
milestone42.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1182740 - treat keypair and certificate as an atomic pair to avoid invalid assertions. r=stomlinson
browser/components/customizableui/test/browser_946320_tabs_from_other_computers.js
services/fxaccounts/FxAccounts.jsm
services/fxaccounts/tests/xpcshell/test_accounts.js
services/sync/modules-testing/utils.js
--- a/browser/components/customizableui/test/browser_946320_tabs_from_other_computers.js
+++ b/browser/components/customizableui/test/browser_946320_tabs_from_other_computers.js
@@ -116,22 +116,18 @@ function configureFxAccountIdentity() {
   let MockInternal = {
     newAccountState(credentials) {
       isnot(credentials, "not expecting credentials");
       let storageManager = new MockFxaStorageManager();
       // and init storage with our user.
       storageManager.initialize(user);
       return new AccountState(storageManager);
     },
-    getCertificate(data, keyPair, mustBeValidUntil) {
-      this.cert = {
-        validUntil: this.now() + 10000,
-        cert: "certificate",
-      };
-      return Promise.resolve(this.cert.cert);
+    _getAssertion(audience) {
+      return Promise.resolve("assertion");
     },
     getCertificateSigned() {
       return Promise.resolve();
     },
   };
   let mockTSC = { // TokenServerClient
     getTokenFromBrowserIDAssertion: function(uri, assertion, cb) {
       token.uid = "username";
--- a/services/fxaccounts/FxAccounts.jsm
+++ b/services/fxaccounts/FxAccounts.jsm
@@ -494,85 +494,50 @@ FxAccountsInternal.prototype = {
           this.startVerifiedCheck(credentials);
         }
       }).then(() => {
         return currentAccountState.resolve();
       });
     })
   },
 
-  /**
-   * returns a promise that fires with the keypair.
-   */
-  getKeyPair: Task.async(function* (mustBeValidUntil) {
-    // If the debugging pref to ignore cached authentication credentials is set for Sync,
-    // then don't use any cached key pair, i.e., generate a new one and get it signed.
-    // The purpose of this pref is to expedite any auth errors as the result of a
-    // expired or revoked FxA session token, e.g., from resetting or changing the FxA
-    // password.
-    let ignoreCachedAuthCredentials = false;
-    try {
-      ignoreCachedAuthCredentials = Services.prefs.getBoolPref("services.sync.debug.ignoreCachedAuthCredentials");
-    } catch(e) {
-      // Pref doesn't exist
-    }
-    let currentState = this.currentAccountState;
-    let accountData = yield currentState.getUserAccountData("keyPair");
-    if (!ignoreCachedAuthCredentials && accountData.keyPair && (accountData.keyPair.validUntil > mustBeValidUntil)) {
-      log.debug("getKeyPair: already have a keyPair");
-      return accountData.keyPair.keyPair;
-    }
-    // Otherwse, create a keypair and set validity limit.
-    let willBeValidUntil = this.now() + KEY_LIFETIME;
-    let kp = yield new Promise((resolve, reject) => {
-      jwcrypto.generateKeyPair("DS160", (err, kp) => {
-        if (err) {
-          return reject(err);
-        }
-        log.debug("got keyPair");
-        let toUpdate = {
-          keyPair: {
-            keyPair: kp,
-            validUntil: willBeValidUntil
-          },
-          cert: null
-        };
-        currentState.updateUserAccountData(toUpdate).then(() => {
-          resolve(kp);
-        }).catch(err => {
-          log.error("Failed to update account data with keypair and cert");
-        });
-      });
-    });
-    return kp;
-  }),
 
   /**
    * returns a promise that fires with the assertion.  If there is no verified
    * signed-in user, fires with null.
    */
   getAssertion: function getAssertion(audience) {
+    return this._getAssertion(audience);
+  },
+
+  // getAssertion() is "public" so screws with our mock story. This
+  // implementation method *can* be (and is) mocked by tests.
+  _getAssertion: function _getAssertion(audience) {
     log.debug("enter getAssertion()");
     let currentState = this.currentAccountState;
-    let mustBeValidUntil = this.now() + ASSERTION_USE_PERIOD;
     return currentState.getUserAccountData().then(data => {
       if (!data) {
         // No signed-in user
         return null;
       }
       if (!this.isUserEmailVerified(data)) {
         // Signed-in user has not verified email
         return null;
       }
-      return this.getKeyPair(mustBeValidUntil).then(keyPair => {
-        return this.getCertificate(data, keyPair, mustBeValidUntil)
-          .then(cert => {
-            return this.getAssertionFromCert(data, keyPair, cert, audience);
-          });
-      });
+      if (!data.sessionToken) {
+        // can't get a signed certificate without a session token, but that
+        // should be impossible - make log noise about it.
+        log.error("getAssertion called without a session token!");
+        return null;
+      }
+      return this.getKeypairAndCertificate(currentState).then(
+        ({keyPair, certificate}) => {
+          return this.getAssertionFromCert(data, keyPair, certificate, audience);
+        }
+      );
     }).then(result => currentState.resolve(result));
   },
 
   /**
    * Resend the verification email fot the currently signed-in user.
    *
    */
   resendVerificationEmail: function resendVerificationEmail() {
@@ -827,44 +792,101 @@ FxAccountsInternal.prototype = {
     return this.fxAccountsClient.signCertificate(
       sessionToken,
       JSON.parse(serializedPublicKey),
       lifetime
     );
   },
 
   /**
-   * returns a promise that fires with a certificate.
+   * returns a promise that fires with {keyPair, certificate}.
    */
-  getCertificate: Task.async(function* (data, keyPair, mustBeValidUntil) {
+  getKeypairAndCertificate: Task.async(function* (currentState) {
+    // If the debugging pref to ignore cached authentication credentials is set for Sync,
+    // then don't use any cached key pair/certificate, i.e., generate a new
+    // one and get it signed.
+    // The purpose of this pref is to expedite any auth errors as the result of a
+    // expired or revoked FxA session token, e.g., from resetting or changing the FxA
+    // password.
+    let ignoreCachedAuthCredentials = false;
+    try {
+      ignoreCachedAuthCredentials = Services.prefs.getBoolPref("services.sync.debug.ignoreCachedAuthCredentials");
+    } catch(e) {
+      // Pref doesn't exist
+    }
+    let mustBeValidUntil = this.now() + ASSERTION_USE_PERIOD;
+    let accountData = yield currentState.getUserAccountData(["cert", "keyPair", "sessionToken"]);
+
+    let keyPairValid = !ignoreCachedAuthCredentials &&
+                       accountData.keyPair &&
+                       (accountData.keyPair.validUntil > mustBeValidUntil);
+    let certValid = !ignoreCachedAuthCredentials &&
+                    accountData.cert &&
+                    (accountData.cert.validUntil > mustBeValidUntil);
     // TODO: get the lifetime from the cert's .exp field
-    let currentState = this.currentAccountState;
-    let accountData = yield currentState.getUserAccountData("cert");
-    if (accountData.cert && accountData.cert.validUntil > mustBeValidUntil) {
-      log.debug(" getCertificate already had one");
-      return accountData.cert.cert;
+    if (keyPairValid && certValid) {
+      log.debug("getKeypairAndCertificate: already have keyPair and certificate");
+      return {
+        keyPair: accountData.keyPair.rawKeyPair,
+        certificate: accountData.cert.rawCert
+      }
     }
+    // We are definately going to generate a new cert, either because it has
+    // already expired, or the keyPair has - and a new keyPair means we must
+    // generate a new cert.
+
+    // A keyPair has a longer lifetime than a cert, so it's possible we will
+    // have a valid keypair but an expired cert, which means we can skip
+    // keypair generation.
+    // Either way, the cert will require hitting the network, so bail now if
+    // we know that's going to fail.
     if (Services.io.offline) {
       throw new Error(ERROR_OFFLINE);
     }
-    let willBeValidUntil = this.now() + CERT_LIFETIME;
-    let cert = yield this.getCertificateSigned(data.sessionToken,
-                                               keyPair.serializedPublicKey,
-                                               CERT_LIFETIME);
-    log.debug("getCertificate got a new one: " + !!cert);
-    if (cert) {
+
+    let keyPair;
+    if (keyPairValid) {
+      keyPair = accountData.keyPair;
+    } else {
+      let keyWillBeValidUntil = this.now() + KEY_LIFETIME;
+      keyPair = yield new Promise((resolve, reject) => {
+        jwcrypto.generateKeyPair("DS160", (err, kp) => {
+          if (err) {
+            return reject(err);
+          }
+          log.debug("got keyPair");
+          resolve({
+            rawKeyPair: kp,
+            validUntil: keyWillBeValidUntil,
+          });
+        });
+      });
+    }
+
+    // and generate the cert.
+    let certWillBeValidUntil = this.now() + CERT_LIFETIME;
+    let certificate = yield this.getCertificateSigned(accountData.sessionToken,
+                                                      keyPair.rawKeyPair.serializedPublicKey,
+                                                      CERT_LIFETIME);
+    log.debug("getCertificate got a new one: " + !!certificate);
+    if (certificate) {
+      // Cache both keypair and cert.
       let toUpdate = {
+        keyPair,
         cert: {
-          cert: cert,
-          validUntil: willBeValidUntil
-        }
+          rawCert: certificate,
+          validUntil: certWillBeValidUntil,
+        },
       };
       yield currentState.updateUserAccountData(toUpdate);
     }
-    return cert;
+    return {
+      keyPair: keyPair.rawKeyPair,
+      certificate: certificate,
+    }
   }),
 
   getUserAccountData: function() {
     return this.currentAccountState.getUserAccountData();
   },
 
   isUserEmailVerified: function isUserEmailVerified(data) {
     return !!(data && data.verified);
--- a/services/fxaccounts/tests/xpcshell/test_accounts.js
+++ b/services/fxaccounts/tests/xpcshell/test_accounts.js
@@ -161,16 +161,32 @@ function MockFxAccounts() {
       _("mock getCertificateSigned\n");
       this._getCertificateSigned_calls.push([sessionToken, serializedPublicKey]);
       return this._d_signCertificate.promise;
     },
     fxAccountsClient: new MockFxAccountsClient()
   });
 }
 
+/*
+ * Some tests want a "real" fxa instance - however, we still mock the storage
+ * to keep the tests fast on b2g.
+ */
+function MakeFxAccounts(internal = {}) {
+  if (!internal.newAccountState) {
+    // we use a real accountState but mocked storage.
+    internal.newAccountState = function(credentials) {
+      let storage = new MockStorageManager();
+      storage.initialize(credentials);
+      return new AccountState(storage);
+    };
+  }
+  return new FxAccounts(internal);
+}
+
 add_test(function test_non_https_remote_server_uri_with_requireHttps_false() {
   Services.prefs.setBoolPref(
     "identity.fxaccounts.allowHttp",
     true);
   Services.prefs.setCharPref(
     "identity.fxaccounts.remote.signup.uri",
     "http://example.com/browser/browser/base/content/test/general/accounts_testRemoteCommands.html");
   do_check_eq(fxAccounts.getAccountsSignUpURI(),
@@ -190,26 +206,18 @@ add_test(function test_non_https_remote_
   }, "Firefox Accounts server must use HTTPS");
 
   Services.prefs.clearUserPref("identity.fxaccounts.remote.signup.uri");
 
   run_next_test();
 });
 
 add_task(function test_get_signed_in_user_initially_unset() {
-  // This test, unlike many of the the rest, uses a (largely) un-mocked
-  // FxAccounts instance.
-  let account = new FxAccounts({
-    newAccountState(credentials) {
-      // we use a real accountState but mocked storage.
-      let storage = new MockStorageManager();
-      storage.initialize(credentials);
-      return new AccountState(storage);
-    },
-  });
+  _("Check getSignedInUser initially and after signout reports no user");
+  let account = MakeFxAccounts();
   let credentials = {
     email: "foo@example.com",
     uid: "1234@lcip.org",
     assertion: "foobar",
     sessionToken: "dead",
     kA: "beef",
     kB: "cafe",
     verified: true
@@ -236,60 +244,135 @@ add_task(function test_get_signed_in_use
   let localOnly = true;
   yield account.signOut(localOnly);
 
   // user should be undefined after sign out
   result = yield account.getSignedInUser();
   do_check_eq(result, null);
 });
 
-add_task(function* test_getCertificate() {
-  _("getCertificate()");
-  // This test, unlike many of the the rest, uses a (largely) un-mocked
-  // FxAccounts instance.
-  // We do mock the storage to keep the test fast on b2g.
-  let fxa = new FxAccounts({
-    newAccountState(credentials) {
-      // we use a real accountState but mocked storage.
-      let storage = new MockStorageManager();
-      storage.initialize(credentials);
-      return new AccountState(storage);
-    },
-  });
+add_task(function* test_getCertificateOffline() {
+  _("getCertificateOffline()");
+  let fxa = MakeFxAccounts();
   let credentials = {
     email: "foo@example.com",
     uid: "1234@lcip.org",
-    assertion: "foobar",
     sessionToken: "dead",
-    kA: "beef",
-    kB: "cafe",
-    verified: true
+    verified: true,
   };
+
   yield fxa.setSignedInUser(credentials);
 
   // Test that an expired cert throws if we're offline.
-  fxa.internal.currentAccountState.cert = {
-    validUntil: Date.parse("Mon, 13 Jan 2000 21:45:06 GMT")
-  };
   let offline = Services.io.offline;
   Services.io.offline = true;
-  // This call would break from missing parameters ...
-  yield fxa.internal.getCertificate().then(
+  yield fxa.internal.getKeypairAndCertificate(fxa.internal.currentAccountState).then(
     result => {
       Services.io.offline = offline;
       do_throw("Unexpected success");
     },
     err => {
       Services.io.offline = offline;
       // ... so we have to check the error string.
       do_check_eq(err, "Error: OFFLINE");
     }
   );
+  yield fxa.signOut(/*localOnly = */true);
+});
+
+add_task(function* test_getCertificateCached() {
+  _("getCertificateCached()");
+  let fxa = MakeFxAccounts();
+  let credentials = {
+    email: "foo@example.com",
+    uid: "1234@lcip.org",
+    sessionToken: "dead",
+    verified: true,
+    // A cached keypair and cert that remain valid.
+    keyPair: {
+      validUntil: Date.now() + KEY_LIFETIME + 10000,
+      rawKeyPair: "good-keypair",
+    },
+    cert: {
+      validUntil: Date.now() + CERT_LIFETIME + 10000,
+      rawCert: "good-cert",
+    },
+  };
+
+  yield fxa.setSignedInUser(credentials);
+  let {keyPair, certificate} = yield fxa.internal.getKeypairAndCertificate(fxa.internal.currentAccountState);
+  // should have the same keypair and cert.
+  do_check_eq(keyPair, credentials.keyPair.rawKeyPair);
+  do_check_eq(certificate, credentials.cert.rawCert);
+  yield fxa.signOut(/*localOnly = */true);
 });
 
+add_task(function* test_getCertificateExpiredCert() {
+  _("getCertificateExpiredCert()");
+  let fxa = MakeFxAccounts({
+    getCertificateSigned() {
+      return "new cert";
+    }
+  });
+  let credentials = {
+    email: "foo@example.com",
+    uid: "1234@lcip.org",
+    sessionToken: "dead",
+    verified: true,
+    // A cached keypair that remains valid.
+    keyPair: {
+      validUntil: Date.now() + KEY_LIFETIME + 10000,
+      rawKeyPair: "good-keypair",
+    },
+    // A cached certificate which has expired.
+    cert: {
+      validUntil: Date.parse("Mon, 13 Jan 2000 21:45:06 GMT"),
+      rawCert: "expired-cert",
+    },
+  };
+  yield fxa.setSignedInUser(credentials);
+  let {keyPair, certificate} = yield fxa.internal.getKeypairAndCertificate(fxa.internal.currentAccountState);
+  // should have the same keypair but a new cert.
+  do_check_eq(keyPair, credentials.keyPair.rawKeyPair);
+  do_check_neq(certificate, credentials.cert.rawCert);
+  yield fxa.signOut(/*localOnly = */true);
+});
+
+add_task(function* test_getCertificateExpiredKeypair() {
+  _("getCertificateExpiredKeypair()");
+  let fxa = MakeFxAccounts({
+    getCertificateSigned() {
+      return "new cert";
+    },
+  });
+  let credentials = {
+    email: "foo@example.com",
+    uid: "1234@lcip.org",
+    sessionToken: "dead",
+    verified: true,
+    // A cached keypair that has expired.
+    keyPair: {
+      validUntil: Date.now() - 1000,
+      rawKeyPair: "expired-keypair",
+    },
+    // A cached certificate which remains valid.
+    cert: {
+      validUntil: Date.now() + CERT_LIFETIME + 10000,
+      rawCert: "expired-cert",
+    },
+  };
+
+  yield fxa.setSignedInUser(credentials);
+  let {keyPair, certificate} = yield fxa.internal.getKeypairAndCertificate(fxa.internal.currentAccountState);
+  // even though the cert was valid, the fact the keypair was not means we
+  // should have fetched both.
+  do_check_neq(keyPair, credentials.keyPair.rawKeyPair);
+  do_check_neq(certificate, credentials.cert.rawCert);
+  yield fxa.signOut(/*localOnly = */true);
+});
 
 // Sanity-check that our mocked client is working correctly
 add_test(function test_client_mock() {
   let fxa = new MockFxAccounts();
   let client = fxa.internal.fxAccountsClient;
   do_check_eq(client._verified, false);
   do_check_eq(typeof client.signIn, "function");
 
--- a/services/sync/modules-testing/utils.js
+++ b/services/sync/modules-testing/utils.js
@@ -179,24 +179,20 @@ this.configureFxAccountIdentity = functi
       if (credentials) {
         throw new Error("Not expecting to have credentials passed");
       }
       let storageManager = new MockFxaStorageManager();
       storageManager.initialize(config.fxaccount.user);
       let accountState = new AccountState(storageManager);
       return accountState;
     },
-    getCertificate(data, keyPair, mustBeValidUntil) {
-      let cert = {
-        validUntil: this.now() + CERT_LIFETIME,
-        cert: "certificate",
-      };
-      this.currentAccountState.updateUserAccountData({cert: cert});
-      return Promise.resolve(cert.cert);
+    _getAssertion(audience) {
+      return Promise.resolve("assertion");
     },
+
   };
   fxa = new FxAccounts(MockInternal);
 
   let mockTSC = { // TokenServerClient
     getTokenFromBrowserIDAssertion: function(uri, assertion, cb) {
       config.fxaccount.token.uid = config.username;
       cb(null, config.fxaccount.token);
     },