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

RTCSctpTransport lacks remote port #625

Closed
lgrahl opened this issue Dec 16, 2016 · 36 comments
Closed

RTCSctpTransport lacks remote port #625

lgrahl opened this issue Dec 16, 2016 · 36 comments

Comments

@lgrahl
Copy link
Contributor

lgrahl commented Dec 16, 2016

If I haven't overlooked something, there's no way to determine the SCTP port of the remote peer.

For WebRTC, the remote port can be found in the offer/answer SDP.

I'm proposing to add the remote port to RTCSctpCapabilities as a remotePort port attribute. port in the constructor should also be renamed to localPort to avoid confusion. Furthermore, we should discuss whether RTCSctpCapabilities could be renamed to RTCSctpParameters because an SCTP port is not really a 'capability'.

Edit: I've withdrawn my suggestion to rename the attributes as other transports that provide parameters do not prefix the attributes with local or remote either.

Cheers,
Lennart

@lgrahl
Copy link
Contributor Author

lgrahl commented Dec 17, 2016

On second thought, adding port to the capabilities would not be sufficient for the current specified behaviour. For that to work, we would also have to specify that the SCTP INIT is triggered when the RTCSctpTransport is being started, instead of triggering it when it is being created. To be fair, that would make more sense to me anyway.

@aboba
Copy link
Contributor

aboba commented Dec 17, 2016

Since getCapabilities is static, it can only return generic capabilities like maxMessageSize, and not things like the local port, which are specific to an RTCSctpTransport object.

Which makes me wonder whether it shouldn't be getLocalParameters() instead.

@lgrahl
Copy link
Contributor Author

lgrahl commented Dec 17, 2016

Oh, it's static. I haven't seen that.

I'd upvote changing it to a non-static getLocalParameters method and including the port there.

@taylor-b
Copy link

Something related I ran into recently: in WebRTC 1.0, the local/remote SCTP ports are technically allowed to change.

Mapping this to ORTC: If the remote port changes, you can just call start again, but if the local port changes, you need to create a new ORTC RTCSctpTransport. This means there's a difference between an ORTC RTCSctpTransport and a WebRTC one; an ORTC RTCSctpTransport is tied to a specific local port number, but a WebRTC RTCSctpTransport isn't.

Unless we do want a new WebRTC RTCSctpTransport when the local port changes, but that's not documented in the WebRTC spec currently. Thoughts, @aboba?

@lgrahl
Copy link
Contributor Author

lgrahl commented Dec 22, 2016

Can you give a reference to a related RFC/draft that clarifies how this works for WebRTC? Does it use SCTP multihoming to add an address/port pair to the association? I haven't found anything in Google's code or in Mozilla's code regarding this (no calls to sctp_bindx. Also, SCTP address reconfiguration support in WebRTC is optional as stated here).

Furthermore, ORTC doesn't really specify what's supposed to happen when calling start on the RTCSctpTransport instance. But passing the remote port as part of a parameters object on start would mean that we have to change the description of the start method anyway as the handshake would need to be triggered when start is being called (otherwise, the remote port is unknown which makes it impossible to initiate the SCTP handshake).

@taylor-b
Copy link

JSEP points to draft-ietf-mmusic-sctp-sdp which says:

In order to trigger the closure of an existing SCTP
association, and establishment of a new SCTP association, the SDP
'sctp-port' attribute [Section 5] is used to indicate a new
(different than the ones currently used) SCTP port. The existing
SCTP association is closed, and the new SCTP association is
established, if one or both endpoints signal a new SCTP port. The
'connection' attribute does apply to establishment of underlying TCP
connections.

So I think the whole association is just closed and recreated. Chrome doesn't quite do this properly, but it does happen if you change the "m=" section direction to "inactive" and then back.

@lgrahl
Copy link
Contributor Author

lgrahl commented Dec 22, 2016

I hope you don't mind me asking but do you know of a use case for this procedure?

To add support for this, we would have to move most of the description for the RTCSctpTransport constructor into the start method (to be precise, everything after the first sentence excluding the very last sentence). When calling start again, the current SCTP association would need to be shut down before going through the starting procedure again. Furthermore, we could either choose to update the local port via the attribute directly or by moving the local port into the start method as well (I'd prefer that). Thoughts?

@lgrahl
Copy link
Contributor Author

lgrahl commented Dec 22, 2016

Another question I have is: What are the implications for open data channels? What if I choose to send a message at the very same moment where no SCTP association is available. Do you know if this is specified somewhere @taylor-b? I'm especially interested in the SCTP and data channel state relationship. (Sorry if this is a little bit off-topic)

@taylor-b
Copy link

What if I choose to send a message at the very same moment where no SCTP association is available.

The data channel state wouldn't be "open" then, so you'd get an InvalidStateError.

I hope you don't mind me asking but do you know of a use case for this procedure?

I don't know a use case. Maybe the easiest solution is to just say "this isn't allowed" in JSEP. Or do what I mentioned earlier, and create a new SctpTransport for each association. I'll raise this question in the WebRTC working group: w3c/webrtc-pc#979

@aboba
Copy link
Contributor

aboba commented Dec 22, 2016

Some possibilities:

  1. add a remotePort argument to the start method
  2. add a remotePort argument to the constructor
  3. add a getLocalParameters method returning a dictionary which includes the port and change the argument of the start method to RTCSctpParameters remoteParams rather than RTCSctpCapabilities remoteCaps.

Approach 1 allows the start method to be called again to change the remote port. Approach 2 would require a new object if either the local or remote port changes.

@aboba aboba closed this as completed Dec 22, 2016
@aboba aboba reopened this Dec 22, 2016
@lgrahl
Copy link
Contributor Author

lgrahl commented Dec 22, 2016

Option 1 opens up a new problem: The average user doesn't want to specify which local port is going to be used. Furthermore, no check can be made whether the local port is actually available before calling the constructor. But when the constructor is being called, the remote port must already be known. Effectively, two peers would have to announce their local port to each other before knowing whether that port can be used.

I'm voting for 3 as it seems much more consistent to the rest of the API to have a parameters dict for SCTP as well. Furthermore, I'm suggesting to keep the local port argument at the constructor but add it to the start method as an optional argument as well. The local port of the start method would supersede the local port specified in the constructor. Here's why:

In ORTC, RTCDataChannel instances are created by passing a RTCDataTransport instance to the constructor. This indicates that the data channels are bound to the data transport. In case I want to change the local port, creating a new RTCSctpTransport instance would permanently close all data channels that have been created using that transport instance. At least this is my understanding of ORTC's behaviour.

If I understand correctly (and please correct me if I'm wrong @taylor-b), this is different to WebRTC where the data channels are only temporarily closed until the local port has been changed and a new data transport instance has been established. Then, they are reopened.

So to maintain compatibility, we would have to be able to change both local and remote port without creating a new RTCSctpTransport instance. Is this making sense to you guys?

@taylor-b
Copy link

taylor-b commented Dec 22, 2016

I'd vote for 3 too.

If I understand correctly (and please correct me if I'm wrong @taylor-b), this is different to WebRTC where the data channels are only temporarily closed until the local port has been changed and a new data transport instance has been established. Then, they are reopened.

Actually, I think WebRTC is the same in this respect:

An RTCDataChannel object's underlying data transport [aka an RTCSctpTransport] may be torn down in a non-abrupt manner by running the closing procedure. When that happens the user agent must, unless the procedure was initiated by the close method, queue a task that sets the object's readyState attribute to closing.

And there's no mention of it returning from "closing"/"closed".

So to maintain compatibility, we would have to be able to change both local and remote port without creating a new RTCSctpTransport instance. Is this making sense to you guys?

Unless we say "there's one RTCSctpTransport per association". That's what I'm leaning towards here: w3c/webrtc-pc#979

I don't think WebRTC specifies this behavior right now, unless I'm overlooking something.

@lgrahl
Copy link
Contributor Author

lgrahl commented Dec 22, 2016

Thanks for your correction. Fair enough, in that case option 3 as suggested by @aboba should do. Considering that WebRTC doesn't specify this behaviour (yet), maybe we should keep it simple for now and describe that subsequent calls to start will be ignored?

@pthatcherg
Copy link

On the topic of remote ports: in the SDP/JSEP world, the local (client) and remote (server) ports must be the same. See draft-ietf-mmusic-sctp-sdp section 9.3:

" When an SCTP association is established, both SCTP endpoints MUST
initiate the SCTP association (i.e. both SCTP endpoints take the
'active' role), and MUST use the same SCTP port as client port and
server port (in order to prevent two separate SCTP associations from
being established)"

We have the same model in ORTC. In other words, local port == remote port. Thus you only need to have one value passed in as a parameter to the SctpTransport, not two. Just like SIDs for data channels. There's no need to have them be different and everything is simplified by having them be the same.

By the way, this came up before: #227.

@lgrahl
Copy link
Contributor Author

lgrahl commented Jan 3, 2017

When I'm reading this text, I come to a different conclusion. In order to prevent two separate SCTP associations being established, both peers need to connect by using the corresponding local port the other peer chose. They simply have to know each other's local port. However, there's no technical reason why these ports shouldn't be different.

Not being able to choose a local port freely has implications on implementations that may have multiple SCTP connections using the same stack. Application 1 might have been established by the mutual choice of port 5000. However, now I'm using the same stack in the role of the answerer and I'm receiving an offer with sctp-port mapped to 5000. I can't use port 5000 as it's in use. What do we do in this case? (This doesn't seem to be a problem if we use usrsctp but there will be applications that utilise kernel-backed SCTP stacks, for example dctt)

We can easily work around this issue by letting both parties specify their local ports. It is a trivially solved problem (code-wise at least) compared to finding a proper solution for the issue raised in the previous paragraph. Also, one has to ask: If you're correct and this is the intended way, then why are both parties sending an SCTP port number in the offer/answer SDP? See draft-ietf-mmusic-sctp-sdp-20 section 10.2 and 10.3., especially the example in section 13.1.

@pthatcherg
Copy link

pthatcherg commented Jan 3, 2017

Yes, perhaps I did misread draft-ietf-mmusic-sctp-sdp. It's saying that it must listen on and connect with the same port, not that both sides need to pick the same local and remote port.

However, it was the intention of the ORTC design to have the local and remote port always be the same. I suppose that means that ORTC as currently designed would not allow a full draft-ietf-mmusic-sctp-sdp implementation.

But I'd go back to the same logic that I believe we had back then: it's not worth it the extra complexity in the API to be able to support different ports on each side.

@taylor-b
Copy link

taylor-b commented Jan 3, 2017

Given the example above, it seems like it might be worth the added complexity after all. Otherwise the application would need to negotiate to find a port that's not already in-use by either peer.

@pthatcherg
Copy link

The example give sounds like a problem in dctt that needs to be fixed, or some workaround found. It doesn't make sense to add extra complexity to the API and to all implementation of it just because of some limitation in dctt. In principle, there is no reason you can't use port 5000 in that example because it's all virtualized over a completely different DtlsTransport that isn't using port 5000 (in other words, the ports are scoped to DtlsTransports). It's just a limitation of dctt (which might have some other work around). So my answer would be to either fix dctt or find a work around. But don't add a bunch of complexity to everything else.

@lgrahl
Copy link
Contributor Author

lgrahl commented Jan 3, 2017

dctt requires a relaying party that handles DTLS. The workaround is to change the port number (easy) at the relay station and recalculate the checksum (meh!) in the SCTP packet. Yes, it can be done but it's rather messy. And let's not forget that there might be other use cases in the future.

I still believe it's worth the effort and there might be more parameters we want to exchange in the future that are not static (and then we'll have the same discussion again while WebRTC on the other hand can easily cope with it). It makes the API more consistent (as every other transport has parameters, why should it be different for SCTP) and I can't see any substantial changes implementation-wise either (at least in my implementation it was really easy). The SCTP INIT triggered by the constructor can simply be moved into the start method and that's it.

@pthatcherg
Copy link

Perhaps your use case is different than I thought at first.

It sounds like you want to communicate with an SCTP endpoint through a relay that terminates DTLS but not SCTP. Is that right?

That sounds like a more reasonable use case. However, why is a simple solution not to just let that remote endpoint choose the port and use that port for both the local and remote port? I know that doesn't fit the SDP model without an extra round trip, but there's an easy solution for that: don't use SDP.

If we did add something to make this easier, then I would be in favor of #2 (add another parameter to the constructor). It would be a simple enough addition and make this use case slightly easier. But I think option #3 is too much complexity being added for too little benefit.

@lgrahl
Copy link
Contributor Author

lgrahl commented Jan 3, 2017

It sounds like you want to communicate with an SCTP endpoint through a relay that terminates DTLS but not SCTP. Is that right?

Yep.

However, why is a simple solution not to just let that remote endpoint choose the port and use that port for both the local and remote port?

Yes, I know that I'm stretching it a little bit with the following example: What if both terminate at a (possibly even the same) kernel stack? I don't know of a good use case other than for testing purposes but I can tell you that I've had this exact problem in the past and I had to work around it.

If we did add something to make this easier, then I would be in favor of #2 (add another parameter to the constructor). It would be a simple enough addition and make this use case slightly easier. But I think option #3 is too much complexity being added for too little benefit.

I still don't understand where the complexity for option 3 is. We only have do to very few changes to the API and existing implementations:

  1. Make getCapabilities non-static, add the port to it and rename it to getLocalParameters.
  2. Add the port to remoteCaps in the start method and rename remoteCaps to remoteParameters.
  3. SCTP INIT moves from the constructor into the start method. (Implementation-wise, move the connect call into start)

Edit: I can provide a PR to these changes if you want to see it in detail.

@pthatcherg
Copy link

If both endpoints are doing SCTP but not DTLS (and thus not ORTC), then where does ORTC fit into the picture?

As for complexity, I don't think there's any issue with changing the INIT to be at start time (but I may have missed something). I'm mainly opposed to changing from a "caps" model to a "caps and params" or just "params" model. I'm not opposed to that in general (it works well for ICE and DTLS), but it does add complexity for both the user of the API and the implementer, and it's been nice to avoid that complexity for SCTP. The way you describe the addition is just the tip of the iceberg. There would be a lot more to it as we went to work out all the details and edge cases. There always is (and there was for ICE and DTLS). So we should only add it if there's a clear benefit. And I just don't see much benefit. The only benefit I see could be equally served by passing in the remote port to the constructor, which is a lot more simple. So if we're going to add anything, I think it should be the simplest thing that meets the use cases, which is option #2.

@lgrahl
Copy link
Contributor Author

lgrahl commented Jan 3, 2017

If both endpoints are doing SCTP but not DTLS (and thus not ORTC), then where does ORTC fit into the picture?

Good question. I did this for testing purposes only (@tuexen asked me to write a low footprint test tool for WebRTC data channels and I simply chose to implement the ORTC API in C and handle all the SDP-related stuff in the browser using adapterjs, and while writing that I also tested dctt to dctt on the same machine). As I said, I have no real world use case other than that and we don't know what the future will bring us.

Even though I understand your concern about the complexity a little better now, I still think option 3 would be the most adequate one for reasons I've already mentioned in the previous posts (API consistency, WebRTC compatibility (?), future-proofness).

Independent from whatever consensus we reach here, I think we should raise an issue regarding draft-ietf-mmusic-sctp-sdp section 9.3 to further improve the wording there and to clarify which one of our interpretations is correct.

@pthatcherg
Copy link

I don't think draft-ietf-mmusic-sctp-sdp needs to change 9.3 to be more clear. But I did fail to find anywhere that said something like "the value of the sctp-port attribute in the remote description MUST be used as the remote port and the value of the sctp-port attribute in the local description MUST be used as the local port". But perhaps I just didn't see it. See if you can find it. If not, that's what I would suggest as clarification.

@lgrahl
Copy link
Contributor Author

lgrahl commented Jan 3, 2017

In general, I have the feeling that your interpretation of section 9.3 could be the correct one but then it's not clear that the sctp-port attribute is simply repeated if not set to 0 by the answerer and the example in section 13.1 makes no sense because the ports aren't equal.

@tuexen
Copy link

tuexen commented Jan 9, 2017

Just a possible clarification: When using the SDP stuff, each side uses a single SCTP port number. The two numbers can be different, they can be the same. Each side uses only its single port number for communicating with the peer (so not two number for the two roles: client and server).

I don't think it is a limitation to require that both endpoints use the same port number, since the port numbers are only used to multiplex/demultiplex the SCTP connections running over a single DTLS connection. So if a port number is used over one DTLS connection, the same port number can be used over a different DTLS connection. They don't affect each other.

@pthatcherg
Copy link

In response to tuexen:

You have a good point that in the case where there is only one SCTP connection over a DTLS connection, we don't really need to tell the local SctpTransport what port it will receive SCTP on, or even to assume it's the same it will send SCTP on, since there is only one SCTP connection anyway. If one wishes to have more than one SctpTransport over the DtlsTransport, then the remote port (or port if they are equal) needs to be known to demux. But it is an option to simply require that there be only one SctpTransport per DtlsTransport and thus the remote port is irrelevant.

I recall some discussion at the IETF over whether or not anyone would want to have more than one SCTP association, and I recall the answer being that no one really needed it. Do you know if draft-ietf-mmusic-sctp-sdp allows for it or not?

@rshpount
Copy link

rshpount commented Jan 9, 2017

In draft-ietf-mmusic-sctp-sdp we considered multiple SCTP associations over the same DTLS association out of scope (https://tools.ietf.org/html/draft-ietf-mmusic-sctp-sdp-20#section-5.3). sctp-port attribute is required and only a single value of this attribute is allowed per m= line. There are no limitation on the values of sctp-port attribute (they can be the same or different on both sides), as long as they are non 0. While the same pair of values continues to be used, the same SCTP association should be used. Change of sctp-port value on either side of the connection will cause new SCTP association to be established. Setting sctp-port to 0, closes SCTP association. When implementing, sctp-port value should be set to a random number for a new SCTP association, and should be set to previous value to continue using the existing association.

@tuexen
Copy link

tuexen commented Jan 9, 2017

I'm sure I was asked at one point of time to make sure that you can use multiple SCTP associations over a single DTLS connection and made sure this works for DTLS/SCTP. However, I'm not an expert in SDP, but the above comment might answer your question.

@rshpount
Copy link

rshpount commented Jan 9, 2017

@tuexen I think it possible to make multiple SCTP associations to work over the same DTLS association, but we decide that this is going to be out of scope for the current specification. I think this was not needed for rtcweb, so we decided to skip this.

@tuexen
Copy link

tuexen commented Jan 9, 2017

I was referring to making sure it works in draft-ietf-tsvwg-sctp-dtls-encaps-09 and that is what I did. It was a requirement stated in a very early state of the WebRTC standardisation. It was a noop to provide this feature. I don't think it is used in WebRTC right now, so it is fine not to have it in the SDP spec.

@rshpount
Copy link

rshpount commented Jan 9, 2017

@tuexen It would probably be trivial to add this feature in the future revision of sctp-sdp specification. I think all this will require is an update for mux rules for sctp-port. Since we were under time pressure to publish, this was not done in the current version.

@tuexen
Copy link

tuexen commented Jan 9, 2017

@rshpount: As I said: That is fine from my perspective...

aboba added a commit that referenced this issue Jan 11, 2017
@aboba aboba self-assigned this Jan 11, 2017
@lgrahl
Copy link
Contributor Author

lgrahl commented Jan 11, 2017

The PR has already been merged, so I'm commenting here.

This needs to go into start now because the remote port isn't known until start has been called:

Creation of an RTCSctpTransport causes an SCTP INIT request to be issued over the RTCDtlsTransport from the local RTCSctpTransport to the remote RTCSctpTransport where the remote RTCSctpTransport responds with an SCTP INIT-ACK. Since both local and remote parties must mutually create an RTCSctpTransport, SCTP SO (Simultaneous Open) is used to establish a connection over SCTP.

@aboba
Copy link
Contributor

aboba commented Jan 11, 2017

@lgrahl Right. I've moved the text (see http://draft.ortc.org/).

@lgrahl
Copy link
Contributor Author

lgrahl commented Jan 11, 2017

👍 Can be closed from my side.

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

No branches or pull requests

6 participants