Open Bug 1329028 Opened 7 years ago Updated 2 years ago

DTLS setup role should not switch during renegotiations

Categories

(Core :: WebRTC: Signaling, defect, P3)

defect

Tracking

()

Tracking Status
firefox53 --- affected

People

(Reporter: drno, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Bug 1323723 showed that we actually switch a=setup to actpass for every subsequent offer.

There is discussion going on here https://github.com/rtcweb-wg/jsep/issues/448
and here https://groups.google.com/forum/#!msg/discuss-webrtc/DfpIMwvUfeM/VgCtnJ0cEgAJ

But I think we should (assuming the discussion doesn't change direction completely):
- a subsequent offer sticks for existing m-sections to the role which it previously had negotiated 
- new m-section get offered with a=actpass
- remote re-offer does not switch previously agreed on roles
- only the bundle master m-section requires to have a=setup
- ICE restart resets roles to actpass
After the discussion concluded the todo list is this now:
- a subsequent offer sticks for existing m-sections to the role which it previously had negotiated, but always offer actpass in subsequent offers
- new m-section get offered with a=actpass
- remote re-offer does not switch previously agreed on roles, but start with actpass as well
- only the bundle master m-section requires to have a=setup
- ICE restart resets roles to actpass
Blocks: 1323723
Attachment #8824251 - Attachment is obsolete: true
Attachment #8848710 - Flags: review?(docfaraday)
Attachment #8848711 - Flags: review?(docfaraday)
Attachment #8824250 - Flags: review?(docfaraday)
Miguel,

this build should now finally implement the proper behavior regarding the a=setup attribute: https://queue.taskcluster.net/v1/task/Tq7hCGf6RcqD5p0OvzQGMA/runs/0/artifacts/public/build/target.tar.bz2

Would appreciate if you could test it and let me know what the result is.
Comment on attachment 8824250 [details]
Bug 1329028: add more unit tests for a=setup

https://reviewboard.mozilla.org/r/102762/#review123930
Attachment #8824250 - Flags: review?(docfaraday) → review+
Comment on attachment 8848710 [details]
Bug 1329028: removed unused function arguments

https://reviewboard.mozilla.org/r/121604/#review123934
Attachment #8848710 - Flags: review?(docfaraday) → review+
Comment on attachment 8848711 [details]
Bug 1329028: compare a=setup against existing transports.

https://reviewboard.mozilla.org/r/121606/#review123936

This code doesn't handle bundle renegotiation quite right, and some other cases.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1137
(Diff revision 1)
> +  if (mCurrentLocalDescription && !mRemoteIceIsRestarting) {
> +    size_t level = remoteMsection.GetLevel();
> +    for (auto& trackPair: mNegotiatedTrackPairs) {
> +      if ((trackPair.mLevel = level) && trackPair.HasBundleLevel()) {
> +        level = trackPair.BundleLevel();
> +      }
> +    }
> +    if (mTransports.size() > level) {
> +      const JsepTransport* transp = mTransports.at(level);
> +      if (transp->mComponents > 0) {
> +        JsepDtlsTransport::Role currentRole = transp->mDtls->GetRole();
> +        switch (currentRole) {
> +          case JsepDtlsTransport::kJsepDtlsClient:
> +            role = SdpSetupAttribute::kActive;
> +            break;
> +          case JsepDtlsTransport::kJsepDtlsServer:
> +            role = SdpSetupAttribute::kPassive;
> +            break;
> +          default:
> +            MOZ_CRASH();
> +        }
> +      }
> +    }
> +  }

This code block seems to be assuming that we will reuse the previous transport. This is not necessarily the case.

If this m-section is being turned from a master to a slave, it is not valid to use any of the previous transport stuff.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1138
(Diff revision 1)
>          return NS_ERROR_INVALID_ARG;
>      }
>    }
>  
> +  if (mCurrentLocalDescription && !mRemoteIceIsRestarting) {
> +    size_t level = remoteMsection.GetLevel();

Let's call this transportLevel.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1139
(Diff revision 1)
>      }
>    }
>  
> +  if (mCurrentLocalDescription && !mRemoteIceIsRestarting) {
> +    size_t level = remoteMsection.GetLevel();
> +    for (auto& trackPair: mNegotiatedTrackPairs) {

const auto&

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2062
(Diff revision 1)
> +      mCurrentRemoteDescription->GetMediaSection(i);
> +    if (mSdpHelper.MsectionIsDisabled(newMsection) ||
> +        mSdpHelper.MsectionIsDisabled(oldMsection)) {
> +      continue;
> +    }
> +

Ok, this is really tricky, but simple at the same time. There are some invariants that can guide us.

1. If the new m-section is a slave or disabled, the transport attrs don't matter, and don't need to be validated.
2. If the new m-section is _not_ a slave or disabled, but it _was_ a slave or disabled before, it has a new transport, and no validation is needed.

The upshot of these invariants is that:
1. If transports[i] is closed, no validation is needed (this means this level was either slave or disabled before, see invariant 2).
2. In addition, if the new m-section is a slave or disabled, no validation is needed (invariant 1).
   - A note: If the remote description is an offer, it can be difficult to tell _for sure_ if it is a slave or not, because if bundle is offered we technically don't need to accept it in our answer. We won't do that _now_ though. It could happen in the future. So, to be safe, if we're looking at a remote _offer_, we should only consider it a bundle slave if it is bundled (SdpHelper::IsBundleSlave) _and_ has a=bundle-only. If looking at a remote _answer_, we can tell with just SdpHelper::IsBundleSlave().
3. If neither 1 or 2 holds, we need to validate the m-section against transports[i].

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2083
(Diff revision 1)
> +    const JsepTransport* transp = transports->at(transportLevel);
> +    if (transp->mComponents == 0) {
> +      // A closed transport... hmmm
> +      continue;
> +    }
> +    SdpSetupAttribute::Role newRemoteRole = SdpSetupAttribute::kPassive;

Doesn't this default value depend on whether it is an offer or answer?

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2088
(Diff revision 1)
> +    SdpSetupAttribute::Role newRemoteRole = SdpSetupAttribute::kPassive;
> +    if (newMsection.GetAttributeList().HasAttribute(
> +          SdpAttribute::kSetupAttribute, true)) {
> +      newRemoteRole = newMsection.GetAttributeList().GetSetup().mRole;
> +    }
> +    if (newRemoteRole != SdpSetupAttribute::kActpass) {

Valid in offers, not in answers.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2089
(Diff revision 1)
> +    if (newMsection.GetAttributeList().HasAttribute(
> +          SdpAttribute::kSetupAttribute, true)) {
> +      newRemoteRole = newMsection.GetAttributeList().GetSetup().mRole;
> +    }
> +    if (newRemoteRole != SdpSetupAttribute::kActpass) {
> +      JsepDtlsTransport::Role currentRole = transp->mDtls->GetRole();

Maybe would be more readable if:

    switch (transp->mDtls->GetRole()) {
      case JsepDtlsTransport::kJsepDtlsServer:
        oldRemoteRole = SdpSetupAttribute::Passive;
        break;
      ...
    }

    if (oldRemoteRole != newRemoteRole) {
    ...
    }
Attachment #8848711 - Flags: review?(docfaraday) → review-
Comment on attachment 8848711 [details]
Bug 1329028: compare a=setup against existing transports.

https://reviewboard.mozilla.org/r/121606/#review123936

> Doesn't this default value depend on whether it is an offer or answer?

Yes. But since our new code rejects offers without a=setup and only lets answers w/o pass I should use the answers default value. SO I picked the wrong value here. I'll update it and put a comment here.

> Valid in offers, not in answers.

Right. But the point here is not to check for valid parameters. The previous bug rejects answers with actpass in it already. The important thing here is that who ever has actpass in its SDP does not try to switch roles right now, so we can skip the check.
Hello Nils,
I have already tested it and it is working fine receiving subsequents SDP offers with "a=setup:actpass".
I have also seen that subsequents SDP offers with "a=setup:active" are accepted not following JSEP. Have you done this to support interoperability with older Firefox versions?
(In reply to mparisdiaz from comment #12)
> I have already tested it and it is working fine receiving subsequents SDP
> offers with "a=setup:actpass".
> I have also seen that subsequents SDP offers with "a=setup:active" are
> accepted not following JSEP. Have you done this to support interoperability
> with older Firefox versions?

Great. That helps to know it works now.
JSEP says we should send out new offers with 'actpass'. But it is my understanding that one could still offer either it current role or 'actpass' (if you don't obey to JSEP). So to be able to interoperate with such clients we accept both, as long as the other side does not attempt to switch roles.
Comment on attachment 8848711 [details]
Bug 1329028: compare a=setup against existing transports.

https://reviewboard.mozilla.org/r/121606/#review123936

> This code block seems to be assuming that we will reuse the previous transport. This is not necessarily the case.
> 
> If this m-section is being turned from a master to a slave, it is not valid to use any of the previous transport stuff.

So after tinkering around some time I think this comes down to the following:

- if the m-section is bundle master we use the code above from this patch
- if the m-section is a bundle slave we two options:
  a) basically omit all transport related attributes as they are technically not needed - this breaks a hell lot of our unit test
  b) copy the transport parameters from the bundle master section. The fun here is that the bundle master could be added to the answer after processing this slave section. So I guess we would need to set/add the transport parameters to all m-sections of the answer once we are done with building the answer.
Byron what do you think about comment #14?
Flags: needinfo?(docfaraday)
> JSEP says we should send out new offers with 'actpass'. But it is my
> understanding that one could still offer either it current role or 'actpass'
> (if you don't obey to JSEP). So to be able to interoperate with such clients
> we accept both, as long as the other side does not attempt to switch roles.

Good point!! ;)
(In reply to Nils Ohlmeier [:drno] from comment #14)
> Comment on attachment 8848711 [details]
> Bug 1329028: compare a=setup against existing transports
> 
> https://reviewboard.mozilla.org/r/121606/#review123936
> 
> > This code block seems to be assuming that we will reuse the previous transport. This is not necessarily the case.
> > 
> > If this m-section is being turned from a master to a slave, it is not valid to use any of the previous transport stuff.
> 
> So after tinkering around some time I think this comes down to the following:
> 
> - if the m-section is bundle master we use the code above from this patch
> - if the m-section is a bundle slave we two options:
>   a) basically omit all transport related attributes as they are technically
> not needed - this breaks a hell lot of our unit test
>   b) copy the transport parameters from the bundle master section. The fun
> here is that the bundle master could be added to the answer after processing
> this slave section. So I guess we would need to set/add the transport
> parameters to all m-sections of the answer once we are done with building
> the answer.

We are already doing some parts of "b" I think. "b" also interops better with buggy implementations that pay attention to transport params on slave m-sections.

Which tests does "a" break? I'm guessing stuff in jsep_session_unittest (maybe the candidate tests).
Flags: needinfo?(docfaraday)
Blocks: 1353633
Comment on attachment 8848711 [details]
Bug 1329028: compare a=setup against existing transports.

https://reviewboard.mozilla.org/r/121606/#review129546

Man, this bundle spec is gross. We talked on IRC about transports following their m-section.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:988
(Diff revisions 1 - 2)
> +  if (!mCurrentLocalDescription ||
> +      mRemoteIceIsRestarting) {
> +    // for renegotiation answers this is done later in
> +    // CopyPreviousTransportParams()
> -  SdpSetupAttribute::Role role;
> +    SdpSetupAttribute::Role role;
> -  rv = DetermineAnswererSetupRole(remoteMsection, &role);
> +    rv = DetermineInitialAnswererSetupRole(remoteMsection, &role);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
>  
> -  rv = AddTransportAttributes(&msection, role);
> +    rv = AddTransportAttributes(&msection, role);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +  }

Let's just leave this be, and stomp the value in CopyPreviousTransportParams.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1648
(Diff revisions 1 - 2)
> +  // First fill in the sections which are not bundle slaves
> +  for (size_t i = 0; i < newLocal->GetMediaSectionCount(); ++i) {

I think we probably want to leave this loop alone, mostly. I decided to check |mRemoteIceIsRestarting| inside the loop so it is not hard to add support for partial ICE restart later.

I guess we should tweak it so that if newLocal is an answer, we will not copy any transport attrs into bundle slaves. (Right now we will do this for both offer and answer, we should only be doing this for offers)

As for a=setup, if we end up calling SdpHelper::CopyTransportParams(), we override based on what is in mTransports.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1653
(Diff revisions 1 - 2)
> +    if (newLocal->GetMediaSection(i).GetAttributeList().HasAttribute(
> +          SdpAttribute::kBundleOnlyAttribute) &&
> +        mSdpHelper.IsBundleSlave(*newLocal, i)) {
> +      continue;
> +    }
> +    if (!mSdpHelper.IsBundleSlave(*newLocal, i)) {

If I'm not mistaken, this is equivalent to

    if (mSdpHelper.IsBundleSlave(*newLocal, i)) {
      continue;
    }

I _think_ this is ok, but let's write it simpler.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1666
(Diff revisions 1 - 2)
> -                                              offerersPreviousSdp,
> +                                                offerersPreviousSdp,
> -                                              newOffer,
> +                                                newOffer,
> -                                              i) &&
> -        !mRemoteIceIsRestarting
> -       ) {
> -      // If newLocal is an offer, this will be the number of components we used
> +                                                i)) {
> +        // Bundle master has changed, so we try to take the previous bundle
> +        // master as the reference instead
> +        bool transportLevelUnkown = false;

Typo here.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1667
(Diff revisions 1 - 2)
> -      // last time, and if it is an answer, this will be the number of
> +        if (mSdpHelper.MsectionIsDisabled(oldAnswer.GetMediaSection(i))) {
> +          const auto previousMasterBundleMsection(
> +              mSdpHelper.GetFirstBundleMasterMediaSection(oldAnswer));
> +          if (previousMasterBundleMsection) {
> +            previousTransportLevel = previousMasterBundleMsection->GetLevel();
> +          } else {
> +            transportLevelUnkown = true;
> +          }
> +        } else {
> +          const auto previousMasterBundleMsection(
> +              mSdpHelper.GetBundleMasterMediaSection(oldAnswer, i));
> +          if (previousMasterBundleMsection) {
> +            previousTransportLevel = previousMasterBundleMsection->GetLevel();
> +          } else {
> +            transportLevelUnkown = true;
> +          }
> +        }

What if we have two non-slave m-sections like this, but only one master from last time? Both cannot have its transport.

I think we should never copy transport attributes to an m-section at a different level than before.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1731
(Diff revisions 1 - 2)
> +  // Then fill the bundle slave sections from the bundle master sections
> +  for (size_t i = 0; i < newLocal->GetMediaSectionCount(); ++i) {
> +    auto& newMsection = newLocal->GetMediaSection(i);
> +    if (mSdpHelper.MsectionIsDisabled(newMsection)) {
> +      continue;
> +    }
> +    /* TODO re-enable this as part of bug 1353633
> +    if (newOffer.GetMediaSection(i).GetAttributeList().HasAttribute(
> +          SdpAttribute::kBundleOnlyAttribute) &&
> +        mSdpHelper.IsBundleSlave(newOffer, i)) {
> +      continue;
> +    }
> +    */
> +    if (mSdpHelper.IsBundleSlave(*newLocal, i)) {
> +      // Technically bundle slave sections only need transport parameters in
> +      // offers (to allow unbundeling), but we copy them also in answer to avoid
> +      // clients complaining about the missing transport parameters.
> +      const SdpMediaSection* masterBundleMsection(
> +          mSdpHelper.GetBundleMasterMediaSection(*newLocal, i));
> +      if (!masterBundleMsection) {
> +        continue;
> +      }
> +      // lets assume by default everything bundled also uses rtcp-mux
> +      size_t numComponents = 1;
> +      if (mSdpHelper.HasRtcp(newMsection.GetProtocol()) &&
> +          !masterBundleMsection->GetAttributeList().HasAttribute(
> +            SdpAttribute::kRtcpMuxAttribute)) {
> +        numComponents = 2;
> +      }
> +      nsresult rv = mSdpHelper.CopyTransportParams(
> +          numComponents,
> +          *masterBundleMsection,
> +          &newLocal->GetMediaSection(i));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      SdpSetupAttribute::Role role =
> +          masterBundleMsection->GetAttributeList().GetSetup().mRole;
> +      AddTransportAttributes(&newMsection, role);

We don't want to do this. When building a reoffer, with bundle slaves that _might_ be unbundled by the answerer, the bundle slaves need their own transport attributes.
Attachment #8848711 - Flags: review?(docfaraday) → review-
Comment on attachment 8848711 [details]
Bug 1329028: compare a=setup against existing transports.

https://reviewboard.mozilla.org/r/121606/#review129546

> I think we probably want to leave this loop alone, mostly. I decided to check |mRemoteIceIsRestarting| inside the loop so it is not hard to add support for partial ICE restart later.
> 
> I guess we should tweak it so that if newLocal is an answer, we will not copy any transport attrs into bundle slaves. (Right now we will do this for both offer and answer, we should only be doing this for offers)
> 
> As for a=setup, if we end up calling SdpHelper::CopyTransportParams(), we override based on what is in mTransports.

Regarding the |mRemoteIceIsRestarting|: I'm not convinced that at this level of craziness (thanks Bundle) it is a good idea to try to write future proof code.
Can you provide more details on what you mean by "leave this loop alone"? Do you mean I should restore the old loop over the oldAnswer? I think I ran into some issues with that, but I'll check it again.

"we will not copy any transport attrs into bundle slaves" does unfortunately not work with your comment from above to always call DetermineInitialAnswererSetupRole() and AddTransportAttributes() an all answer m-section. Because DetermineInitialAnswererSetupRole() will determine the wrong a=setup attribute in case of switched O/A roles. And if we then skip over the answer slave sections here, we send them out with the wrong value exactly causing the bug here.
So if we continue to fill all answer m-section by default (which appears to be sensible given some other crap I had to do here), then we would need to explicitly remove transport attrs from bundled answer m-sections. I tried to avoid removing attrs, but maybe that is the less painful path to take here.
Attachment #8824250 - Attachment is obsolete: true
Attachment #8854717 - Attachment is obsolete: true
Comment on attachment 8848711 [details]
Bug 1329028: compare a=setup against existing transports.

https://reviewboard.mozilla.org/r/121606/#review130938

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5351
(Diff revision 5)
> +  ASSERT_NE(match, std::string::npos);
> +  reoffer.replace(match, actpass.length(), "\r\na=setup:active");
> +
> +  SetRemoteOffer(reoffer, NO_CHECKS);
> +  ASSERT_EQ(kJsepStateHaveLocalOffer, mSessionOff->GetState());
> +  ASSERT_EQ(kJsepStateStable, mSessionAns->GetState());

Let's add a comment here that we're expecting SetRemoteOffer to fail.

::: media/webrtc/signaling/gtest/jsep_session_unittest.cpp:5378
(Diff revision 5)
> +  size_t match = reanswer.find(actpass);
> +  ASSERT_NE(match, std::string::npos);
> +  reanswer.replace(match, actpass.length(), "\r\na=setup:passive");
> +
> +  SetRemoteAnswer(reanswer, NO_CHECKS);
> +  ASSERT_EQ(kJsepStateHaveLocalOffer, mSessionOff->GetState());

Same sort of thing here.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1629
(Diff revision 5)
> +JsepSessionImpl::RemoveTransportAttributes(SdpMediaSection* msection)
> +{
> +  SdpAttributeList& attrList = msection->GetAttributeList();
> +  attrList.RemoveAttribute(SdpAttribute::kIceUfragAttribute);
> +  attrList.RemoveAttribute(SdpAttribute::kIcePwdAttribute);
> +  attrList.RemoveAttribute(SdpAttribute::kSetupAttribute);
> +  attrList.RemoveAttribute(SdpAttribute::kRtcpMuxAttribute);
> +  attrList.RemoveAttribute(SdpAttribute::kFingerprintAttribute);
> +  attrList.RemoveAttribute(SdpAttribute::kCandidateAttribute);
>  
>    return NS_OK;

Let's get a=end-of-candidates and a=rtcp too.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:1691
(Diff revision 5)
> +  // Then wipe the transport attributes from bundle slave sections in answers
> +  if (&newOffer != newLocal) {
> +    for (size_t i = 0; i < newLocal->GetMediaSectionCount(); ++i) {
> +      auto& newMsection = newLocal->GetMediaSection(i);
> +      if (mSdpHelper.MsectionIsDisabled(newMsection)) {
> +        continue;
> +      }
> +      if (mSdpHelper.IsBundleSlave(*newLocal, i)) {
> +        RemoveTransportAttributes(&newMsection);
> +      }
>      }
>    }

This is not going to interop with older firefox.

::: media/webrtc/signaling/src/jsep/JsepSessionImpl.cpp:2093
(Diff revision 5)
> +    if (mSdpHelper.IsBundleSlave(description, i) &&
> +        (type == kJsepSdpAnswer ||
> +         (type == kJsepSdpOffer &&
> +          newAttributes.HasAttribute(SdpAttribute::kBundleOnlyAttribute)))) {
> +      // Nothing to compare if this a bundle slave section
> +      continue;
> +    }

So I think we need the following checks:

* If the old msection was disabled, continue
* If the old msection was a bundle slave (determine this by inspecting the old _answer_), continue (because it had no transport of its own)
* If the new msection is disabled, continue
* If the new msection is in an answer, and a bundle slave, continue
* If the new msection is in an offer, and is bundle-only, continue

If we get past all of these checks, then we check the new msection against the old, without trying to look up transport level stuff. This is because we have one of two cases (remember, the transport follows the msection):

* The new msection is not a bundle slave, and the old msection has its own transport, which means the new must match old.
* The new msection is in an offer as a _potential_ bundle slave (no bundle-only); this new msection needs to have the old transport stuff in it, just in case the answerer pulls it out of the bundle.
Attachment #8848711 - Flags: review?(docfaraday) → review-
Mass change P2->P3 to align with new Mozilla triage process.
Priority: P2 → P3
See Also: → 1436555
I don't have the capacity any more to work on this any time soon.
Assignee: drno → nobody
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: