Fix proxying HTTPS requests to IP addresses (cypress-io#4947)

* use own server-destroy implementation that supports secureConnect events * stand up HTTPS server for requests over ssl to IPs * don't need to resolve with * fix tests * stand up a server on 127.0.0.1 for test * tighten up / cleanup code, consolidate + refactor - lazily fs.outputfile’s - move sslIpServers to be global - add remove all CA utility * Improve proxy_spec test * Don't crash on server error events * feedback * derp Co-authored-by: Brian Mann <brian.mann86@gmail.com>
grabartley · Oct 6, 2019 · 7e2f5df · 7e2f5df
1 parent a8c0e61
commit 7e2f5df
Show file tree

Hide file tree

Showing 13 changed files with 186 additions and 124 deletions.
diff --git a/packages/https-proxy/lib/ca.coffee b/packages/https-proxy/lib/ca.coffee
@@ -112,22 +112,35 @@ ServerExtensions = [{
 }]
 
 class CA
+  constructor: (caFolder) ->
+    if not caFolder
+      caFolder = path.join(os.tmpdir(), 'cy-ca')
+
+    @baseCAFolder = caFolder
+    @certsFolder  = path.join(@baseCAFolder, "certs")
+    @keysFolder   = path.join(@baseCAFolder, "keys")
 
- randomSerialNumber: ->
-   ## generate random 16 bytes hex string
-   sn = ""
+  removeAll: ->
+    fs
+    .removeAsync(@baseCAFolder)
+    .catchReturn({ code: "ENOENT" })
 
-   for i in [1..4]
-     sn += ("00000000" + Math.floor(Math.random()*Math.pow(256, 4)).toString(16)).slice(-8)
+  randomSerialNumber: ->
+    ## generate random 16 bytes hex string
+    sn = ""
 
-   sn
+    for i in [1..4]
+      sn += ("00000000" + Math.floor(Math.random()*Math.pow(256, 4)).toString(16)).slice(-8)
+
+    sn
 
   generateCA: ->
     generateKeyPairAsync({bits: 512})
     .then (keys) =>
       cert = pki.createCertificate()
       cert.publicKey = keys.publicKey
       cert.serialNumber = @randomSerialNumber()
+
       cert.validity.notBefore = new Date()
       cert.validity.notAfter = new Date()
       cert.validity.notAfter.setFullYear(cert.validity.notBefore.getFullYear() + 10)
@@ -140,9 +153,9 @@ class CA
       @CAkeys = keys
 
       Promise.all([
-        fs.writeFileAsync(path.join(@certsFolder, "ca.pem"),        pki.certificateToPem(cert))
-        fs.writeFileAsync(path.join(@keysFolder, "ca.private.key"), pki.privateKeyToPem(keys.privateKey))
-        fs.writeFileAsync(path.join(@keysFolder, "ca.public.key"),  pki.publicKeyToPem(keys.publicKey))
+        fs.outputFileAsync(path.join(@certsFolder, "ca.pem"),        pki.certificateToPem(cert))
+        fs.outputFileAsync(path.join(@keysFolder, "ca.private.key"), pki.privateKeyToPem(keys.privateKey))
+        fs.outputFileAsync(path.join(@keysFolder, "ca.public.key"),  pki.publicKeyToPem(keys.publicKey))
       ])
 
   loadCA: ->
@@ -198,9 +211,9 @@ class CA
     dest = mainHost.replace(asterisksRe, "_")
 
     Promise.all([
-      fs.writeFileAsync(path.join(@certsFolder, dest + ".pem"),        certPem)
-      fs.writeFileAsync(path.join(@keysFolder,  dest + ".key"),        keyPrivatePem)
-      fs.writeFileAsync(path.join(@keysFolder,  dest + ".public.key"), keyPublicPem)
+      fs.outputFileAsync(path.join(@certsFolder, dest + ".pem"),        certPem)
+      fs.outputFileAsync(path.join(@keysFolder,  dest + ".key"),        keyPrivatePem)
+      fs.outputFileAsync(path.join(@keysFolder,  dest + ".public.key"), keyPublicPem)
     ])
     .return([certPem, keyPrivatePem])
 
@@ -216,25 +229,12 @@ class CA
     path.join(@certsFolder, "ca.pem")
 
   @create = (caFolder) ->
-    ca = new CA
+    ca = new CA(caFolder)
 
-    if not caFolder
-      caFolder = path.join(os.tmpdir(), 'cy-ca')
-
-    ca.baseCAFolder = caFolder
-    ca.certsFolder  = path.join(ca.baseCAFolder, "certs")
-    ca.keysFolder   = path.join(ca.baseCAFolder, "keys")
-
-    Promise.all([
-      fs.ensureDirAsync(ca.baseCAFolder)
-      fs.ensureDirAsync(ca.certsFolder)
-      fs.ensureDirAsync(ca.keysFolder)
-    ])
-    .then ->
-      fs.statAsync(path.join(ca.certsFolder, "ca.pem"))
-      .bind(ca)
-      .then(ca.loadCA)
-      .catch(ca.generateCA)
+    fs.statAsync(path.join(ca.certsFolder, "ca.pem"))
+    .bind(ca)
+    .then(ca.loadCA)
+    .catch(ca.generateCA)
     .return(ca)
 
 module.exports = CA
diff --git a/packages/https-proxy/lib/server.coffee b/packages/https-proxy/lib/server.coffee
@@ -1,6 +1,5 @@
 _            = require("lodash")
-{ agent, connect } = require("@packages/network")
-allowDestroy = require("server-destroy-vvo")
+{ agent, allowDestroy, connect } = require("@packages/network")
 debug        = require("debug")("cypress:https-proxy")
 fs           = require("fs-extra")
 getProxyForUrl = require("proxy-from-env").getProxyForUrl
@@ -14,6 +13,7 @@ url          = require("url")
 fs = Promise.promisifyAll(fs)
 
 sslServers    = {}
+sslIpServers  = {}
 sslSemaphores = {}
 
 ## https://en.wikipedia.org/wiki/Transport_Layer_Security#TLS_record
@@ -22,9 +22,14 @@ SSL_RECORD_TYPES = [
   128, 0 ## TODO: what do these unknown types mean?
 ]
 
+onError = (err) ->
+  ## these need to be caught to avoid crashing but do not affect anything
+  debug('server error %o', { err })
+
 class Server
-  constructor: (@_ca, @_port) ->
+  constructor: (@_ca, @_port, @_options) ->
     @_onError = null
+    @_ipServers = sslIpServers
 
   connect: (req, browserSocket, head, options = {}) ->
     ## don't buffer writes - thanks a lot, Nagle
@@ -217,50 +222,78 @@ class Server
 
   _getPortFor: (hostname) ->
     @_getCertificatePathsFor(hostname)
-
     .catch (err) =>
       @_generateMissingCertificates(hostname)
-
     .then (data = {}) =>
+      if net.isIP(hostname)
+        return @_getServerPortForIp(hostname, data)
+
       @_sniServer.addContext(hostname, data)
 
       return @_sniPort
 
-  listen: (options = {}) ->
-    new Promise (resolve) =>
-      @_onError = options.onError
+  _listenHttpsServer: (data) ->
+    new Promise (resolve, reject) =>
+      server = https.createServer(data)
+
+      allowDestroy(server)
+
+      server.once "error", reject
+      server.on "upgrade", @_onUpgrade.bind(@, @_options.onUpgrade)
+      server.on "request", @_onRequest.bind(@, @_options.onRequest)
+
+      server.listen 0, '127.0.0.1', =>
+        port = server.address().port
+
+        server.removeListener("error", reject)
+        server.on "error", onError
+
+        resolve({ server, port })
+
+  ## browsers will not do SNI for an IP address
+  ## so we need to serve 1 HTTPS server per IP
+  ## https://github.com/cypress-io/cypress/issues/771
+  _getServerPortForIp: (ip, data) =>
+    if server = sslIpServers[ip]
+      return server.address().port
+
+    @_listenHttpsServer(data)
+    .then ({ server, port }) ->
+      sslIpServers[ip] = server
 
-      @_sniServer = https.createServer({})
+      debug("Created IP HTTPS Proxy Server", { port, ip })
 
-      allowDestroy(@_sniServer)
+      return port
 
-      @_sniServer.on "upgrade", @_onUpgrade.bind(@, options.onUpgrade)
-      @_sniServer.on "request", @_onRequest.bind(@, options.onRequest)
-      @_sniServer.listen 0, '127.0.0.1', =>
-        ## store the port of our current sniServer
-        @_sniPort = @_sniServer.address().port
+  listen: ->
+    @_onError = @_options.onError
 
-        debug("Created SNI HTTPS Proxy on port %s", @_sniPort)
+    @_listenHttpsServer({})
+    .tap ({ server, port}) =>
+      @_sniPort = port
+      @_sniServer = server
 
-        resolve()
+      debug("Created SNI HTTPS Proxy Server", { port })
 
   close: ->
     close = =>
-      new Promise (resolve) =>
-        @_sniServer.destroy(resolve)
+      servers = _.values(sslIpServers).concat(@_sniServer)
+      Promise.map servers, (server) =>
+        Promise.fromCallback(server.destroy)
+        .catch onError
 
     close()
-    .finally ->
-      sslServers = {}
+    .finally(module.exports.reset)
 
 module.exports = {
   reset: ->
     sslServers = {}
+    sslIpServers = {}
 
   create: (ca, port, options = {}) ->
-    srv = new Server(ca, port)
+    srv = new Server(ca, port, options)
 
     srv
-    .listen(options)
+    .listen()
     .return(srv)
 }
diff --git a/packages/https-proxy/package.json b/packages/https-proxy/package.json
@@ -21,10 +21,9 @@
     "debug": "4.1.1",
     "fs-extra": "8.1.0",
     "lodash": "4.17.15",
-    "node-forge": "0.6.49",
+    "node-forge": "0.9.0",
     "proxy-from-env": "1.0.0",
-    "semaphore": "1.1.0",
-    "server-destroy-vvo": "1.0.1"
+    "semaphore": "1.1.0"
   },
   "devDependencies": {
     "@cypress/debugging-proxy": "2.0.1",

diff --git a/packages/https-proxy/test/helpers/https_server.coffee b/packages/https-proxy/test/helpers/https_server.coffee
@@ -1,6 +1,6 @@
 https = require("https")
 Promise = require("bluebird")
-allowDestroy = require("server-destroy-vvo")
+{ allowDestroy } = require("@packages/network")
 certs = require("./certs")
 
 defaultOnRequest = (req, res) ->

diff --git a/packages/https-proxy/test/helpers/proxy.coffee b/packages/https-proxy/test/helpers/proxy.coffee
@@ -1,3 +1,4 @@
+{ allowDestroy } = require("@packages/network")
 http       = require("http")
 path       = require("path")
 httpsProxy = require("../../lib/proxy")
@@ -28,6 +29,8 @@ module.exports = {
   start: (port) ->
     prx = http.createServer()
 
+    allowDestroy(prx)
+
     dir = path.join(process.cwd(), "ca")
 
     httpsProxy.create(dir, port, {
@@ -61,7 +64,7 @@ module.exports = {
 
   stop: ->
     new Promise (resolve) ->
-      prx.close(resolve)
+      prx.destroy(resolve)
     .then ->
       prx.proxy.close()
 }
diff --git a/packages/https-proxy/test/integration/proxy_spec.coffee b/packages/https-proxy/test/integration/proxy_spec.coffee
@@ -4,6 +4,8 @@ process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"
 
 _           = require("lodash")
 DebugProxy  = require("@cypress/debugging-proxy")
+fs          = require("fs-extra")
+https       = require("https")
 net         = require("net")
 network     = require("@packages/network")
 path        = require("path")
@@ -130,6 +132,7 @@ describe "Proxy", ->
       url: "http://localhost:8080/"
       proxy: "http://localhost:3333"
     })
+
     .then (html) ->
       expect(html).to.include("http server")
 
@@ -152,6 +155,33 @@ describe "Proxy", ->
           proxy: "http://localhost:3333"
         })
 
