Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Patch 1 #862

Closed
wants to merge 3 commits into from
Closed

Patch 1 #862

wants to merge 3 commits into from

Conversation

adamroach
Copy link
Contributor

Alternate approach to satisfy #854, in a way that allows for interop between future WebRTC implementations and SIP users of ICE.

Alternate approach to resolving rtcweb-wg#854 which allows for future WebRTC implementations to interop with non-WebRTC ICE users.
Fixing ambiguity about m-section versus entire SDP
@hardie
Copy link

hardie commented Jan 24, 2019

Looks like the after the sip-sdp reference got converted into a and that is triggering the failing check.

fixing broken <t> closing tag
@adamroach
Copy link
Contributor Author

Looks like the after the sip-sdp reference got converted into a and that is triggering the failing check.

I fat-fingered for -- fixed...

Copy link

@nils-ohlmeier nils-ohlmeier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general this looks like a reasonable path forward. Ideally I would like to see some clarity added for trickle ICE implementations (see comment below).

data m= sections, implementations MUST support the
<xref target="RFC5764"></xref>. Implementations MUST indicate this
profile for each media m= line they produce in an offer unless the
media section contains only TCP candidates; if all candidates use TCP as a

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally it would be a little bit more clear what this sentence translates to for trickle ICE implementations. I would expect that a trickle ICE implementation should start with "UDP/TLS/RTP/SAVPF". But I can see how someone could mis-read this in the way he/she thinks the implementation needs to wait for trickle ICE gathering to be finished to decide which protocol to use in the m= line. And thus would void the trickle ICE benefits for the purpose of obeying to this sentence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the IESG review of one of my drafts, Ekr(?) indicated that one can't use "MUST...unless" wording. That triggered some discussions, but there was no outcome. I personally have no problem with it, but just to make sure Ekr is fine with it. One would assume he is, since he is listed as co-author :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is a "JSEP implementation". A browser implementing the JSEP API, or a JS application using it? I understand if the profile will be included in each offer within the API, but why is it a MUST for a JS application to send it in an offer on the wire?

Copy link

@rshpount rshpount Jan 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest that language should be:

In the initial offer or any offer that initiates ICE restart, implementations MUST indicate "UDP/TLS/RTP/SAVPF" profile for each media m= line they produce in an offer. In the subsequent offers which do not initiate ICE restart, if all candidates for this m= section use TCP as a transport, implementations MUST indicate "TCP/DTLS/RTP/SAVPF", to allow for compatibility with non-WebRTC implementations of I-D.ietf-mmusic-ice-sip-sdp.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Roman is correct but this is NUTS. We are doing all of this so we can support middle boxes tampering with our signalling in a way that we know won't work? Seriously ? Note all of this is fine with SBCs as they are actually in the signalling and can signal if they do ICE or not, but we are doing this for ALGs which pretty much no one uses, are a really bad idea, are not recommended by the IETF - how about we get rid of all the ALG stuff and replace it with SDP MUST be transported over a protocol that provides integrity.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider an edge case, where a browser that has an active TCP candidate connects to a MCU with a passive TCP candidate. In this case, the browser TCP candidate is 1.2.3.4:9 - i.e., the discard port. I assume that we would then put '9' into the m= line, as usual?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rshpount, can you point to an example of the weird Chrome behavior with TCP candidates?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lastly, are we saying we would only ever do this when there were only TCP candidates offered, or also in a re-offer case when the selected candidate pair are both TCP candidates? If we go down this path, I would think we would want to do the latter.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ask again: what is meant by "implementation"?

Are we suggesting that a JS application using the JSEP API is required to use "UDP/TLS/RTP/SAVPF"?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@juberti I will try to answer multiple comments in this single one.

When a browser that has an active TCP candidate connects to a MCU with a passive TCP candidate, then the browser TCP candidate is 1.2.3.4:9 - i.e., the discard port and end point puts '9' into the m= line.

According to @adamroach in https://mailarchive.ietf.org/arch/msg/mmusic/Btq1iVG_BrrvshTps1toyzyWWRs , Chrome will add UDP attribute to an offer even if ICE nomination was completed and TCP candidate was nominated. I have not verified this.

During the discussions at MMUSIC we decided that when initial or offer with ICE restart is initiated, the capabilities of the remote end point are not always known. In case of initial offer little is known about remote and ICE restart can be initiated to allow for 3rd party call control, which can change the remote end point. Because of this, since in both cases remote end point might not support ICE TCP or TCP based proto, it is safer to use UDP based proto in the offer, even when TCP candidate pair is currently being used. It was also decided that it is cleaner not to change proto during ICE nomination process, since nominated pair can change during the signaling exchange. This resulted in always using UDP for initial offer, during ICE restart or when ICE nomination is still in progress and using TCP only when ICE nomination is complete and TCP candidate was nominated.

