Correct checkServerIdentity behavior to return a verification failure if an error is returned. Clean up documentation and add a test assertion on returned Error.

packages/grpc-native-core/ext/channel_credentials.cc

@@ -78,9 +78,15 @@ static int verify_peer_callback_wrapper(const char* servername, const char* cert
     argv[1] = Nan::New<v8::String>(cert).ToLocalChecked();
   }
 
-  callback->Call(argc, argv);
+  Local<Value> result = callback->Call(argc, argv);
 
+  // Catch any exception and return with a distinct status code which indicates this
   if (try_catch.HasCaught()) {
+    return 2;
+  }
+
+  // If the result is an error, return a failure
+  if (result->IsNativeError()) {
     return 1;
   }
 

packages/grpc-native-core/index.d.ts

@@ -796,10 +796,11 @@ declare module "grpc" {
 
   /**
    * A callback that will receive the expected hostname and presented peer
-   * certificate as parameters. The callback should throw an error to
+   * certificate as parameters. The callback should return an error to
-   * indicate that the presented certificate is considered invalid.
+   * indicate that the presented certificate is considered invalid and
+   * otherwise returned undefined.
    */
-  export type CheckServerIdentityCallback = (hostname: string, cert: string) => void;
+  export type CheckServerIdentityCallback = (hostname: string, cert: string) => Error | undefined;
 
   /**
    * Additional peer verification options that can be set when creating

packages/grpc-native-core/src/credentials.js

@@ -87,18 +87,10 @@ var _ = require('lodash');
  * @param {Buffer=} private_key The client certificate private key, if
  *     applicable
  * @param {Buffer=} cert_chain The client certificate cert chain, if applicable
- * @param {Object} verify_options Additional peer verification options. Can
+ * @param {Function} verify_options.checkServerIdentity Optional callback
- *     be undefined, in which case default behavior is preserved.
+ *     receiving the expected hostname and peer certificate for additional
- *     Supported options are: "checkServerIdentity": (servername, cert) => {}
+ *     verification. The callback should return an Error if verification
- *     The callback passed to checkServerIdentity will be invoked when the
+ *     fails and otherwise return undefined.
- *     channel is opened in order to provide an opportunity to perform
- *     additional verification of the peer certificate as passed to the
- *     callback in the second parameter. The expected hostname is passed as
- *     the first parameter. If the callback considers the peer certificate
- *     invalid it should throw an error which will cause the handshake to
- *     be terminated. Note that supplying this callback does not disable
- *     the usual hostname verification which will also be performed on the
- *     certificate before this callback is invoked.
  * @return {grpc.credentials~ChannelCredentials} The SSL Credentials object
  */
 exports.createSsl = ChannelCredentials.createSsl;

packages/grpc-native-core/test/credentials_test.js

@@ -309,6 +309,19 @@ describe('client credentials', function() {
       done();
     });
   });
+  it('Verify callback returning an Error causes connection failure', function(done) {
+    var client_ssl_creds = grpc.credentials.createSsl(ca_data, null, null, {
+      "checkServerIdentity": function(host, cert) {
+        return new Error("Verification error");
+      }
+    });
+    var client = new Client('localhost:' + port, client_ssl_creds,
+                            client_options);
+    client.unary({}, function(err, data) {
+      assert.ok(err, "Should have raised an error");
+      done();
+    });
+  });
   it('Should update metadata with SSL creds', function(done) {
     var metadataUpdater = function(service_url, callback) {
       var metadata = new grpc.Metadata();