Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

DTLS - Draft approach #6704

Closed
wants to merge 8 commits into from
Closed

DTLS - Draft approach #6704

wants to merge 8 commits into from

Conversation

migounette
Copy link

All,

This is a flush of the work done on DTLS support for node 0.11.x, this is not a stabilized version BUT I need comment how I modified the tls wrap, dgram and stream classes.

The goal was to keep the tls.cpp a maximum unchanged for the Datagram support and keep the Stream support unchanged.

Main changes philosophy

I tried to keep the maximum common code with TLS, DTLS is very close thanks to the OpenSSL
So, I extended Stream | TLSCallback | UDP in order to have an agnostic Callback wrapper (not stream colorized)
And I add an interface to needed function (Read | Write for Stream and Send | Receive for Datagram)

TODO

A lot stuff to be done in order to insure that is fully functional

  • Pass TSL tests (insure that there is no regression)
  • Add DTLS tests in /tests sections
  • Check DTLS algorithm used
  • ...
  • node.gyp (uncomment for js inclusion)

I will work on it in order to complete the work. But first I need feedback to insure that I am on the good tracks

How to test

[Server]
set NODE_PATH=C:\node.dtls\lib
cd c:\node.dtls\tests
..\debug\node .\dtls-test.js

[Client]
With OpenSSL
Go to .\bin\PEM
..\openssl.exe s_client -connect localhost:8000 -dtls1

Then type any text and return, the server replies pong - {your text}

@Nodejs-Jenkins
Copy link

Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion.

Commit migounette/node@28a512b9ad7e044071d1e639385bd2e3f63c9c26 has the following error(s):

  • Commit message must indicate the subsystem this commit changes

Commit migounette/node@28ed2d92c352ae3576408028bf167154c087cb03 has the following error(s):

  • First line of commit message must be no longer than 50 characters

The following commiters were not found in the CLA:

  • migounette

You can fix all these things without opening another issue.

Please see CONTRIBUTING.md for more information

@indutny
Copy link
Member

indutny commented Dec 14, 2013

@migounette wow, that's a lot of awesome work here :) It is really raw right now, but since it is a work-in-progress I won't comment on style issues just yet.

Your approach with WrapCallbacks seems to be ok for me. One major nit is that you have duplicated a lot of javascript code, I hope it will be reusing more code in the final version.

Thank you for your time!

@migounette
Copy link
Author

Thanks for your comment, for sure I will factorize a little bit all javascripts.

Do you have a test report somewhere ? I am currently running tests, I have a few errors not in the scope of TLS or DTLS, so I was wondering if it's normal or my env. Or do we need to be 100% pass ?

eg. it seems that curl is needed for some operation !

@indutny
Copy link
Member

indutny commented Dec 14, 2013

On current master it should be 100%, if not completely unrelated to tls.

@indutny
Copy link
Member

indutny commented Dec 14, 2013

Also you can try using common.opensslCli for testing: https://github.com/joyent/node/blob/master/test/simple/test-tls-securepair-server.js#L118 . We're now building it for tests purposes on both windows and unixes.

@indutny indutny mentioned this pull request Dec 28, 2013
@bored-engineer
Copy link

One thing I don't see mentioned that will also be needed is the docs for the new api. You may want to add that to the list as well.

@migounette
Copy link
Author

I have some problems with tests:

==> Most of them are due to convertNPNProtocols
What is the goal of the function, is still valid ?

Thanks

On Sun, Dec 29, 2013 at 5:49 AM, Luke Young notifications@github.comwrote:

One thing I don't see mentioned that will also be needed is the docs for
the new api. You may want to add that to the list as well.


Reply to this email directly or view it on GitHubhttps://github.com//pull/6704#issuecomment-31311371
.

@indutny
Copy link
Member

indutny commented Dec 30, 2013

Yes, it converts js array ['spdy/3', 'http/1.1', 'http/1.0' ] to the Buffer: \6spdy/3\7http/1.1\7http/1.0.

@migounette
Copy link
Author

Much better....

Now I need to check what's going wrong with the 20 remaining but it seems
not to be linked to TLS !

On Mon, Dec 30, 2013 at 4:01 PM, Fedor Indutny notifications@github.comwrote:

Yes, it converts js array ['spdy/3', 'http/1.1', 'http/1.0' ] to the
Buffer: \6spdy/3\7http/1.1\7http/1.0.


Reply to this email directly or view it on GitHubhttps://github.com//pull/6704#issuecomment-31349079
.

@trevnorris
Copy link

Impressive.

@indutny When you're ready to review this, ping me. I'll handle the JS side.

@indutny
Copy link
Member

indutny commented Jan 7, 2014

I'll review it once there will be 100% test pass :)

@migounette
Copy link
Author

Not so far :) but I got a problem with my back. Was away for node for a few days.
I will put more stuff before on the end of the week on my fork.

Regards

@indutny
Copy link
Member

indutny commented Jan 8, 2014

Oh, no rush! Hope you'll get better soon. Thank you for working on it.

@mscdex
Copy link

mscdex commented Jan 21, 2014

+1

@migounette
Copy link
Author

@indutny @trevnorris

Shall I need to wait 0.13.x for flushing the DTLS stuff,

Any idea when 0.12.x will be out ?

Depending on your answer I will resync with 0.11.12 or I will wait 0.13.x for flushing DTLS stuff.

Regards

@indutny
Copy link
Member

indutny commented Mar 27, 2014

@migounette I think we should aim for v0.13.

@jpillora
Copy link

+1

@bushje
Copy link

bushje commented Aug 1, 2014

DTLS is highly desirable for CoAP/IoT applications so would be very nice to have native support for it in node asap.

@trevnorris
Copy link

This patch is aging and needs to be rebased on latest master. Sorry, that's probably not going to be trivial.

@migounette
Copy link
Author

@trevnorris @indutny

I have nearly finished a huge work with 0.11.x around webRTC and big native node, works fine. I need to write some notes on the debug stuff around memory leakages and native crashes.

The previous work was based on version 0.11.9, I can resync on the top of trunk, but I think you prefer to target DTLS just after 0.12.x.

So, I can try to add it on branch 0.11.x ? maybe too late for a such modifications.
Or just after 0.12.x but I have not idea of the date.

Just let me know.

My 2 cents

@indutny
Copy link
Member

indutny commented Aug 2, 2014

@migounette thank you for all your efforts! Personally, I am still afraid of introducing this in v0.11, but may be @trevnorris or @tjfontaine has something to say about it?

Also, how risky do you think it would be to include in in v0.12?

@trevnorris
Copy link

@indutny I can't give an authoritative say regarding anything to do w/ crypto and stability. Though I'll be happy to do a code review and at least give it a logic/style check.

@migounette
Copy link
Author

@trevnorris or @tjfontaine
I would prefer to push and resync all necessary modifications just after v0.12.x as proposed by @indutny
Any idea of v0.12.x target date (or your wish ?)

@tjfontaine
Copy link

@migounette we have branched 0.12, so tracking master is still appropriate here, you can stay on that tip.

As far as when 0.12 is ready will depend on when the team is satisfied with it, but we're working on it.

@trevnorris
Copy link

@indutny can you give a personal opinion on the general stability of the code (only looking briefly though)?

@bushje
Copy link

bushje commented Aug 19, 2014

Any news on progress with this? I am very keen to test something soon!

@indutny
Copy link
Member

indutny commented Aug 22, 2014

Personal opinion: let's do it after v0.12

@migounette
Copy link
Author

I agree, it's necessary to discuss around the DTLS API options
(Certificate, flavors...) .

But meanwhile, I will backport the code on my branch with v0.12 in order to
avoid a problem with the trunk.

On Fri, Aug 22, 2014 at 3:36 PM, Fedor Indutny notifications@github.com
wrote:

Personal opinion: let's do it after v0.12


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

@bushje
Copy link

bushje commented Aug 22, 2014

@indutny: Hi Fedor, Im a software architect working in the Internet of Things (IoT) industry where things are moving very fast. Standards are forming and technologies are evolving rapidly. Node.js is poised to be in a top spot as a server platform of choice in this area. IoT applications are moving down a route where the CoAP protocol (http://coap.technology/) is coming to the front of the queue for device interfaces. This protocol relies heavily on DTLS for its security. At present folks like us who want to use node to build IoT solutions are forced to build our own DTLS stuff on top of node. This is painful. If node doesn't get DTLS 'out of the box' pretty soon it is going to lose ground. Its not a huge amount of effort to get this stuff merged in. I cant help but think it will be worth it to keep node.js at the front of modern web/internet technology. Please please push this one through for v0.12.

@migounette
Copy link
Author

I will work on top of 0.12.x (trunk) and check if we can pass all tests.
Then, I will let talent guys to discuss if the feature can be safely introduced.
It's also necessary to have statbility for a good adoption of the product.

My 2 cents

@indutny
Copy link
Member

indutny commented Aug 27, 2014

Ok, we'll wait for you :)

@oceddi
Copy link

oceddi commented Feb 1, 2015

I second a comment that was made earlier. There is a big wave of interest coming in the IoT space for DTLS running with the CoAP protocol. Those of us wanting to experiment implementing server side solutions for these systems are keenly anticipating native DTLS support in node.

@migounette
Copy link
Author

Well,

I can certainly re-start the implementation of DTLS, but I am a bit disappointed. I was waiting 0.12.x for starting the operation, meanwhile I have 0.11.15 and io.js.

More than one year that the prototype was ready. In fact more push from the community is the key.

I need feedback on the js interfaces... feedback welcome

@sam-github
Copy link

@migounette I think this is appropriate for targetting in node, because its the exposure of more OpenSSL features. I symphasize with your frustration about lack of comment. I suggest you'd get better traction if you

  1. add API documentation, digging through a mountain of code to determine the API is discouraging
  2. PR to https://github.com/iojs/io.js, it isn't feature frozen (like joyent/node 0.12)

@marcnicholas
Copy link

@migounette If you're able to provide some API docs and get this PR'd to io.js, I'll happily test and give feedback.

I'd love this for COAP and echo the sentiment that COAP is about to get big, and a node-like infrastructure needs to support DTLS to be taken seriously in this context.

Thank you for taking the time to put the prototype together and I'm sorry it's not been received better.

-m

@dmoranj dmoranj mentioned this pull request Feb 13, 2015
@Rantanen
Copy link

Rantanen commented Apr 6, 2015

DTLS is also part of ICE (WebRTC) so this would be needed for a pure Node/JS implementation of WebRTC.

I'm up for testing this feature if the PR was available for Node 0.12.x or io.js.

@guymguym
Copy link

👍

@Rantanen
Copy link

Given WebRTC needs this, I've been working on a JavaScript implementation while waiting for native Node.js support: https://github.com/Rantanen/node-dtls

From security standpoint this is a stopgap solution for a native module given I don't really trust my own code for such security critical work. (Although having gone through OpenSSL codebase while building the JS implementation, I'm not sure if I should trust that either. 😛)

Mainly I'm bringing this to the general attention from an API standpoint. Ideally I'd like to implement the proposed Node.js DTLS API.

I haven't really gone through the API proposed in this pull request, but I've had a brief look at it and there seem to be some fundamental differences. Biggest one that I could see was DTLSSocket inheriting dgram.Socket:

dgram.Sockets are unbound sockets that are able to send packets to any recipient, ie. the send() method requires explicit recipient port and address. However DTLSSocket is strictly tied (through the handshake) to a specific remote endpoint.

Proposition

My current API mimics the tls.createServer calls to create a DtlsServer instance, which uses similar secureConnection events to expose the actual secure DtlsSockets. However unlike tls.Socket the DtlsSocket does not implement the Stream interface but instead the send() method similar to dgram.Socket.send() but without the remote port/address parameters.

My project doesn't implement client handshakes yet, but the planned API would again follow the tls API by providing a method similar to tls.connect(), which would yield a DtlsSocket object that adapts the dgram.Socket interface.

Recap
  • Mimic the createServer and connect methods from Node's tls module.
  • Adapt the actual Socket read/write interface from dgram.Socket but tie it to a specific endpoint instead of providing parameters to define remote port/address per function call.

@rgillan
Copy link

rgillan commented May 19, 2015

Hi, I know this has been floating around for a long time but is there an update/plan for inclusion of DTLS natively in node.js? As many others have stated, it's a mandatory requirement for almost any IoT/smart X project and it's applications that node.js will shine at.

@benjsicam
Copy link

Agree. Native DTLS support for Node.js is needed.

@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

While this is definitely something that is important, given that the active development branch has shifted to http://github.com/nodejs/node, it's likely best to close this here, update it, and get it opened against the other repo. @joyent/node-tsc , any objections to closing this here?

@rgillan
Copy link

rgillan commented Aug 13, 2015

absolutely in favour

@jasnell
Copy link
Member

jasnell commented Aug 13, 2015

@rgillan ... just to be clear, is that in favor or closing here and reopening against nodejs/node master?

@rgillan
Copy link

rgillan commented Aug 13, 2015

I am in favour of it being reopened against nodejs/node master, although know others are working on it as well.

@jasnell jasnell self-assigned this Aug 13, 2015
@jasnell
Copy link
Member

jasnell commented Aug 16, 2015

Ok, Opened a new issue in nodejs/node to track and continue the discussion. Closing this PR here as it will never land here. Please do continue the discussion there! Thanks all!

@jasnell jasnell closed this Aug 16, 2015
@ghost
Copy link

ghost commented Aug 18, 2015

bad discussion!

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

Successfully merging this pull request may close these issues.