| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
In ssl.onerror event, `this` refers `ssl` so that `this._secureEstablished` is always undefined. Fix it to refer TLSSocket.
There was a problem hiding this comment.
Femto-style nit: can you capitalize 'see' and insert a link to the issue?
Sorry, something went wrong.
|
LGTM with suggestions. A regression test would be really nice though. |
Sorry, something went wrong.
|
Oh, right. How did I forget about it in a first place. @shigeki please add a test indeed. |
Sorry, something went wrong.
|
I've just update the comments. I tried to implement a test but it is very hard to simulate Firefox for sending encrypted TLS alert just after handshake completed. Could you give me any idea? |
Sorry, something went wrong.
|
I guess you could use the raw socket to send the TLS alert frame (in a Buffer). And for ciphers you could try using 'NULL-SHA256' to make sure that you can send the non-encrypted alert. The frame could be generated either by hand or with https://github.com/indutny/tls.js . Please let me know if you need any help with any of these. |
Sorry, something went wrong.
|
Using NULL-SHA256! I never think of that. Okay I try it. |
Sorry, something went wrong.
SSL_read() returns 0 when fatal TLS Alert is received. Fix to invoke ssl error callback in this case.
|
@indutny Using NULL-SHA256 needs mac_write_key so that we have to obtain master_secret and client/server random. I think it is not the best way to implement TLS client in the test. |
Sorry, something went wrong.
|
LGTM, landing. |
Sorry, something went wrong.
In ssl.onerror event, `this` refers `ssl` so that `this._secureEstablished` is always undefined. Fix it to refer TLSSocket. PR-URL: #1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
SSL_read() returns 0 when fatal TLS Alert is received. Fix to invoke ssl error callback in this case. PR-URL: #1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
Sorry, something went wrong.
In ssl.onerror event, `this` refers `ssl` so that `this._secureEstablished` is always undefined. Fix it to refer TLSSocket. PR-URL: nodejs#1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
SSL_read() returns 0 when fatal TLS Alert is received. Fix to invoke ssl error callback in this case. PR-URL: nodejs#1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
In ssl.onerror event, `this` refers `ssl` so that `this._secureEstablished` is always undefined. Fix it to refer TLSSocket. PR-URL: nodejs/node#1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
SSL_read() returns 0 when fatal TLS Alert is received. Fix to invoke ssl error callback in this case. PR-URL: nodejs/node#1661 Reviewed-By: Fedor Indutny <fedor@indutny.com>
Like errno, OpenSSL's API requires SSL_get_error and error queue be checked immediately after the failing operation, otherwise the error queue or SSL object may have changed state and no longer report information about the operation the caller wanted. TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read picks up a closing alert (detected by checking SSL_get_shutdown), Node calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to dispatch on the error. But, by this point, Node has already re-entered JS, which may change the error. In particular, I've observed that, on close_notify, JS seems to sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I think this comes from onStreamRead in stream_base_commons.js?) Instead, SSL_get_error and the error queue should be sampled earlier. Back in nodejs#1661, Node needed to account for GetSSLError being called after ssl_ was destroyed. This was the real cause. With this fixed, there's no need to account for this. (Any case where ssl_ may be destroyed before SSL_get_error is a case where ssl_ or the error queue could change state, so it's a bug either way.) This is the first of two fixes in error-handling here. The EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the peer. Some of the ECONNRESET expectations in the tests aren't actually correct. The next commit will fix this as well.
Like errno, OpenSSL's API requires SSL_get_error and error queue be checked immediately after the failing operation, otherwise the error queue or SSL object may have changed state and no longer report information about the operation the caller wanted. TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read picks up a closing alert (detected by checking SSL_get_shutdown), Node calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to dispatch on the error. But, by this point, Node has already re-entered JS, which may change the error. In particular, I've observed that, on close_notify, JS seems to sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I think this comes from onStreamRead in stream_base_commons.js?) Instead, SSL_get_error and the error queue should be sampled earlier. Back in #1661, Node needed to account for GetSSLError being called after ssl_ was destroyed. This was the real cause. With this fixed, there's no need to account for this. (Any case where ssl_ may be destroyed before SSL_get_error is a case where ssl_ or the error queue could change state, so it's a bug either way.) This is the first of two fixes in error-handling here. The EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the peer. Some of the ECONNRESET expectations in the tests aren't actually correct. The next commit will fix this as well. PR-URL: #44563 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Like errno, OpenSSL's API requires SSL_get_error and error queue be checked immediately after the failing operation, otherwise the error queue or SSL object may have changed state and no longer report information about the operation the caller wanted. TLSWrap almost heeds this rule, except in TLSWrap::ClearOut. If SSL_read picks up a closing alert (detected by checking SSL_get_shutdown), Node calls out to JS with EmitRead(UV_EOF) and only afterwards proceeds to dispatch on the error. But, by this point, Node has already re-entered JS, which may change the error. In particular, I've observed that, on close_notify, JS seems to sometimes call back into TLSWrap::DoShutdown, calling SSL_shutdown. (I think this comes from onStreamRead in stream_base_commons.js?) Instead, SSL_get_error and the error queue should be sampled earlier. Back in #1661, Node needed to account for GetSSLError being called after ssl_ was destroyed. This was the real cause. With this fixed, there's no need to account for this. (Any case where ssl_ may be destroyed before SSL_get_error is a case where ssl_ or the error queue could change state, so it's a bug either way.) This is the first of two fixes in error-handling here. The EmitRead(UV_EOF) seems to additionally swallow fatal alerts from the peer. Some of the ECONNRESET expectations in the tests aren't actually correct. The next commit will fix this as well. PR-URL: #44563 Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
| Back | FazBrowse Home | New Git URL |
This is a fix of the error handling of tls and tls_wrap. Especially it fixes the error handling when the server receiving a fatal TLS alert just after TLS handshake completed. Testing is very hard to implement within node and openssl s_client so that we take this fix to resolve the issue of #1642 from the comments from reporters.
CI results are https://jenkins-iojs.nodesource.com/job/iojs+any-pr+multi/655/ where the test failures on Windows are related to child process not related this.
Fix: #1642
R= @indutny