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

Should dtlsTransport.getLocalParameters().role reflect the effective value? #690

Open
ibc opened this Issue May 11, 2017 · 29 comments

Comments

Projects
None yet
4 participants
@ibc
Contributor

ibc commented May 11, 2017

In Edge latest version I call dtlsTransport.start() with remote role: 'server'. ICE correctly completed and DTLS does connect with the peer, but dtlsTransport.getLocalParameters().role remains "auto" (rather than "client").

Should it reflect the effective value?

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 11, 2017

Why does Edge have a getLocalParameters method on the ICE transport would be my first question? Or did you mean the RTCDtlsTransport instance?

I don't see why the change should be reflected in the DTLS transport but I also don't see any reasons why it shouldn't. But if it should be specified, then I'd vote for "yes, it should be reflected". Otherwise, an implementation would have to remember old parameters which I think is rather pointless. :)

@ibc

This comment has been minimized.

Contributor

ibc commented May 12, 2017

Sorry, yes, I meant RTCDtlsTransport. I've edited the issue description.

@ibc

This comment has been minimized.

Contributor

ibc commented May 12, 2017

I don't see why the change should be reflected in the DTLS transport but I also don't see any reasons why it shouldn't. But if it should be specified, then I'd vote for "yes, it should be reflected". Otherwise, an implementation would have to remember old parameters which I think is rather pointless. :)

The problem is that, theoretically it should not be needed to set remoteDtlsParameters.role when we call dtlsTransport.start() since, by default, the ICE controlling becomes DTLS "server" and the ICE controlled becomes DTLS "client".

I've not tested to leave unset remoteDtlsParameters.role but, assuming it would work, the app would have no way to check whether it's DTLS client or server.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 12, 2017

With leaving it unset you probably meant leaving remoteDtlsParameters.role = 'auto'? I'm using auto all the time in RAWRTC and it works with the mechanism described in the ORTC spec (I have a few issues with that in case of a role conflict but that's another matter - I need to investigate if that's a bug in my implementation before I open an issue for that). I haven't tested it with Edge since Edge has no data channel implementation (shame on you, Edge!) and thus it's pointless to connect Edge with RAWRTC. But if they're following the mechanism described in the spec, there should be no issues with auto.

Still, I'm confused why the application would like to know whether it acts as DTLS server or client. Do you have a use case in mind?

@ibc

This comment has been minimized.

Contributor

ibc commented May 12, 2017

Still, I'm confused why the application would like to know whether it acts as DTLS server or client. Do you have a use case in mind?

Actually I'm coding a RTCPeerConnection shim for Edge. Edge receives the SDP offer so, according to ICE+SDP rules, the SDP offerer is the ICE "controlling". That means that Edge is the ICE "controlled", so if I don't set remoteDtlsParameters.role then Edge should automatically become DTLS "client" role.

But, since ICE role conflicts may happen (so Edge may become ICE controlling and hence DTLS server) I just wanted to check which my local DTLS role is after ICE completes.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 12, 2017

For debugging purposes then I presume? Fair enough. Again, I'd upvote the idea of having the effective role in the parameters. For now, I guess you will have to stare at the STUN packets in Wireshark to debug this.

As you're poking at the same corner case I'm having issues with (role conflict), feel free to ping me if you find anything conflicting in the spec. Maybe, the current behaviour of ORTC isn't entirely compatible to WebRTC (or WebRTC implementations).

