844071 - DTLS roles

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Mamadou]

User Agent: Mozilla/5.0 (Windows NT 6.0) AppleWebKit/537.17 (KHTML, like Gecko) Chrome/24.0.1312.57 Safari/537.17

Steps to reproduce:

Just making a call from chrome to FF through a gateway


Actual results:

I'm using Nightly "22.0a1 (2013-02-20)" and making call from chrome to FF through a gateway. The problem is that there is a role conflict in DTLS. FF is the called party but is sends "Client Hello" message. Our gateway uses rfc4145 to determine roles. If the remote party doesn't support this rfc, we just consider that the called party is the server and the calling the client.
Could someone explain how FF determines the roles?

Comment 1

[Eric Rescorla (:ekr)]

Firefox doesn't do RFC 4145 (or for that matter 4572) role negotiation. Instead,
it assumes that the called party is the client and the calling party is the server.
We will probably eventually do RFC 4572 but have also discussed with Chrome
an optimization where we use ICE controlled/controlling (that's currently what
they do, believe).

Comment 2

[Eric Rescorla (:ekr)]

BTW, Cbrome will do DTLS-SRTP, so you don't need to terminate the media at all to do a gateway call.

Comment 3

[Maire Reavy [:mreavy]]

Ekr -- I've marked this as blocking-webrtc-.  If you disagree, please say so.

Comment 4

[Eric Rescorla (:ekr)]

This also needs to use the ICE roles in the absence of 4145/4572

Comment 5

[Ada [:adalucinet]]

Could anyone please provide an URL to investigate this issue?

Comment 6

[Eric Rescorla (:ekr)]

What are you trying to investigate? This isn't fixed yet and the citations are to the RFCs above.

Comment 7

[Mihai Morar, (:MihaiMorar)]

Through MGCP Gateway or how? please be more specific because with those information mentioned yet here we are not able to reproduce this issue.

Comment 8

[Kahmyong Moon]

Ran into this issue while playing around with a custom server implementation of data channels, and Firefox in the offerer role. I was expecting the offerer to be in the active role (according to RFC 4145), but after ICE, nothing happened. It wasn't until I poked around with a packet sniffer and watched what two Firefoxes did that I realized Firefox expects the answerer to initiate DTLS (as per bug 831764, which was motivated by RFC 5763).

At a minimum, it would be nice if the SDP indicated that this was what Firefox was doing (maybe with a=setup:passive in the offer), even if it didn't actually negotiate the setup roles. Right now, you can't just read the RFCs to interoperate, you need to test against Firefox before discovering this behavior.

With regards to reproducing the issue, maybe one way to do it would be to fire up Wireshark while using any WebRTC application, and watch whether the offerer or answerer side of the connection sends the ClientHello. You'd also need to examine the SDP to see if a=setup is used.

Comment 9

[Ethan Hugg [:ehugg]]

This I think should be our first step (mostly from RFC 4145):

CreateOffer should add these two lines to the SDP right before the fingerprint line:

a=setup:passive
a=connection:new

Which suggests that the offerer play the server role like we currently do.

CreateAnswer should parse the offer SDP and if there is no a=setup line, or if we receive a=setup:passive or a=setup:actpass we should create our SDP with these lines:

a=setup:active
a=connection:new

Which asks to play the client role.  Note that we ignore an a=connection:existing value and always request a new connection.

If we receive an a=setup:active then we should respond with passive

a=setup:passive
a=connection:new

and switch our DTLS role to server.  I'm guessing we should error out if we receive an a=setup:holdconn since I don't think we can handle that request.

If an answer comes back from our offer and has a=setup:active, then we got our request and can proceed normally

If it comes back with a=setup:passive we should be able to handle that by changing our role to client and sending

a=setup:active
a=connection:new

a=setup:actpass is not valid in an answer and we can't do holdconn so those would be errors.


Let me know if you think I'm misunderstanding what is needed.  Otherwise, I'll work on a patch to do it this way.

