rtcweb-wg / jsep

32 stars 32 forks source link

BUNDLE answers should contain bundle-only atttribs #843

Closed juberti closed 3 years ago

juberti commented 6 years ago

per @cdh4u, seems like answers should now be handled differently:

The initial offer is as before, i.e., you assign a unique address, SDP attributes etc to each bundled m- section.

But, in the answer, and in every subsequent offer/answer, you only assign the BUNDLE port to the bundled m- section represented by the offerer/answerer BUNDLE-tag. To every other m- section you assign a zero port value and a bundle-only attribute (previously the bundle-only attribute was only defined for offers).

ekr commented 6 years ago

@juberti @taylor-b do you know why this change was made?

ekr commented 6 years ago

@cdh4u: Assuming I am reading S 8.3.2 of BUNDLE correctly, then you must set the port to 0. The text seems vague on c=, but I see that it is not present in the examples for the bundled m= sections. Is that correct? If so, does the text say so somewhere?

cdh4u commented 6 years ago

The change was based on comments and requests (including Ekr's earlier request to get rid of the shared address concept) that one should not have to repeat the BUNDLE address in multiple m- sections. Therefore, once a BUNDLE group is created, one can now assign the port to only one m- section in both offers and answers.

ekr commented 6 years ago

@cdh4u: well, I wasn't arguing that you shouldn't repeat the address, just that "shared address" wasn't a meaningful way of expressing it. Regardless, can you answer my other question about the c= line?

cdh4u commented 6 years ago

There were others requesting that the address shouldn't be repeated.

Regarding the c= line, is there a reason why you would want to use media level c= lines with BUNDLE

ekr commented 6 years ago

Regarding the c= line, is there a reason why you would want to use media level c= lines with BUNDLE

Not particularly, but ordinarily you need a c= line, so I'm looking for the place where the BUNDLE spec says you don't. Where is that?

cdh4u commented 6 years ago

Ok, I think I understand now: you ask for text explicitly saying that a media level c= line must not be assigned to a bundle-only m= section? Such text does NOT currently exist (I guess I always assumed that one would use a session level c= line with BUNDLE), but we could add it.

(Of course, it would not only apply to bundle-only m= sections in answers, but also in offers.)

ekr commented 6 years ago

Well, JSEP currently says:

  The m= line MUST be followed immediately by a "c=" line, as specified
   in [RFC4566], Section 5.7.  Again, as no candidates are available
   yet, the "c=" line must contain the "dummy" value "IN IP4 0.0.0.0",
   as defined in [I-D.ietf-ice-trickle], Section 5.1.

So probably we actually do need to come down on this.

cdh4u commented 6 years ago

I see no need to include a c= line in a bundle-only m= section, as the whole idea is that the m= section will only be used it becomes part of a BUNDLE group. If people agree, we should clarify that in BUNDLE, and modify the JSEP text above.

Maybe something like this could be added to section 7.1 of BUNDLE:

"According to normal SDP offer/answer procedures, a bundled m= section can implicitly retrieve its IP address from a session-level c= line, or from a media-level c= line that has been explicitly assigned to the bundled m= section. A media-level c= line MUST NOT be assigned to a bundle-only m= section."

juberti commented 6 years ago

@cdh4u do you want to write up a PR for this? We have an opportunity to fix it now as part of the 8445 updates.

cdh4u commented 6 years ago

BUNDLE already references 8445.

But, I have no problem writing a PR if we can add the text without having to take the draft back to the WG etc. After all, it's just a clarification.

juberti commented 3 years ago

@cdh4u, where did we end up on this? Is there a clarifying sentence that should be added here?

juberti commented 3 years ago

@fluffy, @ekr, any further thoughts here or should we just close this out?

fluffy commented 3 years ago

Is there any problem of things that don't do this failing to parse it if it does not have the c= dummy address ?

cdh4u commented 3 years ago

@cdh4u, where did we end up on this? Is there a clarifying sentence that should be added here?

It unfortunately seems like the clarifying sentence was never added.

cdh4u commented 3 years ago

Is there any problem of things that don't do this failing to parse it if it does not have the c= dummy address ?

At the end of the day, it should not matter what address you use, because you would take the port, and the address (in case you have c- lines for each m- section), from the bundled m- section.

juberti commented 3 years ago

It looks like we have 2 issues: 1) the aforementioned c= guidance, which I think is probably mostly cosmetic 2) the original issue where BUNDLE gives different guidance than JSEP on where bundle-only is to be used. JSEP sayeth:

