730989 - Refactor identity and authentication management

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Gregory Szorc [:gps]]

Attachment: Refactor identity and authentication, v1

As part of the protocol 2.0 support, I've refactored management of identity and authentication. I'm not sure I'm complete with this work, but it's definitely at a point I feel comfortable about asking for initial feedback.

If I had planned things better, I would have worked on this before starting the 2.0 protocol work. As it stands, this patch is applied after the hefty set of changes to support protocol 2.0 So, it won't apply cleanly against s-c. Ideally, I'd land this patch before the protocol 2.0 work, as it is theoretically separate and is almost fully compatible with protocol 1.1. I'll have a go at rebasing if someone requests.

Summary of changes:

* Removed old ID and Identity types from identity.js
* Replaced above with IdentityManager type, which has a global instance, Identity.
* Moved Service.{account,username,password,passphrase} into IdentityManager type
* Updated tests to manage identities properly and (mostly) consistently
* Moved some sync key types into keys.js (from record.js)
* Refactored authentication in the resource.js and rest.js HTTP request types
* Removed UTF-8 password nonsense (not needed in protocol 2.0 world - not technically compatible with 1.1)
* Reordered xpcshell tests so they execute from lower to higher level. This makes it easier to pinpoint failures. I could revert this easily enough - it helped immensely when I had over a dozen tests failing.
* Misc style enhancements. Parts of service.js made me vomit. I fixed a few eye sores close to code I changed.
* I now use null instead of "" to denote username, password, etc not being set. I feel this is more Javascript-y. It may break the UX code - I haven't tested.

Areas for Feedback:

* I'm not satisfied with how authentication is integrated with the HTTP stacks. I think the API can be better without being too over-engineered or too tightly coupled.
* status.js may import more than before. This may have performance implications? (I don't know how the magic loading works for clients without Sync configured.) We should be able to restore old behavior, however.

Comment 3

[Gregory Szorc [:gps]]

Attachment: Refactor identity and authentication, v2

I have successfully rebased this patch on top of services-central, before the protocol 2.0 refactor.

Arguably, the patch regressed a little bit because of the rebase.

In the protocol 2.0 world, we didn't have to update URLs when the username changed. As a result (and because of my laziness), I stuck some Service._updateCachedURLs() in some tests. Ideally, there should be a listener in Service that does this whenever Identity.username changes.

Functionally, this patch is nearly identical to the one before it.

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 604246 [details] [diff] [review]
Refactor identity and authentication, v2

Review of attachment 604246 [details] [diff] [review]:
-----------------------------------------------------------------

identity.js and keys.js.

(This is going to be a multi-part feedback/review!)

::: services/sync/modules/identity.js
@@ +33,2 @@
>   *
> + * An instance of this type is available under Weave.Identity. It is and should

Note: "lazily instantiated".

@@ +55,5 @@
> + * If you do monkeypatch, please be advised that Sync expects the core
> + * attributes to have values. You will need to carry at least account and
> + * username forward. If you do not wish to support one of the built-in
> + * authentication mechanisms, you'll probably want to redefine currentAuthState
> + * and any other function that involves the built-in functionality.

o-O

@@ +109,5 @@
> +
> +    // If we change the username, we interpret this as a major change event
> +    // and wipe out the credentials.
> +    this.basicPassword = null;
> +    this.syncKey = null;

And _syncKeyBundle!

@@ +179,5 @@
> +  set syncKey(value) {
> +    if (!value) {
> +      this._log.info("Sync Key has no value. Deleting.");
> +      this._syncKey = null;
> +      this._syncKeyUpdated = true;

And _syncKeyBundle = null!

@@ +189,5 @@
> +    }
> +
> +    this._syncKey = value;
> +    let bundle = this.syncKeyBundle;
> +    bundle.keyStr = value;

You already set keyStr in the syncKeyBundle getter. No point doing it twice.

Add a comment why you're fetching this value here.

@@ +203,5 @@
> +    if (!this._syncKeyBundle) {
> +      this._syncKeyBundle = new SyncKeyBundle(PWDMGR_PASSPHRASE_REALM,
> +                                              this.username);
> +
> +      let key = this.syncKey;

IMO if you don't have a .syncKey, you want to return null and have _syncKeyBundle nulled out.

@@ +336,5 @@
> +    this._basicPassword = null;
> +    this._basicPasswordRetrieved = false;
> +    this._basicPasswordUpdated = false;
> +
> +    this._syncKey = null;

Bundle!

@@ +348,5 @@
> +    if (value && value.match(/[^A-Z0-9._-]/i)) {
> +      return Utils.sha1Base32(value.toLowerCase()).toLowerCase();
> +    }
> +
> +    return value;

toLowerCase()?

@@ +370,5 @@
> +    function getBasicResourceAuthenticator(username, password) {
> +
> +    return function basicAuthenticator(resource) {
> +      return {headers: {authorization: "Basic " +
> +        btoa(username + ":" + password)}};

Indentation, and perhaps a `let`?

@@ +376,5 @@
> +  },
> +
> +  _onResourceRequestBasic: function _onResourceRequestBasic(resource) {
> +    return {headers: {authorization: "Basic " +
> +      btoa(this.username + ":" + this.basicPassword)}};

Same.

Comment 5

[Gregory Szorc [:gps]]

(In reply to Richard Newman [:rnewman] from comment #4)
> @@ +336,5 @@
> > +    this._basicPassword = null;
> > +    this._basicPasswordRetrieved = false;
> > +    this._basicPasswordUpdated = false;
> > +
> > +    this._syncKey = null;
> 
> Bundle!

Not in this case! The _syncKey setter nullifies it. I'll add a comment instead.

Comment 6

[Gregory Szorc [:gps]]

rnewman: I'm sure you've noticed, but one of the changes is that cached credentials (password and sync key) are deleted if the username changes. I had to work around this change in a few tests. I'm not sure if this over-optimistic deletion of the credentials is always desirable.

Do we have any supported scenarios where we need to preserve the sync key across a username change? Or, can a fresh sync or J-PAKE pairing suffice (as I have assumed)?

Comment 7

[Gregory Szorc [:gps]]

Attachment: Refactor identity and authentication, v3

Incorporated rnewman's initial feedback.

I also inlined a function in Service.js related to upgrading the sync key.

Comment 8

[Richard Newman [:rnewman]]

(In reply to Gregory Szorc [:gps] from comment #6)

> Do we have any supported scenarios where we need to preserve the sync key
> across a username change? Or, can a fresh sync or J-PAKE pairing suffice (as
> I have assumed)?

I can't imagine we would ever support changing the username without starting from scratch.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 604570 [details] [diff] [review]
Refactor identity and authentication, v3

Review of attachment 604570 [details] [diff] [review]:
-----------------------------------------------------------------

Lovely!

::: services/sync/modules/service.js
@@ +354,1 @@
>        this._log.info("Weave Sync disabled");

Fixitfixit

::: services/sync/tests/unit/test_errorhandler.js
@@ +172,5 @@
>    }
>  
>    // Make sync fail due to login rejected.
> +  Identity.account = "janedoe";
> +  // We need to update the password and sync key as well to workaround

s/workaround/work around the/

@@ +175,5 @@
> +  Identity.account = "janedoe";
> +  // We need to update the password and sync key as well to workaround
> +  // side-effect of these being deleted when the account changes.
> +  Identity.basicPassword = "irrelevant";
> +  Identity.syncKey = "irrelevant";

Can't you just call setBasicCredentials again?

::: services/sync/tests/unit/test_keys.js
@@ +231,4 @@
>  function test_key_persistence() {
>    _("Testing key persistence.");
> +
> +  /*

TODO indeed.

::: services/sync/tests/unit/test_resource_async.js
@@ +284,5 @@
>  
>  add_test(function test_get_protected_success() {
>    _("GET a password protected resource");
> +  let auth = Identity.getBasicResourceAuthenticator("guest",
> +                                                                 "guest");

Indentation.

::: services/sync/tests/unit/test_service_login.js
@@ +236,5 @@
>    }
>  });
> +
> +// Above test monkeypatches and doesn't restore! If you add a new test, correct
> +// above.

Can we just do the right thing instead? You already do in one of the other tests…

::: services/sync/tests/unit/test_service_startOver.js
@@ +29,5 @@
>  }
>  
>  add_test(function test_resetLocalData() {
>    // Set up.
> +  setBasicCredentials("foobar", "blablabla", // Law Blog

Wat

::: services/sync/tests/unit/test_status_checkSetup.js
@@ +12,3 @@
>    try {
>      _("Verify initial setup.");
> +    Identity.deleteSyncCredentials();

No longer "verify", mm?

@@ +28,3 @@
>  
>      _("Let's provide a password.");
> +    Identity.basicPassword = "carotsalad";

ZOMG HOW DID U GUESS MY PASWORD?

::: services/sync/tps/extensions/tps/modules/sync.jsm
@@ +103,5 @@
>      }
>      catch(e) {}
> +    Weave.Identity.account = prefs.getCharPref('tps.account.username');
> +    Weave.Identity.basicPassword = prefs.getCharPref('tps.account.password');
> +    Weave.Identity.syncKey = prefs.getCharPref('tps.account.passphrase');

Be a gentleman, line these up.

Comment 10

[Gregory Szorc [:gps]]

Attachment: Refactor identity and authentication, v4

Incorporated all review feedback with this patch. In addition:

* MOAR tests in test_identity_manager.js
* Refactored KeyBundle, BulkKeyBundle, and SyncKeyBundle
  * API is now much saner (and simpler) since it isn't conforming to Identity inheritance
  * All keys are now internally stored as raw byte strings. Before, SyncKey was stored in B64 encoded form.

I'm asking for feedback from dchan because I changed how Sync Keys and key pairs are stored. I want to make sure I haven't done anything stupid. dchan: you're probably most interested in identity.js, keys.js, record.js, and service.js, as that is where things interacting with keys live.

Comment 11

[David Chan [:dchan]]

Comment on attachment 608078 [details] [diff] [review]
Refactor identity and authentication, v4

Review of attachment 608078 [details] [diff] [review]:
-----------------------------------------------------------------

The code looks good. I just have a couple questions about the workflow.

::: services/sync/modules/identity.js
@@ +175,5 @@
> +    // Wiping out value.
> +    if (!value) {
> +      this._log.info("Basic password has no value. Removing.");
> +      this._basicPassword = null;
> +      this._basicPasswordUpdated = true;

reset this._basicPasswordRetrieved?

@@ +238,5 @@
> +    if (!value) {
> +      this._log.info("Sync Key has no value. Deleting.");
> +      this._syncKey = null;
> +      this._syncKeyBundle = null;
> +      this._syncKeyUpdated = true;

reset this._syncKeyRetrieved?

::: services/sync/modules/keys.js
@@ +42,5 @@
> +  _sha256HMACHasher: null,
> +
> +  equals: function equals(bundle) {
> +    return bundle &&
> +           (bundle.hmacKey == this.hmacKey) &&

Do we care about type-coercion?

@@ +127,5 @@
> +    return [this.encryptionKey, this.hmacKey];
> +  },
> +
> +  set keyPair(value) {
> +    if (!value || !value.length || value.length != 2) {

instanceof Array instead of value.length?

@@ +140,5 @@
> +    return [this.encryptionKeyB64, this.hmacKeyB64];
> +  },
> +
> +  set keyPairB64(value) {
> +    if (!value || !value.length || value.length != 2) {

ditto

@@ +173,5 @@
> +  /*
> +   * If we've got a string, hash it into keys and store them.
> +   */
> +  generateFromKey: function generateFromKey(username, syncKey) {
> +    if (!username || !username.charAt) {

Does this function take in untrusted data? There could be problems if it is passed a username/syncKey object with a user defined charAt property/function.

@@ +177,5 @@
> +    if (!username || !username.charAt) {
> +      throw new Error("Sync Key cannot be generated from non-string username.");
> +    }
> +
> +    if (!syncKey || !syncKey.charAt) {

ditto above

::: services/sync/modules/resource.js
@@ +209,1 @@
>        if (key == 'authorization')

Consider suppressing Proxy-Authorization as well

Comment 12

[Mozilla RelEng Bot]

Try run for 6d380712aed5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6d380712aed5
Results (out of 139 total builds):
    exception: 2
    success: 114
    warnings: 16
    failure: 7
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6d380712aed5

Comment 13

[Gregory Szorc [:gps]]

(In reply to David Chan [:dchan] from comment #11)
> ::: services/sync/modules/identity.js
> @@ +175,5 @@
> > +    // Wiping out value.
> > +    if (!value) {
> > +      this._log.info("Basic password has no value. Removing.");
> > +      this._basicPassword = null;
> > +      this._basicPasswordUpdated = true;
> 
> reset this._basicPasswordRetrieved?
> 
> @@ +238,5 @@
> > +    if (!value) {
> > +      this._log.info("Sync Key has no value. Deleting.");
> > +      this._syncKey = null;
> > +      this._syncKeyBundle = null;
> > +      this._syncKeyUpdated = true;
> 
> reset this._syncKeyRetrieved?

In both cases, no. If you reset the retrieved flags, the next time the value is accessed, the API might go to the password manager and get a value from there instead of reading the "cached" but not committed null value. If anything, I should set the retrieved flags to true when setting values to null. And, I should probably change the name of the variable to something like _basicPasswordAllowLookup. And, I should add test coverage to ensure this flow works as expected.
 
> ::: services/sync/modules/keys.js
> @@ +42,5 @@
> > +  _sha256HMACHasher: null,
> > +
> > +  equals: function equals(bundle) {
> > +    return bundle &&
> > +           (bundle.hmacKey == this.hmacKey) &&
> 
> Do we care about type-coercion?

Yes, we probably should. I should learn to not blindly copy code.

> 
> @@ +127,5 @@
> > +    return [this.encryptionKey, this.hmacKey];
> > +  },
> > +
> > +  set keyPair(value) {
> > +    if (!value || !value.length || value.length != 2) {
> 
> instanceof Array instead of value.length?

Or Array.isArray. I'll change this.

> @@ +173,5 @@
> > +  /*
> > +   * If we've got a string, hash it into keys and store them.
> > +   */
> > +  generateFromKey: function generateFromKey(username, syncKey) {
> > +    if (!username || !username.charAt) {
> 
> Does this function take in untrusted data? There could be problems if it is
> passed a username/syncKey object with a user defined charAt
> property/function.

More code copied without consideration. I'll do the right thing in the next patch.
 
> ::: services/sync/modules/resource.js
> @@ +209,1 @@
> >        if (key == 'authorization')
> 
> Consider suppressing Proxy-Authorization as well

Good catch. Will do.

Comment 14

[Gregory Szorc [:gps]]

Attachment: Refactor identity and authentication, v5

Incorporated dchan's feedback.

I addressed type coercion on equal() by checking values in the encryptionKey and hmacKey setters. I now ensure that the passed value is defined, a string, and is at least 16 bytes long. Before, there was no validation. I think the crypto layers enforce more stringent length requirements later. I'll deal with validation more properly during the crypto rewrite for bug 636309.

Comment 15

[Richard Newman [:rnewman]]

Comment on attachment 608078 [details] [diff] [review]
Refactor identity and authentication, v4

Review of attachment 608078 [details] [diff] [review]:
-----------------------------------------------------------------

I'm so glad I gave a thorough review for 'feedback'.

::: services/sync/modules/identity.js
@@ +225,5 @@
> +   * removed. However, the Sync Key won't be deleted from the password manager
> +   * until persistSyncCredentials() is called.
> +   *
> +   * If a value is provided, it should be a 32 character "friendly" Base32
> +   * string.

Hyphenated? (I.e., does this function normalize?)

@@ +274,5 @@
> +    }
> +
> +    if (!this.syncKey) {
> +      this._log.warn("Attempted to obtain Sync Key Bundle with no Sync Key " +
> +                     "set!");

In this case I will settle for a slightly long line ;)

::: services/sync/modules/keys.js
@@ +127,5 @@
> +    return [this.encryptionKey, this.hmacKey];
> +  },
> +
> +  set keyPair(value) {
> +    if (!value || !value.length || value.length != 2) {

'undefined' != 2, so you don't need to check length at all.

I don't agree that it must be an instanceof Array; we only care about it being notionally a "pair".

@@ +140,5 @@
> +    return [this.encryptionKeyB64, this.hmacKeyB64];
> +  },
> +
> +  set keyPairB64(value) {
> +    if (!value || !value.length || value.length != 2) {

Ditto.

::: services/sync/modules/service.js
@@ +350,5 @@
>  
>      SyncScheduler.init();
>  
> +    if (!this.enabled) {
> +      this._log.info("Weave Sync disabled.");

s/Weave/Firefox/

Comment 16

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/82c4a54e0437

Comment 17

[David Chan [:dchan]]

Comment on attachment 608499 [details] [diff] [review]
Refactor identity and authentication, v5

Review of attachment 608499 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/keys.js
@@ +54,5 @@
> +    return this._encrypt;
> +  },
> +
> +  set encryptionKey(value) {
> +    if (!value || typeof value != "string") {

nit: typeof will return object if value is assigned as
value = new String(...)
I didn't see any uses of new String(...) so this shouldn't matter

Comment 18

[Gregory Szorc [:gps]]

(In reply to David Chan [:dchan] from comment #17)
> nit: typeof will return object if value is assigned as
> value = new String(...)
> I didn't see any uses of new String(...) so this shouldn't matter

Yeah, we don't use new String() anywhere.

Comment 19

[Gregory Szorc [:gps]]

There is at least one reference to a now defunct Weave.Service.* identity property left in the tree. My work here is not done.

Comment 20

[Gregory Szorc [:gps]]

Attachment: Followup: Remove references to defunct Service properties

This follow-up fixes references that should have been updated with the original patch. I mostly forgot about mobile XUL, but there was one reference in desktop as well that might have thrown off pairing.

With this patch applied, I've verified that pairing between 2 desktop instances works.

Comment 21

[Gregory Szorc [:gps]]

https://hg.mozilla.org/services/services-central/rev/68ee19a92287

Comment 22

[Tracy Walker [:tracy]]

Long method of adding a device to an account gets stuck at password recognition per depends bug just filed bug 739752

Comment 23

[Gregory Szorc [:gps]]

https://hg.mozilla.org/mozilla-central/rev/82c4a54e0437