Skip to content

tls: use SSL_set_cert_cb for async SNI/OCSP #1464

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.

Sign up for GitHub

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

Closed
indutny wants to merge 2 commits into from
Closed

tls: use SSL_set_cert_cb for async SNI/OCSP #1464

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2f6986e
Working on v1.8.2
chrisdickinson Apr 20, 2015
52ec001
tls: use `SSL_set_cert_cb` for async SNI/OCSP
indutny Apr 18, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
tls: use SSL_set_cert_cb for async SNI/OCSP 
Do not enable ClientHello parser for async SNI/OCSP. Use new
OpenSSL-1.0.2's API `SSL_set_cert_cb` to pause the handshake process and
load the cert/OCSP response asynchronously. Hopefuly this will make
whole async SNI/OCSP process much faster and will eventually let us
remove the ClientHello parser itself (which is currently used only for
async session, see #1462 for the discussion of removing it).

NOTE: Ported our code to `SSL_CTX_add1_chain_cert` to use
`SSL_CTX_get0_chain_certs` in `CertCbDone`. Test provided for this
feature.

Fix: #1423
  • Loading branch information
@indutny
indutny committed Apr 24, 2015
commit 52ec001148e61756a80f65a9b7e3e9927b3e10de
49 changes: 23 additions & 26 deletions lib/_tls_wrap.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,29 +141,23 @@ function onclienthello(hello) {
if (err)
return self.destroy(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) {
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) {
Copy link
Copy Markdown
Contributor

@shigeki shigeki Apr 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ctx is undefined if (!servername || !self._SNICallback) in loadSNI. Do we require SNI to use OCSP stapling?

Copy link
Copy Markdown
Member Author

@indutny indutny Apr 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, we don't :)

if (err)
return self.destroy(err);
requestOCSP(self, hello, ctx, function(err) {
if (err)
return self.destroy(err);

self._handle.endParser();
});
self._handle.certCbDone();
});
});
}
Expand Down Expand Up @@ -325,15 +319,18 @@ 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 &&
(listenerCount(this.server, 'resumeSession') > 0 ||
listenerCount(this.server, 'newSession') > 0 ||
listenerCount(this.server, 'OCSPRequest') > 0)) {
ssl.enableSessionCallbacks();
if (this.server) {
if (listenerCount(this.server, 'resumeSession') > 0 ||
listenerCount(this.server, 'newSession') > 0) {
ssl.enableSessionCallbacks();
}
if (listenerCount(this.server, 'OCSPRequest') > 0)
ssl.enableCertCb();
}
} else {
ssl.onhandshakestart = function() {};
Expand Down Expand Up @@ -374,7 +371,7 @@ TLSSocket.prototype._init = function(socket, wrap) {
options.server._contexts.length)) {
assert(typeof options.SNICallback === 'function');
this._SNICallback = options.SNICallback;
ssl.enableHelloParser();
ssl.enableCertCb();
}

if (process.features.tls_npn && options.NPNProtocols)
Expand Down
1 change: 1 addition & 0 deletions src/env.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ 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") \
Expand Down
132 changes: 129 additions & 3 deletions src/node_crypto.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,8 @@ template int SSLWrap<TLSWrap>::SelectNextProtoCallback(
void* arg);
#endif
template int SSLWrap<TLSWrap>::TLSExtStatusCallback(SSL* s, void* arg);
template int SSLWrap<TLSWrap>::SSLCertCallback(SSL* s, void* arg);
template void SSLWrap<TLSWrap>::WaitForCertCb(CertCb cb, void* arg);


