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

Attempt to support browser connections #11

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

chadnickbok
Copy link
Collaborator

No description provided.

@saghul
Copy link
Collaborator

saghul commented Nov 18, 2015

Hi!

Thanks for taking a stab at this. Unfortunately there are 2 important problems with this patch:

  • It breaks incoming connections (I see you left a comment about detecting the direction, but right now you force actpass)
  • The library implements the latest draft (sctp-port), but your proposal would change it to use the old syntax. This is not a good direction moving forward. Sadly, browser vendors don't implement the new syntax yet. I reported it to Chrome here, but I haven't tested it again. Firefox straight-up doesn't work, I also plan to report that. IMHO, the way forward here would be to use a flag for using the pre-draft-8 syntax or the current one, and detect it automagically for incoming.

The DTLS role can go in a separate fix, and basically consists on always offering actpass and replying with active (rejecting any other combination).

Add support for non draft-8 connections. Draft-8 support is disabled
by default, and can be enabled by setting the rtcdc_peer_connection
enable_draft_8 flag. This support is automatically disabled if a
non draft 8 SDP is supplied as offer/answer.

Track whether a remote offer has already been parsed using the field
have_offer. If a remote offer has been parsed, the library switches
to generating answers when rtcdc_generate_offer_sdp is called for the
peer connection.
@chadnickbok
Copy link
Collaborator Author

I haven't split these apart yet, but I've updated the commit with some of your suggestions.

My main remaining question is how the library should "philosophically" handle things like switching between supported drafts and detecting offer/answer. In these changes I've tried to make this as transparent to the user as possible, with flags being added to the rtcdc_peer_connection struct to configure the behavior.

The other option would be to make the functions themselves explicitly accept arguments for things like offer/answer, and make users of the library responsible for tracking their own state a supported features.

Thoughts?

OpenSSL memory BIO's don't preserve message boundaries. But when
using datagram-based protocols such as DTLS, preserving such boundaries
is essential.

This patch changes a *lot* of data structures inside rtcdc to be queues
instead of memory BIOs, or wraps existing memory BIO usage with queues.
This allows for functional integration with the Chrome WebRTC
DataChannel implementation.
@chadnickbok
Copy link
Collaborator Author

So it turns out the way librtcdc uses OpenSSL to wrap messages in DTLS isn't correct. Specifically, using one thread to place encrypted messages into a memory-backed BIO and reading them from another thread isn't a good idea, as it leads to multiple messages being places into a single outgoing packet.

Not only is this wrong, this behavior will crash whatever chrome tab is on the other side.

At this stage I think this pull request has gotten a little crazy. I'd try to split it up, but without XHS himself weighing this library is essentially now mine.

@saghul
Copy link
Collaborator

saghul commented Nov 25, 2015

Hi,

He gave me commit access a while ago, but I could not get in touch with him :-S

So, if you want to pursue the changes, I can merge them, and hopefully he'll be OK with that.

Can you split them into logical fixes?

Thanks!

@chadnickbok
Copy link
Collaborator Author

Okay, perfect! I'll have them split up and make pull requests over the next ~7 days (it's thanksgiving here!)

I'm thinking of splitting this into 3 parts:

  • a small change to add a fallback to older SDP syntax (and hopefully support Firefox)
  • a small change to fix offer/answer mode
  • a larger change to correctly support the boundaries between DTLS messages
  • a larger change to ensure that the library supports both client and server modes, and can both connect and receive from itself and chrome/Firefox

As for supporting offer/answer modes - what do you think makes more sense, implicitly tracking state in the peer connection or extending the c functions to support flags specifying behavior?

Perhaps after that I can get access myself :-P

@xhs
Copy link
Owner

xhs commented Nov 26, 2015

Hi, @chadnickbok @saghul , sorry for the delay...
But I have been too busy recently.

Excellent work! Big thanks!

I will add @chadnickbok to this repo and maybe months later I will come back again... :(

@saghul
Copy link
Collaborator

saghul commented Nov 26, 2015

Sounds like a plan!

As for the offer / answer modes, IMHO it would be a good idea to have
separate functions for setting tthe local and remote descriptions, in the
same fashion as the JS API, sort of.

On Thursday, November 26, 2015, Nick Chadwick notifications@github.com
wrote:

Okay, perfect! I'll have them split up and make pull requests over the
next ~7 days (it's thanksgiving here!)

I'm thinking of splitting this into 3 parts:

  • a small change to add a fallback to older SDP syntax (and hopefully
    support Firefox)
  • a small change to fix offer/answer mode
  • a larger change to correctly support the boundaries between DTLS
    messages
  • a larger change to ensure that the library supports both client and
    server modes, and can both connect and receive from itself and
    chrome/Firefox

As for supporting offer/answer modes - what do you think makes more sense,
implicitly tracking state in the peer connection or extending the c
functions to support flags specifying behavior?

Perhaps after that I can get access myself :-P


Reply to this email directly or view it on GitHub
#11 (comment).

/Saúl
bettercallsaghul.com

@Globik
Copy link

Globik commented Feb 4, 2018

In browser does not work. No idea. Complains about fingerprint.

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.

4 participants