Comment 10

[Eric Rescorla (:ekr)]

1. CreateOffer should send actpass, not passive (see 5763).
2. CreateAnswer should try to do active.
3. SetRemote should accept active or passive.
4. The DTLS role should,as you say,be conditioned on this.

I don't think we need to do the thing where the offerer is
prepared to receive either a clienthello or a serverhello,
since we aren't practically going to start DTLS until we
have an answer.

Yes, I think we can ignore holdconn.

Comment 11

[Adam Roach [:abr]]

For what it's worth, neither of the cited RFCs discusses SCTP. I think the governing document here is http://datatracker.ietf.org/doc/draft-ietf-mmusic-sctp-sdp/ (which is still a work in progress).

Comment 12

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - handle building and parsing of setup and connection attributes

Comment 13

[Ethan Hugg [:ehugg]]

I will try again and put the a=session lines per m-line instead of at the session level.  

ABR's comment seems to imply that this should only be for the datachannel, but I was thinking that every m-line would need an a=session and a=connection. EKR could you clarify this?

Comment 14

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - handle building and parsing of setup and connection attributes

Comment 15

[Ethan Hugg [:ehugg]]

This comment should of course say "a=setup lines" - s/session/setup

(In reply to Ethan Hugg [:ehugg] from comment #13)
> I will try again and put the a=session lines per m-line instead of at the
> session level.  
> 
> ABR's comment seems to imply that this should only be for the datachannel,
> but I was thinking that every m-line would need an a=session and
> a=connection. EKR could you clarify this?

Comment 16

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - Reset DTLS role on SDP negotiation

Comment 17

[Ethan Hugg [:ehugg]]

Attachment: Patch 3 - DTLS role negotiation unit test

Comment 18

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - handle building and parsing of setup and connection attributes

Comment 19

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - Reset DTLS role on SDP negotiation

Comment 20

[Ethan Hugg [:ehugg]]

Attachment: Patch 3 - DTLS role negotiation unit test

Comment 21

[Ethan Hugg [:ehugg]]

Comment on attachment 796046 [details] [diff] [review]
Patch 1 - handle building and parsing of setup and connection attributes


This patch has the SIPCC changes for building/parsing a=setup and a=connection.  The SDP will have the setup values that we are using (offer=passive=server, answer=active=client) for each media line, but they will not be negotiated until patch 2.

Comment 22

[Ethan Hugg [:ehugg]]

Comment on attachment 796058 [details] [diff] [review]
Patch 2 - Reset DTLS role on SDP negotiation


This patch propagates the dtls_setup value down to CreateTransportFlow and negotiates the value to match the request from the other side.  Also removed the Role concept from PeerConnectionImpl.

Comment 23

[Ethan Hugg [:ehugg]]

Comment on attachment 796059 [details] [diff] [review]
Patch 3 - DTLS role negotiation unit test


This should test offer-active/answer-passive, the opposite of the rest of the tests.

Comment 24

[Adam Roach [:abr]]

Comment on attachment 796046 [details] [diff] [review]
Patch 1 - handle building and parsing of setup and connection attributes

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

A general comment that affects the names of several enumerations and variables: the "setup" and "connection" attributes aren't specific to DTLS. I think we want these named more generically, in case we ever make use of the SIP stack as a SIP stack and need to to, say, TCP comedia (RFC 4145).

r- for out-of-spec behavior when receiving "a=setup:holdconn" from the remote party.

Nits inline.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +1822,5 @@
>      if (result != SDP_SUCCESS) {
>          GSM_ERR_MSG("Failed to set attribute");
>      }
>  }
>  

Add function documentation comment block.

