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

BUNDLE answers should contain bundle-only atttribs #843

Closed
juberti opened this issue Dec 10, 2017 · 59 comments · Fixed by #1017
Closed

BUNDLE answers should contain bundle-only atttribs #843

juberti opened this issue Dec 10, 2017 · 59 comments · Fixed by #1017

Comments

@juberti
Copy link
Contributor

juberti commented Dec 10, 2017

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
Copy link
Contributor

ekr commented Dec 27, 2017

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

@ekr
Copy link
Contributor

ekr commented Dec 27, 2017

@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
Copy link
Contributor

cdh4u commented Dec 27, 2017

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
Copy link
Contributor

ekr commented Dec 28, 2017

@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
Copy link
Contributor

cdh4u commented Dec 28, 2017

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
Copy link
Contributor

ekr commented Dec 31, 2017

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
Copy link
Contributor

cdh4u commented Dec 31, 2017

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
Copy link
Contributor

ekr commented Dec 31, 2017

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
Copy link
Contributor

cdh4u commented Jan 1, 2018

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
Copy link
Contributor Author

juberti commented Oct 12, 2018

@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
Copy link
Contributor

cdh4u commented Oct 13, 2018

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
Copy link
Contributor Author

juberti commented Dec 29, 2020

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

@juberti
Copy link
Contributor Author

juberti commented Jan 11, 2021

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

@fluffy
Copy link
Contributor

fluffy commented Jan 11, 2021

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
Copy link
Contributor

cdh4u commented Jan 11, 2021

@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
Copy link
Contributor

cdh4u commented Jan 11, 2021

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
Copy link
Contributor Author

juberti commented Jan 11, 2021

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
Copy link
Contributor Author

juberti commented Jan 11, 2021

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
Copy link
Contributor

cdh4u commented Jan 12, 2021

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
Copy link
Contributor

cdh4u commented Jan 12, 2021

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
Copy link
Contributor Author

juberti commented Jan 12, 2021

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
Copy link
Contributor Author

juberti commented Jan 12, 2021

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
Copy link
Contributor

cdh4u commented Jan 12, 2021

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
Copy link
Contributor

cdh4u commented Jan 12, 2021

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
Copy link
Contributor Author

juberti commented Jan 14, 2021

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
Copy link
Contributor

cdh4u commented Jan 14, 2021

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
Copy link
Contributor Author

juberti commented Jan 14, 2021

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
Copy link
Contributor Author

juberti commented Jan 15, 2021

@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
Copy link
Contributor

cdh4u commented Jan 15, 2021

@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
Copy link
Contributor Author

juberti commented Jan 15, 2021

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
Copy link
Contributor

cdh4u commented Jan 15, 2021

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
Copy link
Contributor Author

juberti commented Jan 15, 2021

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
Copy link
Contributor

cdh4u commented Jan 15, 2021

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
Copy link
Contributor Author

juberti commented Jan 15, 2021

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
Copy link
Contributor

cdh4u commented Jan 15, 2021

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
Copy link
Contributor Author

juberti commented Jan 15, 2021

sure, but how does that change the situation?

@cdh4u
Copy link
Contributor

cdh4u commented Jan 15, 2021

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
Copy link
Contributor Author

juberti commented Jan 15, 2021

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

@cdh4u
Copy link
Contributor

cdh4u commented Jan 15, 2021

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
Copy link
Contributor Author

juberti commented Jan 15, 2021

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

@cdh4u
Copy link
Contributor

cdh4u commented Jan 15, 2021

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
Copy link
Contributor Author

juberti commented Jan 15, 2021

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
Copy link
Contributor

cdh4u commented Jan 16, 2021

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
Copy link
Contributor Author

juberti commented Jan 16, 2021

shared port in the initial offer

@juberti
Copy link
Contributor Author

juberti commented Jan 16, 2021

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

@cdh4u
Copy link
Contributor

cdh4u commented Jan 16, 2021

shared port in the initial offer

Such behavior has never been part of BUNDLE or JSEP (unless you count port 9 as a shared port).

@ekr
Copy link
Contributor

ekr commented Jan 16, 2021

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...

@juberti why does that concern no longer exist? Sorry, still paging all this back in.

@juberti
Copy link
Contributor Author

juberti commented Jan 16, 2021

Exactly one person has complained about Chrome not supporting a=bundle-only over the last 10 years, so I think we have a clear signal that this is not a real-world concern.

However, because we never added this, we now have a real compatibility concern in that apps that use max-bundle may suddenly break when we start sending zero ports. @nils-ohlmeier indicated they hit this as part of adding bundle-only in firefox and may be able to add more specifics here.

IOW, no upside, all downside.

@cdh4u
Copy link
Contributor

cdh4u commented Jan 16, 2021

Exactly one person has complained about Chrome not supporting a=bundle-only over the last 10 years, so I think we have
a clear signal that this is not a real-world concern.

In initial offers, if you don't have bundle-only, you will have to include separate port numbers (and separate candidates, if ICE is used) for each m- line (unless you use trickle, in which case you will use port 9 for each m- line). One of the main reasons for introducing bundle-only was to avoid having to do that.

Using the same non-9 port number in each m- line in the initial offer was shut down more or less the first day we started working on BUNDLE.

@juberti
Copy link
Contributor Author

juberti commented Jan 16, 2021

Remember, we're really only talking about the max-bundle case here (otherwise, separate port numbers are already used). Evidence shows that a shared port works pretty well in this case (as that's the behavior used by apps today), so I think we can just internalize that and move on.

@juberti
Copy link
Contributor Author

juberti commented Jan 16, 2021

Let's continue this discussion on the list so that we don't need to reply in both places.

@cdh4u
Copy link
Contributor

cdh4u commented Jan 16, 2021

Let's continue this discussion on the list so that we don't need to reply in both places.

Yes, I was going to suggest that, and I also noticed that you had suggested it earlier.

@juberti
Copy link
Contributor Author

juberti commented Jul 10, 2021

@cdh4u
Copy link
Contributor

cdh4u commented Jul 10, 2021

The new version of the BUNDLE specification has passed MMUSIC WGLC, and the publication request has been sent to IESG.

https://datatracker.ietf.org/doc/draft-ietf-mmusic-rfc8843bis/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants