884573 - PeerConnection.js persona integration

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Ben Brittain (:bbrittain)]

Implement the Identity provider API for webRTC.

Comment 1

[Ryan Seys]

Attachment: PeerConnection.js changes for Persona auth

In progress, verification not complete.

Comment 2

[Ryan Seys]

Attachment: PeerConnection.js changes for IDP Authentication

Comment 3

[Maire Reavy [:mreavy]]

I'd love to have this in Gecko 27, but I need it for Gecko 28 (early in the cycle -- 1st or 2nd week after Gecko 28 opens).

Comment 4

[Ryan Seys]

Attachment: PeerConnection.js changes for IDP Authentication (slight changes from last patch)

Slight changes to patch, nothing game-changing.

Comment 5

[Maire Reavy [:mreavy]]

I'm reassigning this to ekr since I believe Ryan is back at school (or very soon will be).

Comment 6

[Max Jonas Werner [:Makkes]]

Attachment: 0001-WIP-PeerConnection.js-changes-for-IdP-support.patch

This is based on the current work done in #878941. There are some open questions, though, with regard to the spec that I included as TODO.

Comment 7

[Eric Rescorla (:ekr)]

Comment on attachment 820537 [details] [diff] [review]
0001-WIP-PeerConnection.js-changes-for-IdP-support.patch

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

This looks like it is on the right track.

Can you write some mochitests for it and address the issues below

::: dom/media/PeerConnection.js
@@ +935,5 @@
> +
> +PeerConnectionIdP.prototype = {
> +  setIdentityProvider: function(provider, protocol, username) {
> +    // TODO(max): currently only one IdP is supported
> +    this._idpchannel = new IDPChannel(this.onMessage.bind(this), {provider: "https://" + provider, protocol: protocol, identity: username});

Since IDPChannel constructs the rest of the URL, I suggest we let it add the https://

@@ +944,5 @@
> +    let matches = sdp.match(pattern);
> +    if(!matches) {
> +      return null;
> +    }
> +    return matches[1].trim();

This is going to be a problem because it doesn't track the m-lines. You need to first
break it up by m-line.

@@ +981,5 @@
> +        sdp += "a=identity: " + encodedAssertion;
> +      }
> +      callback(sdp);
> +    };
> +    this.getIdentityAssertion(onAssertion);

Same problem here about where the identity goes.

I propose for now we put it before the first m-line and make it apply
to the whole SDP.

@@ +1035,5 @@
> +      case "READY":
> +        this._ready = true;
> +        while(callback = this._waitingForProvider.shift()) {
> +          // request assertion again now that IdP proxy is ready
> +          this.getIdentityAssertion(callback);

Do we need to condition this on if someone gets an identity?

@@ +1045,5 @@
> +      case "SUCCESS":
> +        callback = this._pending[message.id];
> +        delete this._pending[message.id];
> +        callback(message.message);
> +        //TODO(max): Set _dompc.peerIdentity here?

yeah, and render some UI.

@@ +1093,5 @@
> +      this.callCB(this._dompc._onCreateOfferSuccess,
> +                  new this._dompc._win.mozRTCSessionDescription({ type: "offer",
> +                                                                  sdp: sdp }));
> +      this._dompc._executeNext();
> +    }.bind(this));

What guarantees ordering here and below?

Comment 8

[Max Jonas Werner [:Makkes]]

(In reply to Eric Rescorla (:ekr) from comment #7)
> Comment on attachment 820537 [details] [diff] [review]
> 0001-WIP-PeerConnection.js-changes-for-IdP-support.patch
> 
> Review of attachment 820537 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks like it is on the right track.
> 
> Can you write some mochitests for it and address the issues below

Yes.

> ::: dom/media/PeerConnection.js
> @@ +935,5 @@
> > +
> > +PeerConnectionIdP.prototype = {
> > +  setIdentityProvider: function(provider, protocol, username) {
> > +    // TODO(max): currently only one IdP is supported
> > +    this._idpchannel = new IDPChannel(this.onMessage.bind(this), {provider: "https://" + provider, protocol: protocol, identity: username});
> 
> Since IDPChannel constructs the rest of the URL, I suggest we let it add the
> https://

Okay.

> @@ +944,5 @@
> > +    let matches = sdp.match(pattern);
> > +    if(!matches) {
> > +      return null;
> > +    }
> > +    return matches[1].trim();
> 
> This is going to be a problem because it doesn't track the m-lines. You need
> to first
> break it up by m-line.

I wrote this function to get the fingerprint or identity attribute from the SDP. These are "global" attributes equal for every m-line, as I understand it. So even if an SDP has multiple fingerprint attributes these should all be the same. Is this assumption correct? If so, this function does what we need here, I think.

> @@ +981,5 @@
> > +        sdp += "a=identity: " + encodedAssertion;
> > +      }
> > +      callback(sdp);
> > +    };
> > +    this.getIdentityAssertion(onAssertion);
> 
> Same problem here about where the identity goes.
> 
> I propose for now we put it before the first m-line and make it apply
> to the whole SDP.

Okay.

> @@ +1035,5 @@
> > +      case "READY":
> > +        this._ready = true;
> > +        while(callback = this._waitingForProvider.shift()) {
> > +          // request assertion again now that IdP proxy is ready
> > +          this.getIdentityAssertion(callback);
> 
> Do we need to condition this on if someone gets an identity?

Can you clarify? What I did here is just defer all calls to getIdentityAssertion() until the IdP proxy says it's ready.

> @@ +1045,5 @@
> > +      case "SUCCESS":
> > +        callback = this._pending[message.id];
> > +        delete this._pending[message.id];
> > +        callback(message.message);
> > +        //TODO(max): Set _dompc.peerIdentity here?
> 
> yeah, and render some UI.

Good.

> @@ +1093,5 @@
> > +      this.callCB(this._dompc._onCreateOfferSuccess,
> > +                  new this._dompc._win.mozRTCSessionDescription({ type: "offer",
> > +                                                                  sdp: sdp }));
> > +      this._dompc._executeNext();
> > +    }.bind(this));
> 
> What guarantees ordering here and below?

As discussed in IRC the call to callCB() is deferred until after an identity assertion has been obtained from the IdP proxy. So ordering is guaranteed by the callback. This also works with no IdP because then getIdentityAssertion() just returns immediately.

I'll buff this up, add tests and missing functionality and re-submit for review.

Comment 9

[Max Jonas Werner [:Makkes]]

Attachment: 0001-Bug-878941-Incorporate-IdP-support-into-PeerConnecti.patch

Comment 10

[Eric Rescorla (:ekr)]

Comment on attachment 826816 [details] [diff] [review]
0001-Bug-878941-Incorporate-IdP-support-into-PeerConnecti.patch

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

This looks pretty good to me, but I think we should probably see another
round.

I'm also forwarding to JIB for feedback on some of the PC.js and WebIDL stuff.

::: dom/media/PeerConnection.js
@@ +199,4 @@
>    }
>  };
>  
> +function RTCIdentityAssertion(idp, name) {

Let's make all this functionality contingent on some pref for now.

@@ +622,4 @@
>              "Invalid type " + desc.type + " provided to setRemoteDescription");
>      }
>  
> +    this._idp.verifyIdentityFromSDP(desc.sdp, function(message) {

Do we really want "verify"? This is like "I have it" right?

@@ +971,5 @@
> +  /**
> +   * Asks the IdP proxy to verify an a=identity line from the given SDP.
> +   * If the verification succeeds callback is called with the message from the
> +   * IdP proxy as parameter, else (verification failed OR no a=identity line in
> +   * SDP at all) null is passed to callback.

What if it fails?

@@ +986,5 @@
> +    let id = this._currentId++;
> +    let origin = this._getOrigin();
> +    this._pending[id] = callback;
> +
> +    //TODO(max): Depending on the domain we need to load another IdP proxy here

It seems like a real problem to not be able to do cross-domain. Can you fix before we land.

@@ +1040,5 @@
> +    let origin = this._getOrigin();
> +    let [algorithm, digest] = this._dompc._getPC().fingerprint.split(" ");
> +    let id = this._currentId++;
> +    this._pending[id] = callback;
> +    // TODO(max): Cache the assertion

This caching seems like something we should do now.

@@ +1068,5 @@
> +          delayed.func.apply(this, delayed.args);
> +        }
> +        break;
> +      case "ERROR":
> +        this._dompc.reportError("Received error from IdP proxy: " + message.error);

Does this mean that nobody is ever notified on error?

::: dom/media/tests/mochitest/idp-proxy.js
@@ +1,1 @@
> +function IDPProxy() {

I am trying to use the term proxy to indicate the code in the browser.

Let's use "IDP.js" or soemthing

::: dom/media/tests/mochitest/test_getIdentityAssertion.html
@@ +50,5 @@
> +      test.pcLocal._pc.getIdentityAssertion();
> +    }
> +  ]
> +  ]);
> +  test.run();

Should you cut out a lot of the other offer/answer stuff?

Comment 11

[Eric Rescorla (:ekr)]

Comment on attachment 826816 [details] [diff] [review]
0001-Bug-878941-Incorporate-IdP-support-into-PeerConnecti.patch

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

::: dom/media/PeerConnection.js
@@ +992,5 @@
> +      type: "VERIFY",
> +      id: id,
> +      origin: origin,
> +      message: assertion.assertion
> +    });

We still need to verify that the fingerprint in the assertion matches the one in the SDP.

For now, let's verify that there is exactly one fingerprint (or at least that they are all the same).

Comment 12

[Max Jonas Werner [:Makkes]]

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

[...]
> > +function RTCIdentityAssertion(idp, name) {
> 
> Let's make all this functionality contingent on some pref for now.

I'll add one.

> @@ +622,4 @@
> >              "Invalid type " + desc.type + " provided to setRemoteDescription");
> >      }
> >  
> > +    this._idp.verifyIdentityFromSDP(desc.sdp, function(message) {
> 
> Do we really want "verify"? This is like "I have it" right?

When setRemoteDescription() is called there are currently 3 possibilities w.r.t. IdP stuff:

1. No IdP has been configured (e.g. via setIdentityProvider())
   In this case PC behaves exactly like before and just sets the remote desc.

2. An IdP has been set but the SDP doesn't contain an assertion
   In this case the user is notified about that ("Identity of your WebRTC peer is not verified")

3. An IdP has been set and the SDP contains an assertion
   Ask the IdP proxy to "VERIFY" the assertion. Depending on the outcome an appropriate notification is displayed.

Perhaps we need to split option 1 into "No IdP has been configured and the SDP does not carry an assertion" and "No IdP has been configured and the SDP carries an assertion". What do you think?

> @@ +971,5 @@
> > +  /**
> > +   * Asks the IdP proxy to verify an a=identity line from the given SDP.
> > +   * If the verification succeeds callback is called with the message from the
> > +   * IdP proxy as parameter, else (verification failed OR no a=identity line in
> > +   * SDP at all) null is passed to callback.
> 
> What if it fails?

When verification fails the callback should be called with null as parameter. Looking through my code I saw that on "ERROR" from the IdP proxy JS we currently don't do this. I'll add the code.

> @@ +986,5 @@
> > +    let id = this._currentId++;
> > +    let origin = this._getOrigin();
> > +    this._pending[id] = callback;
> > +
> > +    //TODO(max): Depending on the domain we need to load another IdP proxy here
> 
> It seems like a real problem to not be able to do cross-domain. Can you fix
> before we land.

This could lead to multiple PeerConnectionIdP objects or at least multiple IDPChannels around somehow, e.g. when the application calls 'setIdentityProvider' and then an SDP arrives with another IdP's domain. I'll figure out a way to handle this but I assume I'll have to rework some of the code to achieve this.

> @@ +1040,5 @@
> > +    let origin = this._getOrigin();
> > +    let [algorithm, digest] = this._dompc._getPC().fingerprint.split(" ");
> > +    let id = this._currentId++;
> > +    this._pending[id] = callback;
> > +    // TODO(max): Cache the assertion
> 
> This caching seems like something we should do now.

Ok.

> @@ +1068,5 @@
> > +          delayed.func.apply(this, delayed.args);
> > +        }
> > +        break;
> > +      case "ERROR":
> > +        this._dompc.reportError("Received error from IdP proxy: " + message.error);
> 
> Does this mean that nobody is ever notified on error?

Yeah, see above. I'll add code to invoke the appropriate callback here.

> ::: dom/media/tests/mochitest/idp-proxy.js
> @@ +1,1 @@
> > +function IDPProxy() {
> 
> I am trying to use the term proxy to indicate the code in the browser.
> 
> Let's use "IDP.js" or soemthing

Ok.

> ::: dom/media/tests/mochitest/test_getIdentityAssertion.html
> @@ +50,5 @@
> > +      test.pcLocal._pc.getIdentityAssertion();
> > +    }
> > +  ]
> > +  ]);
> > +  test.run();
> 
> Should you cut out a lot of the other offer/answer stuff?

I thought it would be wise to leave it in there so that we can be sure the IdP code doesn't break the other stuff?!

Comment 13

[Max Jonas Werner [:Makkes]]

(In reply to Eric Rescorla (:ekr) from comment #11)
> Comment on attachment 826816 [details] [diff] [review]
> 0001-Bug-878941-Incorporate-IdP-support-into-PeerConnecti.patch
> 
> Review of attachment 826816 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/media/PeerConnection.js
> @@ +992,5 @@
> > +      type: "VERIFY",
> > +      id: id,
> > +      origin: origin,
> > +      message: assertion.assertion
> > +    });
> 
> We still need to verify that the fingerprint in the assertion matches the
> one in the SDP.
> 
> For now, let's verify that there is exactly one fingerprint (or at least
> that they are all the same).

I tried my best to find the requirement that the assertion from the IdP's proxy JS needs to include the fingerprint but couldn't find it in http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07#section-5.6

5.6.5.2.2 says that a response must carry an "idp" field and an "assertion" field and that the "assertion" field's value is only interpretable by the IdP or the IdP's proxy JS. Perhaps 5.6.5.2.2 needs to be re-phrased so that it says that the "assertion" must carry the initial message to be signed?

5.6.5.2.3 says that a response to "VERIFY" must return an object containing a "contents" field that contains the string provided by the AP in the "SIGN" message. I think the missing part is a sentence in 5.6.5.2.2 that states that this field must be included in an answer to "SIGN", otherwise we'd have no way to determine what was in the "SIGN" message, right?

Comment 14

[Eric Rescorla (:ekr)]

That's in S 5.6.4.1 which describes the "input to the assertion generation process" (though I see it doesn't seem to
say it is "message").

From the browser's perspective, he supplies the "message" field to the IDP assertion generation process and it get reproduced by the IDP to the RP on the other end as output of the assertion verification process. That could mean that the message field is in the assertion, but it could also be that the assertion is just a pointer to a stored value somewhere. That's the IDP's business.

Comment 15

[Eric Rescorla (:ekr)]

(In reply to Max Jonas Werner from comment #12)

> > @@ +622,4 @@
> > >              "Invalid type " + desc.type + " provided to setRemoteDescription");
> > >      }
> > >  
> > > +    this._idp.verifyIdentityFromSDP(desc.sdp, function(message) {
> > 
> > Do we really want "verify"? This is like "I have it" right?


Never mind, I just had a thinko here...


> When setRemoteDescription() is called there are currently 3 possibilities
> w.r.t. IdP stuff:
> 
> 1. No IdP has been configured (e.g. via setIdentityProvider())
>    In this case PC behaves exactly like before and just sets the remote desc.
> 
> 2. An IdP has been set but the SDP doesn't contain an assertion
>    In this case the user is notified about that ("Identity of your WebRTC
> peer is not verified")
> 
> 3. An IdP has been set and the SDP contains an assertion
>    Ask the IdP proxy to "VERIFY" the assertion. Depending on the outcome an
> appropriate notification is displayed.
> 
> Perhaps we need to split option 1 into "No IdP has been configured and the
> SDP does not carry an assertion" and "No IdP has been configured and the SDP
> carries an assertion". What do you think?

Well, we need to process an assertion whether or not an IDP has been
configured, because it's the AP's IDP that is important here, not ours.


> > @@ +986,5 @@
> > > +    let id = this._currentId++;
> > > +    let origin = this._getOrigin();
> > > +    this._pending[id] = callback;
> > > +
> > > +    //TODO(max): Depending on the domain we need to load another IdP proxy here
> > 
> > It seems like a real problem to not be able to do cross-domain. Can you fix
> > before we land.
> 
> This could lead to multiple PeerConnectionIdP objects or at least multiple
> IDPChannels around somehow, e.g. when the application calls
> 'setIdentityProvider' and then an SDP arrives with another IdP's domain.
> I'll figure out a way to handle this but I assume I'll have to rework some
> of the code to achieve this.


I think you just need an _localIdp and _remoteIdp and appropriate
substitutions.


> > ::: dom/media/tests/mochitest/test_getIdentityAssertion.html
> > @@ +50,5 @@
> > > +      test.pcLocal._pc.getIdentityAssertion();
> > > +    }
> > > +  ]
> > > +  ]);
> > > +  test.run();
> > 
> > Should you cut out a lot of the other offer/answer stuff?
> 
> I thought it would be wise to leave it in there so that we can be sure the
> IdP code doesn't break the other stuff?!

Good thinking.

Comment 16

[Eric Rescorla (:ekr)]

Comment on attachment 794717 [details] [diff] [review]
PeerConnection.js changes for IDP Authentication (slight changes from last patch)

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

Taking Jed off the review list. He has moved on.

Comment 17

[Max Jonas Werner [:Makkes]]

Attachment: 0001-Bug-878941-Incorporate-IdP-support-into-PeerConnecti.patch

Incorporated all feedback and recent discussion. Only thing missing is caching the assertions. The question is whether this is really necessary since an assertion is mostly only needed when creating offers which is called once on a PC. If we decide to cache local assertions we should decide on a good key (DTLS fingerprint?).

Comment 18

[Max Jonas Werner [:Makkes]]

Attachment: 0001-Bug-878941-Incorporate-IdP-support-into-PeerConnecti.patch

Comment 19

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8333805 [details] [diff] [review]
0001-Bug-878941-Incorporate-IdP-support-into-PeerConnecti.patch

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

I couldn't get the JS constract to load, so I wasn't able to step through the JS, but overall it looked good to me. Sorry for sitting on it. Let me post what I have. Mere nits really.

::: dom/media/PeerConnection.js
@@ +200,4 @@
>    }
>  };
>  
> +function RTCIdentityAssertion(idp, name) {

You'll never get these args from webidl here, they come in __init() which you have, so I would remove and maybe initialize to null here.

@@ +629,5 @@
> +                           .defaultView
> +                           .gBrowser;
> +      let notificationBox = gBrowser.getNotificationBox();
> +
> +      if(message !== null) {

I see you use triple compare throughout, which may be a style thing.

I'd prefer if(message) here i.e. I'd want the else clause if message is ever undefined. Even if this invariant is provided by the (currently one) caller, it's an unobvious dependency I don't think we should rely on.

@@ +640,5 @@
> +          notificationBox.PRIORITY_INFO_HIGH,
> +          []
> +        );
> +      } else {
> +        notificationBox.appendNotification(

Looks good to me, but I think a DOM reviewer might want to look at this.

@@ +674,5 @@
> +    // TODO: Spec says "If the setIdentityProvider() method has not been
> +    // called, then the browser shall use an IdP configured into the browser.
> +    // If more than one such IdP is configured, the browser should provide the
> +    // user with a chooser interface."
> +    if(this.signalingState === 'closed') {

== is sufficient.

@@ +680,5 @@
> +          "Cannot get identity assertion on closed connection");
> +    }
> +
> +    this._localIdp.getIdentityAssertion(function(assertion) {
> +      if(assertion === null) {

if(!assertion)

@@ +688,5 @@
> +        this.dispatchEvent(new this._win.Event("identityresult"));
> +      }
> +    }.bind(this));
> +  },
> +    

whitespace

@@ +961,5 @@
> +   */
> +  verifyIdentityFromSDP: function(sdp, callback) {
> +    let fingerprint = this._getAttributeFromSDP(sdp, "fingerprint");
> +    let identity = this._getAttributeFromSDP(sdp, "identity");
> +    if(fingerprint === null || identity === null) {

if (!fingerprint || !identity) {

::: dom/webidl/RTCIdentityAssertion.webidl
@@ +11,5 @@
> + JSImplementation="@mozilla.org/dom/rtcidentityassertion;1",
> + Constructor(DOMString idp, DOMString name)]
> +interface RTCIdentityAssertion {
> +  attribute DOMString? idp;
> +  attribute DOMString? name;

The '?'s deviate from http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcidentityassertion-type - Any reason?

Comment 20

[Martin Thomson [:mt:]]

Comment on attachment 8333805 [details] [diff] [review]
0001-Bug-878941-Incorporate-IdP-support-into-PeerConnecti.patch

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

I think that ekr already hinted that this doesn't handle multiple assertions or fingerprints.  I think that we need to sit down and work out how

I have some concerns about the UX around this, which I understand is rough for now, as a proof of concept.  Ultimately however, the UX will need to drive the design.  My initial thoughts are that a deeper integration with the gUM door-hanger will be needed, but even then, that doesn't cover the case where you want to be able to show that you are *receiving* authenticated media.

::: dom/media/PeerConnection.js
@@ +629,5 @@
> +                           .defaultView
> +                           .gBrowser;
> +      let notificationBox = gBrowser.getNotificationBox();
> +
> +      if(message !== null) {

I'm with jib when it comes to this instance (don't want to miss undefined), but less keen to suggest use of == over ===.  Type coercion is a great source of bugs.  Truthy/falsy coercion is idiomatic, but other forms of coercion are trouble.

@@ +633,5 @@
> +      if(message !== null) {
> +        this._peerIdentity = new this._win.RTCIdentityAssertion(
> +          this._remoteIdp._provider, message.identity.name);
> +        notificationBox.appendNotification(
> +          "Identity of your WebRTC peer " + this._peerIdentity.name + " is verified by " + this._peerIdentity.idp,

The fact that an IdP can produce a different suffix to it's authenticated name is not a property we want to allow right now.  You need to check that the IdP produces an 822-form string that ends in ('@' + this._peerIdentity.idp).

You also want to be doubly certain that this._peerIdentity.idp can't be changed by the site in the meantime so that it can generate lies.  Rather than getting the IdP name from _peerIdentity (which could change in response to setRemoteDescription, maybe, correct me if I'm wrong), you should get the identity provider name from the current context.

@@ +949,5 @@
> +    let matches = sdp.match(pattern);
> +    if(!matches) {
> +      return null;
> +    }
> +    return matches[1].trim();

ekr has already talked about this.  This requires considerably more work to get right.

There's the case where the attribute is not shared by all m= sections.

Then there is also the case where there are multiple attributes.  This makes the IdP job more difficult (and might require some changes to the IdP protocol if we're serious about making this performant).

@@ +958,5 @@
> +   * If the verification succeeds callback is called with the message from the
> +   * IdP proxy as parameter, else (verification failed OR no a=identity line in
> +   * SDP at all) null is passed to callback.
> +   */
> +  verifyIdentityFromSDP: function(sdp, callback) {

This is going to need a timeout (though maybe not directly here).  IdPs aren't reliable, as we've found out.

Generating an assertion also needs a timeout.

@@ +1024,5 @@
> +   * given SDP with an a=identity line and calls callback with the new SDP as
> +   * parameter. If no IdP is configured the original SDP (without a=identity
> +   * line) is passed to the callback.
> +   */
> +  appendIdentityToSDP: function(sdp, callback) {

The timeout comment applies here.

Note that we also need some way for a site to "prime" an IdP so that the necessary cookies/etc... are in place for the IdP to actually do this work.  We're relying on faith right now. (This is a spec change, I just wanted to get things on paper before I go home.)

Comment 21

[Max Jonas Werner [:Makkes]]

(In reply to Martin Thomson [:mt] from comment #20)
[...]
> @@ +633,5 @@
> > +      if(message !== null) {
> > +        this._peerIdentity = new this._win.RTCIdentityAssertion(
> > +          this._remoteIdp._provider, message.identity.name);
> > +        notificationBox.appendNotification(
> > +          "Identity of your WebRTC peer " + this._peerIdentity.name + " is verified by " + this._peerIdentity.idp,
> 
> The fact that an IdP can produce a different suffix to it's authenticated
> name is not a property we want to allow right now.  You need to check that
> the IdP produces an 822-form string that ends in ('@' +
> this._peerIdentity.idp).

To speak in the terms of http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07#section-5.6: We only want to allow authoritative IdPs? What if an IdP sends a different identity name with an assertion? Show an error?

> You also want to be doubly certain that this._peerIdentity.idp can't be
> changed by the site in the meantime so that it can generate lies.  Rather
> than getting the IdP name from _peerIdentity (which could change in response
> to setRemoteDescription, maybe, correct me if I'm wrong), you should get the
> identity provider name from the current context.

I'll check that.

> @@ +949,5 @@
> > +    let matches = sdp.match(pattern);
> > +    if(!matches) {
> > +      return null;
> > +    }
> > +    return matches[1].trim();
> 
> ekr has already talked about this.  This requires considerably more work to
> get right.
> 
> There's the case where the attribute is not shared by all m= sections.
> 
> Then there is also the case where there are multiple attributes.  This makes
> the IdP job more difficult (and might require some changes to the IdP
> protocol if we're serious about making this performant).

Is it possible that different m= lines belong to different identities or the media is sent to different recipients? In my understanding this is not the case and Firefox only has one a=fingerprint line per SDP that an IdP could assert to.

> @@ +958,5 @@
> > +   * If the verification succeeds callback is called with the message from the
> > +   * IdP proxy as parameter, else (verification failed OR no a=identity line in
> > +   * SDP at all) null is passed to callback.
> > +   */
> > +  verifyIdentityFromSDP: function(sdp, callback) {
> 
> This is going to need a timeout (though maybe not directly here).  IdPs
> aren't reliable, as we've found out.
> 
> Generating an assertion also needs a timeout.
Definitely. I'll find out how we can handle this from PC.js or RTCIdentity.jsm.

> @@ +1024,5 @@
> > +   * given SDP with an a=identity line and calls callback with the new SDP as
> > +   * parameter. If no IdP is configured the original SDP (without a=identity
> > +   * line) is passed to the callback.
> > +   */
> > +  appendIdentityToSDP: function(sdp, callback) {
> 
> The timeout comment applies here.

Ok.

Comment 22

[Martin Thomson [:mt:]]

(In reply to Max Jonas Werner from comment #21)
> To speak in the terms of
> http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07#section-5.6:
> We only want to allow authoritative IdPs? What if an IdP sends a different
> identity name with an assertion? Show an error?

That's right, authoritative only.  Any system that relies on assertions about third parties (like the CA system) relies on another layer of trust, where the relying party establishes whether they trust the assertion being made by the identity provider.  We don't really have any mechanisms in place for determining whether facebook.com is or isn't trusted for the purposes of asserting google.com identities.  Until we do, we should treat all non-authoritative assertions as inherently suspect.

> Is it possible that different m= lines belong to different identities or the
> media is sent to different recipients? In my understanding this is not the
> case and Firefox only has one a=fingerprint line per SDP that an IdP could
> assert to.

While it is true that Firefox is only going to offer up a single identity for a session, there is no guarantee that audio and video are being sent to the same remote identity.  This is part of the challenge with RTCPeerConnection.  The API can't currently handle multiple remote identities in any sort of sensible fashion, even though it can generate multiple DTLS connections.

I love it.  I wonder if it is coincidence that this is comment #22.

Comment 23

[Martin Thomson [:mt:]]

Looking at this more, I wonder if we can't do the IdP retrieval stuff in parallel to all the createOffer/createAnswer processing.  Is there any reason that createOffer or createAnswer might block on the network or a device or something like that?  abr?

Comment 24

[Max Jonas Werner [:Makkes]]

(In reply to Martin Thomson [:mt] from comment #22)
> (In reply to Max Jonas Werner from comment #21)
> > To speak in the terms of
> > http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07#section-5.6:
> > We only want to allow authoritative IdPs? What if an IdP sends a different
> > identity name with an assertion? Show an error?
> 
> That's right, authoritative only.

This way we wouldn't be able to use e.g. Mozilla's Persona service, 3rd party OpenID etc. I have no problem with this, just want to raise awareness that this may hinder deployment.

> Any system that relies on assertions
> about third parties (like the CA system) relies on another layer of trust,
> where the relying party establishes whether they trust the assertion being
> made by the identity provider.  We don't really have any mechanisms in place
> for determining whether facebook.com is or isn't trusted for the purposes of
> asserting google.com identities.  Until we do, we should treat all
> non-authoritative assertions as inherently suspect.

Agreed. Actually I explicitly added the IdP's domain name to the notification box so that the user can decide. Nevertheless, the spec even says that eventually the user must decide if she trusts the IdP no matter whether it is authoritative or not.

> > Is it possible that different m= lines belong to different identities or the
> > media is sent to different recipients? In my understanding this is not the
> > case and Firefox only has one a=fingerprint line per SDP that an IdP could
> > assert to.
> 
> While it is true that Firefox is only going to offer up a single identity
> for a session, there is no guarantee that audio and video are being sent to
> the same remote identity.  This is part of the challenge with
> RTCPeerConnection.  The API can't currently handle multiple remote
> identities in any sort of sensible fashion, even though it can generate
> multiple DTLS connections.

Let me try to sum this up:

It's possible that we establish an RTCPeerConnection sending audio to peer X and video to peer Y (assuming that the remote SDP contains different candidates for each m= line) resulting in two DTLS connections and two a=fingerprint attributes. Correct?

In this case we'd have to extend the message structure of SIGN messages so that an IdP can assert to multiple fingerprints. Also we'd have to verify multiple a=identity lines contained in an SDP, either in one go or in multiple roundtrips to the IdP (or even different IdPs) AND tell the user "Video is coming from X, audio is coming from Y". IMO this is going to totally mess up the user's mind (and most probably application performance).

Comment 25

[Martin Thomson [:mt:]]

(In reply to Max Jonas Werner from comment #24)
> This way we wouldn't be able to use e.g. Mozilla's Persona service, 3rd
> party OpenID etc. I have no problem with this, just want to raise awareness
> that this may hinder deployment.

This doesn't prevent it.  It just means that you are required to do some additional validation for each service that intends to make third party assertions.

> Agreed. Actually I explicitly added the IdP's domain name to the
> notification box so that the user can decide. Nevertheless, the spec even
> says that eventually the user must decide if she trusts the IdP no matter
> whether it is authoritative or not.

I don't believe that to be sufficient.  We should be providing strong guarantees about the information we provide in these dialogs.  Allowing example.com to make assertions about example.org weakens the strength of the statement we make.  Everything the browser chrome says should not be subject to weakening like that.

We don't want to completely preclude third party assertions, but the security story for them needs to be every bit as strong as for authoritative assertions if this is going to be useful information.

> It's possible that we establish an RTCPeerConnection sending audio to peer X
> and video to peer Y (assuming that the remote SDP contains different
> candidates for each m= line) resulting in two DTLS connections and two
> a=fingerprint attributes. Correct?

Correct.  The two different a=fingerprint attributes are in the answer.

BTW, it's also possible for there to be two different a=fingerprint attributes in the *same* m= section, but that means that these are alternatives.  We would need to accept either when establishing the connection, and to validate the identity to either.

> In this case we'd have to extend the message structure of SIGN messages so
> that an IdP can assert to multiple fingerprints. Also we'd have to verify
> multiple a=identity lines contained in an SDP, either in one go or in
> multiple roundtrips to the IdP (or even different IdPs) AND tell the user
> "Video is coming from X, audio is coming from Y". IMO this is going to
> totally mess up the user's mind (and most probably application performance).

The SIGN messages don't need to change.  We can place a constraint on the browser fairly easy that says "A single RTCPeerConnection instance can only use a single identity."  This means a single a=fingerprint line for the entire session too.  No need to offer alternatives.

And again, the IdP proxy doesn't need to change for validation.  It's the API.  That fucking API.

Comment 26

[Martin Thomson [:mt:]]

I'm going to suggest that we treat signing very, very carefully when we get down to it.

As it stands, the TransportFlowDtls class accepts any a=fingerprint attribute that it finds (any session level attribute, or any media level attribute, depending on what is there).  At the same time, the code that is in this patch independently looks at the a=fingerprint attribute.  

That means that you can mount a spoofing attack trivially with what is here.  Since this code only checks the first a=fingerprint attribute, I can construct SDP with the first a=fingerprint from the user's fingerprint, but then add my own a=fingerprint afterwards and have you connect to me.  All the checks pass, and I am successfully impersonating the user.

The fix is to ignore the SDP when validating fingerprints, instead using the fingerprint attached to the actual flows.  I'll start working on a patch.

Comment 27

[Max Jonas Werner [:Makkes]]

(In reply to Martin Thomson [:mt] from comment #25)
> (In reply to Max Jonas Werner from comment #24)
> > This way we wouldn't be able to use e.g. Mozilla's Persona service, 3rd
> > party OpenID etc. I have no problem with this, just want to raise awareness
> > that this may hinder deployment.
> 
> This doesn't prevent it.  It just means that you are required to do some
> additional validation for each service that intends to make third party
> assertions.

In my understanding disallowing third-party assertions means that persona.org is not allowed to assert identities that have a domain name on the RHS that's different from persona.org.

Comment 28

[Martin Thomson [:mt:]]

(In reply to Max Jonas Werner from comment #27)
> In my understanding disallowing third-party assertions means that
> persona.org is not allowed to assert identities that have a domain name on
> the RHS that's different from persona.org.

Quite right.  What I should have said is that it doesn't preclude the addition of features that allow this, but until we have those features in place we cannot permit persona.org to assert for any other site.

Comment 29

[Martin Thomson [:mt:]]

Attachment: Implement identity provider assertions and verification for WebRTC

Apparently, this needs to land in fairly short order, so I've be asked to make a few changes.  

I've taken the patch Max provided and made some changes:
- The IdP times out in case of any of the myriad errors that might occur.  Fixed at 5s for now, might want to add a pref for this later.
- The validation is now quite narrow.  Third party identity assertions just fail.
- The SDP handling is also very narrow.  Identity assertions are only looked at at the session level.  There can only be a single fingerprint used across the entire session as well.  Both require the same hash function in their description.  Anything that doesn't fit this narrow definition, doesn't get the nice identity validation message.
- As much as possible, I've made the identity provider lowercase when displaying and comparing.  This also allows for assertions from providers that are identified with "host.example.com:8443" as well as preventing dodgy providers like "host.example.com/mypath".
- I've taken :jib's comments into consideration.
- I've changed the icons - the infobar is still a bad place to display this info.

Note, all this will remain behind a pref until we sort out the many and varied issues related to this code, the specs and whatnot.

Comment 30

[Max Jonas Werner [:Makkes]]

Comment on attachment 8342778 [details] [diff] [review]
Implement identity provider assertions and verification for WebRTC

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

::: dom/media/PeerConnection.js
@@ +692,5 @@
> +          this._remoteIdp._provider, message.identity.name);
> +        notificationBox.appendNotification(
> +          "Identity of your WebRTC peer is " + this._peerIdentity.name,
> +          "webrtc-auth",
> +          "chrome://global/skin/notification/info-icon.png",

This icon doesn't show up in my build.

@@ +700,5 @@
> +      } else {
> +        notificationBox.appendNotification(
> +          "Identity of your WebRTC peer is not verified",
> +          "webrtc-auth",
> +          "chrome://global/skin/notification/warning-icon.png",

This icon doesn't show up in my build.

@@ +1014,5 @@
> +   */
> +  _queueOrRun: function(obj) {
> +    let timeout = this._dompc._win.setTimeout(function idpTimedout() {
> +      timeout = null;
> +      obj.callback(null);

If the provider is not ready, yet, we'll have to remove the callback from this._waitingForProvider. Otherwise, obj.callback would be called twice.

@@ +1015,5 @@
> +  _queueOrRun: function(obj) {
> +    let timeout = this._dompc._win.setTimeout(function idpTimedout() {
> +      timeout = null;
> +      obj.callback(null);
> +    }, 5000); // TODO pref

This immediately starts the timer. So if the provider is ready we wait 5s for it to actually respond. If it is not we wait 5s for it to become READY and to respond. Don't we want to have 2 timeouts, one for the provider to get READY and one for it to respond?

@@ +1032,5 @@
> +      this._waitingForProvider.push({ func: obj.func, args: args });
> +    }
> +  },
> +
> +  _getFingerprintFromSdp: function PeerConnectionIdP_getFingerprintFromSdp(sdp) {

nitpicking: I defined the functions without name. I'm unemotional here, we just should be consistent. Same applies below.

@@ +1117,5 @@
> +    });
> +  },
> +
> +  /**
> +   * Checks that the name in the identity provided by the IdP is OK.

Please state here that this function changes the parameter. Or better: Don't change the parameter but return a changed clone instead of true.

@@ +1156,5 @@
> +            this._verifyName(message.identity)) {
> +          callback(message);
> +          return;
> +        }
> +      } catch(e) {}

Empty catch block, hmm. I would prefer to log the exception here.

Comment 31

[Max Jonas Werner [:Makkes]]

Either I'm not allowed to set the review flag or I just don't find out how.

Comment 32

[Martin Thomson [:mt:]]

(In reply to Max Jonas Werner from comment #30)
> This icon doesn't show up in my build.

I see what is going on here.  You chose something from the linux skin, I chose something from the osx skin and neither of us can see the other's choice.

I'll see what I can do to just take the existing styles from the skin.  The icons should be picked up by the skin CSS if I do it right.  This might take some time; the rest are straightforward and mechanical (and done already).

> @@ +1014,5 @@
> > +   */
> > +  _queueOrRun: function(obj) {
> > +    let timeout = this._dompc._win.setTimeout(function idpTimedout() {
> > +      timeout = null;
> > +      obj.callback(null);
> 
> If the provider is not ready, yet, we'll have to remove the callback from
> this._waitingForProvider. Otherwise, obj.callback would be called twice.

Actually, what happens is that the target function (obj.func) gets called when the provider is ready.  Then, the done() method is called.  However, since we've cleared timeout when we timed out, the done method will just return silently.  Obviously, we need this protection regardless of anything we do elsewhere, because the function might have already been run when we time out.

The cost is that extra work happens after we no longer care about it.  That needs a little reorganizing to fix.  Expect something in the next revision.

I noticed and fixed a leak when I went through to fix this up.

> @@ +1015,5 @@
> > +  _queueOrRun: function(obj) {
> > +    let timeout = this._dompc._win.setTimeout(function idpTimedout() {
> > +      timeout = null;
> > +      obj.callback(null);
> > +    }, 5000); // TODO pref
> 
> This immediately starts the timer. So if the provider is ready we wait 5s
> for it to actually respond. If it is not we wait 5s for it to become READY
> and to respond. Don't we want to have 2 timeouts, one for the provider to
> get READY and one for it to respond?

There's no sense in having two levels of timeout.  A single time budget for the whole process is simpler.  That allows an IdP to spend the budget as they see fit.  Maybe they want to take ages to load lots of stuff and get connections ready before sending out a READY.  Maybe they load quickly, but have to go to lots of servers out there to perform validation.  Adding two points for failures is going to make things more brittle (and it's much more code to write :).

> nitpicking: I defined the functions without name. I'm unemotional here, we
> just should be consistent. Same applies below.

I noticed that ekr was doing this, and it's nice not to have (anonymous function) everywhere in stack traces and debugging.  I'll make the whole class more consistent.

> > +  /**
> > +   * Checks that the name in the identity provided by the IdP is OK.
> 
> Please state here that this function changes the parameter. Or better: Don't
> change the parameter but return a changed clone instead of true.

Yeah, that was lazy, wasn't it.  I'll return the name and the caller can make the changes.

> @@ +1156,5 @@
> > +            this._verifyName(message.identity)) {
> > +          callback(message);
> > +          return;
> > +        }
> > +      } catch(e) {}
> 
> Empty catch block, hmm. I would prefer to log the exception here.

That's what the very next line does.  This is only a JSON.parse failure, and there are so many other problems we're tracking that singling it out seemed like needless busy-work.  Better to just log the thing that was busted in the one place.

(In reply to Max Jonas Werner from comment #31)
> Either I'm not allowed to set the review flag or I just don't find out how.

You should be able to go to the overview in splinter and set '-' next to your name.

Comment 33

[Max Jonas Werner [:Makkes]]

(In reply to Martin Thomson [:mt] from comment #32)
> (In reply to Max Jonas Werner from comment #31)
> > Either I'm not allowed to set the review flag or I just don't find out how.
> 
> You should be able to go to the overview in splinter and set '-' next to
> your name.

Ok, than I lack the permission because there is no UI element next to my name.

Comment 34

[Martin Thomson [:mt:]]

Attachment: Implement identity provider assertions and verification for WebRTC (updated)

Addressing the comments from Max's review.

Note that removing icons altogether was the path to good consistent UX.  I also downgraded the error notification to a warning one.

Comment 35

[Adam Roach [:abr]]

Comment on attachment 8343354 [details] [diff] [review]
Implement identity provider assertions and verification for WebRTC (updated)

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

::: dom/media/tests/mochitest/idp-proxy.js
@@ +8,5 @@
> +IDPJS.prototype.receiveMessage = function (ev) {
> +    var message = ev.data;
> +    switch(message.type) {
> +        case "SIGN":
> +	    this.port.postMessage({

Please remove the tab character from this line.

Comment 36

[Adam Roach [:abr]]

Comment on attachment 8343354 [details] [diff] [review]
Implement identity provider assertions and verification for WebRTC (updated)

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

I'll go ahead and grab the token back for this review, given that we're not landing in 29.

Comment 37

[Martin Thomson [:mt:]]

Attachment: Implement identity provider assertions and verification for WebRTC (no tab)

Comment 38

[Max Jonas Werner [:Makkes]]

(In reply to Martin Thomson [:mt] from comment #32)
> (In reply to Max Jonas Werner from comment #30)
[...]
> > @@ +1015,5 @@
> > > +  _queueOrRun: function(obj) {
> > > +    let timeout = this._dompc._win.setTimeout(function idpTimedout() {
> > > +      timeout = null;
> > > +      obj.callback(null);
> > > +    }, 5000); // TODO pref
> > 
> > This immediately starts the timer. So if the provider is ready we wait 5s
> > for it to actually respond. If it is not we wait 5s for it to become READY
> > and to respond. Don't we want to have 2 timeouts, one for the provider to
> > get READY and one for it to respond?
> 
> There's no sense in having two levels of timeout.  A single time budget for
> the whole process is simpler.  That allows an IdP to spend the budget as
> they see fit.  Maybe they want to take ages to load lots of stuff and get
> connections ready before sending out a READY.  Maybe they load quickly, but
> have to go to lots of servers out there to perform validation.  Adding two
> points for failures is going to make things more brittle (and it's much more
> code to write :).

I dare to disagree. We give the IdP proxy a fixed time budget for two cumulative tasks. In the first case we give it 5s to get READY _and_ to SIGN/VERIFY, in the second case (we already have received READY) we give it 5s to just SIGN/VERIFY.

This is like saying "You have 2 hours to fix the warp drive" at one time and at another time saying "You have 2 hours to fix the warp drive AND bring us to Vulcan".

Comment 39

[Martin Thomson [:mt:]]

(In reply to Max Jonas Werner from comment #38)
> I dare to disagree. We give the IdP proxy a fixed time budget for two
> cumulative tasks. In the first case we give it 5s to get READY _and_ to
> SIGN/VERIFY, in the second case (we already have received READY) we give it
> 5s to just SIGN/VERIFY.

In what case do we not perform the load -> READY -> SIGN/VERIFY in totality?  Second-round offers/answers?

If you wanted to disadvantage IdPs in the case that they are already loaded, you could change this to:

[...].setTimeout([...], this._ready ? 3000 : 5000);

But I don't see how to turn that into a positive thing.

Comment 40

[Adam Roach [:abr]]

Comment on attachment 8344839 [details] [diff] [review]
Implement identity provider assertions and verification for WebRTC (no tab)

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

As nice as it would be to have this functionality, I'm afraid that this patch still needs a lot of polishing before it's ready to land. The big flashing red showstopper is that the way we render validation of an identity is trivially spoofable by webpages.

There are a lot of places where failures are simply dropped on the floor without any logging at all, which would make troubleshooting a misbehaving identity provider incredibly frustrating.

The "identity" SDP attribute is different enough -- as is its handling -- from our existing attributes that we minimally need a mochi test that:
 - Calls createOffer
 - Calls setLocalDescription with the resulting SDP
 - Retrieves the SDP from the PC using the localDescription attribute
 - Verifies that the "a=identity" line is present and not truncated

I note that we have a test case that checks for the opposite behavior. Presumably, this test passes, which tells me that the current patch does not handle this situation correctly.

We also need a test that delays the "READY" from the IdP by, say, 1000 ms, to test the code that handles queuing up operations waiting for the IdP to get ready.

We also need a test that does not ever send a "READY" message from the IdP, and that the call continues anyway.

We also need a test that does not respond to a normal IdP message (e.g., "SIGN" or "VERIFY") to make sure the timeout code behaves as expected.

Additional nits:
PeerConnection.js:690: Space between 'if' and parenthesis
PeerConnection.js:735: Space between 'if' and parenthesis
PeerConnection.js:741: Space between 'if' and parenthesis
PeerConnection.js:1206: Space between 'if' and parenthesis
PeerConnection.js:1278: Space between 'if' and parenthesis
test_getIdentityAssertion.html:42: Space between 'if' and parenthesis
test_getIdentityAssertion.html:44: Space between 'if' and parenthesis
RTCIdentity.jsm:93: Space between 'if' and parenthesis

::: dom/media/PeerConnection.js
@@ +1,1 @@
> +/* vim: set sw=2: */

If we're adding a vim line, I'd prefer something more complete; something like:

/* vim: set ts=2 sts=2 et sw=2 tw=80: */

@@ +1,2 @@
> +/* vim: set sw=2: */
> +/*jshint moz:true, browser:true */

space before jshint

@@ +269,5 @@
> +  init: function(win) { this._win = win; },
> +
> +  __init: function(idp, name) {
> +    this.idp = idp;
> +    this.name  = name;

I'm not even knowledgeable enough to consider myself an XPCOM and/or webidl beginner, but something smells fishy about having duplicated code between __init and the construction function. Please seek out an expert to comment on this situation.

@@ +677,4 @@
>              "Invalid type " + desc.type + " provided to setRemoteDescription");
>      }
>  
> +    this._remoteIdp.verifyIdentityFromSDP(desc.sdp, function(message) {

Can we pull the callback function out into a separate method on the PC? this is a bit unwieldy in-line. Something more like:

this._remoteIdp.verifyIdentityFromSDP(desc.sdp, this.processRemoteIdentity.bind(this));

@@ +677,5 @@
>              "Invalid type " + desc.type + " provided to setRemoteDescription");
>      }
>  
> +    this._remoteIdp.verifyIdentityFromSDP(desc.sdp, function(message) {
> +      let gBrowser = this._win.QueryInterface(Ci.nsIInterfaceRequestor)

I'm pretty sure the "g" stands for "global." Let's call the local version something different.

@@ +686,5 @@
> +                           .defaultView
> +                           .gBrowser;
> +      let notificationBox = gBrowser.getNotificationBox();
> +
> +      if(message !== null) {

Comparison against "null" is discouraged except in the extraordinarily rare cases that it needs to be distinguished from other falsy values. That doesn't appear to be the case here, so proper form would be:

if (message) {

@@ +689,5 @@
> +
> +      if(message !== null) {
> +        this._peerIdentity = new this._win.RTCIdentityAssertion(
> +          this._remoteIdp._provider, message.identity.name);
> +        notificationBox.appendNotification(

There's a huge UX issue with using notificationBox here: even though it is part of the chrome, its proximity to page content makes it subject to page spoofing. And this is *exactly* the kind of information we want to make sure can't be spoofed by a page. I wish I had a suggestion of what to replace it with -- you'll probably need to consult the UX folks to get input.

I'm afraid that landing with this UI, even temporarily, will train users to trues page-top banners to make this kind of assertion, which would be dangerous and difficult to undo.

@@ +706,5 @@
> +          []
> +        );
> +      }
> +
> +      this._queueOrRun({

Moving this into the identity callback seems to serialize two operations that could otherwise be parallelized (keep in mind that the operations handled by setRemoteDescription take place mostly off the main thread). Given that the remote description processing doesn't have any dependency on the identity verification, and given that call setup times are something we're currently a little worried about, I think we need to move this back outside the callback.

@@ +734,5 @@
> +    // user with a chooser interface."
> +    if(this.signalingState === "closed") {
> +      throw new this._win.DOMError("",
> +          "Cannot get identity assertion on closed connection");
> +    }

Why not re-use PeerConnection._checkClosed()? If you just want a custom method, update _checkClosed() to take a message as its parameter (and use the default text currently there if undefined).

In particular, I really don't like seeing this code using PeerConnection.signalingState where every other method uses PeerConnection._closed.

@@ +737,5 @@
> +          "Cannot get identity assertion on closed connection");
> +    }
> +
> +    this._localIdp.getIdentityAssertion(function(assertion) {
> +      if(assertion === null) {

if (!assertion) {

@@ +740,5 @@
> +    this._localIdp.getIdentityAssertion(function(assertion) {
> +      if(assertion === null) {
> +        throw new this._win.DOMError("",
> +            "Could not get identity assertion");
> +      } else {

Remove this else block -- just put the dispatchEvent after the 'if' block.

@@ +830,5 @@
>    },
>  
> +  get peerIdentity() {
> +    return this._peerIdentity;
> +  },

Match style with "get" functions below (i.e., put on one line)

@@ +990,4 @@
>    ]
>  };
>  
> +function PeerConnectionIdP(dompc) {

Can we move the PCIdp class out into its own file?

We've had a nontrivial number of bugs in here due to the presence of multiple classes in one file. In particular the face that we now have two _queueOrRun methods is going to lead to difficult-to-diagnose hijinks if someone gets the wires crossed.

The radically different meanings of PeerConnection._pending and PeerConnectionIdp._pending could make for some real fun, too.

@@ +995,5 @@
> +  this._ready = false;
> +
> +  // for tracking messages outstanding to the IdP
> +  this._currentId = 0;
> +  this._pending = {};

In fact, can we re-name this to something more descriptive like _pendingCallback?

@@ +1004,5 @@
> +}
> +
> +PeerConnectionIdP._mLinePattern = new RegExp("^m=", "m");
> +PeerConnectionIdP._identityPattern = new RegExp("^a=[iI][dD][eE][nN][tT][iI][tT][yY]:(\\S+)", "m");
> +PeerConnectionIdP._fingerprintPattern = new RegExp("^a=[fF][iI][nN][gG][eE][rR][pP][rR][iI][nN][tT]:(\\S+) (\\S+)", "m");

These two lines are very long; please wrap.

@@ +1007,5 @@
> +PeerConnectionIdP._identityPattern = new RegExp("^a=[iI][dD][eE][nN][tT][iI][tT][yY]:(\\S+)", "m");
> +PeerConnectionIdP._fingerprintPattern = new RegExp("^a=[fF][iI][nN][gG][eE][rR][pP][rR][iI][nN][tT]:(\\S+) (\\S+)", "m");
> +
> +PeerConnectionIdP.prototype = {
> +  setIdentityProvider: function PeerConnectionIdP_setIdentityProvider(provider, protocol, username) {

This line is very long. Please wrap.

@@ +1017,5 @@
> +  /**
> +   * Add a function to the queue or run it immediately depending on the IdP
> +   * proxy's READY state.  Adds a timeout to the call at the same time.
> +   */
> +  _queueOrRun: function PeerConnectionIdP_queueOrRun(obj) {

The name implies that operations will be executed in the order in which they are added. With our current javascript engine, this happens to be true (as far as I know, at least); however, if our Object handling ever changes to produce keys() in an order other than that in which they were defined, then pre-READY operations could be pulled out and executed in a different order.

I suspect that we really do want an ordering here rather than arbitrary execution. Is there some reason we're using an object with non-numeric keys (that we take explicit steps to make non-numeric!) rather than simply using an array?

Also, there are some hardcoded assumptions in here that the task we're executing (or setting up to execute) always must be a dispatch to the Idp. It would help in reading this code if the method name reflected that fact.

@@ +1020,5 @@
> +   */
> +  _queueOrRun: function PeerConnectionIdP_queueOrRun(obj) {
> +    let waitingHandle = null;
> +    let timeout = this._dompc._win.setTimeout(function idpTimedout() {
> +      this._dompc.reportWarning("RTC identity: IdP timeout for " + this._idpchannel.well_known, null, 0);

This line is very long. Please wrap.

@@ +1024,5 @@
> +      this._dompc.reportWarning("RTC identity: IdP timeout for " + this._idpchannel.well_known, null, 0);
> +      timeout = null;
> +      delete this._waitingForProvider[waitingHandle];
> +      obj.callback(null);
> +    }.bind(this), 5000); // TODO pref

Please add a bug number to this "TODO".

@@ +1037,5 @@
> +    let args = obj.args.concat(done.bind(this));
> +    if (this._ready) {
> +      obj.func.apply(this, args);
> +    } else {
> +      waitingHandle = "x" + (++this._waitingId);

This is a plain object, not an array, right? The "x" seems unnecessary.

@@ +1051,5 @@
> +        let remainder = sect.substring(m.index + m[0].length);
> +        if (!remainder.match(PeerConnectionIdP._fingerprintPattern)) {
> +          return { algorithm: m[1], digest: m[2] };
> +        }
> +        this._dompc.reportWarning("RTC identity: two fingerprint values in same m= section are not supported: " + remainder, null, 0);

This line is very long. Please wrap.

@@ +1072,5 @@
> +        if (existing && current &&
> +            existing.algorithm === current.algorithm &&
> +            existing.digest === current.digest) {
> +          return existing;
> +        }

Ow, my head hurts. This is a bit of a baroque way of ensuring uniformity of an array, and I fear that future maintainers may have a bit of difficulty dealing with it.

@@ +1073,5 @@
> +            existing.algorithm === current.algorithm &&
> +            existing.digest === current.digest) {
> +          return existing;
> +        }
> +        this._dompc.reportWarning("RTC identity: different fingerprint across m= sections not supported", null, 0);

Long line; wrap.

@@ +1076,5 @@
> +        }
> +        this._dompc.reportWarning("RTC identity: different fingerprint across m= sections not supported", null, 0);
> +        // undefined !
> +      }.bind(this), first);
> +    }

We need code here to handle the situation that we have have a fingerprint at the session level *and* in the media sections.

@@ +1081,5 @@
> +  },
> +
> +  _getIdentityFromSdp: function PeerConnectionIdP_getIdentityFromSdp(sdp) {
> +    // we only pull from the session level right now
> +    // TODO allow for per-m=-section identity

Please include a bug number in this TODO.

@@ +1082,5 @@
> +
> +  _getIdentityFromSdp: function PeerConnectionIdP_getIdentityFromSdp(sdp) {
> +    // we only pull from the session level right now
> +    // TODO allow for per-m=-section identity
> +    let sessionLevel = sdp.substring(0, sdp.match(PeerConnectionIdP._mLinePattern).index);

Long line; wrap.

@@ +1089,5 @@
> +      let assertion = {};
> +      try {
> +        assertion = JSON.parse(atob(m[1]));
> +      } catch (e) {
> +        this._dompc.reportWarning("RTC identity: invalid identity assertion: " + e, null, 0);

Long line; wrap.

@@ +1096,5 @@
> +          typeof assertion.idp.domain === "string" &&
> +          typeof assertion.assertion === "string") {
> +        return assertion;
> +      }
> +      this._dompc.reportWarning("RTC identity: identity assertion missing idp/idp.domain/assertion", null, 0);

Long line; wrap.

@@ +1102,5 @@
> +    // undefined!
> +  },
> +
> +  /**
> +   * Queues a task to verify the a=identity line the given SDP contains, if any.

Only queues if the IdP isn't ready yet, right?

@@ +1135,5 @@
> +  _extractName: function PeerConnectionIdP_extractName(identity) {
> +    let atIdx = identity.name.indexOf("@");
> +    if (atIdx > 0) {
> +      // no third party assertions... for now
> +      let tail = identity.name.substring(atIdx + 1).toLowerCase();

See comment in RTCIdentity.jsm regarding lowercasing of domains and IDN.

@@ +1144,5 @@
> +      if (providerPortIdx > 0) {
> +        provider = provider.substring(0, providerPortIdx);
> +      }
> +      if (tail === provider) {
> +        // downcase the tail of the name

This comment makes no sense here.

@@ +1181,5 @@
> +            return;
> +          }
> +        }
> +      } catch(e) {} // JSON was invalid
> +      this._dompc.reportWarning("RTC identity: malformed IdP response:" + JSON.stringify(message), null, 0);

More precise diagnostics (e.g., indicating which of the preceding checks went wrong) would probably be appreciated by users.

@@ +1187,5 @@
> +    }
> +
> +    let id = this._insertPendingCallback(onVerification.bind(this)),
> +    origin = this._getOrigin();
> +    this._idpchannel.send({

The two-step "insertPendingCallback()" / "ipdchannel.send()" approach seems somewhat fragile in terms of future maintainability. I would think we want to add a method to PeerConnectionIdP that takes an IDPChannel message and a callback, and takes care of calling these methods in the right way.

@@ +1201,5 @@
> +   * given SDP with an a=identity line and calls callback with the new SDP as
> +   * parameter. If no IdP is configured the original SDP (without a=identity
> +   * line) is passed to the callback.
> +   */
> +  appendIdentityToSDP: function PeerConnectionIdP_appendIdentityToSDP(sdp, callback) {

Let's not call this "append". It implies that the identity is being *appended* to the SDP, which isn't what's happening. Suggestion: "add".

@@ +1253,5 @@
> +  _getIdentityAssertion: function PeerConnectionIdP__getIdentityAssertion(callback) {
> +    let origin = this._getOrigin();
> +    let [algorithm, digest] = this._dompc._getPC().fingerprint.split(" ");
> +    let id = this._insertPendingCallback(callback);
> +    // TODO(max): Cache the assertion

Add a bug # to this TODO.

@@ +1274,5 @@
> +    callback(arg);
> +  },
> +
> +  _onMessage: function PeerConnectionIdP_onMessage(message) {
> +    if(!message) {

Log?

@@ +1294,5 @@
> +      break;
> +    case "SUCCESS":
> +      this._firePendingCallback(message.id, message.message);
> +      break;
> +    default:

Log: "Unexpected message type from Identity Provider: " + message.type

::: dom/media/tests/mochitest/bogus.html
@@ +1,1 @@
> +<!DOCTYPE html>

Perhaps reame this "bogus-idp.html"?

::: dom/media/tests/mochitest/error.html
@@ +1,1 @@
> +<!DOCTYPE html>

Perhaps reame this "error-idp.html"?

::: dom/media/tests/mochitest/test_setIdentityProvider.html
@@ +39,5 @@
> +      test.next();
> +    }
> +  ],
> +  [
> +    "LOCAL_AND_REMOTE_DESC_DOES_NOT_INCLUDE_IDENTITY",

I think the sense of this test is exactly backwards, and that you're ensuring that our behavior is incorrect. I can't find anywhere in http://dev.w3.org/2011/webrtc/editor/webrtc.html#identity or http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07 where we are supposed to exclude identity from the "localDescription" or "remoteDescription" attributes.

::: dom/media/tests/mochitest/test_setIdentityProviderWithErrors.html
@@ +29,5 @@
> +      test.next();
> +    }
> +  ],
> +  [
> +    "OFFERS_AND_ANSWERS_INCLUDE_IDENTITY",

Rename test case: "..._DO_NOT_INCLUDE_..."

@@ +35,5 @@
> +      ok(!test.pcLocal._last_offer.sdp.contains("a=identity"), "a=identity not contained in the offer SDP");
> +      ok(!test.pcRemote._last_answer.sdp.contains("a=identity"), "a=identity not contained in the answer SDP");
> +      test.next();
> +    }
> +  ],

Add a test here to verify proper behavior of RTCPeerConnection.getIdentityAssertion() when the IdP returns "ERROR".

::: dom/webidl/RTCPeerConnection.webidl
@@ +87,5 @@
>  // moz-prefixed until sufficiently standardized.
>  interface mozRTCPeerConnection : EventTarget  {
> +  [Pref="media.peerconnection.identity"]
> +  void setIdentityProvider (DOMString provider,
> +                            DOMString protocol,

"protocol" is marked optional in the current spec. If we need to deviate from current spec (e.g., due to our limitation or due to a belief that the spec is incorrect), please include a comment indicating why.

::: toolkit/identity/RTCIdentity.jsm
@@ +94,5 @@
> +  // basic domain name:port sanity check
> +  // importantly, disallows '@', '/' and excess '.' or ':' use
> +  // disallows bare IP addresses of v!=4 as a consequence, which is OK
> +  // it would be wise to disallow IPv4 too, but that gets complex
> +  if (!aOptions.provider.match(/^[^@:\/\.]+(?:\.[^@:\/\.]+)*\.?(?::\d+)?$/)) {

This is hard to validate by casual inspection.

I'm hesitant to use a regexp to stand in as a proxy for how our URL parser will ultimately treat this string. Given the security sensitivity here, I think we really need to invoke the nsIURI parser and inspect its individual elements to ensure that the passed-in provider corresponds to the host-part.

@@ +97,5 @@
> +  // it would be wise to disallow IPv4 too, but that gets complex
> +  if (!aOptions.provider.match(/^[^@:\/\.]+(?:\.[^@:\/\.]+)*\.?(?::\d+)?$/)) {
> +      throw new Error("Identity provider is invalid");
> +  }
> +  this.provider = aOptions.provider.toLowerCase();

Given the possibility of IDN here, I'm a bit concerned about what toLowerCase() might do. Is there a good reason to do this?

Comment 41

[Adam Roach [:abr]]

It also occurs to me that a page loaded via file:///.../ is going to be able to assert identities of the form "adam.roach@", which could possibly be used to effect great confusion. Ditto for PCs invoked from add-ons (I know at least one add-on author taking advantage of this, and I'm sure we'll see many more as time goes on). We need to work through how we want these to be rendered. Minimally, we should put something after the @ that clues users in on the fact that this identity is something that they need to treat with extreme skepticism.

Comment 42

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

(In reply to Adam Roach [:abr] from comment #40)
> Comment on attachment 8344839 [details] [diff] [review]
> @@ +269,5 @@
> > +  init: function(win) { this._win = win; },
> > +
> > +  __init: function(idp, name) {
> > +    this.idp = idp;
> > +    this.name  = name;
> 
> I'm not even knowledgeable enough to consider myself an XPCOM and/or webidl
> beginner, but something smells fishy about having duplicated code between
> __init and the construction function. Please seek out an expert to comment
> on this situation.

The constructor args can be removed, since public webidl constructor args arrive in __init(), thanks to Ci.nsIDOMGlobalPropertyInitializer. The only time constructor args matter is if you butter your own webidl sandwich like RTCStatsReport does by creating the chrome and content separately using the ._create() function. The only reason to do that is if the constructor is private.

> ::: toolkit/identity/RTCIdentity.jsm
> @@ +94,5 @@
> > +  // basic domain name:port sanity check
> > +  // importantly, disallows '@', '/' and excess '.' or ':' use
> > +  // disallows bare IP addresses of v!=4 as a consequence, which is OK
> > +  // it would be wise to disallow IPv4 too, but that gets complex
> > +  if (!aOptions.provider.match(/^[^@:\/\.]+(?:\.[^@:\/\.]+)*\.?(?::\d+)?$/)) {
> 
> This is hard to validate by casual inspection.
> 
> I'm hesitant to use a regexp to stand in as a proxy for how our URL parser
> will ultimately treat this string. Given the security sensitivity here, I
> think we really need to invoke the nsIURI parser and inspect its individual
> elements to ensure that the passed-in provider corresponds to the host-part.

Sounds like a good idea. Feel free to look at how STUN URI parsing is pieced together from spare nsURI parts (Bug 825703).

Comment 43

[Martin Thomson [:mt:]]

(In reply to Adam Roach [:abr] from comment #41)
> It also occurs to me that a page loaded via file:///.../ is going to be able
> to assert identities of the form "adam.roach@", which could possibly be used
> to effect great confusion. Ditto for PCs invoked from add-ons (I know at
> least one add-on author taking advantage of this, and I'm sure we'll see
> many more as time goes on). We need to work through how we want these to be
> rendered. Minimally, we should put something after the @ that clues users in
> on the fact that this identity is something that they need to treat with
> extreme skepticism.

You are only able to generate assertions from 'https:' paths because we specifically load https://<provider>/.well-known/idp/<protocol>

Given that the above is hard-coded in, I don't see how it would be possible to generate assertions without a domain.  (I don't know what happens with an empty domain, but that's an easy check to add in.)

Comment 44

[Martin Thomson [:mt:]]

All these comments about formatting make me very sad.  This is not a comment about Adam's review, which is excellent, it's that Mozilla is wasting the time of people who could be doing much more important things on minutiae.

You could say that this is my fault, and it's true.  Sort of.  I could spend more time checking that I added spaces in the right places.  I probably won't though.  I prefer to work on understanding how nsIProvider can be implemented properly so that all this IdP stuff doesn't break.  It's one thing to demand a higher standard from patches, but it's easier to get that higher standard if you allow people to concentrate on the actual issue rather than wasting time checking boxes.

This problem has been solved (e.g., https://github.com/einars/js-beautify).  We're wasting everyone's time on what should be entirely automated.  I don't know where to escalate this, but I'm going to see if I can find out.

Oh, and I don't want to start a holy war, but 80 characters is stupid.  I don't think that this is an unreasonably long line, but it's more than 80 (and more than 100):
  setIdentityProvider: function PeerConnectionIdP_setIdentityProvider(provider, protocol, username) {
I mean FFS, we can't even get a component ID within the 80 character limit.

Comment 45

[Eric Rescorla (:ekr)]

Adam,

Since this is currently preffed off, I think it's fine to land with spoofable UI. Getting
this code landed is about getting the flow working, and then later we can clean up the
UX.

I am happy to have a separate bug for unpreffing that is gated on a UX bug.

Fair?

Comment 46

[Adam Roach [:abr]]

(In reply to Eric Rescorla (:ekr) from comment #45)
> I am happy to have a separate bug for unpreffing that is gated on a UX bug.
> 
> Fair?

Sure. But there's no process fail-safe here, so if we're going to go down that path, I'd like a comment in all.js, right before 'pref("media.peerconnection.identity", false);' indicating that bug XXXXX must be resolved before preffing on.

Comment 47

[Eric Rescorla (:ekr)]

That's fine with me.

Comment 48

[Max Jonas Werner [:Makkes]]

(In reply to Adam Roach [:abr] from comment #40)
> ::: dom/media/tests/mochitest/test_setIdentityProvider.html
> @@ +39,5 @@
> > +      test.next();
> > +    }
> > +  ],
> > +  [
> > +    "LOCAL_AND_REMOTE_DESC_DOES_NOT_INCLUDE_IDENTITY",
> 
> I think the sense of this test is exactly backwards, and that you're
> ensuring that our behavior is incorrect. I can't find anywhere in
> http://dev.w3.org/2011/webrtc/editor/webrtc.html#identity or
> http://tools.ietf.org/html/draft-ietf-rtcweb-security-arch-07 where we are
> supposed to exclude identity from the "localDescription" or
> "remoteDescription" attributes.

This test is actually the result of me discovering that localDescription and remoteDescription are immutable. I talked with ehugg (IIRC) about this and he pointed me to bug #784514. As long as that has not landed I fear that a=identity attributes won't be able to be stored in local/remoteDescription.

> @@ +35,5 @@
> > +      ok(!test.pcLocal._last_offer.sdp.contains("a=identity"), "a=identity not contained in the offer SDP");
> > +      ok(!test.pcRemote._last_answer.sdp.contains("a=identity"), "a=identity not contained in the answer SDP");
> > +      test.next();
> > +    }
> > +  ],
> 
> Add a test here to verify proper behavior of
> RTCPeerConnection.getIdentityAssertion() when the IdP returns "ERROR".

That would probably better fit into test_getIdentityAssertion.html.

Comment 49

[Adam Roach [:abr]]

(In reply to Max Jonas Werner from comment #48)

> This test is actually the result of me discovering that localDescription and
> remoteDescription are immutable. I talked with ehugg (IIRC) about this and
> he pointed me to bug #784514. As long as that has not landed I fear that
> a=identity attributes won't be able to be stored in local/remoteDescription.

Well, sure, but don't test for the wrong behavior.

Ideally, we would call down into PeerConnectionImpl.cpp, which would send a message to SIPCC that adds a fingerprint to the stored SDP. But, given that Ethan is working on a patch that probably makes this all a moot point, it's probably not worth the effort.

In any case, when the behavior changes to be correct, it would be nice if this test didn't suddenly break.

Comment 50

[Martin Thomson [:mt:]]

I think that localDescription and remoteDescription are perfectly mutable, so most of this discussion is moot.

Comment 51

[Martin Thomson [:mt:]]

Attachment: Adding the ability to carry a=identity to sipcc

Comment 52

[Martin Thomson [:mt:]]

Attachment: Adding the ability to carry a=identity to sipcc

Comment 53

[Martin Thomson [:mt:]]

Attachment: Identity assertion generation and verification for WebRTC

Comment 54

[Martin Thomson [:mt:]]

Attachment: Splitting identity event into two.

Comment 55

[Martin Thomson [:mt:]]

OK, fun times for all are to be had here.  If you intend to look at this, it's dependent on bug 878941 in a big way.  Other than that, this is ready for the next round of peeks and pokes.

Comment 56

[Adam Roach [:abr]]

Comment on attachment 8350842 [details] [diff] [review]
Adding the ability to carry a=identity to sipcc

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

Heading in the right direction. In general, I think you're over-specializing a bit: identity is a simple string, and there is already common handling for simple strings in SDP.

Also note that right now, these changes are only going to allow storing the identity received from the remote side. The way things are currently plumbed, our local identity won't make it down to the SIPCC layer. You'll want to make sure you coordinate this behavior with Ethan, as he's handling Bug 784514 and its sub-bugs.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1889,1 @@
>   * fingerprint     - fingerprint attribute to set

Thanks for cleaning this up. Can we add "sdp_attr" to the list?

Ideally, sdp_attr would be removed from this function, since it never makes sense to pass anything other than SDP_ATTR_FINGERPRINT -- but that's not the subject of this patch.

@@ +1926,5 @@
> + * identity     - attribute value to set
> + * identity_len - string len of fingerprint
> + */
> +static void
> +gsmsdp_set_identity_attribute (sdp_attr_e sdp_attr, uint16_t level, void *sdp_p,

Unless you can think of a scenario in which using something other than SDP_ATTR_IDENTITY makes sense, let's remove the sdp_attr parameter here, and hardcode it to SDP_ATTR_IDENTITY below.

@@ +1938,5 @@
> +        GSM_ERR_MSG("Failed to add attribute");
> +        return;
> +    }
> +
> +    result = sdp_attr_set_identity_attribute(sdp_p, level, 0, sdp_attr,

sdp_attr_set_simple_string

@@ +5793,5 @@
> +    /* For now, the dcb doesn't need to include identity, that is added at
> +     * the JavaScript layer; sipcc is able to carry, but not generate, identity
> +      if (dcb_p->identity)
> +      gsmsdp_set_identity_attribute(SDP_ATTR_IDENTITY, SDP_SESSION_LEVEL,
> +          dcb_p->sdp->src_sdp, dcb_p->identity); */

I think we're better of just omitting any changes here until (and unless) we move identity handling down to this layer.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +4776,5 @@
> +sdp_result_e sdp_parse_attr_identity_attr (sdp_t *sdp_p, sdp_attr_t *attr_p,
> +                                           const char *ptr)
> +{
> +    return sdp_parse_attr_fingerprint_attr(sdp_p, attr_p, ptr);
> +}

Remove this; see my comment below in sdp_main.c

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr_access.c
@@ +4339,5 @@
> + *              SDP_INVALID_PARAMETER Specified attribute is not defined.
> + */
> +sdp_result_e sdp_attr_get_identity_attribute (void *sdp_ptr, u16 level,
> +                                  u8 cap_num, sdp_attr_e sdp_attr, u16 inst_num,
> +                                  char **out)

This seems redundant with sdp_attr_get_simple_string.

@@ +4377,5 @@
> + * Returns:     SDP_SUCCESS            Attribute param was set successfully.
> + *              SDP_INVALID_PARAMETER  Specified attribute is not defined.
> + */
> +sdp_result_e sdp_attr_set_identity_attribute(void *sdp_ptr, u16 level,
> +        u8 cap_num, sdp_attr_e sdp_attr, u16 inst_num, const char *identity)

This seems redundant with sdp_attr_set_simple_string.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_main.c
@@ +168,5 @@
>        sdp_parse_attr_rtcp_mux_attr, sdp_build_attr_rtcp_mux_attr},
>      {"fingerprint", sizeof("fingerprint"),
>        sdp_parse_attr_fingerprint_attr, sdp_build_attr_simple_string},
> +    {"identity", sizeof("identity"),
> +      sdp_parse_attr_identity_attr, sdp_build_attr_simple_string},

Use sdp_parse_attr_simple_string here rather than sdp_parse_attr_identity_attr.

Basically, I want to avoid someone coming along later and deciding to rewrite sdp_parse_attr_fingerprint_attr to handle the algorithm and the fingerprint separately, thereby breaking identity in the process. Since identity needs no special handling, let's go ahead and use the the established pattern of just treating it as a simple string.

Comment 57

[Adam Roach [:abr]]

Comment on attachment 8350844 [details] [diff] [review]
Splitting identity event into two.

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

This all looks reasonable, assuming the WebRTC WG accepts the new event type as described in your email. Some nits:

PeerConnection.js:746: trailing whitespace
PeerConnection.js:749: Line is 82 characters long; please wrap to 80
PeerConnection.js:1062: Line is 90 characters long; please wrap to 80
PeerConnection.js:1080: Line is 90 characters long; please wrap to 80
PeerConnectionIdp.jsm:268: trailing whitespace
test_setIdentityProvider.html:33: trailing whitespace

::: dom/media/PeerConnectionIdp.jsm
@@ +264,5 @@
>        fingerprint, callback) {
>      if (!this._idpchannel) {
>        throw new Error("IdP not set");
>      }
> +    

GAH!

Comment 58

[Adam Roach [:abr]]

Comment on attachment 8350844 [details] [diff] [review]
Splitting identity event into two.

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

This all looks reasonable, assuming the WebRTC WG accepts the new event type as described in your email. Some nits:

PeerConnection.js:746: trailing whitespace
PeerConnection.js:749: Line is 82 characters long; please wrap to 80
PeerConnection.js:1062: Line is 90 characters long; please wrap to 80
PeerConnection.js:1080: Line is 90 characters long; please wrap to 80
PeerConnectionIdp.jsm:268: trailing whitespace
test_setIdentityProvider.html:33: trailing whitespace

::: dom/media/PeerConnectionIdp.jsm
@@ +264,5 @@
>        fingerprint, callback) {
>      if (!this._idpchannel) {
>        throw new Error("IdP not set");
>      }
> +    

GAH!

Comment 59

[Martin Thomson [:mt:]]

Attachment: 0001-Adding-the-ability-to-carry-a-identity-to-sipcc.patch

This should address the concerns from the review.

Comment 60

[Martin Thomson [:mt:]]

Attachment: 0003-Splitting-identity-event-into-two.patch

I'll follow up with 0002 later, assuming that you are still reviewing that one.

Comment 61

[Adam Roach [:abr]]

(In reply to Martin Thomson [:mt] from comment #60)
> Created attachment 8356918 [details] [diff] [review]
> 0003-Splitting-identity-event-into-two.patch
> 
> I'll follow up with 0002 later, assuming that you are still reviewing that
> one.

Yes, I'm still working on that. Thanks.

Comment 62

[Adam Roach [:abr]]

Comment on attachment 8356917 [details] [diff] [review]
0001-Adding-the-ability-to-carry-a-identity-to-sipcc.patch

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

r- for phantom function declarations. I would recommend simply pulling sdp.h and sdp_private.h out of this patch, as they do not need to change to provide the functionality you're adding.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp.h
@@ +2075,5 @@
> +    u8 cap_num, sdp_attr_e sdp_attr, u16 inst_num, const char *dtls_fingerprint);
> +
> +sdp_result_e
> +sdp_attr_get_identity_attribute(void *sdp_ptr, u16 level,
> +    u8 cap_num, sdp_attr_e sdp_attr, u16 inst_num, char **out);

Remove this declaration.

@@ +2080,5 @@
> +
> +sdp_result_e
> +sdp_attr_set_identity_attribute(void *sdp_ptr, u16 level,
> +    u8 cap_num, sdp_attr_e sdp_attr, u16 inst_num, const char *identity);
> +

Remove this declaration.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_private.h
@@ +213,5 @@
>  	sdp_t *sdp_p, sdp_attr_t *attr_p, const char *ptr);
>  extern sdp_result_e sdp_parse_attr_fingerprint_attr (
>      sdp_t *sdp_p, sdp_attr_t *attr_p, const char *ptr);
> +extern sdp_result_e sdp_parse_attr_identity_attr (
> +    sdp_t *sdp_p, sdp_attr_t *attr_p, const char *ptr);

Remove this declaration.

Comment 63

[Martin Thomson [:mt:]]

Attachment: 0001-a-identity-support-for-sipcc.patch

Sorry, not used to dealing with languages that put entities in two different places.

Comment 64

[Adam Roach [:abr]]

Comment on attachment 8357309 [details] [diff] [review]
0001-a-identity-support-for-sipcc.patch

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

r+ because we're not using the function below (and, as soon as we do, the problem will become apparent) -- but it would be good to fix this now anyway.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1944,5 @@
> +        GSM_ERR_MSG("Failed to add attribute");
> +        return;
> +    }
> +
> +    result = sdp_attr_set_simple_string(sdp_p, level, 0, SDP_ATTR_IDENTITY,

Sorry for not mentioning it earlier, but you'll need to add SDP_ATTR_IDENTITY to the list of allowed attributes inside sdp_attr_access.c:sdp_attr_set_simple_string.

Comment 65

[Martin Thomson [:mt:]]

Attachment: 0001-a-identity-support-for-sipcc.patch

No sense in doing this wrong.

Comment 66

[Adam Roach [:abr]]

Comment on attachment 8350843 [details] [diff] [review]
Identity assertion generation and verification for WebRTC

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

r- for RTCPeerConnection.setIdentityProvider being out-of-spec in webidl declaration

Nits:
IdpProxy.jsm:192: trailing whitespace
IdpProxy.jsm:243: trailing whitespace
PeerConnection.js:710: trailing whitespace
PeerConnection.js:752: trailing whitespace
PeerConnection.js:759: trailing whitespace
PeerConnection.js:769: trailing whitespace
PeerConnection.js:771: trailing whitespace
PeerConnection.js:871: Space between 'if' and parenthesis
PeerConnectionIdp.jsm:17: trailing whitespace
PeerConnectionIdp.jsm:45: trailing whitespace
PeerConnectionIdp.jsm:161: trailing whitespace
PeerConnectionIdp.jsm:164: trailing whitespace
PeerConnectionIdp.jsm:172: trailing whitespace
PeerConnectionIdp.jsm:188: trailing whitespace

And, depending on the outcome of the ongoing line-length discussions:
PeerConnection.js:375: Line is 88 characters long; please wrap to 80
PeerConnection.js:686: Line is 87 characters long; please wrap to 80
PeerConnection.js:724: Line is 84 characters long; please wrap to 80
PeerConnection.js:753: Line is 93 characters long; please wrap to 80
PeerConnection.js:756: Line is 81 characters long; please wrap to 80
PeerConnection.js:763: Line is 86 characters long; please wrap to 80
PeerConnectionIdp.jsm:54: Line is 81 characters long; please wrap to 80
PeerConnectionIdp.jsm:101: Line is 81 characters long; please wrap to 80
PeerConnectionIdp.jsm:108: Line is 89 characters long; please wrap to 80
pc.js:1097: Line is 87 characters long; please wrap to 80

::: dom/media/PeerConnection.js
@@ +362,1 @@
>                               Services.tm.currentThread);

Reindent to match original style (align with opening paren).

@@ +680,5 @@
>              "Invalid type " + desc.type + " provided to setRemoteDescription");
>      }
>  
> +    // wait for the session to be applied successfully
> +    // otherwise the IdP code is fully exposed to bad SDP

This seems to be of questionable value. Nearly any attack that I can envision could just as easily be mounted within the context of syntactically valid SDP.

@@ +796,1 @@
>                                    (cand.sdpMLineIndex === null)? 0 :

Re-indent to match original style

::: dom/media/PeerConnectionIdp.jsm
@@ +23,5 @@
> +  this._win = window;
> +  this._timeout = timeout || 5000;
> +  this._warning = warningFunc;
> +
> +  this.assertion = null;

this.provider = null;

@@ +36,5 @@
> +  PeerConnectionIdp._fingerprintPattern = new RegExp(pattern, "m");
> +})();
> +
> +PeerConnectionIdp.prototype = {
> +  setIdentityProvider: function PeerConnectionIdp_setIdentityProvider(

This format of function declaration is apparently deprecated (cf. Bug 895603). This comment applies to all the methods in this class.

@@ +43,5 @@
> +    this._idpchannel = new IdpProxy(provider, protocol, username);
> +  },
> +  
> +  close: function PeerConnectionIdp_close() {
> +    this.provider = null;

this.assertion = null?

@@ +152,5 @@
> +      let providerPortIdx = provider.indexOf(":");
> +      if (providerPortIdx > 0) {
> +        provider = provider.substring(0, providerPortIdx);
> +      }
> +      if (tail.toLowerCase() !== provider.toLowerCase()) {

Please at least add a comment here that this is potentially fragile in IDN circumstances, and may need some fine-tuning.

@@ +316,5 @@
> +    return function idpDone(message) {
> +      if (!timeout) {
> +        return;
> +      }
> +      this._win.clearTimeout(timeout);

timeout = null;

We don't want a misbehaving IDP to provide multiple responses for this message, right?

::: dom/media/tests/mochitest/test_getIdentityAssertion.html
@@ +22,5 @@
> +  [
> +    "GET_IDENTITY_ASSERTION_FAILS_WITHOUT_PROVIDER",
> +    function(test) {
> +      test.pcLocal._pc.getIdentityAssertion(function(err) {
> +        ok(err, "getIdentityAssertion must throw without provider");

s/throw/invoke error callback/

::: dom/media/tests/mochitest/test_setIdentityProvider.html
@@ +63,5 @@
> +  [
> +    "OFFERS_AND_ANSWERS_INCLUDE_IDENTITY",
> +    function(test) {
> +      ok(test.pcLocal._last_offer.sdp.contains("a=identity"), "a=identity is contained in the offer SDP");
> +      ok(test.pcRemote._last_answer.sdp.contains("a=identity"), "a=identity is contained in the offer SDP");

s/offer SDP/answer SDP/

@@ +71,5 @@
> +  [
> +    "DESCRIPTIONS_CONTAIN_IDENTITY",
> +    function(test) {
> +      ok(test.pcLocal.localDescription.sdp.contains("a=identity"), "a=identity is contained in the local description");
> +      ok(test.pcRemote.localDescription.sdp.contains("a=identity"), "a=identity is contained in the local description");

Please make these strings differentiable by specifying which PC is being tested.

::: dom/media/tests/mochitest/test_setIdentityProviderWithErrors.html
@@ +31,5 @@
> +      test.next();
> +    }
> +  ],
> +  [
> +    "OFFERS_AND_ANSWERS_INCLUDE_IDENTITY",

..._DONT_INCLUDE_...

::: dom/webidl/RTCPeerConnection.webidl
@@ +87,5 @@
>  // moz-prefixed until sufficiently standardized.
>  interface mozRTCPeerConnection : EventTarget  {
> +  [Pref="media.peerconnection.identity.enabled"]
> +  void setIdentityProvider (DOMString provider,
> +                            DOMString protocol,

"protocol" is marked optional in the current spec (cf http://dev.w3.org/2011/webrtc/editor/webrtc.html#rtcpeerconnection-interface-extensions-3). Our code seems to handle a missing protocol just fine; but my understanding of how webidl works means that this declaration will prevent calling code from omitting protocol.

::: dom/webidl/RTCPeerConnectionIdentityEvent.webidl
@@ +3,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/.
> + *
> + * The origin of this IDL file is
> + * http://dev.w3.org/2011/webrtc/editor/webrtc.html#idl-def-RTCPeerConnectionIdentityEvent

I don't think so.

Google only finds it here: http://lists.w3.org/Archives/Public/public-webrtc/2013Dec/0104.html

Comment 67

[Adam Roach [:abr]]

After a brief discussion with EKR, we've concluded that the toLowercase needs to have a comment with a "TODO (Bug xxxxx)", with the corresponding Bug xxxxx blocking preffing on.

The crux of the matter here is that we can't perform a transformation that might cause our comparison to produce "true" when the underlying hosts might resolve to different authorities. So we either need an exact match, or we need to perform nameprep before comparison (which would involve putting in an xpcom bridge to the nameprep code we use for hostname processing).

Comment 68

[Martin Thomson [:mt:]]

Regarding toLowercase, I just followed the origin restrictions code down to the bottom.

The thing that this doesn't have is the IDN ACE conversion that is done here:
http://dxr.mozilla.org/mozilla-central/source/netwerk/base/src/nsStandardURL.cpp#1077

Raised bug 958741 to cover that, will add comment (and pref-block) accordingly.

Comment 69

[Martin Thomson [:mt:]]

Attachment: 0002-Identity-assertion-generation-and-verification-for-W.patch

Fixed everything you found.

Comment 70

[Martin Thomson [:mt:]]

Attachment: 0003-Splitting-identity-event-into-two.patch

Updating 0003 to prevent apply failure due to changes in 0002.

Comment 71

[Martin Thomson [:mt:]]

Attachment: 0002-Identity-assertion-generation-and-verification-for-W.patch

This covers the code review, plus a new bug that introduced (yay, tests!).

Comment 72

[Martin Thomson [:mt:]]

Attachment: 0003-Splitting-identity-event-into-two.patch

Resetting baseline.  Carrying forward r+ from abr.

Comment 73

[Martin Thomson [:mt:]]

Comment on attachment 8357361 [details] [diff] [review]
0001-a-identity-support-for-sipcc.patch

Carrying forward r+ from abr.

Comment 74

[Adam Roach [:abr]]

Comment on attachment 8358704 [details] [diff] [review]
0002-Identity-assertion-generation-and-verification-for-W.patch

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

Looks good to me.

Comment 75

[Martin Thomson [:mt:]]

Attachment: 0002-Identity-assertion-generation-and-verification-for-W.patch

Unbitrotting based on changes in bug 878941.  Carrying r+ from abr forward.

Comment 76

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6779278f87df
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e6c7217e92a
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb7097d12256

Comment 77

[Ryan VanderMeulen [:RyanVM]]

Backed out for Cpp unit test failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4aadf8ea4e0

https://tbpl.mozilla.org/php/getParsedLog.php?id=32979827&tree=Mozilla-Inbound

Also, we hit bustage on inbound because I mistakenly thought that this depended on bug 878941, but it was actually the opposite. Please be careful when setting dependencies so that bugs don't get landed in the wrong order.

Comment 78

[Ryan VanderMeulen [:RyanVM]]

And crashtest failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=32980583&tree=Mozilla-Inbound

Please make sure this has a full Try run before requesting checkin again.

Comment 79

[Florian Quèze [:florian]]

Comment on attachment 8359356 [details] [diff] [review]
0002-Identity-assertion-generation-and-verification-for-W.patch

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

::: dom/media/PeerConnection.js
@@ +733,5 @@
> +      .QueryInterface(Ci.nsIDocShell)
> +      .chromeEventHandler
> +      .ownerDocument
> +      .defaultView
> +      .gBrowser;

This is touching Firefox Desktop specific things in a component of the platform. This seems surprising (I would expect the code to send a notification through nsIObserverService, and the UI to handle the notification), so I think the patch should have a review from a browser peer before landing.

@@ +745,5 @@
> +      // for one, they are highly spoofable, which is very bad
> +      // but they are also ugly and unnecessary
> +      // Bug 942372 should provide a better approach
> +      notificationBox.appendNotification(
> +          "Identity of your WebRTC peer is " + this._peerIdentity.name,

This string looks like it will be shown in the UI, so it should be localized.

@@ +758,5 @@
> +                                                            args);
> +      this.dispatchEvent(ev);
> +    } else {
> +      notificationBox.appendNotification(
> +          "Identity of your WebRTC peer is not verified",

Same here.

Comment 80

[Adam Roach [:abr]]

(In reply to Florian Quèze [:florian] [:flo] from comment #79)
> Comment on attachment 8359356 [details] [diff] [review]
> 0002-Identity-assertion-generation-and-verification-for-W.patch
> 
> Review of attachment 8359356 [details] [diff] [review]:
> -----------------------------------------------------------------

> @@ +745,5 @@
> > +      // for one, they are highly spoofable, which is very bad
> > +      // but they are also ugly and unnecessary
> > +      // Bug 942372 should provide a better approach
> > +      notificationBox.appendNotification(
> > +          "Identity of your WebRTC peer is " + this._peerIdentity.name,
> 
> This string looks like it will be shown in the UI, so it should be localized.
> 
> @@ +758,5 @@
> > +                                                            args);
> > +      this.dispatchEvent(ev);
> > +    } else {
> > +      notificationBox.appendNotification(
> > +          "Identity of your WebRTC peer is not verified",
> 
> Same here.

For the l18n issues, I think we want to open a separate bug, and block preffing on until the strings are localized. In particular, I'll note that notificationBox isn't even the final form this alert will take (due to spoofing concerns), so this is inherently temporary behavior.

Comment 81

[Adam Roach [:abr]]

Comment on attachment 8359356 [details] [diff] [review]
0002-Identity-assertion-generation-and-verification-for-W.patch

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

::: dom/media/PeerConnection.js
@@ +733,5 @@
> +      .QueryInterface(Ci.nsIDocShell)
> +      .chromeEventHandler
> +      .ownerDocument
> +      .defaultView
> +      .gBrowser;

This is temporary behavior for a feature that's hidden behind a pref. Bug 942372 will need to address this in a more universal fashion. I don't think we need to block landing this patch on putting a generalized approach in this throwaway code.

Comment 82

[Ryan VanderMeulen [:RyanVM]]

And mochitest-3 failures.
https://tbpl.mozilla.org/php/getParsedLog.php?id=32981376&tree=Mozilla-Inbound

Comment 83

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Adam Roach [:abr] from comment #74)
> Comment on attachment 8358704 [details] [diff] [review]
> 0002-Identity-assertion-generation-and-verification-for-W.patch
> 
> Review of attachment 8358704 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good to me.

This is not acceptable, Adam. test_interfaces.html clearly states that only a DOM peer can review changes to it, which you are not. Martin, you should have known better than to ask for a review from someone who doesn't have the power to grant it.

Comment 84

[:Gavin Sharp [email: [e-mail]]]

(In reply to Adam Roach [:abr] from comment #81)
> This is temporary behavior for a feature that's hidden behind a pref. Bug
> 942372 will need to address this in a more universal fashion. I don't think
> we need to block landing this patch on putting a generalized approach in
> this throwaway code.

Why is landing this so pressing that you need to take un-shippable shortcuts?

Comment 85

[Eric Rescorla (:ekr)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #84)
> (In reply to Adam Roach [:abr] from comment #81)
> > This is temporary behavior for a feature that's hidden behind a pref. Bug
> > 942372 will need to address this in a more universal fashion. I don't think
> > we need to block landing this patch on putting a generalized approach in
> > this throwaway code.
> 
> Why is landing this so pressing that you need to take un-shippable shortcuts?

This is a reasonably big feature in terms of complexity and we want to get the plumbing
pieces in place so that we can experiment with two pieces which can then
be worked on independently, namely the UX and the identity implementations
proper. Cleaning up the L10N is part of that, but since this isn't anything
like the final UI, and it's preffed off, I don't think this is a blocker.

Comment 86

[Martin Thomson [:mt:]]

After a brief chat with Ms2ger, it's clear that we can mark the new event [ChromeOnly].  That doesn't mean we can avoid changing the spec, but it does mean that we avoid increasing the DOM surface area (win).  I'm testing that change out now, and it looks OK so far.

With respect to the UX stuff, the plan is to rip this entirely.  The ultimate goal (something I'm working on now) is to update the WebRTC doorhanger.  L10N is naturally part of that.  The crappy UX only existed to provide us with something to point it in demos.  I can always patch that in on private builds.  Would people be happier (:Gavin, :florian) if this crappy UX were removed entirely?

Comment 87

[:Gavin Sharp [email: [e-mail]]]

(In reply to Eric Rescorla (:ekr) from comment #85)
> This is a reasonably big feature in terms of complexity and we want to get
> the plumbing
> pieces in place so that we can experiment with two pieces which can then
> be worked on independently, namely the UX and the identity implementations
> proper. Cleaning up the L10N is part of that, but since this isn't anything
> like the final UI, and it's preffed off, I don't think this is a blocker.

"Fix l10n and remove UI hacks later" isn't really acceptable in the general case, and I still don't really see anything here that merits an exception. Dealing with UI stuff in a followup is fine, pushing broken UI code to m-c is not.

If there are team-dynamics/communication problems that are making it difficult to develop the UI bits properly, I'd be happy to discuss how they could be improved.

Comment 88

[Eric Rescorla (:ekr)]

(In reply to :Gavin Sharp (email [e-mail]) from comment #87)
> (In reply to Eric Rescorla (:ekr) from comment #85)
> > This is a reasonably big feature in terms of complexity and we want to get
> > the plumbing
> > pieces in place so that we can experiment with two pieces which can then
> > be worked on independently, namely the UX and the identity implementations
> > proper. Cleaning up the L10N is part of that, but since this isn't anything
> > like the final UI, and it's preffed off, I don't think this is a blocker.
> 
> "Fix l10n and remove UI hacks later" isn't really acceptable in the general
> case, and I still don't really see anything here that merits an exception.
> Dealing with UI stuff in a followup is fine, pushing broken UI code to m-c
> is not.

So based on this you would be happy to have us simply pull the UI at this
point? That's satisfactory to me.


> If there are team-dynamics/communication problems that are making it
> difficult to develop the UI bits properly, I'd be happy to discuss how they
> could be improved.

Comment 89

[:Gavin Sharp [email: [e-mail]]]

Sure!

Comment 90

[Martin Thomson [:mt:]]

Getting errors in Try that don't appear on regular builds that seem to be caused by the sandbox.

Most interesting case is here:
 3 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/media/tests/mochitest/test_idpproxy.html | uncaught exception - NS_ERROR_FAILURE: Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIAppShellService.hiddenDOMWindow] at resource://gre/modules/identity/Sandbox.jsm:80

http://dxr.mozilla.org/mozilla-central/source/toolkit/identity/Sandbox.jsm#80

At first, I was concerned that this was basically untried code, but hiddenDOMWindow (the line in question) seems to be accessed all over the place already, including in mochitests.  So now I think that I'm doing something wrong.

https://tbpl.mozilla.org/?tree=Try&rev=ad24496ff248

Comment 91

[Bobby Holley (:bholley)]

Sorry for the delay - work week last week sucked up most of my bandwidth.

So, the error is coming from here:

http://mxr.mozilla.org/mozilla-central/source/xpfe/appshell/src/nsAppShellService.cpp#644

Does this reproduce in a debug build? It would be helpful to get the NS_ENSURE spew to see where we go wrong.

FWIW, the try run appears to have gone away, so I can't see the code that triggers the issue.

Comment 92

[Martin Thomson [:mt:]]

Debug try @ https://tbpl.mozilla.org/?tree=Try&rev=b9d9bcd06ac4

The actual problem is that mHiddenWindow is null, at least by the time it reaches the second failed test.  I added a bunch of debug to the code that runs in content in the sandbox.  A bunch of console.log calls, which I hooked to dump().  It looks like the sandbox isn't even being loaded.  (Well, no output is never reassuring, but when it works on my machine, I get plenty of output.)

Basically, there should be a bunch of +++DOMWINDOW lines immediately after the line that has "23602 INFO TEST-PASS", but that never even happens.  Once try re-opens I've a patch that enables logging of sandbox message to stderr as well, but I don't expect to get much more useful info from that.

Comment 93

[Bobby Holley (:bholley)]

It's probably worth adding a bunch of logging throughout the mHiddenWindow lifetime management stuff in nsAppShellService.cpp to figure out when it goes away (is DestroyHiddenWindow being called? If so, why?).

Comment 94

[Martin Thomson [:mt:]]

Status report.  I have narrowed my issues down to a mochitest problem on Android and an e10s issue on FirefoxOS.  The former needs to be fixed.  The latter requires some structural work in either the IdP bridging code, or the sandbox itself and I plan to move this to another bug.

Problems demonstrated in: https://tbpl.mozilla.org/?tree=Try&rev=d49018132f80  (cpp test problems fixed already, android problems on mochitest 6, b2g issues on mochitest 9 & 10, others are inexplicable, but seem to be not related/intermittent).

Comment 95

[Martin Thomson [:mt:]]

Attachment: 0001-Bug-884573-Add-a-identity-support-for-sipcc.patch

Carrying forward r+ from abr.

Comment 96

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-884573-Identity-assertion-generation-and-verific.patch

This is a squash of the two r+'d patches (abr).  As I mentioned in https://bugzilla.mozilla.org/show_bug.cgi?id=878941#c59 , this is not running tests on android and B2G, but it does seem to be passing finally.

Try run here: https://tbpl.mozilla.org/?tree=Try&rev=ec6a5f183be7

Will attempt to land tomorrow absent objections.

Comment 97

[Bobby Holley (:bholley)]

Does this need to include the machinery from bug 901261? If not, how does that stuff fit in here?

Comment 98

[Martin Thomson [:mt:]]

(In reply to Bobby Holley (:bholley) from comment #97)
> Does this need to include the machinery from bug 901261? If not, how does
> that stuff fit in here?

This is a building block that contributes toward bug 901261.  There are a lot of other pieces to do before that is done.

Comment 99

[Martin Thomson [:mt:]]

Just realized that I should have marked Bug 878941 checkin-needed first, correcting that mistake (if indeed it is :)

Comment 100

[Martin Thomson [:mt:]]

Attachment: 0002-Bug-884573-Identity-assertion-generation-and-verific.patch

Fixing commit author.  Carrying forward r+ from abr.

Comment 101

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e633c972f226
https://hg.mozilla.org/integration/mozilla-inbound/rev/dad774c849bb

Comment 102

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

https://hg.mozilla.org/mozilla-central/rev/e633c972f226
https://hg.mozilla.org/mozilla-central/rev/dad774c849bb