Note that regardless of the presence of "a=bundle-only" in the offer, no "m=" sections in the answer should have an "a=bundle-only" line.

juberti commented 3 years ago

re 1), I don't see anything in bundle-54 indicating that a c= line can be omitted from a m= section, meaning that the guidance from RFC 4566 still applies:

A session description MUST contain either at least one "c=" field in
each media description or a single "c=" field at the session level.
cdh4u commented 3 years ago

re 1), I don't see anything in bundle-54 indicating that a c= line can be omitted from a m= section, meaning that the guidance from RFC 4566 still applies:

A session description MUST contain either at least one "c=" field in
each media description or a single "c=" field at the session level.

That is correct. BUNDLE allows "c=" fields in each media description. The question (AFAIU) is whether we need to say something about that in BUNDLE, i.e., whether to use the real address in such "c=" fields, or a dummy address.

cdh4u commented 3 years ago

It looks like we have 2 issues:

  1. the aforementioned c= guidance, which I think is probably mostly cosmetic
  2. the original issue where BUNDLE gives different guidance than JSEP on where bundle-only is to be used. JSEP sayeth:

Note that regardless of the presence of "a=bundle-only" in the offer, no "m=" sections in the answer should have an "a=bundle-only" line.

Regarding 2), would it be possible to simply remove that statement from JSEP? After all, it doesn't seem normative, but rather "referencing" procedures defined elsewhere.

juberti commented 3 years ago

We could, but we'd probably need to add a note to the section about generating answers indicating that a=bundle-only lines are to be inserted (and may need to update the examples as well)

juberti commented 3 years ago

Regarding c=, JSEP currently says that once an address is known, all c= lines for the bundle group should use that address (rather than 0.0.0.0).

cdh4u commented 3 years ago

We could, but we'd probably need to add a note to the section about generating answers indicating that a=bundle-only lines are to be inserted (and may need to update the examples as well)

Correct.

cdh4u commented 3 years ago

Regarding c=, JSEP currently says that once an address is known, all c= lines for the bundle group should use that address (rather than 0.0.0.0).

I think a BUNDLE implementation should be able to handle that.

Does a JSEP implementation also assume a c= line in each m= section in SDPs received from a peer?

juberti commented 3 years ago

JSEP can handle session-level c= lines, but it doesn't say anything about a SDP where only some m= sections have c= lines.

cdh4u commented 3 years ago

JSEP can handle session-level c= lines, but it doesn't say anything about a SDP where only some m= sections have c= lines.

I am pretty sure that most (all?) SDPs received from non-JSEP endpoints will only contain a session level c= line. So, whatever JSEP requires, I guess the receiving stack will have to "correct" the SDP before passing it on to JSEP.

juberti commented 3 years ago

Not sure what you mean. I think the key question for 1) at this point is whether c= lines in bundled m= sections need to use 0.0.0.0 or can use the shared address for the bundle group.

juberti commented 3 years ago

@fluffy as a BUNDLE author, can you see what needs to be done here?

fluffy commented 3 years ago

hmm - not sure I can but I will take a look

juberti commented 3 years ago

JSEP previously was in accordance with BUNDLE. The problematic changes seem to have happened in this PR: https://github.com/cdh4u/draft-sdp-bundle/pull/45, which was merged with no review (and a single email to the list that received no replies on Nov 24, 2017). In fairness, this github issue was opened shortly afterwards, but I confess I didn't understand the actual issue until now.

The discrepancies with JSEP are essentially the same as noted above; bundled m= lines now always have zero ports and a=bundle-only attributes, regardless of initial offer status, which contradicts JSEP's guidance.

With some more time I think I could tweak JSEP's handling of bundle-only to match BUNDLE; this is largely syntactical. However the zero port issue may affect how we select dead m= sections to recycle, and it does seem somewhat odd that we use this zero port hack in situations where it is not needed. That will require some effort to unwind.

@cdh4u, @alvestrand, @fluffy, do you recall the impetus behind making this change in BUNDLE? Somebody is going to have to do some work, so let's try to figure out where we want here and then we can figure out where to make the patch.

@mskucherawy @ekr as FYI

cdh4u commented 3 years ago

JSEP previously was in accordance with BUNDLE. The problematic changes seem to have happened in this PR: cdh4u/draft->sdp-bundle#45, which was merged with no review (and a single email to >the list that received no replies on Nov 24, 2017). In fairness, this github issue was opened shortly afterwards, but I confess I >didn't understand the actual issue until now.

The changes were also mentioned when the new version (-41) of the BUNDLE implementing the PR draft was submitted:

https://mailarchive.ietf.org/arch/msg/mmusic/r4W46ikvkE6xcoE6Xt1OxbUInss/

See below for the e-mail discussion behind the PR.

The discrepancies with JSEP are essentially the same as noted above; bundled m= lines now always have zero ports and >a=bundle-only attributes, regardless of initial offer status, which contradicts JSEP's guidance.

With some more time I think I could tweak JSEP's handling of bundle-only to match BUNDLE; this is largely syntactical. >However the zero port issue may affect how we select dead m= sections to recycle, and it does seem somewhat odd that we >use this zero port hack in situations where it is not needed. That will require some effort to unwind.

@cdh4u, @alvestrand, @fluffy, do you recall the impetus behind making this change in BUNDLE? Somebody is going to have >to do some work, so let's try to figure out where we want here and then we can figure out where to make the patch.

The removal of the "shared address" concept in general was based on comments from Ekr.

However, when it comes to the specific change regarding port zero and the bundle-only attribute, it was suggested by Taylor B.

https://mailarchive.ietf.org/arch/msg/mmusic/-jENmu4iaapy7NdJ9_SLHpNjSDc/

cdh4u commented 3 years ago

Not sure what you mean. I think the key question for 1) at this point is whether c= lines in bundled m= sections need to use 0.0.0.0 or can use the shared address for the bundle group.

I agree that is the issue.

(I just wanted to point out that SDPs from non-JSEP endpoints most likely will only contain a session-level c= line, and IF that is a problem for JSEP then the receiving SDP stack need to "fix" the SDP before passing the received SDP to JSEP.)

juberti commented 3 years ago

@cdh4u, the c= line is just one part of the issue. The zero port stuff is the bigger issue, and not something that can be trivially fixed up.

Generally, I think that this change was made to bundle without sufficient discussion, and as a result we now have two specs that contradict each other.

cdh4u commented 3 years ago

@cdh4u, the c= line is just one part of the issue. The zero port stuff is the bigger issue, and not something that can be trivially fixed up.

Generally, I think that this change was made to bundle without sufficient discussion, and as a result we now have two specs that contradict each other.

Well, I was very frustrated throughout the whole BUNDLE process, since people didn't comment on the changes - including people who asked for changes. I did send out e-mails on the list, I did send e-mails to people in private, and I did open pull requests. My opinion has always been that if one is interested in a draft, one is expected to follow the mail discussions. Also, after these changes had been made, there were WGLC reviews etc.

However, discussing history won't solve the issue: what we need to do to figure out now is how to move forward.

When it comes to port zero and bundle-only, my suggestion is to make the change in JSEP.

When it comes to media-level c= lines, we can for sure add clarification text in BUNDLE and/or JSEP, if needed.

juberti commented 3 years ago

The problem though is that BUNDLE hasn't articulated a migration strategy from the previous behavior to the new behavior, meaning that all current implementations are not in compliance and there's no easy way to change that. JSEP, on the other hand, is consistent with current implementations.

Therefore I think the correct path forward is to revert the new behavior from BUNDLE before publishing.

cdh4u commented 3 years ago

The problem though is that BUNDLE hasn't articulated a migration strategy from the previous behavior to the new behavior, >meaning that all current implementations are not in compliance and there's no easy way to change that. JSEP, on the other >hand, is consistent with current implementations.

Therefore I think the correct path forward is to revert the new behavior from BUNDLE before publishing.

Both BUNDLE and JSEP drafts were still work-in-progress back in 2017, and implementers need to be aware that there can be changes. And, we DID identify (described in this GitHub issue) the change back then, nobody objected to it, but we forgot to fix it.

So, until JSEP implementations have been updated, I think the web application developer can "manually" fix the SDP before sending it out on the wire. Doesn't the web app anyway have to modify the SDP for other reasons, or do I remember wrong?

juberti commented 3 years ago

At some point every change needs to be done with a migration in mind, and we are well past that point (even in 2017). We cannot simply tell every application they need to fix up their SDP, or that we are suddenly going to change what is emitted by the browser, unless there is some clear and urgent motivation.

It is regrettable that we are only realizing the extent of this issue now, but here we are (and at least we're not finding this months after publication). I'm discussing with Murray how to best sort this out; unless we can demonstrate that this change doesn't break existing apps, I think we need to fix it as an erratum to bundle.

@taylor-b in case he has any thoughts on how to handle the situation.

cdh4u commented 3 years ago

At some point every change needs to be done with a migration in mind, and we are well past that point (even in 2017).

Nobody made that claim, or objected to the change, when this JSEP issue was raised in 2017. The problem is that we forgot to implement the change in the JSEP draft.

We cannot simply tell every application they need to fix up their SDP, or that we are suddenly going to change what is emitted by the browser, unless there is some clear and urgent motivation.

It is regrettable that we are only realizing the extent of this issue now, but here we are (and at least we're not finding this >months after publication). I'm discussing with Murray how to best sort this out; unless we can demonstrate that this change >doesn't break existing apps, I think we need to fix it as an erratum to bundle.

Changing it could break existing BUNDLE implementations (non-JSEP).

And, we don't know if there are other standard specifications referencing BUNDLE, and how such change would affect those specs.

juberti commented 3 years ago

I don't think we understood the extent of the change when the issue was initially raised.

There are literally billions of WebRTC endpoints that would be affected by this change, so unless we can show that there are a comparable number of non-WebRTC BUNDLE endpoints that would be affected by a rollback, I think that's the clearest path forward.

I'm going to send a note to the C238 list now, we can continue the discussion there.

cdh4u commented 3 years ago

I don't think we understood the extent of the change when the issue was initially raised.

There are literally billions of WebRTC endpoints that would be affected by this change, so unless we can show that there are a comparable number of non-WebRTC BUNDLE endpoints that would be affected by a rollback, I think that's the clearest path forward.

But the endpoints still need a JavaScript web app that implements some signalling protocol (SIP/SDP, or something else) that will take care of the wire traffic, won't it?

juberti commented 3 years ago

sure, but how does that change the situation?

cdh4u commented 3 years ago

sure, but how does that change the situation?

You can do an intermediary fix by updating those "signaling web apps" on their servers. The browsers will download the latest version of the app from the server whenever they are going to run the app. Also, I would assume that every pro-2017 web app that supports JSEP and BUNDLE have already done the fix, if they have implemented the latest versions of the spec.

The actual JSEP API implementation, that is part of the browser, will be updated whenever people update their browser.

juberti commented 3 years ago

We can't even get apps to update to improve their security story, they are not going to update for this issue.

cdh4u commented 3 years ago

We can't even get apps to update to improve their security story, they are not going to update for this issue.

If there are pre-2017 web apps out there, that have since then not been updated with whatever changes (e.g., related to BUNDLE) and improvements (e.g., related to security) that have been made since then, maybe it's a good thing that they don't work.

juberti commented 3 years ago

We're not the internet police, we don't get to decide that. This ship has sailed.

cdh4u commented 3 years ago

We're not the internet police, we don't get to decide that. This ship has sailed.

We should also not be held hostage by people who don't bother to update their implementations - especially if they implement something that is still work in progress.

juberti commented 3 years ago

I looked into this more and Chrome/Edge/Safari never generate a=bundle-only, and Firefox only does so on initial offers (like JSEP). Given this, I think the chance of someone misinterpreting a zero port is pretty high, and we ought to simply scrap that behavior entirely.

cdh4u commented 3 years ago

I looked into this more and Chrome/Edge/Safari never generate a=bundle-only, and Firefox only does so on initial offers (like JSEP). Given this, I think the chance of someone misinterpreting a zero port is pretty high, and we ought to simply scrap that behavior entirely.

How does C/E/S request (in an initial offer) that an m= line shall only be accepted by the answerer if it puts the m= line in the BUNDLE group?

juberti commented 3 years ago

shared port in the initial offer

juberti commented 3 years ago

which is what we would have originally done, but we were concerned that non-bundle-aware endpoints might malfunction upon receiving non-unique ports. That concern no longer really exists, but now we have the new concern that bundle-aware endpoints might malfunction upon receiving zero ports... hence why I think we should junk a=bundle-only, at least in the WebRTC context