For my interest: Do you actually follow the SDP spec in your shim or do you just rip the values out the SDP (no disrespect intended - I'd rather not bother with the SDP spec either) as adapterjs does currently? :)

@ibc

This comment has been minimized.

Contributor

ibc commented May 12, 2017

I think I follow the SDP spec as much as possible. Here the code. It implements Plan-B since that's what lib-jitsi-meet internally uses.

BTW ICE sometimes does not connect (Edge gathers local candidates and receives remote ones, but iceTransport.state remains "checking"). Not sure if it could be due to ICE roles conflicts. Must check.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 12, 2017

Yeah, check out the STUN packets in Wireshark if this happens again. I had a similar issue once with Firefox <-> RAWRTC where both implementations where just constantly changing their role due to a role conflict but had the same pacing interval. It reminded of those awkward situations where you try to avoid another pedestrian on the walkway and you're constantly sidestepping but the other person does exactly the same 😅.

@ibc

This comment has been minimized.

Contributor

ibc commented May 12, 2017

Yep, I'll check that. Thanks!

@robin-raymond

This comment has been minimized.

Contributor

robin-raymond commented May 12, 2017

Right now the role for DTLS does not update; I'm not sure why you would need it since in the case of auto you can always determine what role you went into by looking at the current role result of the ICE transport after DTLS state changes to connected. The role of ICE will determine the role of the DTLS. Is this sufficient or do you think there's a need to update the local parameters? The reason is that the DTLS local parameters will remain auto despite having chosen a role in the end as it was chosen via an auto mechanism. This role value does not reflect the active and current role in DTLS.

@aboba

This comment has been minimized.

Contributor

aboba commented May 12, 2017

Specifically, when dtlsTransport.state = "connected", check dtlsTransport.transport.role.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 12, 2017

Right now the role for DTLS does not update [...]

Right... that's Edge's behaviour. But where is that specified? I can't find it in the spec.

Furthermore, the ICE transport role does not necessarily reflect who is DTLS server and who is client. In auto mode, you have to accept incoming DTLS connections before RTCDtlsTransport.start is being called. Thus, you can end up in the DTLS server role even though the ICE role is controlled... unless I've misunderstood the spec in this point.

@robin-raymond

This comment has been minimized.

Contributor

robin-raymond commented May 12, 2017

@lgrahl Yes, we should describe this part a bit more; it was implied but not clear.

I'm not sure how the order matters. If you say auto, then the role is determined as a result of the ICE exchange. The only case where this might mismatch is if one side chooses auto and the other side chooses client (bad code) and both sides decide they are in client mode and the wrong side wins the DTLS hello connection race. However, that's a misuse of the API and anyone who actively cares about the resulting DTLS role can easily avoid this scenario and obtain a deterministic result.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 12, 2017

IIRC this can also happen in case of a role conflict (both clients think they're controlled, one of the clients wins the DTLS connect race). Again, you may say that's an implementation bug (both shouldn't have been ICE-controlled) and you may be right, but I've seen it happening plenty of times and we will definitely get this scenario in the real world as signalling is up to the application.

@robin-raymond

This comment has been minimized.

Contributor

robin-raymond commented May 12, 2017

@lgrahl Yes, I don't doubt people can and do make mistakes. I guess I question the value of the JS learning the resulting DTLS role (outside perhaps debugging info). And if you truly care about the role, it's certainly possible to be sure by doing the right thing in coding. It seems trivial but none of the objects return "active" parameters. E.g. RTCRtpReceiver does not return SSRCs resulted from auto latching in parameters that get filled automatically. So I'm a bit hesitant to make some getParameters() on some objects return current information and not others without a good use case to justify that inconsistency and behaviour difference.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 12, 2017

I agree that this isn't a good enough use case if it would be inconsistent towards the rest of the API. I just wasn't aware of that - haven't looked at A/V-stuff so far. Need to add this to my to do list though because we're currently returning effective parameters in RAWRTC.

However, I have a question: If we shouldn't return effective parameters, what should the parameters return... the parameters before start has been called? The last parameters that have been returned to the user?

@aboba aboba changed the title from Should iceTransport.getLocalParameters().role reflect the effective value? to Should dtlsTransport.getLocalParameters().role reflect the effective value? May 18, 2017

@aboba

This comment has been minimized.

Contributor

aboba commented May 18, 2017

@robin-raymond The RTCDtlsTransport does not have a role attribute or an onrolechange EventHandler, so if RTCDTlsTransport.getLocalParameters().role does not change, Section 4.6 does not make sense:

auto: Since RTCDtlsRole is initialized to auto on construction of an RTCDtlsTransport object, transport.getLocalParameters().role will have an initial value of auto.

[BA] "initial" seems to imply that the value of RTCDtlsTransport.getLocalParameters().role can change.

client: The DTLS client role. A transition to client will occur if start(remoteParameters) is called with remoteParameters.RTCDtlsRole having a value of server. If RTCDtlsRole had previously had a value of server (e.g. due to the RTCDtlsTransport having previously received packets from a DTLS client), then the DTLS session is reset prior to transitioning to the client role.

[BA] If there is no role attribute or onrolechange EventHandler and getLocalParameters().role does not change, what is making the "transition to client"? Similarly, what "previously had a value of server"?

server: The DTLS server role. If RTCDtlsRole has a value of auto and the RTCDtlsTransport receives a DTLS client_hello packet, RTCDtlsRole will transition to server, even before start() is called. A transition from auto to server will also occur if start(remoteParameters) is called with remoteParameters.RTCDtlsRole having a value of client.

[BA] If there is no role attribute or onrolechange EventHandler and getLocalParameters().role does not change, what "will transition to server"? Similarly, how does a "transition from auto to server" occur?

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 19, 2017

@aboba One could interpret it as an internal parameters variable that is being updated which is not necessarily reflected in the public parameters handed out to the application. But if that would be the case, then I'm still not sure at which point in time the public parameters become frozen (see my previous posting).

@aboba

This comment has been minimized.

Contributor

aboba commented May 19, 2017

@lgrahl Since there is no role attribute or onrolechange EventHandler, if dtlsTransport.getLocalParameters().role does not change, then text implying that "role" changes must be referring to a [[role]] internal slot. I'll work on a PR based on that assumption.

@aboba

This comment has been minimized.

Contributor

aboba commented May 23, 2017

As has been pointed out in the reviews of PR 704, it seems odd for getParameters().role to always return "auto". For example, what if the DtlsTransport has received an incoming DTLS client_hello packet before start() is called and is now functioning as a server. Shouldn't getParameters().role return "server"? Also, the specification appears to be wrong about what happens if an RTCDtlsTransport receives an incoming DTLS client_hello and subsequently start(remoteParameters) is called with remoteParameters.role == "server" (which is a contradiction). In practice, the connection is not reset, start() does not throw an exception and the value of [[role]] does not change, correct?

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 23, 2017

Shouldn't getParameters().role return "server"?

This and all the questions above raised in this issue give me the feeling that it would be much easier to simply return the effective role even if it is slightly inconsistent to the rest of the API. @robin-raymond Can you elaborate where exactly it would clash? I haven't found a parameters-like method for RTCRtpReceiver.

In practice, the connection is not reset, start() does not throw an exception and the value of [[role]] does not change, correct?

What scenario do you have in mind?

@aboba

This comment has been minimized.

Contributor

aboba commented May 23, 2017

@lgrahl In the description of the client role, it says:

"If RTCDtlsRole had previously had a value of server (e.g. due to the RTCDtlsTransport having previously received packets from a DTLS client), then the DTLS session is reset prior to transitioning to the client role."

AFAIK, existing implementations do not behave this way.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 23, 2017

RAWRTC implements it but I've never explicitly tested it so far. I don't really know if it's helpful because if the other client connects (for whatever reason), then it acts as the client and I act as the server. What do we gain by closing that connection? Before discussing whether to drop it or not, I'd like to understand the thoughts of the author of that section. Edit: We should also check out behaviour defined by the relevant SDP section for compatibility.

@aboba

This comment has been minimized.

Contributor

aboba commented May 24, 2017

@ibc @lgrahl Here is a way to determine the role of the RTCDtlsTransport without having to poll getLocalParameters():

Once RTCIceTransport.state is "connected" (e.g. when the RTCIceTransport's statechange event is fired with event.state === "connected"), the ICE peers roles have been determined. This in turn determines the DTLS roles.

If iceTransport.role is "controlling" then the RTCDtlsTransport role is "server"; if iceTransport.role is "controlled" then the RTCDtlsTransport role is "client".

Note that if the RTCIceTransport.role is "controlling" then RTCDtlsTransport.start needs to be called with remoteParameters.role of "client" or "auto" (remoteParameters.role of "server" presumes that neither side will send a client_hello).

If the RTCIceTransport.role is "controlled"then RTCDtlsTransport.start needs to be called with remoteParameters.role of "server" or "auto"  (remoteParameters.role of "client" presumes that both sides will send a client_hello).

Make sense?

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented May 24, 2017

@aboba I understand your point on how the effective role can be determined but that's not my concern at the moment. My point is that returning the effective role seems much simpler (also see the discussion in #704). I cannot assess if this would be inconsistent towards the API of RTCRtpReceiver but I couldn't find anything so far and @robin-raymond didn't answer, yet.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented Jun 21, 2017

PR has been merged now but I still would like to understand where the inconsistency in the API is. Furthermore, I believe the current description is overly complicated due to the fact that we don't return the effective value and there's no good reason why we should not return the effective value. API consistency and simplicity should be carefully weighed up. I cannot judge this as long as I don't see the inconsistency.

@ibc

This comment has been minimized.

Contributor

ibc commented Jun 22, 2017

I cannot judge this as long as I don't see the inconsistency.

@lgrahl I think that you perfectly described such an inconsistency:

the current description is overly complicated due to the fact that we don't return the effective value and there's no good reason why we should not return the effective value.

@aboba

This comment has been minimized.

Contributor

aboba commented Jun 23, 2017

@ibc Returning the current role value is not very useful by itself - what you really want is an event so you don't have to poll. But the API already has an EventHandler that can be used to determine the current role - dtls.transport.onstatechange.

@lgrahl

This comment has been minimized.

Contributor

lgrahl commented Jun 23, 2017

@aboba: Basically this issue can be split into two:

  1. There's a use case for the effective role. However, you already described how this can be done without being able to directly request the effective role. Both solutions are fine by me in this context and I believe @ibc has no objections either (if not, please say so).
  2. However, not returning the effective role results not only in more complicated code but also in a more complicated ORTC API description. This is the one I care about and I want to understand what legitimises this.

@aboba aboba added 1.2 and removed PR exists labels Feb 12, 2018

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