diff --git a/lib/_tls_wrap.js b/lib/_tls_wrap.js index 1bff757..0db0b7a 100644 --- a/lib/_tls_wrap.js +++ b/lib/_tls_wrap.js @@ -150,26 +150,31 @@ function onclienthello(hello) { if (err) return self.destroy(err); - self._handle.endParser(); - }); -} - - -function oncertcb(info) { - var self = this; - var servername = info.servername; - - loadSNI(self, servername, function(err, ctx) { - if (err) - return self.destroy(err); - requestOCSP(self, info, ctx, function(err) { + // Servername came from SSL session + // NOTE: TLS Session ticket doesn't include servername information + // + // Another note, From RFC3546: + // + // If, on the other hand, the older + // session is resumed, then the server MUST ignore extensions appearing + // in the client hello, and send a server hello containing no + // extensions; in this case the extension functionality negotiated + // during the original session initiation is applied to the resumed + // session. + // + // Therefore we should account session loading when dealing with servername + var servername = session && session.servername || hello.servername; + loadSNI(self, servername, function(err, ctx) { if (err) return self.destroy(err); - - if (!self._handle) - return self.destroy(new Error('Socket is closed')); - - self._handle.certCbDone(); + requestOCSP(self, hello, ctx, function(err) { + if (err) + return self.destroy(err); + + if (!self._handle) + return self.destroy(new Error('Socket is closed')); + self._handle.endParser(); + }); }); }); } @@ -391,18 +396,15 @@ TLSSocket.prototype._init = function(socket, wrap) { ssl.onhandshakestart = onhandshakestart.bind(this); ssl.onhandshakedone = onhandshakedone.bind(this); ssl.onclienthello = onclienthello.bind(this); - ssl.oncertcb = oncertcb.bind(this); ssl.onnewsession = onnewsession.bind(this); ssl.lastHandshakeTime = 0; ssl.handshakes = 0; - if (this.server) { - if (this.server.listenerCount('resumeSession') > 0 || - this.server.listenerCount('newSession') > 0) { - ssl.enableSessionCallbacks(); - } - if (this.server.listenerCount('OCSPRequest') > 0) - ssl.enableCertCb(); + if (this.server && + (this.server.listenerCount('resumeSession') > 0 || + this.server.listenerCount('newSession') > 0 || + this.server.listenerCount('OCSPRequest') > 0)) { + ssl.enableSessionCallbacks(); } } else { ssl.onhandshakestart = function() {}; @@ -444,7 +446,7 @@ TLSSocket.prototype._init = function(socket, wrap) { options.server._contexts.length)) { assert(typeof options.SNICallback === 'function'); this._SNICallback = options.SNICallback; - ssl.enableCertCb(); + ssl.enableHelloParser(); } if (process.features.tls_npn && options.NPNProtocols) diff --git a/src/env.h b/src/env.h index 1107dcb..ad5e532 100644 --- a/src/env.h +++ b/src/env.h @@ -56,7 +56,6 @@ namespace node { V(bytes_parsed_string, "bytesParsed") \ V(callback_string, "callback") \ V(change_string, "change") \ - V(oncertcb_string, "oncertcb") \ V(onclose_string, "_onclose") \ V(code_string, "code") \ V(compare_string, "compare") \ diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 40f065e..0a1020f 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -163,8 +163,6 @@ template int SSLWrap::SelectNextProtoCallback( #endif template int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg); template void SSLWrap::DestroySSL(); -template int SSLWrap::SSLCertCallback(SSL* s, void* arg); -template void SSLWrap::WaitForCertCb(CertCb cb, void* arg); static void crypto_threadid_cb(CRYPTO_THREADID* tid) { @@ -552,8 +550,7 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx, } while ((ca = PEM_read_bio_X509(in, nullptr, CryptoPemCallback, nullptr))) { - // NOTE: Increments reference count on `ca` - r = SSL_CTX_add1_chain_cert(ctx, ca); + r = SSL_CTX_add_extra_chain_cert(ctx, ca); if (!r) { X509_free(ca); @@ -987,7 +984,7 @@ void SecureContext::SetTicketKeys(const FunctionCallbackInfo& args) { void SecureContext::SetFreeListLength(const FunctionCallbackInfo& args) { SecureContext* wrap = Unwrap(args.Holder()); - wrap->ctx_->freelist_max_len = args[0]->Int32Value(); + //wrap->ctx_->freelist_max_len = args[0]->Int32Value(); } @@ -1126,7 +1123,6 @@ void SSLWrap::AddMethods(Environment* env, Local t) { env->SetProtoMethod(t, "verifyError", VerifyError); env->SetProtoMethod(t, "getCurrentCipher", GetCurrentCipher); env->SetProtoMethod(t, "endParser", EndParser); - env->SetProtoMethod(t, "certCbDone", CertCbDone); env->SetProtoMethod(t, "renegotiate", Renegotiate); env->SetProtoMethod(t, "shutdownSSL", Shutdown); env->SetProtoMethod(t, "getTLSTicket", GetTLSTicket); @@ -2015,122 +2011,6 @@ int SSLWrap::TLSExtStatusCallback(SSL* s, void* arg) { template -void SSLWrap::WaitForCertCb(CertCb cb, void* arg) { - cert_cb_ = cb; - cert_cb_arg_ = arg; -} - - -template -int SSLWrap::SSLCertCallback(SSL* s, void* arg) { - Base* w = static_cast(SSL_get_app_data(s)); - - if (!w->is_server()) - return 1; - - if (!w->is_waiting_cert_cb()) - return 1; - - if (w->cert_cb_running_) - return -1; - - Environment* env = w->env(); - HandleScope handle_scope(env->isolate()); - Context::Scope context_scope(env->context()); - w->cert_cb_running_ = true; - - Local info = Object::New(env->isolate()); - - SSL_SESSION* sess = SSL_get_session(s); - if (sess != nullptr) { - if (sess->tlsext_hostname == nullptr) { - info->Set(env->servername_string(), String::Empty(env->isolate())); - } else { - Local servername = OneByteString(env->isolate(), - sess->tlsext_hostname, - strlen(sess->tlsext_hostname)); - info->Set(env->servername_string(), servername); - } - info->Set(env->tls_ticket_string(), - Boolean::New(env->isolate(), sess->tlsext_ticklen != 0)); - } - bool ocsp = s->tlsext_status_type == TLSEXT_STATUSTYPE_ocsp; - info->Set(env->ocsp_request_string(), Boolean::New(env->isolate(), ocsp)); - - Local argv[] = { info }; - w->MakeCallback(env->oncertcb_string(), ARRAY_SIZE(argv), argv); - - if (!w->cert_cb_running_) - return 1; - - // Performing async action, wait... - return -1; -} - - -template -void SSLWrap::CertCbDone(const FunctionCallbackInfo& args) { - Base* w = Unwrap(args.Holder()); - Environment* env = w->env(); - - CHECK(w->is_waiting_cert_cb() && w->cert_cb_running_); - - Local object = w->object(); - Local ctx = object->Get(env->sni_context_string()); - Local cons = env->secure_context_constructor_template(); - - // Not an object, probably undefined or null - if (!ctx->IsObject()) - goto fire_cb; - - if (cons->HasInstance(ctx)) { - SecureContext* sc = Unwrap(ctx.As()); - w->sni_context_.Reset(); - w->sni_context_.Reset(env->isolate(), ctx); - - int rv; - - // NOTE: reference count is not increased by this API methods - X509* x509 = SSL_CTX_get0_certificate(sc->ctx_); - EVP_PKEY* pkey = SSL_CTX_get0_privatekey(sc->ctx_); - STACK_OF(X509)* chain; - - rv = SSL_CTX_get0_chain_certs(sc->ctx_, &chain); - if (rv) - rv = SSL_use_certificate(w->ssl_, x509); - if (rv) - rv = SSL_use_PrivateKey(w->ssl_, pkey); - if (rv && chain != nullptr) - rv = SSL_set1_chain(w->ssl_, chain); - if (!rv) { - unsigned long err = ERR_get_error(); - if (!err) - return env->ThrowError("CertCbDone"); - return ThrowCryptoError(env, err); - } - } else { - // Failure: incorrect SNI context object - Local err = Exception::TypeError(env->sni_context_err_string()); - w->MakeCallback(env->onerror_string(), 1, &err); - return; - } - - fire_cb: - CertCb cb; - void* arg; - - cb = w->cert_cb_; - arg = w->cert_cb_arg_; - - w->cert_cb_running_ = false; - w->cert_cb_ = nullptr; - w->cert_cb_arg_ = nullptr; - - cb(arg); -} - - -template void SSLWrap::SSLGetter(Local property, const PropertyCallbackInfo& info) { HandleScope scope(info.GetIsolate()); @@ -2236,10 +2116,6 @@ int Connection::HandleSSLError(const char* func, DEBUG_PRINT("[%p] SSL: %s want read\n", ssl_, func); return 0; - } else if (err == SSL_ERROR_WANT_X509_LOOKUP) { - DEBUG_PRINT("[%p] SSL: %s want x509 lookup\n", ssl_, func); - return 0; - } else if (err == SSL_ERROR_ZERO_RETURN) { HandleScope scope(ssl_env()->isolate()); @@ -2420,7 +2296,7 @@ inline int VerifyCallback(int preverify_ok, X509_STORE_CTX* ctx) { SSL* ssl = static_cast( X509_STORE_CTX_get_ex_data(ctx, SSL_get_ex_data_X509_STORE_CTX_idx())); - if (SSL_is_server(ssl)) + if (ssl->server) return 1; // Client needs to check if the server cert is listed in the @@ -2447,7 +2323,7 @@ int Connection::SelectSNIContextCallback_(SSL *s, int *ad, void* arg) { // Call the SNI callback and use its return value as context if (!conn->sniObject_.IsEmpty()) { - conn->sni_context_.Reset(); + conn->sniContext_.Reset(); Local sni_obj = PersistentToLocal(env->isolate(), conn->sniObject_); @@ -2463,7 +2339,7 @@ int Connection::SelectSNIContextCallback_(SSL *s, int *ad, void* arg) { Local secure_context_constructor_template = env->secure_context_constructor_template(); if (secure_context_constructor_template->HasInstance(ret)) { - conn->sni_context_.Reset(env->isolate(), ret); + conn->sniContext_.Reset(env->isolate(), ret); SecureContext* sc = Unwrap(ret.As()); InitNPN(sc); SSL_set_SSL_CTX(s, sc->ctx_); @@ -2502,8 +2378,6 @@ void Connection::New(const FunctionCallbackInfo& args) { InitNPN(sc); - SSL_set_cert_cb(conn->ssl_, SSLWrap::SSLCertCallback, conn); - #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB if (is_server) { SSL_CTX_set_tlsext_servername_callback(sc->ctx_, SelectSNIContextCallback_); diff --git a/src/node_crypto.h b/src/node_crypto.h index 3bec02c..1049e14 100644 --- a/src/node_crypto.h +++ b/src/node_crypto.h @@ -164,10 +164,7 @@ class SSLWrap { kind_(kind), next_sess_(nullptr), session_callbacks_(false), - new_session_wait_(false), - cert_cb_(nullptr), - cert_cb_arg_(nullptr), - cert_cb_running_(false) { + new_session_wait_(false) { ssl_ = SSL_new(sc->ctx_); env_->isolate()->AdjustAmountOfExternalAllocatedMemory(kExternalSize); CHECK_NE(ssl_, nullptr); @@ -184,9 +181,6 @@ class SSLWrap { npn_protos_.Reset(); selected_npn_proto_.Reset(); #endif -#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - sni_context_.Reset(); -#endif #ifdef NODE__HAVE_TLSEXT_STATUS_CB ocsp_response_.Reset(); #endif // NODE__HAVE_TLSEXT_STATUS_CB @@ -197,11 +191,8 @@ class SSLWrap { inline bool is_server() const { return kind_ == kServer; } inline bool is_client() const { return kind_ == kClient; } inline bool is_waiting_new_session() const { return new_session_wait_; } - inline bool is_waiting_cert_cb() const { return cert_cb_ != nullptr; } protected: - typedef void (*CertCb)(void* arg); - // Size allocated by OpenSSL: one for SSL structure, one for SSL3_STATE and // some for buffers. // NOTE: Actually it is much more than this @@ -229,7 +220,6 @@ class SSLWrap { static void VerifyError(const v8::FunctionCallbackInfo& args); static void GetCurrentCipher(const v8::FunctionCallbackInfo& args); static void EndParser(const v8::FunctionCallbackInfo& args); - static void CertCbDone(const v8::FunctionCallbackInfo& args); static void Renegotiate(const v8::FunctionCallbackInfo& args); static void Shutdown(const v8::FunctionCallbackInfo& args); static void GetTLSTicket(const v8::FunctionCallbackInfo& args); @@ -258,12 +248,10 @@ class SSLWrap { void* arg); #endif // OPENSSL_NPN_NEGOTIATED static int TLSExtStatusCallback(SSL* s, void* arg); - static int SSLCertCallback(SSL* s, void* arg); static void SSLGetter(v8::Local property, const v8::PropertyCallbackInfo& info); void DestroySSL(); - void WaitForCertCb(CertCb cb, void* arg); inline Environment* ssl_env() const { return env_; @@ -275,12 +263,6 @@ class SSLWrap { SSL* ssl_; bool session_callbacks_; bool new_session_wait_; - - // SSL_set_cert_cb - CertCb cert_cb_; - void* cert_cb_arg_; - bool cert_cb_running_; - ClientHelloParser hello_parser_; #ifdef NODE__HAVE_TLSEXT_STATUS_CB @@ -292,10 +274,6 @@ class SSLWrap { v8::Persistent selected_npn_proto_; #endif // OPENSSL_NPN_NEGOTIATED -#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB - v8::Persistent sni_context_; -#endif - friend class SecureContext; }; @@ -307,6 +285,7 @@ class Connection : public SSLWrap, public AsyncWrap { ~Connection() override { #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB sniObject_.Reset(); + sniContext_.Reset(); servername_.Reset(); #endif } @@ -321,6 +300,7 @@ class Connection : public SSLWrap, public AsyncWrap { #ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB v8::Persistent sniObject_; + v8::Persistent sniContext_; v8::Persistent servername_; #endif diff --git a/src/tls_wrap.cc b/src/tls_wrap.cc index b466afb..97d7a48 100644 --- a/src/tls_wrap.cc +++ b/src/tls_wrap.cc @@ -142,8 +142,6 @@ void TLSWrap::InitSSL() { InitNPN(sc_); - SSL_set_cert_cb(ssl_, SSLWrap::SSLCertCallback, this); - if (is_server()) { SSL_set_accept_state(ssl_); } else if (is_client()) { @@ -354,7 +352,6 @@ Local TLSWrap::GetSSLError(int status, int* err, const char** msg) { case SSL_ERROR_NONE: case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: - case SSL_ERROR_WANT_X509_LOOKUP: break; case SSL_ERROR_ZERO_RETURN: return scope.Escape(env()->zero_return_string()); @@ -751,6 +748,12 @@ void TLSWrap::EnableSessionCallbacks( "EnableSessionCallbacks after destroySSL"); } wrap->enable_session_callbacks(); + EnableHelloParser(args); +} + + +void TLSWrap::EnableHelloParser(const FunctionCallbackInfo& args) { + TLSWrap* wrap = Unwrap(args.Holder()); NodeBIO::FromBIO(wrap->enc_in_)->set_initial(kMaxHelloLength); wrap->hello_parser_.Start(SSLWrap::OnClientHello, OnClientHelloParseEnd, @@ -775,12 +778,6 @@ void TLSWrap::DestroySSL(const FunctionCallbackInfo& args) { } -void TLSWrap::EnableCertCb(const FunctionCallbackInfo& args) { - TLSWrap* wrap = Unwrap(args.Holder()); - wrap->WaitForCertCb(OnClientHelloParseEnd, wrap); -} - - void TLSWrap::OnClientHelloParseEnd(void* arg) { TLSWrap* c = static_cast(arg); c->Cycle(); @@ -879,8 +876,8 @@ void TLSWrap::Initialize(Local target, env->SetProtoMethod(t, "start", Start); env->SetProtoMethod(t, "setVerifyMode", SetVerifyMode); env->SetProtoMethod(t, "enableSessionCallbacks", EnableSessionCallbacks); + env->SetProtoMethod(t, "enableHelloParser", EnableHelloParser); env->SetProtoMethod(t, "destroySSL", DestroySSL); - env->SetProtoMethod(t, "enableCertCb", EnableCertCb); StreamBase::AddMethods(env, t, StreamBase::kFlagHasWritev); SSLWrap::AddMethods(env, t); diff --git a/src/tls_wrap.h b/src/tls_wrap.h index 47cbf27..f516798 100644 --- a/src/tls_wrap.h +++ b/src/tls_wrap.h @@ -132,7 +132,7 @@ class TLSWrap : public crypto::SSLWrap, static void SetVerifyMode(const v8::FunctionCallbackInfo& args); static void EnableSessionCallbacks( const v8::FunctionCallbackInfo& args); - static void EnableCertCb( + static void EnableHelloParser( const v8::FunctionCallbackInfo& args); static void DestroySSL(const v8::FunctionCallbackInfo& args); @@ -161,6 +161,10 @@ class TLSWrap : public crypto::SSLWrap, // If true - delivered EOF to the js-land, either after `close_notify`, or // after the `UV_EOF` on socket. bool eof_; + +#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB + v8::Persistent sni_context_; +#endif // SSL_CTRL_SET_TLSEXT_SERVERNAME_CB }; } // namespace node diff --git a/test/parallel/parallel.status b/test/parallel/parallel.status index 42c32d0..69a1547 100644 --- a/test/parallel/parallel.status +++ b/test/parallel/parallel.status @@ -12,7 +12,6 @@ test-tls-ticket-cluster : PASS,FLAKY [$system==linux] test-cluster-worker-forced-exit : PASS,FLAKY test-http-client-timeout-event : PASS,FLAKY -test-tls-no-sslv3 : PASS,FLAKY test-child-process-buffering : PASS,FLAKY test-child-process-exit-code : PASS,FLAKY diff --git a/test/parallel/test-tls-cnnic-whitelist.js b/test/parallel/test-tls-cnnic-whitelist.js index 85e1d90..fc5898e 100644 --- a/test/parallel/test-tls-cnnic-whitelist.js +++ b/test/parallel/test-tls-cnnic-whitelist.js @@ -53,7 +53,9 @@ var testCases = [ port: common.PORT, rejectUnauthorized: true }, - errorCode: 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY' + // LibreSSL returns CERT_UNTRUSTED in this case, OpenSSL UNABLE_TO_GET_ISSUER_CERT_LOCALLY. + errorCode: 'CERT_UNTRUSTED' + //errorCode: 'UNABLE_TO_GET_ISSUER_CERT_LOCALLY' } ]; diff --git a/test/parallel/test-tls-no-sslv3.js b/test/parallel/test-tls-no-sslv3.js deleted file mode 100644 index 9777397..0000000 --- a/test/parallel/test-tls-no-sslv3.js +++ /dev/null @@ -1,46 +0,0 @@ -'use strict'; -var common = require('../common'); -var assert = require('assert'); - -if (!common.hasCrypto) { - console.log('1..0 # Skipped: missing crypto'); - return; -} -var tls = require('tls'); - -var fs = require('fs'); -var spawn = require('child_process').spawn; - -if (common.opensslCli === false) { - console.log('1..0 # Skipped: node compiled without OpenSSL CLI.'); - return; -} - -var cert = fs.readFileSync(common.fixturesDir + '/test_cert.pem'); -var key = fs.readFileSync(common.fixturesDir + '/test_key.pem'); -var server = tls.createServer({ cert: cert, key: key }, assert.fail); - -server.listen(common.PORT, '127.0.0.1', function() { - var address = this.address().address + ':' + this.address().port; - var args = ['s_client', - '-no_ssl2', - '-ssl3', - '-no_tls1', - '-no_tls1_1', - '-no_tls1_2', - '-connect', address]; - - // for the performance and stability issue in s_client on Windows - if (common.isWindows) - args.push('-no_rand_screen'); - - var client = spawn(common.opensslCli, args, { stdio: 'inherit' }); - client.once('exit', common.mustCall(function(exitCode) { - assert.equal(exitCode, 1); - server.close(); - })); -}); - -server.once('clientError', common.mustCall(function(err, conn) { - assert(/:wrong version number/.test(err.message)); -})); diff --git a/test/parallel/test-tls-sni-server-client.js b/test/parallel/test-tls-sni-server-client.js index bad5e10..910c8c3 100644 --- a/test/parallel/test-tls-sni-server-client.js +++ b/test/parallel/test-tls-sni-server-client.js @@ -36,11 +36,6 @@ var SNIContexts = { 'asterisk.test.com': { key: loadPEM('agent3-key'), cert: loadPEM('agent3-cert') - }, - 'chain.example.com': { - key: loadPEM('agent6-key'), - // NOTE: Contains ca3 chain cert - cert: loadPEM('agent6-cert') } }; @@ -48,29 +43,32 @@ var serverPort = common.PORT; var clientsOptions = [{ port: serverPort, + key: loadPEM('agent1-key'), + cert: loadPEM('agent1-cert'), ca: [loadPEM('ca1-cert')], servername: 'a.example.com', rejectUnauthorized: false }, { port: serverPort, + key: loadPEM('agent2-key'), + cert: loadPEM('agent2-cert'), ca: [loadPEM('ca2-cert')], servername: 'b.test.com', rejectUnauthorized: false }, { port: serverPort, + key: loadPEM('agent2-key'), + cert: loadPEM('agent2-cert'), ca: [loadPEM('ca2-cert')], servername: 'a.b.test.com', rejectUnauthorized: false }, { port: serverPort, + key: loadPEM('agent3-key'), + cert: loadPEM('agent3-cert'), ca: [loadPEM('ca1-cert')], servername: 'c.wrong.com', rejectUnauthorized: false -}, { - port: serverPort, - ca: [loadPEM('ca1-cert')], - servername: 'chain.example.com', - rejectUnauthorized: false }]; var serverResults = [], @@ -82,7 +80,6 @@ var server = tls.createServer(serverOptions, function(c) { server.addContext('a.example.com', SNIContexts['a.example.com']); server.addContext('*.test.com', SNIContexts['asterisk.test.com']); -server.addContext('chain.example.com', SNIContexts['chain.example.com']); server.listen(serverPort, startTest); @@ -109,9 +106,7 @@ function startTest() { } process.on('exit', function() { - assert.deepEqual(serverResults, [ - 'a.example.com', 'b.test.com', 'a.b.test.com', 'c.wrong.com', - 'chain.example.com' - ]); - assert.deepEqual(clientResults, [true, true, false, false, true]); + assert.deepEqual(serverResults, ['a.example.com', 'b.test.com', + 'a.b.test.com', 'c.wrong.com']); + assert.deepEqual(clientResults, [true, true, false, false]); });