+    ## https://github.com/cypress-io/cypress/issues/771
+    it "generates certs and can proxy requests for HTTPS requests to IPs", ->
+      @sandbox.spy(@proxy, "_generateMissingCertificates")
+      @sandbox.spy(@proxy, "_getServerPortForIp")
+
+      Promise.all([
+        httpsServer.start(8445),
+        @proxy._ca.removeAll()
+      ])
+      .then =>
+        request({
+          strictSSL: false
+          url: "https://127.0.0.1:8445/"
+          proxy: "http://localhost:3333"
+        })
+      .then =>
+        ## this should not stand up its own https server
+        request({
+          strictSSL: false
+          url: "https://localhost:8443/"
+          proxy: "http://localhost:3333"
+        })
+      .then =>
+        expect(@proxy._ipServers["127.0.0.1"]).to.be.an.instanceOf(https.Server)
+        expect(@proxy._getServerPortForIp).to.be.calledWith('127.0.0.1').and.be.calledOnce
+        expect(@proxy._generateMissingCertificates).to.be.calledTwice
+
   context "closing", ->
     it "resets sslServers and can reopen", ->
       request({
@@ -256,7 +286,7 @@ describe "Proxy", ->
       })
       .then =>
         throw new Error('should not succeed')
-      .catch { message: 'Error: socket hang up' }, =>
+      .catch { message: "Error: socket hang up" }, =>
         expect(createProxyConn).to.not.be.called
         expect(createSocket).to.be.calledWith({
           port: @proxy._sniPort

diff --git a/packages/network/lib/allow-destroy.ts b/packages/network/lib/allow-destroy.ts
@@ -0,0 +1,31 @@
+import net from 'net'
+
+/**
+ * `allowDestroy` adds a `destroy` method to a `net.Server`. `destroy(cb)`
+ * will kill all open connections and call `cb` when the server is closed.
+ *
+ * Note: `server-destroy` NPM package cannot be used - it does not track
+ * `secureConnection` events.
+ */
+export function allowDestroy (server: net.Server) {
+  let connections: net.Socket[] = []
+
+  function trackConn (conn) {
+    connections.push(conn)
+
+    conn.on('close', () => {
+      connections = connections.filter((connection) => connection !== conn)
+    })
+  }
+
+  server.on('connection', trackConn)
+  server.on('secureConnection', trackConn)
+
+  // @ts-ignore Property 'destroy' does not exist on type 'Server'.
+  server.destroy = function (cb) {
+    server.close(cb)
+    connections.map((connection) => connection.destroy())
+  }
+
+  return server
+}
diff --git a/packages/network/lib/index.ts b/packages/network/lib/index.ts
@@ -1,6 +1,9 @@
 import agent from './agent'
 import * as connect from './connect'
+import { allowDestroy } from './allow-destroy'
 
-export { agent }
-
-export { connect }
+export {
+  agent,
+  allowDestroy,
+  connect,
+}