static void crypto_threadid_cb(CRYPTO_THREADID* tid) {
Expand Down Expand Up @@ -509,7 +511,8 @@ int SSL_CTX_use_certificate_chain(SSL_CTX* ctx,
}

while ((ca = PEM_read_bio_X509(in, nullptr, CryptoPemCallback, nullptr))) {
r = SSL_CTX_add_extra_chain_cert(ctx, ca);
// NOTE: Increments reference count on `ca`
r = SSL_CTX_add1_chain_cert(ctx, ca);

if (!r) {
X509_free(ca);
Expand Down Expand Up @@ -978,6 +981,7 @@ void SSLWrap<Base>::AddMethods(Environment* env, Handle<FunctionTemplate> 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);
Expand Down Expand Up @@ -1860,6 +1864,122 @@ int SSLWrap<Base>::TLSExtStatusCallback(SSL* s, void* arg) {
#endif // NODE__HAVE_TLSEXT_STATUS_CB


template <class Base>
void SSLWrap<Base>::WaitForCertCb(CertCb cb, void* arg) {
cert_cb_ = cb;
cert_cb_arg_ = arg;
}


template <class Base>
int SSLWrap<Base>::SSLCertCallback(SSL* s, void* arg) {
Base* w = static_cast<Base*>(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<Object> 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<String> 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<Value> argv[] = { info };
w->MakeCallback(env->oncertcb_string(), ARRAY_SIZE(argv), argv);
Copy link
Copy Markdown
Contributor

@shigeki shigeki Apr 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we avoid invoking the JS callback if no status_type and no server name? Furthermore can we check options.requestOCSP and options.SNICallback here? The cert callback is called in every handshakes so I would like to reduce its overhead.

Copy link
Copy Markdown
Member Author

@indutny indutny Apr 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is not invoked unless options.SNICallback or OCSPRequest event handler is set.

Good point about not invoking it, but let's do it in a follow-up. I'm going to remove most of the ClientHelloParser and cleanup the code.

Copy link
Copy Markdown
Contributor

@shigeki shigeki Apr 20, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@indutny Okay. Let's do it later.


if (!w->cert_cb_running_)
return 1;

// Performing async action, wait...
return -1;
}


template <class Base>
void SSLWrap<Base>::CertCbDone(const FunctionCallbackInfo<Value>& args) {
Base* w = Unwrap<Base>(args.Holder());
Environment* env = w->env();

CHECK(w->is_waiting_cert_cb() && w->cert_cb_running_);

Local<Object> object = w->object();
Local<Value> ctx = object->Get(env->sni_context_string());
Local<FunctionTemplate> 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<SecureContext>(ctx.As<Object>());
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<Value> 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 <class Base>
void SSLWrap<Base>::SSLGetter(Local<String> property,
const PropertyCallbackInfo<Value>& info) {
Expand Down Expand Up @@ -1955,6 +2075,10 @@ 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());

Expand Down Expand Up @@ -2120,7 +2244,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->sniContext_.Reset();
conn->sni_context_.Reset();

Local<Value> arg = PersistentToLocal(env->isolate(), conn->servername_);
Local<Value> ret = conn->MakeCallback(env->onselect_string(), 1, &arg);
Expand All @@ -2129,7 +2253,7 @@ int Connection::SelectSNIContextCallback_(SSL *s, int *ad, void* arg) {
Local<FunctionTemplate> secure_context_constructor_template =
env->secure_context_constructor_template();
if (secure_context_constructor_template->HasInstance(ret)) {
conn->sniContext_.Reset(env->isolate(), ret);
conn->sni_context_.Reset(env->isolate(), ret);
SecureContext* sc = Unwrap<SecureContext>(ret.As<Object>());
InitNPN(sc);
SSL_set_SSL_CTX(s, sc->ctx_);
Expand Down Expand Up @@ -2168,6 +2292,8 @@ void Connection::New(const FunctionCallbackInfo<Value>& args) {

InitNPN(sc);

SSL_set_cert_cb(conn->ssl_, SSLWrap<Connection>::SSLCertCallback, conn);

#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
if (is_server) {
SSL_CTX_set_tlsext_servername_callback(sc->ctx_, SelectSNIContextCallback_);
Expand Down
27 changes: 24 additions & 3 deletions src/node_crypto.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,10 @@ class SSLWrap {
kind_(kind),
next_sess_(nullptr),
session_callbacks_(false),
new_session_wait_(false) {
new_session_wait_(false),
cert_cb_(nullptr),
cert_cb_arg_(nullptr),
cert_cb_running_(false) {
ssl_ = SSL_new(sc->ctx_);
CHECK_NE(ssl_, nullptr);
}
Expand All @@ -157,6 +160,9 @@ 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
Expand All @@ -167,8 +173,11 @@ 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);

static void InitNPN(SecureContext* sc);
static void AddMethods(Environment* env, v8::Handle<v8::FunctionTemplate> t);

Expand All @@ -190,6 +199,7 @@ class SSLWrap {
static void VerifyError(const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetCurrentCipher(const v8::FunctionCallbackInfo<v8::Value>& args);
static void EndParser(const v8::FunctionCallbackInfo<v8::Value>& args);
static void CertCbDone(const v8::FunctionCallbackInfo<v8::Value>& args);
static void Renegotiate(const v8::FunctionCallbackInfo<v8::Value>& args);
static void Shutdown(const v8::FunctionCallbackInfo<v8::Value>& args);
static void GetTLSTicket(const v8::FunctionCallbackInfo<v8::Value>& args);
Expand Down Expand Up @@ -218,9 +228,12 @@ 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<v8::String> property,
const v8::PropertyCallbackInfo<v8::Value>& info);

void WaitForCertCb(CertCb cb, void* arg);

inline Environment* ssl_env() const {
return env_;
}
Expand All @@ -231,6 +244,12 @@ 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
Expand All @@ -242,6 +261,10 @@ class SSLWrap {
v8::Persistent<v8::Value> selected_npn_proto_;
#endif // OPENSSL_NPN_NEGOTIATED

#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
v8::Persistent<v8::Value> sni_context_;
#endif

friend class SecureContext;
};

Expand All @@ -253,7 +276,6 @@ class Connection : public SSLWrap<Connection>, public AsyncWrap {
~Connection() override {
#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
sniObject_.Reset();
sniContext_.Reset();
servername_.Reset();
#endif
}
Expand All @@ -268,7 +290,6 @@ class Connection : public SSLWrap<Connection>, public AsyncWrap {

#ifdef SSL_CTRL_SET_TLSEXT_SERVERNAME_CB
v8::Persistent<v8::Object> sniObject_;
v8::Persistent<v8::Value> sniContext_;
v8::Persistent<v8::String> servername_;
#endif

Expand Down
Loading