@@ +1824,5 @@
>      }
>  }
>  
> +static void
> +gsmsdp_set_dtls_setup_attribute(sdp_attr_e sdp_attr, uint16_t level,

sdp_attr is superfluous here -- there is only one value that can ever be valid here, so let's assume that's the attribute we're after. See, e.g., gsmsdp_set_rtcp_fb_ack_attribute.

@@ +1841,5 @@
> +    if (result != SDP_SUCCESS) {
> +        GSM_ERR_MSG("Failed to set attribute");
> +    }
> +}
> +

Add function documentation comment block.

@@ +1843,5 @@
> +    }
> +}
> +
> +static void
> +gsmsdp_set_dtls_connection_attribute(sdp_attr_e sdp_attr, uint16_t level,

Same comment as above regarding sdp_attr.

@@ +4991,5 @@
> +                  /* DTLS setup and connection attributes.
> +                     We are setting our ANSWER SDP to be ACTIVE in every case
> +                     except when the REMOTE SDP requests ACTIVE, then we
> +                     declare PASSIVE and play the server role.
> +                     The role will be set when the TransportFlow is created */

I think the logic below yields the correct result for processing received answers, but the comment is a bit confusing, since it implies that the code that follows is executed only when we receive an offer.

@@ +4997,5 @@
> +                      remote_setup_type == SDP_DTLS_SETUP_ACTIVE) {
> +                      media->dtls_setup = SDP_DTLS_SETUP_PASSIVE;
> +                  } else {
> +                      media->dtls_setup = SDP_DTLS_SETUP_ACTIVE;
> +                  }

We're actually going to exhibit out-of-spec behavior here if someone sends us a "holdconn" attribute (cf. RFC 4145, section 4.1). I suspect we want to actually have a branch in here where we set media->dtls_setup to holdconn if the received SDP indicates holdconn so that we do the right thing on the wire.

Of course, we also need to wire this up so that the connection is actually not setup under those conditions -- I think the easiest way to handle this would be to set media->direction=SDP_DIRECTION_INACTIVE at the same time.

@@ +5003,5 @@
> +                  gsmsdp_set_dtls_setup_attribute(SDP_ATTR_DTLS_SETUP,
> +                    media->level, dcb_p->sdp->src_sdp, media->dtls_setup);
> +
> +                  /* We only support connection:new so we are hardcoding
> +                     it here */

Please add a comment that this needs to be revisited once we start supporting SDP renegotiation. Cite bug 857115.

Alternately, make the behavior predicated on the combination of the remote value and whether the media section is new or pre-existing.

@@ +5528,5 @@
> +          /* DTLS setup and connection attributes */
> +          gsmsdp_set_dtls_setup_attribute(SDP_ATTR_DTLS_SETUP,
> +            level, dcb_p->sdp->src_sdp, media->dtls_setup);
> +
> +          /* We only support connection:new so we are hardcoding it here */

This comment is a bit misleading: even when we support a=connection:existing, this will be "new" simply because we're adding a new media line (right?) I would prefer the comment reflect that instead.

::: media/webrtc/signaling/src/sipcc/core/gsm/h/fsm.h
@@ +207,5 @@
>       */
>      boolean        rtcp_mux;
>  
> +    /* The value of the a=setup line for DTLS.
> +       This affects whether we play client or server role */

Adjust comment style to match the rest of this structure, please.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp.h
@@ +2093,5 @@
> +sdp_result_e
> +sdp_attr_set_dtls_connection_attribute(void *sdp_ptr, u16 level,
> +    u8 cap_num, sdp_attr_e sdp_attr, u16 inst_num,
> +    sdp_dtls_connection_type_e connection_type);
> +

We seem to be missing an accessor for the connection attribute. Is that on purpose? We're going to need this once we do SDP renegotiation.

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr.c
@@ +4994,5 @@
> +    /* We do not support "holdconn" as a setup value */
> +    switch (attr_p->attr.dtls_setup) {
> +    case SDP_DTLS_SETUP_ACTIVE:
> +    case SDP_DTLS_SETUP_PASSIVE:
> +    case SDP_DTLS_SETUP_ACTPASS:

I don't think we want to exclude "holdconn" here -- there are places in the code where we take previously decoded (and possibly modified) SDP and re-encode it to create a serialized version. I don't think we want the encoding to fail under those circumstances.

I also don't think parsing/serialization is the proper place to enforce this kind of semantic restriction.

Finally, I think we do need to encode holdconn as a value, or we're going to be operating outside the behavior specified by RFC 4145.

@@ +5054,5 @@
> +sdp_result_e sdp_build_attr_connection(sdp_t *sdp_p,
> +                                       sdp_attr_t *attr_p,
> +                                       flex_string *fs)
> +{
> +    /* We only support "new" as a connection value */

Similar to above, I think we need the ability to encode both values here (and, even if we don't, this isn't where I think we should enforce it).

::: media/webrtc/signaling/src/sipcc/core/sdp/sdp_attr_access.c
@@ +4085,5 @@
>  }
>  
> +
> +/* Function:    sdp_attr_get_dtls_setup_attribute
> + * Description: Returns the value of an dtls_setup attribute at a given level

s/an/a/

@@ +4125,5 @@
> +    return SDP_SUCCESS;
> +}
> +
> +/* Function:    sdp_attr_set_dtls_setup_attribute
> + * Description: Sets the value of an dtls_setup attribute parameter

s/an/a/

@@ +4132,5 @@
> + *              level          The level to set the attribute.
> + *              cap_num        The capability number associated with the
> + *                             attribute if any.  If none, should be zero.
> + *              inst_num       The attribute instance number to check.
> + *              setup_type     dtls setup attribute bool to set

s/bool/value/

@@ +4164,5 @@
> +    return SDP_SUCCESS;
> +}
> +
> +/* Function:    sdp_attr_set_dtls_connection_attribute
> + * Description: Sets the value of an rtcp_mux attribute parameter

a/an/a/

Comment 25

[Ethan Hugg [:ehugg]]

>We seem to be missing an accessor for the connection attribute. Is that on purpose? We're 
>going to need this once we do SDP renegotiation.

That was on purpose, I generally don't add code that isn't called.  I didn't consider that we would need it soon, so I'll add it.

Comment 26

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - handle building and parsing of setup and connection attributes

Comment 27

[Ethan Hugg [:ehugg]]

Attachment: Patch 1 - handle building and parsing of setup and connection attributes

Comment 28

[Ethan Hugg [:ehugg]]

Comment on attachment 796925 [details] [diff] [review]
Patch 1 - handle building and parsing of setup and connection attributes


This one should fix the issues raised in the review.

Comment 29

[Eric Rescorla (:ekr)]

Comment on attachment 796058 [details] [diff] [review]
Patch 2 - Reset DTLS role on SDP negotiation

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

lgtm

Comment 30

[Ethan Hugg [:ehugg]]

Attachment: Interdiff for patch 1 since last review.

Unfortunately this interdiff is huge because of the naming changes.

Comment 31

[Eric Rescorla (:ekr)]

Comment on attachment 796059 [details] [diff] [review]
Patch 3 - DTLS role negotiation unit test

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

r- because I would like a few more tests.

This test looks good.

Could you please add a test in which:

1. The offerer offers only passive and verify it works.
2. You introduce a mismatch and verify that the DTLS handshake fails.

Comment 32

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - Reset DTLS role on SDP negotiation

Comment 33

[Ethan Hugg [:ehugg]]

Comment on attachment 796934 [details] [diff] [review]
Patch 2 - Reset DTLS role on SDP negotiation


Updated this patch to reflect the naming changes in patch 1.  Bringing forward the r+ from EKR.

Comment 34

[Ethan Hugg [:ehugg]]

Attachment: Patch 3 - DTLS role negotiation unit test

Comment 35

[Ethan Hugg [:ehugg]]

Comment on attachment 796961 [details] [diff] [review]
Patch 3 - DTLS role negotiation unit test

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

Added two more unit tests.

Comment 36

[Eric Rescorla (:ekr)]

Comment on attachment 796961 [details] [diff] [review]
Patch 3 - DTLS role negotiation unit test

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

This looks good.

If you haven't you should add some unit tests that test that the negotiation freaks out
if you offer bogus values in these fields. But that's part 1.

Comment 37

[Adam Roach [:abr]]

Comment on attachment 796925 [details] [diff] [review]
Patch 1 - handle building and parsing of setup and connection attributes

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

Looks good to me. One spelling typo below.

::: media/webrtc/signaling/src/sipcc/core/gsm/gsm_sdp.c
@@ +5036,5 @@
> +                  gsmsdp_set_setup_attribute(media->level, dcb_p->sdp->src_sdp,
> +                    media->setup);
> +
> +                  /* TODO(ehugg) we are not yet supporting existing connections
> +                     See bug 857115.  We curenntly always respond with

s/curenntly/currently/

Comment 38

[Adam Roach [:abr]]

(In reply to Eric Rescorla (:ekr) from comment #36)
> Comment on attachment 796961 [details] [diff] [review]
> Patch 3 - DTLS role negotiation unit test
> 
> Review of attachment 796961 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good.
> 
> If you haven't you should add some unit tests that test that the negotiation
> freaks out
> if you offer bogus values in these fields. But that's part 1.

Part 1 doesn't include any unit test changes -- when doing my review, I had assumed they were part of a later patch. If this testing isn't covered elsewhere, then I would second the assertion that we need some unit testing of unexpected values here.

Comment 39

[Ethan Hugg [:ehugg]]

Attachment: Patch 2 - Reset DTLS role on SDP negotiation

Comment 40

[Ethan Hugg [:ehugg]]

Comment on attachment 800176 [details] [diff] [review]
Patch 2 - Reset DTLS role on SDP negotiation


Updated to apply on latest M-I.  Bringing forward r+ from EKR.

Comment 41

[Ethan Hugg [:ehugg]]

Attachment: Patch 3 - DTLS role negotiation unit test

Comment 42

[Ethan Hugg [:ehugg]]

Comment on attachment 800196 [details] [diff] [review]
Patch 3 - DTLS role negotiation unit test


Added two more tests for garbage values in a=setup and a=connection.  The first two tests have not changed since last r+.

Comment 43

[Ethan Hugg [:ehugg]]

Try run to check Windows build.

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

Comment 44

[Ethan Hugg [:ehugg]]

Attachment: Patch 3 - DTLS role negotiation unit test

Comment 45

[Ethan Hugg [:ehugg]]

Comment on attachment 800437 [details] [diff] [review]
Patch 3 - DTLS role negotiation unit test


Added two more tests for removing a=setup and a=connection on offer and then again on answer.

Comment 46

[Ethan Hugg [:ehugg]]

Bug 906843 just hit M-I, I will change these new tests to use the new ASSERT_TRUE_WAIT instead of PR_SLEEP as soon as it hits M-C.

Comment 47

[Eric Rescorla (:ekr)]

Comment on attachment 800437 [details] [diff] [review]
Patch 3 - DTLS role negotiation unit test

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

lgtm

Comment 48

[Ethan Hugg [:ehugg]]

Attachment: Patch 3 - DTLS role negotiation unit test

Comment 49

[Ethan Hugg [:ehugg]]

Comment on attachment 800972 [details] [diff] [review]
Patch 3 - DTLS role negotiation unit test


Updated unittests to match pattern from bug 906843 which landed today.  Bringing forward r+ from EKR.

Comment 50

[Ethan Hugg [:ehugg]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4a9b805c313
https://hg.mozilla.org/integration/mozilla-inbound/rev/55d6779a2cc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/842a3da6e311

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f4a9b805c313
https://hg.mozilla.org/mozilla-central/rev/55d6779a2cc6
https://hg.mozilla.org/mozilla-central/rev/842a3da6e311