1115340 - [meta] Implement ability to add context to conversations (v1)

Status: RESOLVED FIXED

(Hello (Loop) :: General, defect, P5)

Comment 1

[:shell escalante]

i'm tempted to triage all sub-bugs to Fx39... and pull in a couple early - after further conversations happen and a lock on the plan for co-browsing scenario implementation plan.  

we don't want to start evaluating these as is for 38 if we still expect we might get conflicting feedback on direction/change direction.

do you object to that approach?  we can't logically be trying to track features implementation timing, until we've locked what they are.

Comment 2

[Romain Testard [:RT]]

The bugs under this meta make-up the MVP for sharing context, they should not be implemented in separate releases.
Also this is different from the co-browsing/datachannel discussion. Co-browsing will come in post Fx38 (annotations, pointer share and other things we need to prioritize based on user research outcome).

This meta can be implemented regardless of where we end-up WRT co-browsing.

Comment 3

[:shell escalante]

putting at P5 as it's actively being worked in PM and the creative team to prioritize co-browsing scenarios.  it's not actionable to put as higher priority until the scenarios and prioritization and UX are finalized.

Comment 4

[Alexis Metaireau (:alexis)]

Adam put together a proposal about this, it's here: https://wiki.mozilla.org/Loop/Architecture/Context

I'll comment here since I don't know where the original feed for this is.

1. First, If we introduce a new key, we should do a Key Derivation on it rather than using the Key directly to do anything, as it allows not to share the "root" key with anyone. Hence, I would recommend deriving Kr before using it. (Using HKDF is our prefered way currently);

2. Changes proposed on the server side sounds good to me, except that I would rather define everything as one attribute (with sub-keys) rather than multiple keys (for key, algo and context). Also, we could benefit of using JWT sementics here rather than creating a new one.

3. I really think Crypto should be done on the client side, so that's going to the right direction!

4. If we convey part of the crypto to client as a hash in the URLs, it means URLs will get longer than they currently are. As it was a big concern to PM to start with, we should make sure they agree with this.

Comment 5

[Rémy Hubscher (:natim)]

My main concern is to have the key passed through the URL.
Was not the aim to keep small rooms urls that users can potentially remember?

Is this key the room-metadata key or the room key? If it is the room keys (meaning that anything needed to be encrypted in that room will eventually use this key) the will should definitely use kR as a root key and derive it for each kind of encrypted blob.

I don't see yet anything else than metadata to be encrypted with this key but to be future proof we should consider doing that.

Comment 6

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #4)
> Adam put together a proposal about this, it's here:
> https://wiki.mozilla.org/Loop/Architecture/Context
> 
> I'll comment here since I don't know where the original feed for this is.

That's because I'm not done yet, and hadn't put it out for broader review. :)

> 1. First, If we introduce a new key, we should do a Key Derivation on it
> rather than using the Key directly to do anything, as it allows not to share
> the "root" key with anyone. Hence, I would recommend deriving Kr before
> using it.

Sure. It's hard to come up with use cases for other room-related keys, but I'm happy future-proofing it in this way.

> 2. Changes proposed on the server side sounds good to me, except that I
> would rather define everything as one attribute (with sub-keys) rather than
> multiple keys (for key, algo and context).

I'm ambivalent about this, and don't object to putting these in a single field.

> Also, we could benefit of using
> JWT sementics here rather than creating a new one.

The issue here is that JWT doesn't (as far as I know) provide a format for conveying a wrapped key. It would, of course, be possible to create a JWT representation of the AES-GCM key, and then encrypt that -- but the only real result of doing so would be a somewhat larger blob for the Loop server to store.

> 4. If we convey part of the crypto to client as a hash in the URLs, it means
> URLs will get longer than they currently are. As it was a big concern to PM
> to start with, we should make sure they agree with this.

The owners for this decision are PM, UX, and Privacy/Legal. They've all been involved in this plan, and the design reflects the consensus view of all three decision owners.

Comment 7

[Rémy Hubscher (:natim)]

Great thank you for this update.

Comment 8

[Alexis Metaireau (:alexis)]

Okay, I'll let you update the bug when you have something ready for review then!

Comment 9

[Adam Roach [:abr]]

(In reply to Alexis Metaireau (:alexis) from comment #8)
> Okay, I'll let you update the bug when you have something ready for review
> then!

Okay, I think this is pretty much functionally complete, and ready for general feedback: https://wiki.mozilla.org/Loop/Architecture/Context

Comment 10

[Richard Barnes [:rbarnes]]

Adam asked me to take a look at this.  The room generation and sharing half (upper-right part of the diagram) looks pretty sane to me.  But the sync-between-instances part raises a few major concerns.

(0) This description should be clearer about what happens on the client vs. on the server.  I presume that pretty much all of this is being done client-side.  Likewise, it would help to show how things get fetched and decrypted, in addition to how they get encrypted and stored.

(1) I'm curious why this architecture is so enthusiastic about key derivation, vs. just making ephemeral keys that are wrapped with the long-lived keys.  Key derivation only makes sense when (a) the source key and the derived key are held in different security contexts (otherwise you might as well just use the source key) and (b) the scope/lifetime of the two keys is similar (otherwise, use a fresh key and wrap).

(1.a) So the kB -> kBr derivation might make sense, assuming that kB and kBr are in different security contexts, e.g., an FxA iframe holding kB inside a loop window holding kBr.  If the same code handles both of them, just use kB.

(1.b) The kBr -> kRWrapper derivation almost certainly doesn't make sense.  I expect that they are handled by the same code (loop) and they have dramatically different lifetimes (life of account vs. life of room).  It seems like kRWrapper should just be an ephemeral key, wrapped in kBr.

(2) This might not be the right bug for this comment, but looking at the FxA key derivation architecture fills me with WTF.  Curious why normal PBE is not sufficient for this use case, and all this special stuff was invented.  Also, why are we storing symmetric keys and deriving, as opposed to storing asymmetric keys and wrapping?

If you had asked me to design this, I would have made kB a static DH key pair, with the private half cached in wrapped form (using normal PBE) on the server.  Then the client creating the room key just uses the public key to wrap kR (no need to get the private key), and the client syncing out the room key decrypts the private key and uses it to unwrap kR.  Also, encapsulate the FxA code in an iframe that provides an encryption/decryption oracle to the loop app.   Simpler design, better security properties.

Comment 11

[Chris Karlof [:ckarlof]]

Richard and I had a chat to provide more context around the WTFs.

Summary is: while Adam was considerate in mimicking the key derivation approach used by Sync, we may not want to carry that legacy forward (which favored simplicity and compatibility with the existing Sync API over future use cases and API design elegance, at the time). 

Action item is that Richard is going to provide an alternative design for how relying applications can make use of the user's master key, kB.

Comment 12

[Richard Barnes [:rbarnes]]

So here's what I would propose: 

Instead of providing loop with a long-lived key kBr, FxA exposes an encryption/decryption oracle, where the encryption interface would look something like:

encrypt(data) -> opaque

... from the applications perspective.  FxA would need to define the contents of the opaque thing such that it knows how to decrypt and enforce the right access controls.  I would propose something like:

* For each encryption, generate an ephemeral content key kC
* For each app, derive an app key kApp = KDF(kB, appOrigin)
* For each version of kB, assign a key tag (either random or key hash)
* Assign some global FxA key to ensure that the content is really opaque to the app 
* opaque = keyTag_FxA, E_FxA(keyTag_kB, E_kA(kC, appOrigin), E_kC(content))

Then the decryption oracle can unwrap the bundle, check that it's got the right version of kB (to deal with password reset), then derive kA and decrypt to make sure that it's got the right origin, then decrypt the content and return it.

Using oracles instead of handing out keys makes it clear that FxA is in control of what happens to users' data.  In particular, FxA can revoke an application's access to FxA-protected data at any time, something that's not possible with the derived-key approach.  The encryption oracle also allows the flexibility to do things like application-specific keys or asymmetric keys, without the application having to adapt.

Comment 13

[Adam Roach [:abr]]

(In reply to Chris Karlof [:ckarlof] from comment #11)
> Summary is: while Adam was considerate in mimicking the key derivation
> approach used by Sync, we may not want to carry that legacy forward (which
> favored simplicity and compatibility with the existing Sync API over future
> use cases and API design elegance, at the time).

What implications does this have for timelines?

Comment 14

[Chris Karlof [:ckarlof]]

kA an kC have other special meanings in FxA, so I'm to rewrite Richard's proposal for those that might be confused by that:

> * For each encryption, generate an ephemeral content key kCon
> * For each app, derive an app key kApp = KDF(kB, appOrigin)
> * For each version of kB, assign a key tag (either random or key hash)
> * Assign some global FxA key to ensure that the content is really opaque to the app 
> * opaque = keyTag_FxA, E_FxA(keyTag_kB, E_kApp(kCon, appOrigin), E_kCon(content))

Generally makes sense, and it indeed seems to address Loop's use case.

Some questions:

1) Why is the purpose of encrypting appOrigin, as opposed to say, "appOrigin, E_kApp(kCon)"? It would certainly make debugging easier if it was outside, and decryption should fail if the wrong origin is used in the derivation of kApp. 

2) I don't quite understand the role of the FxA key. How is this global key supposed to be managed and what value does it provide? This is all being done on the client side, so it's not clear to me how the FxA key could be kept secret from anyone. Not knowing any better, for the last step I would have just done something like:

