Set secureContext for extending root CAs

postmanlabs · Jun 14, 2024 · e94066c · e94066c
1 parent 00acf08
commit e94066c
Show file tree

Hide file tree

Showing 6 changed files with 15 additions and 228 deletions.
diff --git a/README.md b/README.md
@@ -809,23 +809,16 @@ request({url: 'https://www.google.com', verbose: true}, function (error, respons
 
 ### Extending root CAs
 
-When this feature is enabled, the root CAs can be extended using the `extraCA` option. The file should consist of one or more trusted certificates in PEM format.
+The root CAs can be extended using the `extraCA` option. The file should consist of one or more trusted certificates in PEM format.
 
 This is similar to [NODE_EXTRA_CA_CERTS](https://nodejs.org/api/cli.html#cli_node_extra_ca_certs_file). But, if `options.ca` is specified, those will be extended as well.
 
 ```js
-// enable extending CAs
-request.enableNodeExtraCACerts();
-
 // request with extra CA certs
 request.get({
     url: 'https://api.some-server.com/',
     extraCA: fs.readFileSync('Extra CA Certificates .pem')
 });
-
-// disable this feature
-request.disableNodeExtraCACerts()
-
 ```
 
 [back to top](#table-of-contents)

diff --git a/index.js b/index.js
@@ -14,7 +14,6 @@
 
 'use strict'
 
-var tls = require('tls')
 var extend = require('extend')
 var cookies = require('./lib/cookies')
 
@@ -131,92 +130,6 @@ request.forever = function (agentOptions, optionsArg) {
   return request.defaults(options)
 }
 
-// As of now (Node v10.x LTS), the only way to extend the well known "root" CA
-// is by using an environment variable called `NODE_EXTRA_CA_CERTS`.
-// This function enables the same functionality and provides a programmatic way
-// to extend the CA certificates.
-// Refer: https://nodejs.org/docs/latest-v10.x/api/cli.html#cli_node_extra_ca_certs_file
-//
-// @note Unlike NODE_EXTRA_CA_CERTS, this method extends the CA for every
-// request sent and since its an expensive operation its advised to use a
-// keepAlive agent(agentOptions.keepAlive: true) when this is enabled.
-//
-//   Benchmarks using a local server:
-//     NODE_EXTRA_CA_CERTS (keepAlive: false) : 422 ops/sec ±1.73% (77 runs sampled)
-//     NODE_EXTRA_CA_CERTS (keepAlive: true)  : 2,096 ops/sec ±4.23% (69 runs sampled)
-//
-//     enableNodeExtraCACerts (keepAlive: false) : 331 ops/sec ±5.64% (77 runs sampled)
-//     enableNodeExtraCACerts (keepAlive: true)  : 2,045 ops/sec ±5.20% (69 runs sampled)
-//
-// @note Enabling this will override the singleton `tls.createSecureContext` method
-// which will be affected for every request sent (using native HTTPS etc.) on the
-// same process. BUT, this will only be effective when `extraCA` option is
-// passed to `tls.createSecureContext`, which is limited to this library.
-request.enableNodeExtraCACerts = function (callback) {
-  // @note callback is optional to catch missing tls method
-  !callback && (callback = function () {})
-
-  // bail out if already enabled
-  if (tls.__createSecureContext) {
-    return callback()
-  }
-
-  // enable only if `SecureContext.addCACert` is present
-  // otherwise return callback with error.
-  // @note try-catch is used to make sure testing this will not break
-  // the main process due to OpenSSL error.
-  try {
-    var testContext = tls.createSecureContext()
-
-    if (!(testContext && testContext.context &&
-        typeof testContext.context.addCACert === 'function')) {
-      return callback(new Error('SecureContext.addCACert is not a function'))
-    }
-  } catch (err) {
-    return callback(err)
-  }
-
-  // store the original tls.createSecureContext method.
-  // used to extend existing functionality as well as restore later.
-  tls.__createSecureContext = tls.createSecureContext
-
-  // override tls.createSecureContext with extraCA support
-  // @note if agent is keepAlive, same context will be reused.
-  tls.createSecureContext = function () {
-    // call original createSecureContext and store the context
-    var secureContext = tls.__createSecureContext.apply(this, arguments)
-
-    // if `extraCA` is present in options, extend CA certs
-    // @note this request option is available here because all the
-    // Request properties are passed to HTTPS Agent.
-    if (arguments[0] && arguments[0].extraCA) {
-      // extend root CA with specified CA certificates
-      // @note `addCACert` is an undocumented API and performs an expensive operations
-      // Refer: https://github.com/nodejs/node/blob/v10.15.1/lib/_tls_common.js#L97
-      secureContext.context.addCACert(arguments[0].extraCA)
-    }
-
-    return secureContext
-  }
-
-  // enabled extra CA support
-  return callback()
-}
-
-// disable the extended CA certificates feature
-request.disableNodeExtraCACerts = function () {
-  // bail out if not enabled
-  if (typeof tls.__createSecureContext !== 'function') {
-    return
-  }
-
-  // reset `tls.createSecureContext` with the original method
-  tls.createSecureContext = tls.__createSecureContext
-
-  // delete the reference of original method
-  delete tls.__createSecureContext
-}
-
 // Exports
 
 module.exports = request

diff --git a/request.js b/request.js
@@ -639,7 +639,12 @@ Request.prototype.init = function (options) {
   if (self.pool === false) {
     self.agent = false
   } else {
-    self.agent = self.agent || self.getNewAgent()
+    try {
+      self.agent = self.agent || self.getNewAgent()
+    } catch (error) {
+      // tls.createSecureContext() throws on bad options
+      return self.emit('error', error)
+    }
   }
 
   self.on('pipe', function (src) {
@@ -824,11 +829,16 @@ Request.prototype.getNewAgent = function () {
     }
 
     // only add when NodeExtraCACerts is enabled
-    if (tls.__createSecureContext && options.extraCA) {
+    if (options.extraCA) {
       if (poolKey) {
         poolKey += ':'
       }
       poolKey += options.extraCA
+
+      // Create a new secure context to add the extra CA
+      var secureContext = tls.createSecureContext(options)
+      secureContext.context.addCACert(options.extraCA)
+      options.secureContext = secureContext
     }
 
     if (typeof options.rejectUnauthorized !== 'undefined') {

diff --git a/tests/test-api.js b/tests/test-api.js
@@ -1,14 +1,12 @@
 'use strict'
 
-var tls = require('tls')
 var http = require('http')
 
 var tape = require('tape')
 
 var request = require('../index')
 
 var server
-var origCreateSecureContext = tls.createSecureContext // fixture
 
 tape('setup', function (t) {
   server = http.createServer()
@@ -33,111 +31,6 @@ tape('callback option', function (t) {
   })
 })
 
-tape('enableNodeExtraCACerts', function (t) {
-  request.enableNodeExtraCACerts(function (err) {
-    t.error(err)
-    t.equal(typeof tls.createSecureContext, 'function')
-    t.equal(typeof tls.__createSecureContext, 'function')
-    t.equal(tls.__createSecureContext, origCreateSecureContext) // backup
-    request.disableNodeExtraCACerts() // RESET
-    t.end()
-  })
-})
-
-tape('enableNodeExtraCACerts: without callback', function (t) {
-  request.enableNodeExtraCACerts()
-
-  t.equal(typeof tls.createSecureContext, 'function')
-  t.equal(typeof tls.__createSecureContext, 'function')
-  t.equal(tls.__createSecureContext, origCreateSecureContext) // backup
-  request.disableNodeExtraCACerts() // RESET
-  t.end()
-})
-
-tape('enableNodeExtraCACerts: with missing addCACert', function (t) {
-  // override createSecureContext
-  tls.createSecureContext = function () {
-    return {
-      context: {
-        addCACert: undefined
-      }
-    }
-  }
-
-  request.enableNodeExtraCACerts(function (err) {
-    t.ok(err)
-    t.equal(err.message, 'SecureContext.addCACert is not a function')
-    t.equal(typeof tls.__createSecureContext, 'undefined')
-    tls.createSecureContext = origCreateSecureContext // RESET
-    t.end()
-  })
-})
-
-tape('enableNodeExtraCACerts: on createSecureContext error', function (t) {
-  // override createSecureContext
-  tls.createSecureContext = function () {
-    throw new Error('something went wrong')
-  }
-
-  request.enableNodeExtraCACerts(function (err) {
-    t.ok(err)
-    t.equal(err.message, 'something went wrong')
-    t.equal(typeof tls.__createSecureContext, 'undefined')
-    tls.createSecureContext = origCreateSecureContext // RESET
-    t.end()
-  })
-})
-
-tape('enableNodeExtraCACerts: called twice', function (t) {
-  request.enableNodeExtraCACerts(function (err) {
-    t.error(err)
-    t.equal(typeof tls.createSecureContext, 'function')
-    t.equal(typeof tls.__createSecureContext, 'function')
-    t.equal(tls.__createSecureContext, origCreateSecureContext)
-
-    // called twice
-    request.enableNodeExtraCACerts(function (err) {
-      t.error(err)
-      t.equal(typeof tls.createSecureContext, 'function')
-      t.equal(typeof tls.__createSecureContext, 'function')
-      t.equal(tls.__createSecureContext, origCreateSecureContext)
-      request.disableNodeExtraCACerts() // RESET
-      t.end()
-    })
-  })
-})
-
-tape('disableNodeExtraCACerts', function (t) {
-  // enable first
-  request.enableNodeExtraCACerts(function (err) {
-    t.error(err)
-
-    // disable
-    request.disableNodeExtraCACerts()
-
-    t.equal(typeof tls.createSecureContext, 'function')
-    t.equal(typeof tls.__createSecureContext, 'undefined')
-    t.equal(tls.createSecureContext, origCreateSecureContext) // restored
-    t.end()
-  })
-})
-
-tape('disableNodeExtraCACerts: called twice', function (t) {
-  // enable first
-  request.enableNodeExtraCACerts(function (err) {
-    t.error(err)
-
-    // disable
-    request.disableNodeExtraCACerts()
-    request.disableNodeExtraCACerts()
-
-    t.equal(typeof tls.createSecureContext, 'function')
-    t.equal(typeof tls.__createSecureContext, 'undefined')
-    t.equal(tls.createSecureContext, origCreateSecureContext) // restored
-    t.end()
-  })
-})
-
 tape('cleanup', function (t) {
   server.close(t.end)
 })
diff --git a/tests/test-ssl.js b/tests/test-ssl.js
@@ -106,10 +106,7 @@ tape('pfx + passphrase(invalid)', function (t) {
   })
 })
 
-tape('extraCA(NodeExtraCACerts: enabled)', function (t) {
-  // enable extraCA support
-  request.enableNodeExtraCACerts()
-
+tape('extraCA', function (t) {
   request({
     url: sslServer.url,
     extraCA: ca,
@@ -118,27 +115,11 @@ tape('extraCA(NodeExtraCACerts: enabled)', function (t) {
   }, function (err, res, body) {
     t.equal(err, null)
     t.equal(body.toString(), 'authorized')
-    request.disableNodeExtraCACerts() // RESET
     t.end()
   })
 })
 
-tape('extraCA(NodeExtraCACerts: disabled)', function (t) {
-  request({
-    url: sslServer.url,
-    extraCA: ca,
-    key: clientKey,
-    cert: clientCert
-  }, function (err, res, body) {
-    t.ok(err)
-    t.end()
-  })
-})
-
-tape('ca + extraCA(NodeExtraCACerts: enabled)', function (t) {
-  // enable extraCA support
-  request.enableNodeExtraCACerts()
-
+tape('ca + extraCA', function (t) {
   request({
     url: sslServer.url,
     ca: ca,
@@ -148,7 +129,6 @@ tape('ca + extraCA(NodeExtraCACerts: enabled)', function (t) {
   }, function (err, res, body) {
     t.equal(err, null)
     t.equal(body.toString(), 'authorized')
-    request.disableNodeExtraCACerts() // RESET
     t.end()
   })
 })

diff --git a/tests/test-tunnel.js b/tests/test-tunnel.js
@@ -494,15 +494,13 @@ tape('setup', function (t) {
     ss.listen(0, function () {
       ss2.listen(0, 'localhost', function () {
         ss3.listen(0, 'localhost', function () {
-          request.enableNodeExtraCACerts()
           addTests()
           tape('cleanup', function (t) {
             s.destroy(function () {
               ss.destroy(function () {
                 ss2.destroy(function () {
                   ss3.destroy(function () {
                     t.end()
-                    request.disableNodeExtraCACerts()
                   })
                 })
               })