Merge pull request #7955 from deepak1556/veto-ssl-verify

session: provide chromium cert verification result to setCertificateVerifiy callback

atom/browser/api/atom_api_session.cc

@@ -204,6 +204,18 @@ struct Converter<net::ProxyConfig> {
   }
 };
 
+template<>
+struct Converter<atom::VerifyRequestParams> {
+  static v8::Local<v8::Value> ToV8(v8::Isolate* isolate,
+                                   atom::VerifyRequestParams val) {
+    mate::Dictionary dict = mate::Dictionary::CreateEmpty(isolate);
+    dict.Set("hostname", val.hostname);
+    dict.Set("certificate", val.certificate);
+    dict.Set("verificationResult", val.default_result);
+    return dict.GetHandle();
+  }
+};
+
 }  // namespace mate
 
 namespace atom {
@@ -738,7 +750,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)

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,130 @@ 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<Response> {
+ 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>* 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::NetLogWithSource& 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;
+    std::unique_ptr<VerifyRequestParams> request(new VerifyRequestParams());
+    request->hostname = params_.hostname();
+    request->default_result = net::ErrorToString(error);
+    request->certificate = params_.certificate();
+    BrowserThread::PostTask(
+        BrowserThread::UI, FROM_HERE,
+        base::Bind(&CertVerifierRequest::OnVerifyRequestInUI,
+                   weak_ptr_factory_.GetWeakPtr(),
+                   cert_verifier_->verify_proc(),
+                   base::Passed(&request)));
+  }
+
+  void OnVerifyRequestInUI(const AtomCertVerifier::VerifyProc& verify_proc,
+                           std::unique_ptr<VerifyRequestParams> request) {
+    verify_proc.Run(*(request.get()),
+                    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>* 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<Response>;
+
+  const AtomCertVerifier::RequestParams params_;
+  AtomCertVerifier* cert_verifier_;
+  int error_;
+  int custom_response_;
+  bool first_response_;
+  ResponseList response_list_;
+  net::CertVerifyResult result_;
+  std::unique_ptr<AtomCertVerifier::Request> default_verifier_request_;
+  base::WeakPtrFactory<CertVerifierRequest> weak_ptr_factory_;
+};
+
 AtomCertVerifier::AtomCertVerifier(AtomCTDelegate* ct_delegate)
     : default_cert_verifier_(net::CertVerifier::CreateDefault()),
       ct_delegate_(ct_delegate) {}
@@ -51,23 +167,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<CertVerifierRequest> new_request =
+          base::MakeUnique<CertVerifierRequest>(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

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 <map>
 #include <memory>
 #include <string>
 
@@ -13,19 +14,30 @@
 namespace atom {
 
 class AtomCTDelegate;
+class CertVerifierRequest;
+
+struct VerifyRequestParams {
+  std::string hostname;
+  std::string default_result;
+  scoped_refptr<net::X509Certificate> certificate;
+};
 
 class AtomCertVerifier : public net::CertVerifier {
  public:
   explicit AtomCertVerifier(AtomCTDelegate* ct_delegate);
   virtual ~AtomCertVerifier();
 
-  using VerifyProc =
-      base::Callback<void(const std::string& hostname,
-                          scoped_refptr<net::X509Certificate>,
-                          const base::Callback<void(bool)>&)>;
+  using VerifyProc = base::Callback<void(const VerifyRequestParams& request,
+                                         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 +49,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<RequestParams, CertVerifierRequest*> inflight_requests_;
   VerifyProc verify_proc_;
   std::unique_ptr<net::CertVerifier> default_cert_verifier_;
   AtomCTDelegate* ct_delegate_;

docs/api/session.md

@@ -250,15 +250,22 @@ the original network configuration.
 #### `ses.setCertificateVerifyProc(proc)`
 
 * `proc` Function
-  * `hostname` String
-  * `certificate` [Certificate](structures/certificate.md)
+  * `request` Object
+    * `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
-verification is requested. Calling `callback(true)` accepts the certificate,
-calling `callback(false)` rejects it.
+`proc(request, callback)` whenever a server certificate
+verification is requested. Calling `callback(0)` accepts the certificate,
+calling `callback(-2)` rejects it.
 
 Calling `setCertificateVerifyProc(null)` will revert back to default certificate
 verify proc.

docs/tutorial/planned-breaking-changes.md

@@ -91,6 +91,19 @@ process.versions.electron
   read-only properties for consistency with the other `process.versions`
   properties set by Node.
 
+## `session`
+
+```js
+// Deprecated
+ses.setCertificateVerifyProc(function (hostname, certificate, callback) {
+  callback(true)
+})
+// Replace with
+ses.setCertificateVerifyProc(function (request, callback) {
+  callback(0)
+})
+```
+
 ## `Tray`
 
 ```js

lib/browser/api/session.js

@@ -20,3 +20,16 @@ Object.setPrototypeOf(Cookies.prototype, EventEmitter.prototype)
 Session.prototype._init = function () {
   app.emit('session-created', this)
 }
+
+Session.prototype.setCertificateVerifyProc = function (verifyProc) {
+  if (verifyProc != null && verifyProc.length > 2) {
+    // TODO(kevinsawicki): Remove in 2.0, deprecate before then with warnings
+    this._setCertificateVerifyProc(({hostname, certificate, verificationResult}, cb) => {
+      verifyProc(hostname, certificate, (result) => {
+        cb(result ? 0 : -2)
+      })
+    })
+  } else {
+    this._setCertificateVerifyProc(verifyProc)
+  }
+}

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)

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 {

spec/api-ipc-spec.js

@@ -85,6 +85,13 @@ describe('ipc module', function () {
       assert.equal(foo.baz(), 123)
     })
 
+    it('includes the length of functions specified as arguments', function () {
+      var a = remote.require(path.join(fixtures, 'module', 'function-with-args.js'))
+      assert.equal(a(function (a, b, c, d, f) {}), 5)
+      assert.equal(a((a) => {}), 1)
+      assert.equal(a((...args) => {}), 0)
+    })
+
     it('handles circular references in arrays and objects', function () {
       var a = remote.require(path.join(fixtures, 'module', 'circular.js'))
 

spec/api-session-spec.js

@@ -557,8 +557,9 @@ 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, verificationResult}, callback) {
+        assert.equal(verificationResult, 'net::ERR_CERT_AUTHORITY_INVALID')
+        callback(0)
       })
 
       w.webContents.once('did-finish-load', function () {
@@ -568,8 +569,37 @@ describe('session module', function () {
       w.loadURL(`https://127.0.0.1:${server.address().port}`)
     })
 
+    describe('deprecated function signature', function () {
+      it('supports accepting the request', function (done) {
+        session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) {
+          assert.equal(hostname, '127.0.0.1')
+          callback(true)
+        })
+
+        w.webContents.once('did-finish-load', function () {
+          assert.equal(w.webContents.getTitle(), 'hello')
+          done()
+        })
+        w.loadURL(`https://127.0.0.1:${server.address().port}`)
+      })
+
+      it('supports rejecting the request', function (done) {
+        session.defaultSession.setCertificateVerifyProc(function (hostname, certificate, callback) {
+          assert.equal(hostname, '127.0.0.1')
+          callback(false)
+        })
+
+        var url = `https://127.0.0.1:${server.address().port}`
+        w.webContents.once('did-finish-load', function () {
+          assert.equal(w.webContents.getTitle(), url)
+          done()
+        })
+        w.loadURL(url)
+      })
+    })
+
     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, verificationResult}, callback) {
         assert.equal(hostname, '127.0.0.1')
         assert.equal(certificate.issuerName, 'Intermediate CA')
         assert.equal(certificate.subjectName, 'localhost')
@@ -580,7 +610,8 @@ 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)
+        assert.equal(verificationResult, 'net::ERR_CERT_AUTHORITY_INVALID')
+        callback(-2)
       })
 
       var url = `https://127.0.0.1:${server.address().port}`

spec/fixtures/module/function-with-args.js

@@ -0,0 +1,3 @@
+module.exports = function (cb) {
+  return cb.length
+}