Implement RTCPeerConnection identity provider proxy

RESOLVED FIXED in mozilla30

Status

()

Core
WebRTC
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: jedp, Assigned: mt)

Tracking

(Blocks: 1 bug)

22 Branch
mozilla30
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [WebRTC][blocking-webrtc-])

Attachments

(2 attachments, 18 obsolete attachments)

2.25 KB, text/plain
Details
18.67 KB, patch
Details | Diff | Splinter Review
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

Updated

5 years ago
Blocks: 796896

Updated

5 years ago
Whiteboard: [WebRTC][blocking-webrtc-]
rseys and I spoke with ekr last Tuesday about requirements and implementation.
Prior work done in this space: Anant's AuthModule is attached in Bug 781344.
Created attachment 761782 [details] [diff] [review]
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)
Created attachment 761789 [details] [diff] [review]
IdP proxy and communication channel for peer connection (wip)
Attachment #761782 - Attachment is obsolete: true
Created attachment 765141 [details] [diff] [review]
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.)
Attachment #761789 - Attachment is obsolete: true
Created attachment 766092 [details] [diff] [review]
IdP proxy and communication channel for peer connection (wip)

Updated tests
Attachment #765141 - Attachment is obsolete: true
Assignee: nobody → jparsons
Depends on: 778817
Created attachment 774297 [details] [diff] [review]
IdP proxy and communication channel for peer connection

Refactor and yak shave.  Sandbox iframe and remove unneeded capabilities.
Attachment #766092 - Attachment is obsolete: true
Corresponding BrowserID PR: https://github.com/mozilla/browserid/pull/3654
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
Attachment #774297 - Flags: feedback?(dolske)
Depends on: 769856
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.
Flags: needinfo?(ekr)
Desktop only is fine for now.
Flags: needinfo?(ekr)
Depends on: 884573
Blocks: 884573
No longer depends on: 884573

Updated

4 years ago
No longer blocks: 796896
Created attachment 775051 [details] [diff] [review]
IdP proxy and communication channel for peer connection

Rebase
Attachment #774297 - Attachment is obsolete: true
Attachment #774297 - Flags: feedback?(dolske)
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.
Attachment #775051 - Flags: feedback?(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.
(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 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.
Attachment #775051 - Flags: feedback?(dolske)

Updated

4 years ago
Blocks: 901261
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.
Created attachment 786006 [details] [diff] [review]
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?
Attachment #775051 - Attachment is obsolete: true
(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.
Created attachment 786074 [details] [diff] [review]
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'.
Attachment #786006 - Attachment is obsolete: true
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
Flags: needinfo?(ekr)
That URI is fine for now.
Flags: needinfo?(ekr)
Created attachment 787906 [details] [diff] [review]
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.
Attachment #786074 - Attachment is obsolete: true
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.
(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.)
Created attachment 793006 [details] [diff] [review]
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"}, "*");
Attachment #787906 - Attachment is obsolete: true
(Whoops - obviously replace that "*" in the last example with the window's origin; it should just be posting to itself.)
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.
Target Milestone: --- → mozilla27
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.
This spec should really be reviewed by DOM people before we ship it.
Summary: Implement RTCPeerConnection authentication with Persona Identity Providers → Implement RTCPeerConnection identity provider proxy
Created attachment 796351 [details] [diff] [review]
the new approach

The browser module and framescript implementing the design in Comment 29
Attachment #793006 - Attachment is obsolete: true
Created attachment 796354 [details]
bogus.js

An example IdP Proxy script, demonstrating the implementation in Comment 29
Attachment #796351 - Flags: feedback?(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.
Attachment #796351 - Flags: feedback?(ekr) → feedback+
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.
I suggest we just pref on the feature in PC, i.e. in Ryan's code.
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?
Flags: needinfo?(mreavy)

Comment 37

4 years ago
I could make the appropriate pref changes. What should the pref be named?
Hi Jed, I talked to Ekr, and he's going to help you get this landed.
Flags: needinfo?(mreavy)
Assignee: jparsons → ekr
Created attachment 820423 [details] [diff] [review]
New identity work
Created attachment 823486 [details] [diff] [review]
0001-New-identity-work.patch

This fixes some minor problems with ekr's most recent patch.
Created attachment 824857 [details] [diff] [review]
0001-New-identity-work.patch

Minor corrections to ekr's original patch + adding the 'https://' when constructing the well-known URL.
Attachment #823486 - Attachment is obsolete: true
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).
Attachment #824857 - Flags: feedback?(jst)
Created attachment 8333875 [details] [diff] [review]
0001-Bug-878941-Implement-RTCPeerConnection-identity-prov.patch

Updated your patch as discussed.
Attachment #824857 - Attachment is obsolete: true
Attachment #824857 - Flags: feedback?(jst)
Attachment #8333875 - Flags: feedback?(ekr)

Updated

4 years ago
Attachment #8333875 - Flags: feedback?(jst)
Target Milestone: mozilla27 → mozilla28
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...
Attachment #8333875 - Flags: feedback?(jst) → review?(bobbyholley+bmo)
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.
Attachment #8333875 - Flags: review?(bobbyholley+bmo) → feedback+
(Assignee)

Comment 46

4 years ago
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?
Flags: needinfo?(ekr)
(Assignee)

Updated

4 years ago
No longer depends on: 769856
Flags: needinfo?(ekr)
(Assignee)

Comment 47

4 years ago
Created attachment 8342723 [details] [diff] [review]
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.)
Assignee: ekr → martin.thomson
Attachment #8333875 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8333875 - Flags: feedback?(ekr)
Attachment #8342723 - Flags: review?(adam)

Comment 48

4 years ago
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., alice@example.com)

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

@@ +90,5 @@
> + *                          identity:  user identity (e.g., alice@example.com)
> + */
> +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/
Attachment #8342723 - Flags: review?(adam) → review-
(Assignee)

Comment 49

4 years ago
(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.
(Assignee)

Comment 50

4 years ago
Created attachment 8350838 [details] [diff] [review]
Adding IdP proxy code in, with tests this time.

Comment 51

4 years ago
(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?

Updated

4 years ago
Flags: needinfo?(martin.thomson)
(Assignee)

Updated

4 years ago
Attachment #8342723 - Attachment is obsolete: true
(Assignee)

Comment 52

4 years ago
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.
Attachment #8350838 - Flags: review?(adam)
Flags: needinfo?(martin.thomson)
(Assignee)

Updated

4 years ago
Attachment #820423 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #796351 - Attachment is obsolete: true

Comment 53

4 years ago
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.
Attachment #8350838 - Flags: review?(adam) → review+
(Assignee)

Comment 54

4 years ago
Created attachment 8359353 [details] [diff] [review]
0001-Bug-878941-Adding-IdP-proxy-code-in-with-tests-this-.patch

Adding backslash check in, with tests.  Carrying r+ from abr forward.
Attachment #8350838 - Attachment is obsolete: true
Attachment #8359353 - Flags: review+
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
Target Milestone: mozilla28 → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/30e1a793fbf9
Flags: in-testsuite+
Keywords: checkin-needed
Backed out because bug 884573 was backed out and this depends on it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4aadf8ea4e0
No longer blocks: 884573
Depends on: 884573
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
(Assignee)

Comment 58

4 years ago
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.
Blocks: 884573
No longer depends on: 884573
(Assignee)

Updated

4 years ago
Depends on: 972168
(Assignee)

Comment 59

4 years ago
Created attachment 8381015 [details] [diff] [review]
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.
Attachment #8359353 - Attachment is obsolete: true
Attachment #8381015 - Flags: review+
Attachment #8381015 - Flags: feedback?(adam)
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3dc9884b988

Please fix your hg commit information so there aren't quotation marks around your name.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/c3dc9884b988
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(Assignee)

Updated

4 years ago
Attachment #8381015 - Flags: feedback?(adam)
You need to log in before you can comment on or make changes to this bug.