Decryption capability: keyTag_kB, appOrigin, E_kApp(kCon)
Content ciphertext: E_kCon(content)

Opaque: keyTag_kB, appOrigin, E_kApp(kCon), E_kCon(content)

Securely determining appOrigins is a concern. For Loop's case, it may not matter much, since it's in privileged chrome anyway. If we do some iframe-y thing for Web content, we could rely on the postMessage origin.

URLBASE64(keyTag_kB).URLBASE64(appOrigin) has no integrity protection, but I view that part of the capability as purely informational. One could throw a MAC on the whole thing I suppose.

Comment 15

[Chris Karlof [:ckarlof]]

> URLBASE64(keyTag_kB).URLBASE64(appOrigin) has no integrity protection, but I view that part of the capability as purely informational. One could throw a MAC on the whole thing I suppose.

This is some garbage that should be ignored. :)

Comment 16

[Ryan Kelly [:rfkelly]]

> What implications does this have for timelines?

I suspect it just went from "we can squeeze this into Q1" to "we would need to drop something else to get this done"...

Comment 17

[Ryan Kelly [:rfkelly]]

> For each version of kB, assign a key tag (either random or key hash)

This makes sense but may have implications for core FxA auth API, if we need to track and expose this information on the server.

Comment 18

[Chris Karlof [:ckarlof]]

(In reply to Adam Roach [:abr] from comment #13)
> (In reply to Chris Karlof [:ckarlof] from comment #11)
> > Summary is: while Adam was considerate in mimicking the key derivation
> > approach used by Sync, we may not want to carry that legacy forward (which
> > favored simplicity and compatibility with the existing Sync API over future
> > use cases and API design elegance, at the time).
> 
> What implications does this have for timelines?

Good question. Richard's proposal isn't too much of a deviation of what you had before -- conceptually it just shifts some of the concerns from Loop -> FxA. 

There are some UX subtleties around the revocation, and we haven't given them enough treatment. Up until now, Loop hasn't been affected when a user resets her FxA password. This feature will change that and introduces some additional complexity, particularly around surfacing to the user when they need to re-enter their password after a password reset.

E.g., 

* user logs into device 1 to Hello
* user resets her password on device 2 and logs into Hello on device 2
* how does it get detected and surfaced to the user she needs to re-enter her new password on device 1?

The "Loop only" revocation case is no UX picnic, and it can get more complex, because the user may or may not be using Sync on device 1. If she is also a Sync user, we wouldn't necessarily want her to have to re-enter her password twice on device 1, although it wouldn't be the end of the world for a v1. As an example, if you're an Apple user, I dare you to reset your AppleID password and see what happens. :) Ryan Feeley tried it recently, and it's not fun. Everything blows up, and he needed to re-enter his password multiple times on the same device. 

To help with this, we have a new API on Desktop to let chrome level reliers get an OAuth token when the user is logged into Sync already (https://bugzilla.mozilla.org/show_bug.cgi?id=1127638). It still needs some work on error cases, but it may be something Loop could look to use in the future when the user just wants to log into Loop with their already-logged-in Sync identity. It would also help with the double-re-auth problem in that same situation.

Comment 19

[Chris Karlof [:ckarlof]]

(In reply to Ryan Kelly [:rfkelly] from comment #17)
> > For each version of kB, assign a key tag (either random or key hash)
> 
> This makes sense but may have implications for core FxA auth API, if we need
> to track and expose this information on the server.

We already do this via "client-state", i.e., H(kb). I don't think we need any server changes to do something similar (if not the same thing) here.

Comment 20

[Ryan Kelly [:rfkelly]]

Richard, thanks for diving into all this!

> (1.b) The kBr -> kRWrapper derivation almost certainly doesn't make sense.  I expect that they are
> handled by the same code (loop) and they have dramatically different lifetimes (life of account vs.
> life of room).  It seems like kRWrapper should just be an ephemeral key, wrapped in kBr.

Heh, so as the guy who repeatedly suggested this derivation, here's the background context.  I've been explicitly treating kB as "master key material for the account" rather than a concrete encryption key.  This was very much informed by the legacy of key management in Sync.  From that perspective, kBr would be "master key material for this specific relier" and then you'd derive special-purpose keys from there.

The "encryption oracle" model certainly simplifies this away, which is very nice.

> * For each version of kB, assign a key tag (either random or key hash)

To clarify this: is the expectation that we'll just error out if the blob was encrypted with a previous version of kB?

Comment 21

[Ryan Kelly [:rfkelly]]

> We already do this via "client-state", i.e., H(kb). I don't think we need any server changes to do
> something similar (if not the same thing) here.

Indeed.  On my first read I interpreted this to mean that we'd have to somehow track old values of kB, but on further inspection that doesn't seem to be the case.

Comment 22

[Chris Karlof [:ckarlof]]

Richard and I hugged it out and we decided to revert to :abr's original proposal:

https://wiki.mozilla.org/File:Loop-keys.png

:rfkelly will drive this one home on the FxA side, end-to-end. 

First action is to hash out an API proposal by the end of the week. Initials thoughts after chatting with :rfkelly are we change the FxAOAuthClient (https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/FxAccountsOAuthClient.jsm) currently used by Hello:

1) The constructor will take an optional keys:true in the params.
2) When keys:true is specified, in the onComplete callback, the second param will be an object containing two JWKs, representing derived keys from kA and kB, resp. 

:rfkelly will also document how the key derivation works internally. I expect it to work much like :abr already described, and that will be reviewed by Richard's team.

I expect he'll need some Desktop assistance for the changes to FxAOAuthClient (with at least review, if not implementation).

Comment 23

[Romain Testard [:RT]]

Sevaan, can you provide the UX for this meta here please?

Comment 24

[Ryan Kelly [:rfkelly]]

For completeness, noting that I spin off a sub-bug for concrete implementation details on the FxA side: Bug 1132293

Comment 25

[Sevaan Franks [:sevaan]]

Attachment: Adding Context to Conversations

(In reply to Romain Testard [:RT] from comment #23)
> Sevaan, can you provide the UX for this meta here please?

Attached.

Comment 26

[Sevaan Franks [:sevaan]]

Attachment: LinkClickerUI

LinkClicker UI also attached.

Comment 27

[Sevaan Franks [:sevaan]]

Attachment: LinkClickerUI without Thumbnail (Favicon only)

Comment 28

[Sevaan Franks [:sevaan]]

Attachment: Conversation Window without Thumbnail (Favicon only)

Comment 29

[Romain Testard [:RT]]

Mark and Mike, please see Sevaan's updated design with only a favicon rather than a Thumbnail being used.
Is that simpler to deliver this experience given where we are now?
If simpler then let's implement based on that design (it will be better aligned with the design refresh that will happen in Fx40/41), if not then let's stick to the thumbnail/original design.

Comment 30

[Mark Banner (:standard8)]

Jared can probably answer this better as he's written the code that re-uses the exist code. AIUI its using the first image it finds in the page at the moment.

Comment 31

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Sevaan Franks [:sevaan] from comment #28)
> Created attachment 8581669 [details]
> Conversation Window without Thumbnail (Favicon only)

(In reply to Romain Testard [:RT] from comment #29)
> Mark and Mike, please see Sevaan's updated design with only a favicon rather
> than a Thumbnail being used.
> Is that simpler to deliver this experience given where we are now?
> If simpler then let's implement based on that design (it will be better
> aligned with the design refresh that will happen in Fx40/41), if not then
> let's stick to the thumbnail/original design.

We have the thumbnail design implemented now. Was there an issue found that is causing us to move away from the original design? ReadingList is using the thumbnail too by the way.

I'm cautious about us changing designs while also being on tight schedule.

Comment 32

[Sevaan Franks [:sevaan]]

A design change is not needed. The thumbnail just has not come up yet in the redesign (things are working better with just the favicon) and I was flagging it as a timesaver in case work had not yet started on implementing the thumbnail (which can still comes in handy for invitations).

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Romain, could you please set the tracking flag relnote-firefox -> "?"? In the template, could you please also add suggested wording and I can then add this to the FF40 Beta release notes. Thanks!

Comment 34

[Romain Testard [:RT]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Hello users are now able to add a URL to any conversation, at the time of conversation creation (gets the active tab URL) or at any time when in a conversation. This URL can be accessed from within the conversation by the conversation creator or anyone else joining the room.
[Suggested wording]: Firefox Hello: Add a URL to your conversations
[Links (documentation, blog post, etc)]: placeholder is https://support.mozilla.org/fr/kb/invite-web-page-firefox-hello

Fabio did you think of how we'll refer to this feature on external communications? The above looks OK?

Comment 35

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

This was added to Firefox 40.0 Beta release notes.

Comment 36

[Fabio Rios [:frios]]

(In reply to Romain Testard [:RT] from comment #34)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: Hello users are now able to add a URL to any
> conversation, at the time of conversation creation (gets the active tab URL)
> or at any time when in a conversation. This URL can be accessed from within
> the conversation by the conversation creator or anyone else joining the room.
> [Suggested wording]: Firefox Hello: Add a URL to your conversations
> [Links (documentation, blog post, etc)]: placeholder is
> https://support.mozilla.org/fr/kb/invite-web-page-firefox-hello
> 
> Fabio did you think of how we'll refer to this feature on external
> communications? The above looks OK?

Thanks Romain. Not clear to me where the suggested wording is needed? On communications we should include the benefit this provides for both parties. Why does anyone need this? I can help.

Comment 37

[Romain Testard [:RT]]

Fabio, this is for the Firefox 40 release note.

Comment 38

[Mark Banner (:standard8)]

This feature is done & shipped, we should just close this - the remaining follow-up bugs will be tracked by themselves.