878941 - Implement RTCPeerConnection identity provider proxy

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Some background information describing the scheme can be found here:

http://dev.w3.org/2011/webrtc/editor/webrtc.html#identity
http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-06#section-5.6

Comment 1

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

rseys and I spoke with ekr last Tuesday about requirements and implementation.

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Prior work done in this space: Anant's AuthModule is attached in Bug 781344.

Comment 3

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection (wip)

wip

It's hard-wired to use a local instance of persona
Set the pref 'toolkit.identity.uri' to the persona uri you want

E.g., 'https://webrtc.personatest.org' (which we have)

Comment 4

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection (wip)

Comment 5

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection (wip)

Implements SIGN using the shim, and VERIFY using an xmlhttprequest connection to the verifier.  (We may wish to implement the verifier natively, but this is a start.)

Comment 6

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection (wip)

Updated tests

Comment 7

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection

Refactor and yak shave.  Sandbox iframe and remove unneeded capabilities.

Comment 8

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Corresponding BrowserID PR: https://github.com/mozilla/browserid/pull/3654

Comment 9

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 774297 [details] [diff] [review]
IdP proxy and communication channel for peer connection

Hi, Justin.  This is a functional first pass at a browserid IdP proxy for the RTC PeerConnection.  I would like to get your feedback.

Per the RTC spec[1] there is no visible UI here.  By loading hosted js code into a sandboxed iframe, and injecting a framescript that enables message passing between chrome and the iframe, it provides a means to get assertions for a user already signed in to the present origin[2], and a means to verify assertions[3].

In this implementation, the verifier actually calls out to the Persona verifier service.  We will have a native verifier implementation when Bug 769856 lands.

Many thanks, 
j.

[1] http://dev.w3.org/2011/webrtc/editor/webrtc.html#identity
[2] https://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-06#section-5.6.5.2.2
[3] https://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-06#section-5.6.5.2.3

Comment 10

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

What is the need for supporting rtc+persona on Android?  Currently, identity modules are not packaged on android.  I believe we're just targeting desktop right now, but I wanted to double-check.

Comment 11

[Eric Rescorla (:ekr)]

Desktop only is fine for now.

Comment 12

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection

Rebase

Comment 13

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 775051 [details] [diff] [review]
IdP proxy and communication channel for peer connection

Accidentally canceled the feedback request when I uploaded a rebased patch.  Sorry about that.

Comment 14

[Justin Dolske [:Dolske]]

Comment on attachment 775051 [details] [diff] [review]
IdP proxy and communication channel for peer connection

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

I started to look at this, but it seems like this is duplicating a lot of code from other not-yet-finished/landed patched. What's up with that?

I'm also missing some general context around what's happening here... How/when does this identity stuff get launched by WebRTC? Who is the user identifying to? If there's no UI, how does a user choose not to identify themselves or select an identity to use?

::: browser/base/content/rtc-persona-framescript.js
@@ +99,5 @@
> +// XXX wha?  why both events
> +// dump out these events - should just be on 'load'
> +addEventListener('DOMContentLoaded', function(e) {
> +  log("got DOMContentLoaded");
> +  content.addEventListener('load', function(e) {

This doesn't make any sense. Why not listen for |load| directly? But why wait for |load| at all, since it takes a long time to fire?

DOMWindowCreated may actually be what you want here, for API injection.

::: toolkit/identity/RTCIdentity.jsm
@@ +55,5 @@
> +};
> +
> +HostFrame.prototype = {
> +  /*
> +   * Create a hidden, sandboxed iframe for hosting the IdP's js shim.

Isn't this exactly what toolkit/identity/Sandbox.jsm does? Should use that instead of reimplementing it. :(

@@ +173,5 @@
> +  }
> +};
> +
> +/*
> + * BrowserIDProxy: IdP Proxy module for the browserid protocol.

Doesn't this also significantly overlap with previous Persona work/patches? I remember one of them having a similar Pipe() thing, at least.

Comment 15

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Justin Dolske [:Dolske] from comment #14)
> I started to look at this, but it seems like this is duplicating a lot of
> code from other not-yet-finished/landed patched. What's up with that?

Thanks for taking a look, Justin.  I realize I asked for your feedback out of thin air and should have given more context.

There are similarities between aspects of this and other code we have landed or are still working on.  Because I'm following the RTC working spec here, I wanted to isolate the code both so that we could move quickly for the RTC group and so that we'd have some mobility to adapt should the spec evolve.  But you're right that I went too far in that direction; more sharing and refactoring should have been done across modules, in particular Sandbox.jsm and the HostFrame worker.

> I'm also missing some general context around what's happening here...
> How/when does this identity stuff get launched by WebRTC? Who is the user
> identifying to? If there's no UI, how does a user choose not to identify
> themselves or select an identity to use?

This is my attempt to implement identity part of the RTC Peer Connection spec.  The idea is as follows:

A user arrives at a site (e.g., ekrpokr.com).  This site can help two parties negotiate an RTC connection.  The user must first sign into the site.  The site may use any authentication system (e.g., OpenID, Persona, etc.).  Let's say the user signs in with Persona.  Now the user wants to initiate an RTC peer connection with another party.  The two parties want assurance that they are in fact talking to each other and haven't been MITM'd or spoofed.  So the RTC Offer and Answer will come with identity assertions by each party.  Those assertions will be uniquely bound to the connection channel.

The 'audience' of the rtc assertion is 'rtcweb://peerconnection'.  Embedded into the assertion payload will be the fingerprint of the ephemeral encryption key associated with the channel.

This assertion must be generated without a user interface, since there may not be a user event that could trigger a pop-up to appear.  But this is ok, because the user has already signed into the site with the identity he or she wants to use with the RTC connection.  So we get a silent assertion.  Crucially, the browserid internal api will change the default audience of the assertion from the present origin, https://ekrpokr.com, to rtcweb://peerconnection (again, provided the user is already signed in to ekrpokr).  If the user isn't signed in, a null assertion is returned and the calling app has to decide what to do.  Sending no assertion is valid; it just means that the other party has no assurance of the identity of the person he or she is really talking to.

So with that, the specific answers to your questions are:

> How/when does this identity stuff get launched by WebRTC?

The PeerConnection creates an IDPChannel() and gives it a key fingerprint and message callback function.  Once it receives a READY message, it can use the send() method on the IDPChannel to send SIGN and VERIFY requests to the idp proxy.  This is done when a user is either creating an offer or an answer to another offer.

> Who is the user identifying to?

The user is identifying to the peer connection channel itself.  Not the RP he or she initially signed into, but the special rtcweb://peerconnection audience.  Inclusion of the key fingerprint in the assertion guarantees that it can only be used in the present conversation.

> If there's no UI, how does a user choose not to identify themselves or select an identity to use?

I've had a lot of questions about this myself.  There is some UI in the PeerConnection side: A chrome popup notification confirms that you want to use the given identity.  Chrome UI also confirms the identity of the person you're talking to, once the idp proxy has verified his or her assertion.  Any changes to your identity would have to happen with the PeerConnection client.

There's more detail in the references in Comment 9 above, but I think that's the gist.

> ::: browser/base/content/rtc-persona-framescript.js
> @@ +99,5 @@
> > +// XXX wha?  why both events
> > +// dump out these events - should just be on 'load'
> > +addEventListener('DOMContentLoaded', function(e) {
> > +  log("got DOMContentLoaded");
> > +  content.addEventListener('load', function(e) {
> 
> This doesn't make any sense. Why not listen for |load| directly? But why
> wait for |load| at all, since it takes a long time to fire?
> 
> DOMWindowCreated may actually be what you want here, for API injection.

This has been driving me nuts and I would love to understand it better.  Other people have recommended mozbrowserloadstart and -end, or ditching one or the other of these listeners.  But so far I have still only been able to get this to work with both event listeners.

I'll try DOMWindowCreated right away.

> ::: toolkit/identity/RTCIdentity.jsm
> @@ +55,5 @@
> > +};
> > +
> > +HostFrame.prototype = {
> > +  /*
> > +   * Create a hidden, sandboxed iframe for hosting the IdP's js shim.
> 
> Isn't this exactly what toolkit/identity/Sandbox.jsm does? Should use that
> instead of reimplementing it. :(

Absolutely correct.  I should be using Sandbox.jsm.

> @@ +173,5 @@
> > +  }
> > +};
> > +
> > +/*
> > + * BrowserIDProxy: IdP Proxy module for the browserid protocol.
> 
> Doesn't this also significantly overlap with previous Persona work/patches?
> I remember one of them having a similar Pipe() thing, at least.

The HostFrame and Pipe bits do overlap, though they've been generalized here.  I've heard from others that it would be useful to have the HostFrame in its own module, so I think the course I should take here is to factor them out and revise our other pending patches to use them instead.

The BrowserIDProxy, though similar in spirit, is functionally distinct because it implements the RTC ProxyIDP protocol (the READY, SIGN, and VERIFY messages).  The injected framescript is likewise specific to RTC.  This one feels tricky to generalize without adding another layer of abstraction that would add more complexity, but I should think more about it and see if I can't come up with a way to do so.

Thanks as always for your feedback.  I will revise and re-submit.

Comment 16

[Justin Dolske [:Dolske]]

Comment on attachment 775051 [details] [diff] [review]
IdP proxy and communication channel for peer connection

I'm still pretty confused about who's identifying to what and when UI is shown. But clearing review flag pending a patch updating the other things.

Comment 17

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Having spoken with ekr, he'd prefer not to implement the persona-specific components in chrome at this time, but just enable a general, web-hosted shim for arbitrary identity providers.

So the only requirements in chrome are to create a sandboxed iframe that sources the remote idp-proxy js.[1]  The RTCIdentity module needs to communicate with this iframe, so a framescript loaded through the frameMessageManager should work, I hope.  This way RTCIdentity will be able to send and receive messages with the hosted idp-proxy shim, with the framescript acting as a message channel.

Comment 18

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection (wip)

(wip - broken) I thought this should work for getting a framescript talking to a hosted idp-proxy shim, but I can't find where the idp-proxy's js objects are!  I thought content.wrappedJSObject was the place to find them.

Running this in the scratchpad:

  let Cu = Components.utils;
  Cu.import('resource://gre/modules/identity/RTCIdentity.jsm');

  let signMessage = {
    type: 'SIGN',
    id: 1234,
    origin: "http://calling-service.com",
    message: "DE:AD:BE:EF"
  };

  function messageCallback(message) {
    alert(JSON.stringify(message, null, 2));
    if (message.type === "READY") {
      idpChannel.send(signMessage);
    }
  }

  let idpChannel = new IDPChannel(messageCallback, {
    provider: 'http://people.mozilla.org/~jparsons',
    protocol: 'bogus'
  });


That script loads the idp-proxy js from 

  http://people.mozilla.org/~jparsons/.well-known/idp-proxy/bogus

the 'bogus' idp-proxy script contains this:

  IDPProxy = {};

  IDPProxy.receiveMessage = function receiveMessage(message, callback) {
    callback({"success": true, "message": message});
  }

But from the framescript, I don't see how to get at the IDPProxy object.  Hrm.

I feel like I'm missing the obvious.  What am I doing wrong?

Comment 19

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Jed Parsons [:jedp, :jparsons] from comment #18)
> I feel like I'm missing the obvious.  What am I doing wrong?

Missing the obvious?  And how.  The idp-proxy/bogus file was a raw text/plain file containing javascript.  Replacing that with actual html that sources a separate js file does indeed result in the IDPProxy object being exposed to the framescript as content.wrappedJSObject.IDPProxy.

Comment 20

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection (wip)

Ok works now using the 'bogus' idp, setting the provider to 'http://people.mozilla.org/~jparsons' and protocol to 'bogus'.  That causes the following to be sourced: 'http://people.mozilla.org/~jparsons/.well-known/idp-proxy/bogus'.

Comment 21

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Eric, a question - the spec says that the idp-proxy JS should be communicating via postMessage with an origin of 'rtcweb://peerconnection'.  But it also asks for a URI person to help out making a better URI.[1]  What do you want us to do for this iteration?

(In the attached patch, I've cheated and made the idp-proxy expose a single function, receiveMessage(message, callback), which it uses to communicate.  That's temporary while I figure out how to get postMessage with an arbitrary origin to work in this stack.)

[1] https://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-06#section-5.6.5.2.3

Comment 22

[Eric Rescorla (:ekr)]

That URI is fine for now.

Comment 23

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: IdP proxy and communication channel for peer connection (wip)

wip - please ignore the tests for now - they need to be updated

Expects the .well-known/idp-proxy/protocol file to be an html document; hosts it in a sandboxed iframe and talks to it through postMessage.

I'm not certain how to set the postMessage origin to 'rtcweb://peerconnection'.  The Components.utils.Sandbox looked to me like it might make this possible, and I spent some time working on it, but couldn't get it right.  Thanks in advance for any tips from anyone.

Comment 24

[Eric Rescorla (:ekr)]

Jed,

Why did you transition from the frameMessageManager to postMessage implementation?

Certainly we can explore how to get postMessage() to set the right origin, but
unless we have a good angle on that, it seems like we would be better using
an implementation that already works and then refining from there.

Comment 25

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(In reply to Eric Rescorla (:ekr) from comment #24)

Sorry to be late in replying.  Was on PTO last week and the Friday before.

> Why did you transition from the frameMessageManager to postMessage
> implementation?

We were hoping to simplify the stack.

I've tried putting the sandbox+framescript layer back (the script injected with frameMessageManager), and giving it a special origin, with the hope that postMessaging up to content from the sandbox+framescript would preserve this origin in the message source.

Having spent more time with :khuey and :rseys on this, we've encountered the problem that postMessage from chrome always sets the message source to null.  So content can't learn the source of a message posted up from a document originating in chrome.  (Although the fact that it is null is proof to content that the message did originate in the system.)

But behold: If we emit an event in the idp-proxy script's context from chrome, the event will have the isTrusted flag set to true, which would not be the case for a content event.  This solves the problem of determining whether the event is sent by the system or not.  And it's a cross-browser feature as well.

Does that sound like an acceptable approach?

(:khuey can give more context on the postMessage issues, if necessary.)

Comment 26

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: 878941-rtc-auth-persona

Using custom events on the content document, which when sent from chrome, have the isTrusted flag set to true.

The idp-proxy script then postMessages results to itself; chrome registers a message listener on the idp-proxy content window.

Example idp-proxy script skeleton:

  addEventListener('rtcmessage', receiveMessage, false);

  function receiveMessage(e) {
    if (e.isTrusted) {
      // got a message from the browser
    }
  };

  postMessage({type: "READY"}, "*");

Comment 27

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

(Whoops - obviously replace that "*" in the last example with the window's origin; it should just be posting to itself.)

Comment 28

[Maire Reavy [:mreavy]]

I love to get this in Gecko/Firefox 26, but if not, I need it in early Gecko 27 (1st or 2nd week that Gecko 27 is open).  So I'm setting Gecko 27 as the target milestone.

Comment 29

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

ekr has requested that we deviate from the provisional spec to get something that works in Firefox now.

Content will receive a custom event from chrome, which therefore isTrusted, with a property showing the origin to be rtcweb://peerconnection

Content will communicate back down to chrome via a function injected in a privileged framescript.

Comment 30

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

This spec should really be reviewed by DOM people before we ship it.

Comment 31

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: the new approach

The browser module and framescript implementing the design in Comment 29

Comment 32

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Attachment: bogus.js

An example IdP Proxy script, demonstrating the implementation in Comment 29

Comment 33

[Eric Rescorla (:ekr)]

Comment on attachment 796351 [details] [diff] [review]
the new approach

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

This looks like it's in the right direction.

In terms of landing strategy: this is only going to be accessible from the
PeerConnection and we will probably be changing some of the interfaces
in future, but this is an important piece of strutwork for getting
the rest of the WebRTC/identity work off the ground. So, I'm not overly
concerned about whether this is 100% spec compliant now, as we're only
exposing interfaces to our own prototype code.

Comment 34

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Thanks, Eric.  I'll work on browser tests next, then.  We should probably also have a way to pref this on, with it off by default.

Comment 35

[Eric Rescorla (:ekr)]

I suggest we just pref on the feature in PC, i.e. in Ryan's code.

Comment 36

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Hi, Maire, I am worried this is slipping.  I'm underwater with work for FirefoxOS, Persona, and Firefox Accounts, and I'm having trouble getting this done, too.  Firefox 27 is looming.  Is there anyone who can help?

Comment 37

[Ryan Seys]

I could make the appropriate pref changes. What should the pref be named?

Comment 38

[Maire Reavy [:mreavy]]

Hi Jed, I talked to Ekr, and he's going to help you get this landed.

Comment 39

[Eric Rescorla (:ekr)]

Attachment: New identity work

Comment 40

[Max Jonas Werner [:Makkes]]

Attachment: 0001-New-identity-work.patch

This fixes some minor problems with ekr's most recent patch.

Comment 41

[Max Jonas Werner [:Makkes]]

Attachment: 0001-New-identity-work.patch

Minor corrections to ekr's original patch + adding the 'https://' when constructing the well-known URL.

Comment 42

[Eric Rescorla (:ekr)]

Comment on attachment 824857 [details] [diff] [review]
0001-New-identity-work.patch

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

JST,

Here is the patch we discussed. Sorry it took so long but the MessageChannel
stuff turned out to be tricky to get right.

Can you take a look and see if you think this is correct and what needs to
be reviewed by specialists (e.g., bholley).

Comment 43

[Max Jonas Werner [:Makkes]]

Attachment: 0001-Bug-878941-Implement-RTCPeerConnection-identity-prov.patch

Updated your patch as discussed.

Comment 44

[Johnny Stenback (:jst)]

Comment on attachment 8333875 [details] [diff] [review]
0001-Bug-878941-Implement-RTCPeerConnection-identity-prov.patch

I talked to bholley about this one, he's going to review this bug, likely next week. Sorry took so long, lots going on this week...

Comment 45

[Bobby Holley (:bholley)]

Comment on attachment 8333875 [details] [diff] [review]
0001-Bug-878941-Implement-RTCPeerConnection-identity-prov.patch

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

I don't think it makes sense for me to review the whole patch, but I'm happy to give feedback on the security bits.

::: toolkit/identity/RTCIdentity.jsm
@@ +43,5 @@
> +   *        (function)      invoked with iframe as argument on success
> +   */
> +  getIframe: function hostFrame_getIframe(aCallback) {
> +    if (this.sandbox) {
> +      return aCallback(

Looks like we never invoke aCallback in the success case. Is it actually necessary?

@@ +47,5 @@
> +      return aCallback(
> +        new Error("Can only get iframe once with HostFrame helper"));
> +    }
> +
> +    this.sandbox = new Sandbox(this.source, function(aSandbox) {

I assume this is the part that you want me to look at.

@@ +49,5 @@
> +    }
> +
> +    this.sandbox = new Sandbox(this.source, function(aSandbox) {
> +        // Inject a message channel into the subframe.
> +        this.messagechannel = new aSandbox._frame.contentWindow.MessageChannel();

Good - this makes sure to invoke the constructor over Xray.

@@ +51,5 @@
> +    this.sandbox = new Sandbox(this.source, function(aSandbox) {
> +        // Inject a message channel into the subframe.
> +        this.messagechannel = new aSandbox._frame.contentWindow.MessageChannel();
> +        Object.defineProperty(
> +            aSandbox._frame.contentWindow.wrappedJSObject,

So, here's where it gets a bit sketchy. You're guaranteed to avoid invoking any content code here, because you're doing defineProperty (which won't invoke setters) on an object that you know is not a proxy (which could trap |defineProperty|).

However, this can still fail, if the content has defined a non-configurable property of the same name on the global. If this is at all a problem, you'll need to wrap this in a try/catch and handle the failure. If there are no correctness issues with it failing, then please write a comment explaining the situation and describing why it doesn't matter.

Comment 46

[Martin Thomson [:mt:]]

If we're going to land the identity stuff soon, I don't think that we need to depend on bug 769856 still.  We can live with the stubbed error handling; we probably should catch (and ignore) the defineProperty exception.  Other than that, what's holding this up?

Comment 47

[Martin Thomson [:mt:]]

Attachment: Implement identity provider proxy for WebRTC

:abr, I'm giving this one to you.  I think that it's ready.

You might like to evaluate it in the context of bug 884573, which (in the patch I'm about to upload), includes measures for dealing with the lack of error handling in this one.

(One day I'll work out how to manage this development process so that I don't have to edit patches by hand.)

Comment 48

[Adam Roach [:abr]]

Comment on attachment 8342723 [details] [diff] [review]
Implement identity provider proxy for WebRTC

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

This patch overall looks pretty rough still. The documentation doesn't match the implementation, errors are unhandled and unreported, resources are apparently leaked, and there appear to be several bits of crufty, abandoned design dead-ends left lying around.

Also, I'm concerned about the security properties here.

http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07#section-5.7.4.1 requires that the origin of the messages sent to the IdP proxy code be associated with a non-content-javascript-generatable URI scheme ("rtcweb://...") to prevent certain MITM attacks. I don't see support for that in this patch. Minimally, we need a TODO and a bug number, and to block preffing on until that bug is resolved.

http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07#section-5.7.4.3 requires that the user be queried before allowing the IdP-provided identity to become visible to the content javascript. There does not appear to be support for this behavior. Perhaps it belongs in PeerConnection.js as part of Bug 884573; but it needs to be checked somewhere. As above: minimally, we need a TODO and a bug number, and to block preffing on until that bug is resolved.


General formatting issues:
RTCIdentity.jsm:95: Space between 'if' and parenthesis
test_rtcidentity_basic.html:16: trailing whitespace
test_rtcidentity_basic.html:49: Space between 'if' and parenthesis
test_rtcidentity_basic.html:82: Space between 'if' and parenthesis


Line-length nits which you may ignore, but I'd be happier if you didn't:
RTCIdentity.jsm:15: Line is 82 characters long; please wrap to 80
RTCIdentity.jsm:18: Line is 81 characters long; please wrap to 80
RTCIdentity.jsm:53: Line is 81 characters long; please wrap to 80
RTCIdentity.jsm:99: Line is 91 characters long; please wrap to 80
test_rtcidentity_basic.html:3: Line is 115 characters long; please wrap to 80
test_rtcidentity_basic.html:4: Line is 115 characters long; please wrap to 80
test_rtcidentity_basic.html:91: Line is 90 characters long; please wrap to 80

::: toolkit/identity/RTCIdentity.jsm
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";
> +
> +this.EXPORTED_SYMBOLS = ["IDPChannel", "HostFrame"];

Why are we exporting HostFrame?

@@ +16,5 @@
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Sandbox",
> +                                  "resource://gre/modules/identity/Sandbox.jsm");
> +
> +function log(...aMessageArgs) {

I think I was expecting this to be used somewhere. I would suggest we either add logging to the operations below, or remove this function and the "Logger" import above.

@@ +39,5 @@
> +  /*
> +   * Create a hidden, sandboxed iframe for hosting the IdP's js shim.
> +   *
> +   * @param aCallback
> +   *        (function)      invoked with iframe as argument on success

I don't think that's right: it appears to be used only in the failure case.

@@ +41,5 @@
> +   *
> +   * @param aCallback
> +   *        (function)      invoked with iframe as argument on success
> +   */
> +  getIframe: function hostFrame_getIframe(aCallback) {

This method doesn't appear to do what its name implies. I mean, sure, it goes out and creates a new Sandbox, which one could term "getting" an "iframe," but it's still a bit surprising.

Perhaps "init"?

@@ +44,5 @@
> +   */
> +  getIframe: function hostFrame_getIframe(aCallback) {
> +    if (this.sandbox) {
> +      return aCallback(
> +        new Error("Can only get iframe once with HostFrame helper"));

Log the condition?

Since this is executed synchronously, it seems we should throw an exception rather than using a callback model.

@@ +47,5 @@
> +      return aCallback(
> +        new Error("Can only get iframe once with HostFrame helper"));
> +    }
> +
> +    this.sandbox = new Sandbox(this.source, function(aSandbox) {

One of the caveats in Sandbox's documentation is:

> * You must call free() when you are finished with the sandbox to explicitly
> * free up all associated resources.

Where does this happen?

@@ +58,5 @@
> +                {
> +                  value: this.messagechannel.port2
> +                }
> +            );
> +        } catch (e) {} // if this fails, so will the IdP, that's OK

Log the failure?

@@ +86,5 @@
> + *                          options:
> + *
> + *                          provider:  identity provider (e.g., example.com)
> + *                          protocol:  identity protocol (e.g., browserid)
> + *                          identity:  user identity (e.g., [e-mail])

I don't see where "identity" is used below. I suspect it doesn't belong here.

@@ +90,5 @@
> + *                          identity:  user identity (e.g., [e-mail])
> + */
> +function IDPChannel(aMessageCallback, aOptions) {
> +  this.receiveResponse = aMessageCallback;
> +  this.provider = aOptions.provider;

Given that "provider" is required to continue, I think you need to check whether it's present, and throw an error if it's not.

@@ +108,5 @@
> +   * message channel down to the frame.
> +   */
> +  init: function idpChannel_init() {
> +    this.hostFrame = new HostFrame(this.well_known, this.receiveResponse);
> +    this.hostFrame.getIframe(function(x) {});

x?

::: toolkit/identity/tests/mochitest/test_rtcidentity_basic.html
@@ +43,5 @@
> +        provider: "example.com",
> +        protocol: proto
> +      });
> +    } catch(e) {
> +      ok(true, "Evil protocol '" + proto + "' raises execption");

s/execption/exception/

Comment 49

[Martin Thomson [:mt:]]

(In reply to Adam Roach [:abr] from comment #48)
> This patch overall looks pretty rough still. 

Land that firmly at the feet of :ekr.  The spec is out of date.  I'll ask him for the pen and see how things go.

> Also, I'm concerned about the security properties here.

I think that I can address most of those.

> http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07#section-5.7.4.
> 1 requires that the origin of the messages sent to the IdP proxy code be
> associated with a non-content-javascript-generatable URI scheme
> ("rtcweb://...") to prevent certain MITM attacks. I don't see support for
> that in this patch. Minimally, we need a TODO and a bug number, and to block
> preffing on until that bug is resolved.

This uses a message channel, which has the properties that are required without the need for origin checking.  Primarily because the browser demonstrates it's god-like powers by placing a variable on the window object.

http://www.w3.org/TR/webmessaging/#message-channels

> http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07#section-5.7.4.
> 3 requires that the user be queried before allowing the IdP-provided
> identity to become visible to the content javascript. There does not appear
> to be support for this behavior. Perhaps it belongs in PeerConnection.js as
> part of Bug 884573; but it needs to be checked somewhere. As above:
> minimally, we need a TODO and a bug number, and to block preffing on until
> that bug is resolved.

The origin property is provided as part of the message (See Bug 884573).  That should be sufficient.

One thing that we really need to avoid is having user interaction during assertion gathering.  Even if it were possible (our implementation doesn't allow it: the sandbox prevents popups entirely), it still wouldn't be advisable because then there is no good way to determine the difference between "IdP talking to user" and "IdP broken".  This is already fragile enough, as we've discovered.

Basically, I have to try to wrestle the pen from ekr so that I can fix this and the other many issues with the security-arch document.

> Line-length nits which you may ignore, but I'd be happier if you didn't:
> RTCIdentity.jsm:15: Line is 82 characters long; please wrap to 80

Hmm, this is clearly a silly rule when lines like this blow the budget so easily.

> One of the caveats in Sandbox's documentation is:
> 
> > * You must call free() when you are finished with the sandbox to explicitly
> > * free up all associated resources.
> 
> Where does this happen?

Noted.  I've rigged up a close, do you know how to hook something into a destructor in JavaScript?  I can't guarantee that it's going to be called though.

Also... I've taken the liberty of completely redoing the code.  I've actually fixed the tests so that they run.  I've moved the files out of toolkit/identity and into dom/media.  It's going to be great.

In other words, this is going to need a whole new review.

Comment 50

[Martin Thomson [:mt:]]

Attachment: Adding IdP proxy code in, with tests this time.

Comment 51

[Adam Roach [:abr]]

(In reply to Martin Thomson [:mt] from comment #50)
> Created attachment 8350838 [details] [diff] [review]
> Adding IdP proxy code in, with tests this time.


Did you mean to [r?] me on this?

Comment 52

[Martin Thomson [:mt:]]

Comment on attachment 8350838 [details] [diff] [review]
Adding IdP proxy code in, with tests this time.

Yes, indeed I did.  I was going to tag :ekr, but he is tainted.  Thanks for the prod.

Comment 53

[Adam Roach [:abr]]

Comment on attachment 8350838 [details] [diff] [review]
Adding IdP proxy code in, with tests this time.

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

This looks very good. Thanks.

IdpProxy.jsm:10: trailing whitespace
IdpProxy.jsm:11: trailing whitespace
IdpProxy.jsm:95: trailing whitespace
IdpProxy.jsm:167: trailing whitespace
idp-proxy.js:37: trailing whitespace

::: dom/media/IdpProxy.jsm
@@ +41,5 @@
> +   *                (function) invoked when this completes, with an error
> +   *                argument if there is a problem, no argument if everything is
> +   *                ok
> +   */
> +  open: function IdpChannel_open(callback) {

Please remove the extra function name, here and elsewhere.

@@ +134,5 @@
> +
> +/**
> + * Checks that the IdP protocol is sane.  In particular, we don't want someone
> + * adding relative paths (e.g., "../../myuri"), which could be used to move
> + * outside of /.well-known/ and into space that they control.

Due to issues like this one, I think we ought to check for backslashes as well: http://bytes.com/topic/html-css/answers/500069-rfc3986-backslash-uri-urls

I recognize that there are other filepath separators (colons on OS 9, for example), but I'd argue that a demonstrable bug on a major operating system deserves special attention.

@@ +218,5 @@
> +        this.send(p.message, p.callback);
> +      }, this);
> +      this.pending = [];
> +    }
> +    else if (this.tracking[message.id]) {

Closing brace and else on the same line (cf. https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style#Control_Structures)

@@ +222,5 @@
> +    else if (this.tracking[message.id]) {
> +      var callback = this.tracking[message.id];
> +      delete this.tracking[message.id];
> +      callback(message);
> +    }

else {
  console.warn ("Received invalid message ID from IDP Proxy: " + message.id);
}

::: dom/media/tests/mochitest/idp-proxy.js
@@ +5,5 @@
> +    this.domain = window.location.host;
> +    // so rather than create a million different IdP configurations and litter
> +    // the world with files all containing near-identical code, let's use the
> +    // hash/URL fragment as a way of generating instructions for the IdP, since
> +    // mochitest seems to be able to intercept every domain name in existence

Clever. I like it.

Comment 54

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-878941-Adding-IdP-proxy-code-in-with-tests-this-.patch

Adding backslash check in, with tests.  Carrying r+ from abr forward.

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30e1a793fbf9

Comment 56

[Ryan VanderMeulen [:RyanVM]]

Backed out because bug 884573 was backed out and this depends on it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4aadf8ea4e0

Comment 57

[Ed Morley [:emorley]]

Also:
115220 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_idpproxy.html | Test timed out.
https://tbpl.mozilla.org/php/getParsedLog.php?id=32981076&tree=Mozilla-Inbound

Comment 58

[Martin Thomson [:mt:]]

Restoring dependencies.  I've no idea why this was applied after bug 884573 because that's the complete opposite of what my revision history says.  No small wonder the build caught fire.

Comment 59

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-878941-Adding-IdP-proxy-for-WebRTC.patch

Un-bitrotted.  Carried r+ from abr.  Going to attempt to land without tests running for Android and B2G.  These require systemic changes.  Actively working on unblocking both Bug 975149 and Bug 975144.

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c3dc9884b988

Please fix your hg commit information so there aren't quotation marks around your name.

Comment 61

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c3dc9884b988