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

Already buffered data when creating a TLSSocket doesn't trigger/throw 'error' #1114

Closed
jameshartig opened this issue Mar 10, 2015 · 6 comments
Labels
tls Issues and PRs related to the tls subsystem.

Comments

@jameshartig
Copy link
Contributor

Brought over from nodejs/node-v0.x-archive#9355

If you pass a socket with already buffered data to TLSSocket and there's an error like:

[Error: 139745410942944:error:1408A0C1:SSL routines:SSL3_GET_CLIENT_HELLO:no shared cipher:s3_srvr.c:1352]

It will fire as a _tlsError immediately while in the constructor instead of either throwing an error (if there were no error handlers) or waiting till the next tick to fire the _tlsError.

This can be solved by wrapping this._init(socket) in a process.nextTick or delaying:

  // Socket already has some buffered data - emulate receiving it
  if (socket && socket._readableState.length) {
    var buf;
    while ((buf = socket.read()) !== null)
      this.ssl.receive(buf);
  }

It seems like the second option (delaying the initial buffer read) is better but let me know what you guys think.

@Fishrock123 Fishrock123 added the tls Issues and PRs related to the tls subsystem. label Mar 10, 2015
@brendanashworth
Copy link
Contributor

Could you, by chance, provide us some example code illustrating the issue? That might speed up a fix.

@jameshartig
Copy link
Contributor Author

Here's some pseudo code based a project that is hindered by this. The project basically opens a port and allows websocket/HTTP/HTTPS/raw connections on the port. It's sniffing the first packet to see what type it is and then forwarding it onto the correct callback.
https://gist.github.com/fastest963/f1b6b8b6e642aa427240

Once again that's pseudo code but its pretty close to the actual code. Basically when calling the httpsConnectionListener, if there's a handshake error (which there would be since new Server() was called without a cert), it will fire _tlsError after processing the initial data in the buffer (see: https://github.com/iojs/io.js/blob/v1.x/lib/_tls_wrap.js#L396). However we still haven't even added the listener for _tlsError since were still stuck at https://github.com/iojs/io.js/blob/v1.x/lib/_tls_wrap.js#L686 and the listener is added on line 725 (https://github.com/iojs/io.js/blob/v1.x/lib/_tls_wrap.js#L725).

There is initial data because the server is inspecting the first packet before sending it to the tls wrapper to make sure its actually a tls header (indicating an HTTPS connection).

@jameshartig
Copy link
Contributor Author

Any update on this? If someone could just point me in the right direction I'd love to submit a PR to fix this. I'm just not sure which option I should go about doing since I'm not very familiar with this code.

Personally I think putting sending the initial data behind a process.nextTick is maybe the quickest solution but might introduce weird bugs if we read data during the rest of the tick? Once again, not too familiar with this stuff to know if that's possible or not.

A possibly better option would be to not call _init from the constructor and instead make it a separate step we can do so we can set up listeners before calling it. This might be an API-change though so it might be more difficult to land...

@brendanashworth
Copy link
Contributor

I don't really know what to say; from the links through the code that you've given, you probably know enough to try and do a PR (at least more than what I know, TLS-wise). I'd say you should throw together some code and PR it in, or at least chat with one of the crypto's in the io.js irc.

Personally what you're trying to do seems hackish to me and looks like it uses at least a few undocumented things, and it really doesn't look like there is an easy, supported way to do it. Good luck though.

@jameshartig
Copy link
Contributor Author

@brendanashworth, I appreciate you taking the time to look at it. I've opened a PR above that fixes this issue. I decided to go with using process.nextTick to read data after we've added event listeners on _tlsError. I brought up the issue in IRC a few times with no reply.

brendanashworth pushed a commit that referenced this issue Jun 17, 2015
Defer reading until user-land has a chance to add listeners. This
allows the TLS wrapper to listen for _tlsError and trigger a
clientError event if the socket already has data that could trigger.

Fixes: #1114
PR-URL: #1496
Reviewed-By: Chris Dickinson <[email protected]>
Reviewed-By: Fedor Indutny <[email protected]>
@brendanashworth
Copy link
Contributor

Fixed by #1496 / 061342a.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants