1132293 - Let reliers access encryption keys through FxAccountsOAuthClient

Status: RESOLVED FIXED

(Cloud Services :: Server: Firefox Accounts, defect)

Description

[Ryan Kelly [:rfkelly]]

(Spinning off a sub-bug from the discussion in Bug 1115340, since I don't think we need to crowd the meta-bug with all these details)

Comment 1

[Ryan Kelly [:rfkelly]]

Here's a concrete proposal based on Bug 1115340 Comment 22 and related discussion.

The Goals
=========

  * Give the Hello FxA relier access to a pair of relier-specific keys (kAr, kBr) derived from
    the account keys (kA, kB).

  * Take advantage of the fact that we're running in trusted code inside Gecko to do it
    simply and quickly.

  * But do it in a way that's forward-compatible with some yet-to-be-defined offering of FxA-backed
    crypto services to generic reliers on the open web.


The Crypto Bits
===============

Desired Outputs:

  * kAr, kBr:  JWKs representing relier-specific FxA key material

Inputs:

  * kA, kB:  the existing account keys (as bytes)
  * rID:     the unique identifier for an oauth relier (as hex string)

To derive the keys, we do HKDF in line with exsiting FxA conventions, using the relier id
as context information:

  kAr_bytes = HKDF_SHA256(kA, info="identity.mozilla.com/picl/v1/oauth/kAr:" + rID,
                          salt="", size=32)

  kBr_bytes = HKDF_SHA256(kB, info="identity.mozilla.com/picl/v1/oauth/kBr:" + rID,
                          salt="", size=32)

We give each key a unique id, which can be used for e.g. tagging encrypted blobs with the
key that was used to encrypt them:

  kAr_id = "kAr-" + base64url(SHA256(kAr_bytes))

  kBr_id = "kBr-" + base64url(SHA256(kBr_bytes))


Finally, we represent each key as a JWK object like so:

  kAr = {
    "kid": kAr_id,
    "k": base64url(kAr_bytes),
    "kty": "oct",
    "key_ops": ["deriveKey"]
  }

  kBr = {
    "kid": kBr_id,
    "k": base64url(kBr_bytes),
    "kty": "oct",
    "key_ops": ["deriveKey"]
  }


The API Bits
============

To request keys from inside Gecko, the relying code should add `keys: true` to the
parameters used when instantiating an FxAccountsOAuthClient.  Its constructor signature
becomes:

  FxAccountsOAuthClient FxAccountsOAuthClient(
    Object options
      Object parameters
        String client_id
        String state
        String oauth_uri
        String content_uri
        [optional] String scope
        [optional] String action
        [optional] Boolean keys
      [optional] String authorizationEndpoint
  );

To receive the keys, the onComplete callback would take an optional second argument:

  client.onComplete = function(tokenData, keys) {
    // tokenData remains unchanged, i.e. { code: code, state: state }
    // keys gives the two JWKs are javascript objects: { kAr: kAr, kBr: kBr }
  }

If keys=true is not specified, or if there is an error in generating the keys, then
the second argument will be `null`.  This class does not seem to have an error-reporting
channel and I don't propose to add one.

Internally, the key derivation is done as part of the existing OAuth flow, by web content loaded from fxa-content-server.  The keys are communicated back back to Gecko via an extra field in the existing "oauth_complete" WebChannel message.

See also: https://github.com/mozilla/fxa-content-server/issues/2088


Notes and Questions
===================

* I'm not at all sure whether the JWK `key_ops` field adds any value for our use-case.
  And if we do use it, then per Richard's earlier comments in this bug, perhaps we should
  directly specify key_ops=["encrypt", "decrypt"] rather than suggesting another layer
  of derivation.

* Directly exposing the names "kAr" and "kBr" to the relying code seems kinda ugly,
  but I didn't come up with anything compellingly better.

* Chris, you originally suggested that we should include the userid in the keyid as a
  debugging aid. We could easily add it but it would make the keyids really long:

    "kBr-cd8001987f4d4cbb983609e815e3f365-47DEQpj8HBSa-_TImW-5JCeuQeRkm5NMpJWZG3hSuFU"

  Thoughts?

* Firefox Accounts tracks a "generation number" as part of your account data, which increases
  every time you change or reset your password.  It would be neat to include this as part of
  kBr_id, so that if you encounter two mismatched values of kBr then it's easy to tell which
  is newer.  But I don't think the server exposes it in a way that would let us do this easily.
  Chris, thoughts?


Chris, Richard, Adam: does this capture everyone's understanding of the proposed path forward?

Comment 2

[Ryan Kelly [:rfkelly]]

(Ugh, looks like I had some accidental line-breaks in there from drafting this in a separate editor, sorry about the formatting weirdness!)

Comment 3

[Ryan Kelly [:rfkelly]]

The other parts seem readable, but here's a re-formatted version of the final questions:


Notes and Questions
===================

* I'm not at all sure whether the JWK key_ops field adds any value for our use-case.  And if we do use it, then per Richard's earlier comments in this bug, perhaps we should directly specify key_ops=["encrypt", "decrypt"] rather than suggesting another layer of derivation.

* Directly exposing the names "kAr" and "kBr" to the relying code seems kinda ugly, but I didn't come up with anything compellingly better.

* Chris, you originally suggested that we should include the userid in the keyid as a debugging aid. We could easily add it but it would make the keyids really long:

    "kBr-cd8001987f4d4cbb983609e815e3f365-47DEQpj8HBSa-_TImW-5JCeuQeRkm5NMpJWZG3hSuFU"

Thoughts?

* Firefox Accounts tracks a "generation number" as part of your account data, which increases every time you change or reset your password.  It would be neat to include this as part of kBr_id, so that if you encounter two mismatched values of kBr then it's easy to tell which is newer.  But I don't think the server exposes it in a way that would let us do this easily.  Chris, thoughts?

Comment 4

[Chris Karlof [:ckarlof]]

> "k": base64url(kBr_bytes),

Why "k" and not something more descriptive, like "key"?

> Directly exposing the names "kAr" and "kBr" to the relying code seems kinda ugly, but I didn't come up with anything compellingly better.

It's fine with me.

> Chris, you originally suggested that we should include the userid in the keyid as a debugging aid. We could easily add it but it would make the keyids really long:

It may be sufficient to just add the client_id and user id in as additional properties in the JWT.

> Firefox Accounts tracks a "generation number" as part of your account data, which increases every time you change or reset your password.  It would be neat to include this as part of kBr_id, so that if you encounter two mismatched values of kBr then it's easy to tell which is newer.

It's in the signed certs, so currently the client code would have to request one of those and parse it out of that. Not sure if it's worth it, but it may have some value. It's kind of ugly though. We also return the generation directly in some Auth server APIs, too, e.g., maybe in the call that fetches the keys. 

Otherwise, it looks good.

After we get verification from Adam and Richard (or Richard's delegate), we can proceed with implementation.

Comment 5

[Ryan Kelly [:rfkelly]]

> Why "k" and not something more descriptive, like "key"?

This is what the JWK spec mandates for symmetric keys.

> It may be sufficient to just add the client_id and user id in as additional properties

Nice, yes let's do this, probably namespaced to FxA like we do in the auth-server assertions.

Comment 6

[Ryan Kelly [:rfkelly]]

> * Firefox Accounts tracks a "generation number" as part of your account data

Actually this won't be useful for the key id because it can change independently of kB, e.g. if the user does a kB-preserving password change rather than a kB-resetting password recovery.  We could surface it as another field in the JWK but I think the value would be pretty minimal there.

Comment 7

[Adam Roach [:abr]]

In general, this looks like it gets us something perfectly functional that we can use short-term until the final oracle-based design is implemented. From my perspective: go for it.

(In reply to Ryan Kelly [:rfkelly] from comment #3)

> * I'm not at all sure whether the JWK key_ops field adds any value for our
> use-case.  And if we do use it, then per Richard's earlier comments in this
> bug, perhaps we should directly specify key_ops=["encrypt", "decrypt"]
> rather than suggesting another layer of derivation.

I think adding this field without it impacting behavior provides no benefit, with the potential for unnecessary confusion. Since key_ops is optional in JWK, let's just omit it. If we come up with some case that the recipient of the key does something different, we can revisit.

> * Directly exposing the names "kAr" and "kBr" to the relying code seems
> kinda ugly, but I didn't come up with anything compellingly better.

I think it's fine for this temporary solution. Once we get an oracle-based solution in place, these names aren't part of what the relying code sees any more. In other words: if things go as planned, the only relying code that ever deals with these names is Hello; and even then, it's temporary.

Comment 8

[Richard Barnes [:rbarnes]]

Overall, this looks sane to me.  Striking resemblance to what ckarlof and I drew on the whiteboard the other day :)

Couple of comments below.


>   * But do it in a way that's forward-compatible with some yet-to-be-defined
> offering of FxA-backed
>     crypto services to generic reliers on the open web.

To be clear, I still think we should do general-web stuff differently.  (Using an FxA iframe to create an encrypt/decrypt oracle, as discussed before.)

My impression from talking to ckarlof is that there was no obvious way to do that for Gecko reliers, so there's no way to do better than to just hand them a key. 


> We give each key a unique id, which can be used for e.g. tagging encrypted
> blobs with the
> key that was used to encrypt them:
> 
>   kAr_id = "kAr-" + base64url(SHA256(kAr_bytes))
> 
>   kBr_id = "kBr-" + base64url(SHA256(kBr_bytes))

I understand why you need a unique tag for each key, and I understand the argument that SHA256 should not be reversible, but this still feels like a thin layer of protection to me.

It seems like there's not a requirement for the relier to be able to derive the key tag from the key, so maybe you could do something like parallel derivation with different salt or info:

kXr_bytes = HKDF_SHA256(kX, info="identity.mozilla.com/picl/v1/oauth/kXr:" + rID,
                            salt="", size=32)

kXr_id    = "kBr-" + kXr_salt + "-" 
          + HKDF_SHA256(kX, info="identity.mozilla.com/picl/v1/oauth/kXr:" + rID,
                            salt=kXr_salt, size=32)
 

It would be nice if there were a way for the app to ask for fresh keys, or at least to keep the door open for this.  You could do this by salting the HKDF derivation of kXr_bytes, and keeping the salt as per-relier server-side state.


> Finally, we represent each key as a JWK object like so:
> 
>   kAr = {
>     "kid": kAr_id,
>     "k": base64url(kAr_bytes),
>     "kty": "oct",
>     "key_ops": ["deriveKey"]
>   }
> 
>   kBr = {
>     "kid": kBr_id,
>     "k": base64url(kBr_bytes),
>     "kty": "oct",
>     "key_ops": ["deriveKey"]
>   }

It doesn't seem like "key_ops" adds that much value here; since the JWKs are not wrapped, the relier can just remove them.  I would just drop them.  It's the job of the app to use its keys wisely.




> Notes and Questions
> ===================
> 
> * I'm not at all sure whether the JWK `key_ops` field adds any value for our
>   use-case. And if we do use it, then per Richard's earlier comments in this bug,
>   perhaps we should directly specify key_ops=["encrypt", "decrypt"] rather than
>   suggesting another layer of derivation.

As above, just kill it.

As for Loop, I would say that they should just use the keys directly to wrap/unwrap the room keys, or to derive/wrap a room-key-wrapping-key if they want to reserve the right to use the FxA keys for other uses.


> * Directly exposing the names "kAr" and "kBr" to the relying code seems
> kinda ugly,
>   but I didn't come up with anything compellingly better.

Seems harmless.

Comment 9

[Ryan Kelly [:rfkelly]]

> >   * But do it in a way that's forward-compatible with some yet-to-be-defined
> > offering of FxA-backed
> >     crypto services to generic reliers on the open web.
>
> To be clear, I still think we should do general-web stuff differently.  (Using an FxA iframe to create
> an encrypt/decrypt oracle, as discussed before.)

Sure.  What I was trying to capture above was something like: the thing we do here could become an internal step done when setting up this iframe.  So we don't rely on state or info that's available inside Gecko but unavailable on the web.

> It doesn't seem like "key_ops" adds that much value here; since the JWKs are not wrapped,
> the relier can just remove them.  I would just drop them.

Agreed.

> It seems like there's not a requirement for the relier to be able to derive the key tag from the key,
> so maybe you could do something like parallel derivation with different salt or info:

Sounds good.  In other places in FxA (e.g. for hawk auth tokens) we've done this by generating extra bytes from the HKDF and using some as the id, like this:

    kXr_data = HKDF_SHA256(kX, info="identity.mozilla.com/picl/v1/oauth/kXr:" + rID,
                           salt="", size=64)
    kXr_id = "kXr-" + base64url(kXr_data[0:32])
    kXr_bytes = kXr_data[32:64]

If the above seems fine, it'd be nice to do it that way just for internal consistency.  But I'll happily take your recommendation otherwise if that strikes you as a bad idea :-)

> It would be nice if there were a way for the app to ask for fresh keys, or at least to keep the door
> open for this.  You could do this by salting the HKDF derivation of kXr_bytes, and keeping the salt
> as per-relier server-side state.

Interesting.  What's the advantage of doing it this way, versus having the app do its own HKDF with custom info string?  I guess putting the salt server-side means we can do it in a more black-box fashion.

Comment 10

[Ryan Kelly [:rfkelly]]

Just an update that the bulk of the work on this is progressing in https://github.com/mozilla/fxa-content-server/pull/2143

Comment 11

[Ryan Kelly [:rfkelly]]

The above PR has landed, and support for this will ship with Firefox Accounts train-32 (Bug 1138743) due to appear in production by the end of next week.  It'll be on our dev environments in a day or two.

I have a work-in progress patch for the gecko side of the equation here which I'll post shortly.

Comment 12

[Ryan Kelly [:rfkelly]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=536afc8fb4ca

Comment 13

[Ryan Kelly [:rfkelly]]

Attachment: oauth-webchannel-keys.patch

Attaching my minimal gecko patch to make use of the new "keys=true" functionality.  I've confirmed this to work with a local oauth+loop server setup and added tests where it seemed appropriate.  :ckarlof thoughts?

Comment 14

[Chris Karlof [:ckarlof]]

Comment on attachment 8573008 [details] [diff] [review]
oauth-webchannel-keys.patch

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

::: browser/base/content/test/general/browser_fxa_oauth.js
@@ +76,5 @@
>      }
> +  },
> +  {
> +    desc: "FxA OAuth - should be able to request keys during OAuth flow",
> +    run: function* () {

We should get markh to eyeball this test.

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +175,5 @@
>                result = {
>                  code: data.code,
>                  state: data.state
>                };
> +              keys = data.keys || null;

Strictly speaking if the requester didn't want keys, then keys should be undefined. If they did want keys and we didn't get them, then I'm not sure what should happen here. The ole 'null' "cheers" isn't awesome. An alternative is to introduce an onError callback. The advantage of onError is that the caller's onComplete code doesn't need to be defensive about things being missing.

Comment 15

[Adam Roach [:abr]]

(In reply to Chris Karlof [:ckarlof] from comment #14)

> Strictly speaking if the requester didn't want keys, then keys should be
> undefined. If they did want keys and we didn't get them, then I'm not sure
> what should happen here. The ole 'null' "cheers" isn't awesome. An
> alternative is to introduce an onError callback. The advantage of onError is
> that the caller's onComplete code doesn't need to be defensive about things
> being missing.

There's a question about how we would communicate partial success under such circumstances, though (i.e., login worked but you get no keys -- can that happen?).

Comment 16

[Ryan Kelly [:rfkelly]]

> (i.e., login worked but you get no keys -- can that happen?)

On our side, I want to guarantee not - you asked for keys, so if you didn't get them then we should be able to tell you that via an error.  But I think it's worth being defensive about it to start here because there are probably corner-cases we wont anticipate.

One can also imagine a future world where the user is presented with an oauth permissions prompt and opts-out of providing the keys.  Not a possibility for loop but it may inform the API design here.

> An alternative is to introduce an onError callback. 

Yeah.  I was hoping to avoid that because there wasn't one there already, but it's probably the Right Thing.

Comment 17

[Ryan Kelly [:rfkelly]]

Comment on attachment 8573008 [details] [diff] [review]
oauth-webchannel-keys.patch

> We should get markh to eyeball this test.

Mark, can you please eyeball this test?

Comment 18

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8573008 [details] [diff] [review]
oauth-webchannel-keys.patch

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

The test is already somewhat broken - it is using a generator function that never yields - but that's not your fault and not particularly worth fixing here IMO.

Comment 19

[Ryan Kelly [:rfkelly]]

> An alternative is to introduce an onError callback. 

I'll also note that there's some precedent for null == error in this code.  If the 'state' parameter doesn't match when the flow returns, it returns a null result and the calling code is supposed to check for this error.  If we add an onError, we should make state-mismatch trigger it as well, which might require some refactoring of calling code.

(And if we go that far, maybe we should make FxAOAuthClient.launchWebFlow() return a promise)

Anyways, I'll have a crack at it and see how the code comes out.

Comment 20

[Ryan Kelly [:rfkelly]]

Attachment: oauth-webchannel-keys.patch

Here's an onError-based version, which didn't turn out too gnarly in the end.  This might also open up the possibility of handler other varieties of error in the future, e.g. the user closing the tab without completing the login flow.

Comment 21

[Chris Karlof [:ckarlof]]

Comment on attachment 8573722 [details] [diff] [review]
oauth-webchannel-keys.patch

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

::: browser/components/loop/MozLoopService.jsm
@@ +980,5 @@
>      let deferred = Promise.defer();
>      this.promiseFxAOAuthClient().then(
>        client => {
>          client.onComplete = this._fxAOAuthComplete.bind(this, deferred);
> +        client.onError = this._fxAOAuthError.bind(this, deferred);

Probably need someone from Hello to look at this.

::: services/fxaccounts/FxAccountsOAuthClient.jsm
@@ +181,5 @@
> +            if (this.parameters.state !== data.state) {
> +              err = new Error("OAuth flow failed. State doesn't match");
> +            } else if (this.parameters.keys && !data.keys) {
> +              err = new Error("OAuth flow failed. Keys were not returned");
> +            } else {

We need tests for these cases.

Comment 22

[Chris Karlof [:ckarlof]]

> maybe we should make FxAOAuthClient.launchWebFlow() return a promise

When it originally landed Tim Taubert and Vlad decided to go with this onComplete callback thing instead of a promise, I believe. I don't recall all the reasons, but given that Hello is turning that abstraction into a promise-based one anyway, it certainly does raise the question.

Comment 23

[Chris Karlof [:ckarlof]]

One thing to keep in mind with a promised based approach is that we need to allow the caller to invoke launchWebFlow multiple times (because the user may close the original opened window), and the user could complete the login in any one of the opened windows. It makes managing the promise a little tricky, and we need to decided what to do with the cached promise after it resolves or rejects (uncache it, I think).

Comment 24

[Ryan Kelly [:rfkelly]]

> One thing to keep in mind with a promised based approach...

And you just talked me right out of it. :-)

I'll post a new version of the existing patch, with extra tests, for a proper r?

Comment 25

[Ryan Kelly [:rfkelly]]

Attachment: oauth-webchannel-keys.patch

OK, here's a new version of the patch with extra tests.  Code itself is unchanged from the previous iteration.

Mark, Chris named you as a good reviewer here, but I know you've got a lot on your plate so don't hesitate to redirect me.

Adam, this includes a small refactor to Loop code due to the addition of an onError callback, can you please sanity-check?

Comment 26

[Ryan Kelly [:rfkelly]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=297b3143fd66

Comment 27

[Adam Roach [:abr]]

(In reply to Ryan Kelly [:rfkelly] from comment #25)
> Adam, this includes a small refactor to Loop code due to the addition of an
> onError callback, can you please sanity-check?

I'm in all-day meetings in London this week. I'll try to give this a look, but it may be a while before I can find a few moments.

Comment 28

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8575072 [details] [diff] [review]
oauth-webchannel-keys.patch

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

::: browser/components/loop/MozLoopService.jsm
@@ +980,5 @@
>      let deferred = Promise.defer();
>      this.promiseFxAOAuthClient().then(
>        client => {
>          client.onComplete = this._fxAOAuthComplete.bind(this, deferred);
> +        client.onError = this._fxAOAuthError.bind(this, deferred);

It looks like the bind to this isn't necessary (this isn't used in any of the handlers), and TBH I think it would be a little nicer and clearer to have both of these inline, so it would just read:

client.onComplete = result => {
  gFxAOAuthClientPromise = null;
  deferred.resolve(result);
};
client.onError = err => {
  gFxAOAuthClientPromise = null;
  deferred.reject(err)
};

and both _fxAOAuthComplete and _fxAOAuthError could be removed - best I can tell they aren't used in tests.

But given this code already existed and the new changes match the existing style I'll leave it to your discretion.

Comment 29

[Ryan Kelly [:rfkelly]]

Thanks Mark.  Since that code is in MozLoopService.jsm I'll defer discretion to Adam or his delegate from the Loop team.

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8575072 [details] [diff] [review]
oauth-webchannel-keys.patch

I'll steal this from Adam as he's busy at the moment. I'll try and take a look at it tomorrow if not before.

Comment 31

[Mark Banner (:standard8)]

Comment on attachment 8575072 [details] [diff] [review]
oauth-webchannel-keys.patch

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

This looks fine to me. I think I'm happy for the format to stay as you've got it - we're due a restructure of the file anyway.

Comment 32

[Ryan Kelly [:rfkelly]]

Hmm, apparently I don't have permission to land this in fx-team, marking checking-needed so as not to block on me figuring that situation out.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/342879febeba

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/342879febeba