added, implementations should use TCP/DTLS/RTP/SAVPF as a
transport token when the candidate in question is using TCP, in
order to facilitate interactions with non-JSEP SIP/SDP
implementations, which assign meaning to the protcol field.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
implementations, which assign meaning to the protcol field.
implementations, which assign meaning to the protocol field.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Current implementations" is tricky, because "current" will be whenever someone reads the spec - today or in 2 years - and what is claimed may change over time. Therefore, I suggest using "At the time of writing this specification", or something similar.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "Current implementations of JSEP" refer to? Browsers implementing the API, or applications using the API? I think we shall make a clear separation, and say that the JSEP API, nor the current applications using it, update the m= or c= lines. I also think that we should recommend that applications that are expected to interact with legacy/non-JSEP endpoints to actually update the c= and m= lines.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this entire paragraph seems unnecessary to me. I suppose we could add a note to say why we made this change, for posterity, but I don't see the need for caveats regarding the current deployed base, given that the browsers typically move faster than the specs.

instead driven by ICE.</t>
</list></t>

<t>For data m= sections, implementations MUST support the
"UDP/DTLS/SCTP" profile and MUST indicate this profile for
Copy link

@rshpount rshpount Jan 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section needs to be updated to match. It should say something like:

For data m= sections, implementations MUST support the "UDP/DTLS/SCTP" profile. In the initial offer or any offer that initiates ICE restart, implementations MUST indicate "UDP/DTLS/SCTP" profile for each data m= line they produce in an offer. In the subsequent offers which do not initiate ICE restart, if all candidates for this m= section use TCP as a transport, implementations MUST indicate "TCP/DTLS/SCTP", to allow for compatibility with non-WebRTC implementations of I-D.ietf-mmusic-sctp-sdp.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Furthermore, "TCP/DTLS/SCTP," should be added to the list of supported protocols below.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree this is needed, although I think this can be written more succinctly - willing to take a shot if there is consensus on the direction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't we have separate sections for ice-initiating offers and other offers?

Copy link
Contributor

@fluffy fluffy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could live with changes proposed here. I could live with stuff staying like it is. I don't see it as making much change for apps that use WebRTC.

profile for each media m= line they produce in an offer unless the
media section contains only TCP candidates; if all candidates use TCP as a
transport, implementations MUST indicate "TCP/DTLS/RTP/SAVPF," to
allow for compatibility with non-WebRTC implementations of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the comparability issue on this ? Keep in mind that if there is a single UDP candidate that will never work nd never be use but is still on the list, then it seems like this will say TCP instead of UDP. I'm not arguing against the change but this text implies that if you don't do this, it won't work with non-WebRTC stuff and I'm just quirous for a concrete example of when it does not work

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Main use case for this (or for the "fix up" offer) is monitoring. When you have a system that sits only on the signaling path between end points and works with both SIP and WebRTC end points at the same time, "fix up" offer is being used to determine the nominated candidate pair. This, in turn, is being used to troubleshoot connection issues based on actual addresses used for media and candidate type (host, relay or reflexive). It is possible to get this information from WebRTC end points using stats interface, but "fix up" offer is the only way to get this from a lot of SIP end points.

During ICE development some people mentioned firewalls which apply different port closing policies based on what they intercept in SDP, but I do not think I saw this in practice.

Also, a lot of things currently being fixed by SDP manipulation either on the client or server side. So, I would expect things not to break now, but to have some sort of SDP manipulation script to work around this when working with WebRTC.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am extremely skeptical about this monitoring case. People do monitoring from SBCs that understand all of ICE or if they don't support trickle, they don't pass the messages. Can someone point me at a commercially available device that is used for monitoring and actually needs these "fix up" messages? I realize we might have needed this is 2002 but I really wonder if we are just holding on to old things that are no longer really needed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider somebody running a SIP proxy which supports SIP-over-WebSockets (opensips), regular VoIP clients (pjsip based which do support ICE and DTLS SRTP), JavaScript based SIP clients (jssip or sipml), voicemail server which support ICE-TPC, and an SBC, which supports ICE-TCP, to place calls out of the system. SIP Homer ( http://sipcapture.org) is used for monitoring. Everything is mostly peer-to-peer and SBC is only used for PSTN calls out of the system, so it is on the media path for a small percentage of the calls.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If people want to remove the "fix up" messages from ICE in general I don't think this is the correct venue for discussing that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Fix up" messages are removed if "ice2" ice-option is set: https://tools.ietf.org/html/draft-ietf-mmusic-ice-sip-sdp-24#section-3.3.4

"Fix up" offer should be automatically suppressed if new implementations are used. In any case, "fix up" is easy enough to implement via API. All application needs to do is call createOffer once ICE nomination is complete. The only thing which is required of the browser is to generate a valid session description with only nominated candidates and correct c=/m= line.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Fix up" messages are removed if "ice2" ice-option is set: https://tools.ietf.org/html/draft-ietf-mmusic-ice-sip-sdp-24#section-3.3.4

"Fix up" offer should be automatically suppressed if new implementations are used. In any case, "fix up" is easy enough to implement via API. All application needs to do is call createOffer once ICE nomination is complete. The only thing which is required of the browser is to generate a valid session description with only nominated candidates and correct c=/m= line.

Correct. But, does the browser do that? If the initial offer included UDP in the c/m- line, but TCP candidates were nominated, will that be reflected in the c/m- line if the application later calls createOffer?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is one of the reasons why I argue browser should include a correct proto in the m= line.

<xref target="I-D.ietf-mmusic-ice-sip-sdp"></xref>.</t>

<t><list style="hanging">
<t hangText="Note:">Current implementations of JSEP do not update m= or c= lines
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess the key question here is if we expect browser to change to do the extra "clean up" transaction or not. IF they are not going to chance, this seems like "YOU SHOULD BUT WE KNOW YOU WON'T" type language that is just harmful to implementations because the spec implies things will do something different than what they do

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently it is fairly simple to use WebRTC JavaScript API to implement something which is RFC 5245 compatible, including generation of the extra "clean up" offer. Browser does not have to force the "clean up" offer. If an application needs such offer, this offer can generated by an application when ICE nomination is complete. What we are trying to make sure here is that SDP generated in such offer is compatible with RFC 5245, i.e. includes only default ICE candidates and have address and protocol information in c=m= line that match the default candidate. What we are trying to avoid is the need to SDP patching to keep this update offer compatible with legacy RFC 5245, which only happens when TCP candidate is nominated.

<t hangText="Note:">Current implementations of JSEP do not update m= or c= lines
after a candidate pair is selected. When these facilities are
added, implementations should use TCP/DTLS/RTP/SAVPF as a
transport token when the candidate in question is using TCP, in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think need to cover the case of what happens after moving to transport X, if ICE conditions change and the transport is changed to Y. Does this all update again ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if a re-offer is ever sent, it simply includes the selected pair and protocol in c=/m.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless already specified, that needs to be clear in the document.

implementations, which assign meaning to the protcol field.
Note that in interactions with other JSEP implementations, the
use of ICE makes any syntactically valid address/port/transport
tuple in the m= and c= lines equivalent, as the signalling is
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"equivalent" seems wrong here. I think you are trying to say that JSEP devices can write data into theses fields that my be read by non JSEP devices but the JSEP device pretty much only read the data in ICE stuff.

Copy link
Contributor

@juberti juberti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not opposed to this direction, but I don't understand the exact position being espoused here. I would suggest we first align on the position and then we can turn our attention to the precise text.

data m= sections, implementations MUST support the
<xref target="RFC5764"></xref>. Implementations MUST indicate this
profile for each media m= line they produce in an offer unless the
media section contains only TCP candidates; if all candidates use TCP as a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's consider an edge case, where a browser that has an active TCP candidate connects to a MCU with a passive TCP candidate. In this case, the browser TCP candidate is 1.2.3.4:9 - i.e., the discard port. I assume that we would then put '9' into the m= line, as usual?

data m= sections, implementations MUST support the
<xref target="RFC5764"></xref>. Implementations MUST indicate this
profile for each media m= line they produce in an offer unless the
media section contains only TCP candidates; if all candidates use TCP as a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rshpount, can you point to an example of the weird Chrome behavior with TCP candidates?

data m= sections, implementations MUST support the
<xref target="RFC5764"></xref>. Implementations MUST indicate this
profile for each media m= line they produce in an offer unless the
media section contains only TCP candidates; if all candidates use TCP as a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lastly, are we saying we would only ever do this when there were only TCP candidates offered, or also in a re-offer case when the selected candidate pair are both TCP candidates? If we go down this path, I would think we would want to do the latter.

<t hangText="Note:">Current implementations of JSEP do not update m= or c= lines
after a candidate pair is selected. When these facilities are
added, implementations should use TCP/DTLS/RTP/SAVPF as a
transport token when the candidate in question is using TCP, in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if a re-offer is ever sent, it simply includes the selected pair and protocol in c=/m.

added, implementations should use TCP/DTLS/RTP/SAVPF as a
transport token when the candidate in question is using TCP, in
order to facilitate interactions with non-JSEP SIP/SDP
implementations, which assign meaning to the protcol field.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, this entire paragraph seems unnecessary to me. I suppose we could add a note to say why we made this change, for posterity, but I don't see the need for caveats regarding the current deployed base, given that the browsers typically move faster than the specs.

instead driven by ICE.</t>
</list></t>

<t>For data m= sections, implementations MUST support the
"UDP/DTLS/SCTP" profile and MUST indicate this profile for
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree this is needed, although I think this can be written more succinctly - willing to take a shot if there is consensus on the direction.

@juberti
Copy link
Contributor

juberti commented Feb 28, 2019

Closing in favor of #863.

@juberti juberti closed this Feb 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants