From b22b1fdd69367de85edb5a5751a1f0281d66e0d2 Mon Sep 17 00:00:00 2001 From: James M Snell Date: Mon, 3 Jun 2019 22:58:18 +0100 Subject: [PATCH] quic: beginning requestCert and rejectUnauthorized PR-URL: https://github.com/nodejs/quic/pull/31 --- lib/internal/quic/core.js | 24 ++++++++++++--- lib/internal/quic/util.js | 4 +++ src/node_crypto.cc | 1 - src/node_quic_crypto.h | 6 ++-- src/node_quic_session.cc | 28 +++++++++++++---- src/node_quic_session.h | 10 ++++-- src/node_quic_socket.cc | 18 ++++++++--- src/node_quic_socket.h | 6 +++- test/parallel/test-quic-client-server.js | 39 ++++++++++++++++-------- 9 files changed, 104 insertions(+), 32 deletions(-) diff --git a/lib/internal/quic/core.js b/lib/internal/quic/core.js index 981b947212..d4b0bba949 100644 --- a/lib/internal/quic/core.js +++ b/lib/internal/quic/core.js @@ -598,13 +598,19 @@ class QuicSocket extends EventEmitter { port, type = AF_INET, } = { ...preferredAddress }; + const { + rejectUnauthorized = !getAllowUnauthorized(), + requestCert = false, + } = transportParams; setTransportParams(transportParams); this[kHandle].listen( this.#serverSecureContext.context, address, type, port, - this.#alpn); + this.#alpn, + rejectUnauthorized, + requestCert); process.nextTick(emit.bind(this, 'listening')); } @@ -650,7 +656,6 @@ class QuicSocket extends EventEmitter { this.#serverSecureContext = createSecureContext(options, initSecureContext); this.#serverListening = true; this.#alpn = alpn; - const doListen = continueListen.bind( this, @@ -1281,10 +1286,8 @@ class QuicClientSession extends QuicSession { constructor(socket, options) { const sc_options = { secureProtocol: 'TLSv1_3_client_method', - rejectUnauthorized: !getAllowUnauthorized(), ...options }; - const { alpn, dcid, @@ -1323,6 +1326,19 @@ class QuicClientSession extends QuicSession { this.destroy(new ERR_TLS_DH_PARAM_SIZE(size)); return false; } + + // TODO(@jasnell): QUIC *requires* that the client verify the + // identity of the server so we'll need to do that here. + // The current implementation of tls.checkServerIdentity is + // less than great and could be rewritten to speed it up + // significantly by running at the C++ layer. As it is + // currently, the method pulls the peer cert data, converts + // it to a javascript object, then processes the javascript + // object... which is more expensive than what is strictly + // necessary. + // + // See: _tls_wrap.js onConnectSecure function + return true; } diff --git a/lib/internal/quic/util.js b/lib/internal/quic/util.js index a4716f249f..2b53f61644 100644 --- a/lib/internal/quic/util.js +++ b/lib/internal/quic/util.js @@ -143,6 +143,8 @@ function validateTransportParams(params, maxCidLen, minCidLen) { IDX_QUIC_SESSION_MAX_PACKET_SIZE_DEFAULT, maxAckDelay = NGTCP2_DEFAULT_MAX_ACK_DELAY, preferredAddress, + rejectUnauthorized, + requestCert, } = { ...params }; validateNumberInRange( maxStreamDataBidiLocal, @@ -193,6 +195,8 @@ function validateTransportParams(params, maxCidLen, minCidLen) { preferredAddress, maxCidLen, minCidLen, + rejectUnauthorized, + requestCert, }; } diff --git a/src/node_crypto.cc b/src/node_crypto.cc index 62d0407bf9..65bd9db0e1 100644 --- a/src/node_crypto.cc +++ b/src/node_crypto.cc @@ -957,7 +957,6 @@ static X509_STORE* NewRootCertStore() { X509_STORE_add_cert(store, cert); } } - return store; } diff --git a/src/node_quic_crypto.h b/src/node_quic_crypto.h index 320b4f3dfb..74d4e9f4ec 100644 --- a/src/node_quic_crypto.h +++ b/src/node_quic_crypto.h @@ -978,6 +978,8 @@ inline int ClearTLS(SSL* ssl, bool continue_on_error = false) { switch (code) { case SSL_ERROR_WANT_READ: case SSL_ERROR_WANT_WRITE: + case SSL_ERROR_WANT_CLIENT_HELLO_CB: + case SSL_ERROR_WANT_X509_LOOKUP: return 0; case SSL_ERROR_SSL: case SSL_ERROR_ZERO_RETURN: @@ -1000,8 +1002,8 @@ inline int DoTLSHandshake(SSL* ssl) { // the data was otherwise successfully read, so return 0 // here but the handshake won't continue until we trigger // things on our side. - case SSL_ERROR_WANT_CLIENT_HELLO_CB: - case SSL_ERROR_WANT_X509_LOOKUP: + case SSL_ERROR_WANT_CLIENT_HELLO_CB: + case SSL_ERROR_WANT_X509_LOOKUP: return 0; case SSL_ERROR_SSL: return NGTCP2_ERR_CRYPTO; diff --git a/src/node_quic_session.cc b/src/node_quic_session.cc index 1e3ca480eb..f0c06f6e20 100644 --- a/src/node_quic_session.cc +++ b/src/node_quic_session.cc @@ -1196,6 +1196,8 @@ void QuicSession::InitTLS() { SSL_set_msg_callback_arg(ssl(), this); SSL_set_key_callback(ssl(), KeyCB, this); SSL_set_cert_cb(ssl(), CertCB, this); + // The verification may be overriden in InitTLS_Post + SSL_set_verify(ssl(), SSL_VERIFY_NONE, crypto::VerifyCallback); // Servers and Clients do slightly different things at // this point. Both QuicClientSession and QuicServerSession @@ -1831,9 +1833,8 @@ int QuicSession::TLSHandshake() { if (initial_) RETURN_RET_IF_FAIL(TLSHandshake_Initial(), 0); - int err = DoTLSHandshake(ssl()); - if (err > 0) { + if (err > 0) { RETURN_RET_IF_FAIL(TLSHandshake_Complete(), 0); Debug(this, "TLS Handshake completed."); SetHandshakeCompleted(); @@ -2107,7 +2108,9 @@ QuicServerSession::QuicServerSession( const ngtcp2_cid* dcid, const ngtcp2_cid* ocid, uint32_t version, - const std::string& alpn) : + const std::string& alpn, + bool reject_unauthorized, + bool request_cert) : QuicSession( socket, wrap, @@ -2116,7 +2119,9 @@ QuicServerSession::QuicServerSession( alpn), pscid_{}, rcid_(*rcid), - draining_(false) { + draining_(false), + reject_unauthorized_(reject_unauthorized), + request_cert_(request_cert) { Init(addr, dcid, ocid, version); } @@ -2301,6 +2306,13 @@ int QuicServerSession::OnKey( void QuicServerSession::InitTLS_Post() { SSL_set_accept_state(ssl()); + + if (request_cert_) { + int verify_mode = SSL_VERIFY_PEER; + if (reject_unauthorized_) + verify_mode |= SSL_VERIFY_FAIL_IF_NO_PEER_CERT; + SSL_set_verify(ssl(), verify_mode, crypto::VerifyCallback); + } } void QuicServerSession::Init( @@ -2353,7 +2365,9 @@ std::shared_ptr QuicServerSession::New( const ngtcp2_cid* dcid, const ngtcp2_cid* ocid, uint32_t version, - const std::string& alpn) { + const std::string& alpn, + bool reject_unauthorized, + bool request_cert) { std::shared_ptr session; Local obj; if (!socket->env() @@ -2370,7 +2384,9 @@ std::shared_ptr QuicServerSession::New( dcid, ocid, version, - alpn)); + alpn, + reject_unauthorized, + request_cert)); session->AddToSocket(socket); return session; diff --git a/src/node_quic_session.h b/src/node_quic_session.h index d2427141a9..bfdb6fe197 100644 --- a/src/node_quic_session.h +++ b/src/node_quic_session.h @@ -761,7 +761,9 @@ class QuicServerSession : public QuicSession { const ngtcp2_cid* dcid, const ngtcp2_cid* ocid, uint32_t version, - const std::string& alpn = NGTCP2_ALPN_H3); + const std::string& alpn = NGTCP2_ALPN_H3, + bool reject_unauthorized = true, + bool request_cert_ = true); void AddToSocket(QuicSocket* socket) override; @@ -797,7 +799,9 @@ class QuicServerSession : public QuicSession { const ngtcp2_cid* dcid, const ngtcp2_cid* ocid, uint32_t version, - const std::string& alpn); + const std::string& alpn, + bool reject_unauthorized, + bool request_cert); void DisassociateCID( const ngtcp2_cid* cid) override; @@ -838,6 +842,8 @@ class QuicServerSession : public QuicSession { ngtcp2_cid pscid_; ngtcp2_cid rcid_; bool draining_; + bool reject_unauthorized_; + bool request_cert_; MallocedBuffer conn_closebuf_; diff --git a/src/node_quic_socket.cc b/src/node_quic_socket.cc index 179a7fd4a9..e332e307da 100644 --- a/src/node_quic_socket.cc +++ b/src/node_quic_socket.cc @@ -69,6 +69,8 @@ QuicSocket::QuicSocket( max_connections_per_host_(max_connections_per_host), server_secure_context_(nullptr), server_alpn_(NGTCP2_ALPN_H3), + reject_unauthorized_(false), + request_cert_(false), token_crypto_ctx_{}, retry_token_expiration_(retry_token_expiration), stats_buffer_( @@ -188,7 +190,9 @@ SocketAddress* QuicSocket::GetLocalAddress() { void QuicSocket::Listen( SecureContext* sc, const sockaddr* preferred_address, - const std::string& alpn) { + const std::string& alpn, + bool reject_unauthorized, + bool request_cert) { // TODO(@jasnell): Should we allow calling listen multiple times? // For now, we guard against it, but we may want to allow it later. CHECK_NOT_NULL(sc); @@ -198,6 +202,8 @@ void QuicSocket::Listen( server_session_config_.Set(env(), preferred_address); server_secure_context_ = sc; server_alpn_ = alpn; + reject_unauthorized_ = reject_unauthorized; + request_cert_ = request_cert; server_listening_ = true; socket_stats_.listen_at = uv_hrtime(); ReceiveStart(); @@ -480,8 +486,9 @@ std::shared_ptr QuicSocket::ServerReceive( &hd->scid, pocid, hd->version, - server_alpn_); - + server_alpn_, + reject_unauthorized_, + request_cert_); Local arg = session->object(); MakeCallback(env()->quic_on_session_ready_function(), 1, &arg); @@ -795,7 +802,10 @@ void QuicSocketListen(const FunctionCallbackInfo& args) { alpn += *val; } - socket->Listen(sc, preferred_address, alpn); + bool reject_unauthorized = args[5]->IsTrue(); + bool request_cert = args[6]->IsTrue(); + + socket->Listen(sc, preferred_address, alpn, reject_unauthorized, request_cert); } void QuicSocketReceiveStart(const FunctionCallbackInfo& args) { diff --git a/src/node_quic_socket.h b/src/node_quic_socket.h index 43912b618f..aaf5b661c3 100644 --- a/src/node_quic_socket.h +++ b/src/node_quic_socket.h @@ -66,7 +66,9 @@ class QuicSocket : public HandleWrap { void Listen( crypto::SecureContext* context, const sockaddr* preferred_address = nullptr, - const std::string& alpn = NGTCP2_ALPN_H3); + const std::string& alpn = NGTCP2_ALPN_H3, + bool reject_unauthorized = true, + bool request_cert = false); int ReceiveStart(); int ReceiveStop(); void RemoveSession( @@ -160,6 +162,8 @@ class QuicSocket : public HandleWrap { QuicSessionConfig server_session_config_; crypto::SecureContext* server_secure_context_; std::string server_alpn_; + bool reject_unauthorized_; + bool request_cert_; std::unordered_map> sessions_; std::unordered_map dcid_to_scid_; CryptoContext token_crypto_ctx_; diff --git a/test/parallel/test-quic-client-server.js b/test/parallel/test-quic-client-server.js index 3b9b284689..7c24410a5d 100644 --- a/test/parallel/test-quic-client-server.js +++ b/test/parallel/test-quic-client-server.js @@ -20,8 +20,9 @@ const Countdown = require('../common/countdown'); const assert = require('assert'); const fs = require('fs'); const fixtures = require('../common/fixtures'); -const key = fixtures.readKey('agent8-key.pem', 'binary'); -const cert = fixtures.readKey('agent8-cert.pem', 'binary'); +const key = fixtures.readKey('agent1-key.pem', 'binary'); +const cert = fixtures.readKey('agent1-cert.pem', 'binary'); +const ca = fixtures.readKey('fake-startcom-root-cert.pem', 'binary'); const { debuglog } = require('util'); const debug = debuglog('test'); @@ -56,7 +57,14 @@ const countdown = new Countdown(2, () => { client.close(); }); -server.listen({ key, cert, alpn: kALPN }); +server.listen({ + key, + cert, + ca, + requestCert: true, + rejectUnauthorized: false, + alpn: kALPN +}); server.on('session', common.mustCall((session) => { debug('QuicServerSession Created'); @@ -86,6 +94,7 @@ server.on('session', common.mustCall((session) => { assert.strictEqual(session.servername, servername); assert.strictEqual(servername, kServerName); assert.strictEqual(session.alpnProtocol, alpn); + assert.strictEqual(session.getPeerCertificate().subject.CN, 'agent1'); const uni = session.openStream({ halfOpen: true }); uni.write(unidata[0]); @@ -125,25 +134,31 @@ server.on('session', common.mustCall((session) => { server.on('ready', common.mustCall(() => { debug('Server is listening on port %d', server.address.port); - client = createSocket({ type: 'udp4', port: 0 }); - const req = client.connect({ + client = createSocket({ type: 'udp4', + port: 0, + client: { + type: 'udp4', + key, + cert, + ca, + maxStreamsUni: 1000, + minCidLen: 5, + maxCidLen: 10, + alpn: kALPN, + } + }); + + const req = client.connect({ address: 'localhost', port: server.address.port, - rejectUnauthorized: false, - maxStreamsUni: 1000, servername: kServerName, - minCidLen: 5, - maxCidLen: 10, - alpn: kALPN, }); client.on('close', () => debug('Client closing')); assert.strictEqual(req.servername, kServerName); - req.on('cert', console.log); - req.on('sessionTicket', common.mustCall((id, ticket, params) => { debug('Session ticket received'); assert(id instanceof Buffer);