From e29b64a18ae03df38beec6550d28ad64d57987b1 Mon Sep 17 00:00:00 2001 From: Greg Nolle Date: Wed, 9 Nov 2016 13:05:46 +0000 Subject: [PATCH] modify CertVerifier Class * respond to multiple similar verification requests. * accept net error result as callback response. --- atom/browser/api/atom_api_session.cc | 2 +- atom/browser/net/atom_cert_verifier.cc | 167 +++++++++++++++++++++---- atom/browser/net/atom_cert_verifier.h | 22 +++- docs/api/session.md | 8 +- lib/browser/api/session.js | 21 +++- lib/browser/rpc-server.js | 1 + lib/renderer/api/remote.js | 3 +- spec/api-session-spec.js | 8 +- 8 files changed, 199 insertions(+), 33 deletions(-) diff --git a/atom/browser/api/atom_api_session.cc b/atom/browser/api/atom_api_session.cc index 45bb9c2cf3bf..e3b55cb37c63 100644 --- a/atom/browser/api/atom_api_session.cc +++ b/atom/browser/api/atom_api_session.cc @@ -738,7 +738,7 @@ void Session::BuildPrototype(v8::Isolate* isolate, .SetMethod("setDownloadPath", &Session::SetDownloadPath) .SetMethod("enableNetworkEmulation", &Session::EnableNetworkEmulation) .SetMethod("disableNetworkEmulation", &Session::DisableNetworkEmulation) - .SetMethod("setCertificateVerifyProc", &Session::SetCertVerifyProc) + .SetMethod("_setCertificateVerifyProc", &Session::SetCertVerifyProc) .SetMethod("setPermissionRequestHandler", &Session::SetPermissionRequestHandler) .SetMethod("clearHostResolverCache", &Session::ClearHostResolverCache) diff --git a/atom/browser/net/atom_cert_verifier.cc b/atom/browser/net/atom_cert_verifier.cc index 61c7439e2733..ec7b8755fcd9 100644 --- a/atom/browser/net/atom_cert_verifier.cc +++ b/atom/browser/net/atom_cert_verifier.cc @@ -7,6 +7,9 @@ #include "atom/browser/browser.h" #include "atom/browser/net/atom_ct_delegate.h" #include "atom/common/native_mate_converters/net_converter.h" +#include "base/containers/linked_list.h" +#include "base/memory/ptr_util.h" +#include "base/memory/weak_ptr.h" #include "content/public/browser/browser_thread.h" #include "net/base/net_errors.h" #include "net/cert/cert_verify_result.h" @@ -19,17 +22,119 @@ namespace atom { namespace { -void OnResult( - net::CertVerifyResult* verify_result, - const net::CompletionCallback& callback, - bool result) { - BrowserThread::PostTask( - BrowserThread::IO, FROM_HERE, - base::Bind(callback, result ? net::OK : net::ERR_FAILED)); -} +class Response : public base::LinkNode { + public: + Response(net::CertVerifyResult* verify_result, + const net::CompletionCallback& callback) + : verify_result_(verify_result), callback_(callback) {} + net::CertVerifyResult* verify_result() { return verify_result_; } + net::CompletionCallback callback() { return callback_; } + + private: + net::CertVerifyResult* verify_result_; + net::CompletionCallback callback_; + + DISALLOW_COPY_AND_ASSIGN(Response); +}; } // namespace +class CertVerifierRequest : public AtomCertVerifier::Request { + public: + CertVerifierRequest(const AtomCertVerifier::RequestParams& params, + AtomCertVerifier* cert_verifier) + : params_(params), + cert_verifier_(cert_verifier), + error_(net::ERR_IO_PENDING), + custom_response_(net::ERR_IO_PENDING), + first_response_(true), + weak_ptr_factory_(this) {} + + ~CertVerifierRequest() { + cert_verifier_->RemoveRequest(params_); + default_verifier_request_.reset(); + while (!response_list_.empty() && !first_response_) { + base::LinkNode* response_node = response_list_.head(); + response_node->RemoveFromList(); + Response* response = response_node->value(); + RunResponse(response); + } + cert_verifier_ = nullptr; + weak_ptr_factory_.InvalidateWeakPtrs(); + } + + void RunResponse(Response* response) { + if (custom_response_ == net::ERR_ABORTED) { + *(response->verify_result()) = result_; + response->callback().Run(error_); + } else { + response->verify_result()->Reset(); + response->verify_result()->verified_cert = params_.certificate(); + cert_verifier_->ct_delegate()->AddCTExcludedHost(params_.hostname()); + response->callback().Run(custom_response_); + } + delete response; + } + + void Start(net::CRLSet* crl_set, + const net::BoundNetLog& net_log) { + int error = cert_verifier_->default_verifier()->Verify( + params_, crl_set, &result_, + base::Bind(&CertVerifierRequest::OnDefaultVerificationDone, + weak_ptr_factory_.GetWeakPtr()), + &default_verifier_request_, net_log); + if (error != net::ERR_IO_PENDING) + OnDefaultVerificationDone(error); + } + + void OnDefaultVerificationDone(int error) { + error_ = error; + BrowserThread::PostTask( + BrowserThread::UI, FROM_HERE, + base::Bind(cert_verifier_->verify_proc(), params_.hostname(), + params_.certificate(), net::ErrorToString(error), + base::Bind(&CertVerifierRequest::OnResponseInUI, + weak_ptr_factory_.GetWeakPtr()))); + } + + void OnResponseInUI(int result) { + BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, + base::Bind(&CertVerifierRequest::NotifyResponseInIO, + weak_ptr_factory_.GetWeakPtr(), result)); + } + + void NotifyResponseInIO(int result) { + custom_response_ = result; + first_response_ = false; + // Responding to first request in the list will initiate destruction of + // the class, respond to others in the list inside destructor. + base::LinkNode* response_node = response_list_.head(); + response_node->RemoveFromList(); + Response* response = response_node->value(); + RunResponse(response); + } + + void AddResponseListener(net::CertVerifyResult* verify_result, + const net::CompletionCallback& callback) { + response_list_.Append(new Response(verify_result, callback)); + } + + const AtomCertVerifier::RequestParams& params() const { return params_; } + + private: + using ResponseList = base::LinkedList; + + const AtomCertVerifier::RequestParams params_; + AtomCertVerifier* cert_verifier_; + int error_; + int custom_response_; + bool first_response_; + ResponseList response_list_; + net::CertVerifyResult result_; + std::unique_ptr default_verifier_request_; + base::WeakPtrFactory weak_ptr_factory_; +}; + AtomCertVerifier::AtomCertVerifier(AtomCTDelegate* ct_delegate) : default_cert_verifier_(net::CertVerifier::CreateDefault()), ct_delegate_(ct_delegate) {} @@ -51,23 +156,43 @@ int AtomCertVerifier::Verify( if (verify_proc_.is_null()) { ct_delegate_->ClearCTExcludedHostsList(); - return default_cert_verifier_->Verify( - params, crl_set, verify_result, callback, out_req, net_log); + return default_cert_verifier_->Verify(params, crl_set, verify_result, + callback, out_req, net_log); + } else { + CertVerifierRequest* request = FindRequest(params); + if (!request) { + out_req->reset(); + std::unique_ptr new_request = + base::MakeUnique(params, this); + new_request->Start(crl_set, net_log); + request = new_request.get(); + *out_req = std::move(new_request); + inflight_requests_[params] = request; + } + request->AddResponseListener(verify_result, callback); + + return net::ERR_IO_PENDING; } - - verify_result->Reset(); - verify_result->verified_cert = params.certificate(); - ct_delegate_->AddCTExcludedHost(params.hostname()); - - BrowserThread::PostTask( - BrowserThread::UI, FROM_HERE, - base::Bind(verify_proc_, params.hostname(), params.certificate(), - base::Bind(OnResult, verify_result, callback))); - return net::ERR_IO_PENDING; } bool AtomCertVerifier::SupportsOCSPStapling() { - return true; + if (verify_proc_.is_null()) + return default_cert_verifier_->SupportsOCSPStapling(); + return false; +} + +void AtomCertVerifier::RemoveRequest(const RequestParams& params) { + auto it = inflight_requests_.find(params); + if (it != inflight_requests_.end()) + inflight_requests_.erase(it); +} + +CertVerifierRequest* AtomCertVerifier::FindRequest( + const RequestParams& params) { + auto it = inflight_requests_.find(params); + if (it != inflight_requests_.end()) + return it->second; + return nullptr; } } // namespace atom diff --git a/atom/browser/net/atom_cert_verifier.h b/atom/browser/net/atom_cert_verifier.h index e321f7d3d933..ff60908ee6ac 100644 --- a/atom/browser/net/atom_cert_verifier.h +++ b/atom/browser/net/atom_cert_verifier.h @@ -5,6 +5,7 @@ #ifndef ATOM_BROWSER_NET_ATOM_CERT_VERIFIER_H_ #define ATOM_BROWSER_NET_ATOM_CERT_VERIFIER_H_ +#include #include #include @@ -13,19 +14,26 @@ namespace atom { class AtomCTDelegate; +class CertVerifierRequest; class AtomCertVerifier : public net::CertVerifier { public: explicit AtomCertVerifier(AtomCTDelegate* ct_delegate); virtual ~AtomCertVerifier(); - using VerifyProc = - base::Callback, - const base::Callback&)>; + using VerifyProc = base::Callback, + const std::string& default_result, + const net::CompletionCallback&)>; void SetVerifyProc(const VerifyProc& proc); + const VerifyProc verify_proc() const { return verify_proc_; } + AtomCTDelegate* ct_delegate() const { return ct_delegate_; } + net::CertVerifier* default_verifier() const { + return default_cert_verifier_.get(); + } + protected: // net::CertVerifier: int Verify(const RequestParams& params, @@ -37,6 +45,12 @@ class AtomCertVerifier : public net::CertVerifier { bool SupportsOCSPStapling() override; private: + friend class CertVerifierRequest; + + void RemoveRequest(const RequestParams& params); + CertVerifierRequest* FindRequest(const RequestParams& params); + + std::map inflight_requests_; VerifyProc verify_proc_; std::unique_ptr default_cert_verifier_; AtomCTDelegate* ct_delegate_; diff --git a/docs/api/session.md b/docs/api/session.md index 58de08d39abb..5155fabaed3d 100644 --- a/docs/api/session.md +++ b/docs/api/session.md @@ -252,8 +252,14 @@ the original network configuration. * `proc` Function * `hostname` String * `certificate` [Certificate](structures/certificate.md) + * `error` String - Verification result from chromium. * `callback` Function - * `isTrusted` Boolean - Determines if the certificate should be trusted + * `verificationResult` Integer - Value can be one of certificate error codes + from [here](https://code.google.com/p/chromium/codesearch#chromium/src/net/base/net_error_list.h). + Apart from the certificate error codes, the following special codes can be used. + * `0` - Indicates success and disables Certificate Transperancy verification. + * `-2` - Indicates failure. + * `-3` - Uses the verification result from chromium. Sets the certificate verify proc for `session`, the `proc` will be called with `proc(hostname, certificate, callback)` whenever a server certificate diff --git a/lib/browser/api/session.js b/lib/browser/api/session.js index ac47244db326..bf2502995291 100644 --- a/lib/browser/api/session.js +++ b/lib/browser/api/session.js @@ -1,5 +1,5 @@ const {EventEmitter} = require('events') -const {app} = require('electron') +const {app, deprecate} = require('electron') const {fromPartition, Session, Cookies} = process.atomBinding('session') // Public API. @@ -20,3 +20,22 @@ Object.setPrototypeOf(Cookies.prototype, EventEmitter.prototype) Session.prototype._init = function () { app.emit('session-created', this) } + +// Remove after 2.0 +Session.prototype.setCertificateVerifyProc = function (verifyProc) { + if (!verifyProc) { + this._setCertificateVerifyProc(null) + return + } + if (verifyProc.length <= 3) { + deprecate.warn('setCertificateVerifyproc(hostname, certificate, callback)', + 'setCertificateVerifyproc(hostname, certificate, error, callback)') + this._setCertificateVerifyProc((hostname, certificate, error, cb) => { + verifyProc(hostname, certificate, (result) => { + cb(result ? 0 : -2) + }) + }) + } else { + this._setCertificateVerifyProc(verifyProc) + } +} diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index 1dac51645926..2cafc9343925 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -222,6 +222,7 @@ const unwrapArgs = function (sender, args) { removeRemoteListenersAndLogWarning(meta, args, callIntoRenderer) } } + Object.defineProperty(callIntoRenderer, 'length', { value: meta.length }) v8Util.setRemoteCallbackFreer(callIntoRenderer, meta.id, sender) rendererFunctions.set(objectId, callIntoRenderer) diff --git a/lib/renderer/api/remote.js b/lib/renderer/api/remote.js index c3c8e3b89cd1..8258a91a6619 100644 --- a/lib/renderer/api/remote.js +++ b/lib/renderer/api/remote.js @@ -79,7 +79,8 @@ const wrapArgs = function (args, visited) { return { type: 'function', id: callbacksRegistry.add(value), - location: v8Util.getHiddenValue(value, 'location') + location: v8Util.getHiddenValue(value, 'location'), + length: value.length } } else { return { diff --git a/spec/api-session-spec.js b/spec/api-session-spec.js index 1f31ff7123e5..6b907b9bf235 100644 --- a/spec/api-session-spec.js +++ b/spec/api-session-spec.js @@ -557,8 +557,8 @@ describe('session module', function () { }) it('accepts the request when the callback is called with true', function (done) { - session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) { - callback(true) + session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, error, callback) { + callback(0) }) w.webContents.once('did-finish-load', function () { @@ -569,7 +569,7 @@ describe('session module', function () { }) it('rejects the request when the callback is called with false', function (done) { - session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) { + session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, error, callback) { assert.equal(hostname, '127.0.0.1') assert.equal(certificate.issuerName, 'Intermediate CA') assert.equal(certificate.subjectName, 'localhost') @@ -580,7 +580,7 @@ describe('session module', function () { assert.equal(certificate.issuerCert.issuerCert.issuer.commonName, 'Root CA') assert.equal(certificate.issuerCert.issuerCert.subject.commonName, 'Root CA') assert.equal(certificate.issuerCert.issuerCert.issuerCert, undefined) - callback(false) + callback(-2) }) var url = `https://127.0.0.1:${server.address().port}`