From 8562cba5583b8bb314c9fde48c867552a8fe2c4d Mon Sep 17 00:00:00 2001 From: Ryan Manuel Date: Fri, 4 Nov 2022 15:27:31 -0500 Subject: [PATCH] feat: add reject unauthorized to api server calls and standardize CA usage (#24493) --- .circleci/config.yml | 6 +- cli/lib/cli.js | 16 +- cli/lib/tasks/download.js | 10 +- cli/test/lib/tasks/download_spec.js | 5 +- packages/network/lib/agent.ts | 54 ++++- packages/network/lib/ca.ts | 54 +++++ packages/network/test/support/servers.ts | 14 +- packages/network/test/unit/agent_spec.ts | 206 +++++++++++++++++- packages/server/lib/cloud/api.ts | 1 + packages/server/test/unit/cloud/api_spec.js | 14 ++ .../cache/dev-darwin/snapshot-meta.cache.json | 4 +- 11 files changed, 353 insertions(+), 31 deletions(-) create mode 100644 packages/network/lib/ca.ts diff --git a/.circleci/config.yml b/.circleci/config.yml index 5c6ae226c0..b656c89ba2 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -27,7 +27,7 @@ mainBuildFilters: &mainBuildFilters branches: only: - develop - - 'feature/v8-snapshots' + - 'ryanm/feat/certificates' # usually we don't build Mac app - it takes a long time # but sometimes we want to really confirm we are doing the right thing @@ -36,7 +36,7 @@ macWorkflowFilters: &darwin-workflow-filters when: or: - equal: [ develop, << pipeline.git.branch >> ] - - equal: [ 'feature/v8-snapshots', << pipeline.git.branch >> ] + - equal: [ 'ryanm/feat/certificates', << pipeline.git.branch >> ] - matches: pattern: "-release$" value: << pipeline.git.branch >> @@ -130,7 +130,7 @@ commands: - run: name: Check current branch to persist artifacts command: | - if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "feature/v8-snapshots" ]]; then + if [[ "$CIRCLE_BRANCH" != "develop" && "$CIRCLE_BRANCH" != "ryanm/feat/certificates" ]]; then echo "Not uploading artifacts or posting install comment for this branch." circleci-agent step halt fi diff --git a/cli/lib/cli.js b/cli/lib/cli.js index 052893ca4c..1cbc8547fb 100644 --- a/cli/lib/cli.js +++ b/cli/lib/cli.js @@ -405,7 +405,21 @@ module.exports = { args = process.argv } - const { CYPRESS_INTERNAL_ENV } = process.env + const { CYPRESS_INTERNAL_ENV, CYPRESS_DOWNLOAD_USE_CA } = process.env + + if (process.env.CYPRESS_DOWNLOAD_USE_CA) { + let msg = ` + ${logSymbols.warning} Warning: It looks like you're setting CYPRESS_DOWNLOAD_USE_CA=${CYPRESS_DOWNLOAD_USE_CA} + + The environment variable "CYPRESS_DOWNLOAD_USE_CA" is no longer required to be set. + + You can safely unset this environment variable. + ` + + logger.log() + logger.warn(stripIndent(msg)) + logger.log() + } if (!util.isValidCypressInternalEnvValue(CYPRESS_INTERNAL_ENV)) { debug('invalid CYPRESS_INTERNAL_ENV value', CYPRESS_INTERNAL_ENV) diff --git a/cli/lib/tasks/download.js b/cli/lib/tasks/download.js index 6b8cd38b69..17b6c733f3 100644 --- a/cli/lib/tasks/download.js +++ b/cli/lib/tasks/download.js @@ -40,13 +40,7 @@ const getBaseUrl = () => { const getCA = () => { return new Promise((resolve) => { - if (!util.getEnv('CYPRESS_DOWNLOAD_USE_CA')) { - resolve() - } - - if (process.env.npm_config_ca) { - resolve(process.env.npm_config_ca) - } else if (process.env.npm_config_cafile) { + if (process.env.npm_config_cafile) { fs.readFile(process.env.npm_config_cafile, 'utf8') .then((cafileContent) => { resolve(cafileContent) @@ -54,6 +48,8 @@ const getCA = () => { .catch(() => { resolve() }) + } else if (process.env.npm_config_ca) { + resolve(process.env.npm_config_ca) } else { resolve() } diff --git a/cli/test/lib/tasks/download_spec.js b/cli/test/lib/tasks/download_spec.js index 13c1176795..7617cdcd4e 100644 --- a/cli/test/lib/tasks/download_spec.js +++ b/cli/test/lib/tasks/download_spec.js @@ -607,7 +607,7 @@ describe('lib/tasks/download', function () { // prevent ambient environment masking of environment variables referenced in this test ;([ - 'CYPRESS_DOWNLOAD_USE_CA', 'NO_PROXY', 'http_proxy', + 'NO_PROXY', 'http_proxy', 'https_proxy', 'npm_config_ca', 'npm_config_cafile', 'npm_config_https_proxy', 'npm_config_proxy', ]).forEach((e) => { @@ -683,7 +683,6 @@ describe('lib/tasks/download', function () { }) it('returns CA from npm_config_ca', () => { - process.env.CYPRESS_DOWNLOAD_USE_CA = 'true' process.env.npm_config_ca = 'foo' return download.getCA().then((ca) => { @@ -692,7 +691,6 @@ describe('lib/tasks/download', function () { }) it('returns CA from npm_config_cafile', () => { - process.env.CYPRESS_DOWNLOAD_USE_CA = 'true' process.env.npm_config_cafile = 'test/fixture/cafile.pem' return download.getCA().then((ca) => { @@ -701,7 +699,6 @@ describe('lib/tasks/download', function () { }) it('returns undefined if failed reading npm_config_cafile', () => { - process.env.CYPRESS_DOWNLOAD_USE_CA = 'true' process.env.npm_config_cafile = 'test/fixture/not-exists.pem' return download.getCA().then((ca) => { diff --git a/packages/network/lib/agent.ts b/packages/network/lib/agent.ts index a4bc0aa46b..36fea530ed 100644 --- a/packages/network/lib/agent.ts +++ b/packages/network/lib/agent.ts @@ -8,11 +8,53 @@ import url from 'url' import { createRetryingSocket, getAddress } from './connect' import { lenientOptions } from './http-utils' import { ClientCertificateStore } from './client-certificates' +import { CaOptions, getCaOptions } from './ca' const debug = debugModule('cypress:network:agent') const CRLF = '\r\n' const statusCodeRe = /^HTTP\/1.[01] (\d*)/ +let baseCaOptions: CaOptions | undefined +const getCaOptionsPromise = (): Promise => { + return getCaOptions().then((options: CaOptions) => { + baseCaOptions = options + + return options + }).catch(() => { + // Errors reading the config are treated as warnings by npm and node and handled by those processes separately + // from what we're doing here. + return {} + }) +} +let baseCaOptionsPromise: Promise = getCaOptionsPromise() + +// This is for testing purposes only +export const _resetBaseCaOptionsPromise = () => { + baseCaOptions = undefined + baseCaOptionsPromise = getCaOptionsPromise() +} + +const mergeCAOptions = (options: https.RequestOptions, caOptions: CaOptions): https.RequestOptions => { + if (!caOptions.ca) { + return options + } + + if (!options.ca) { + return { + ...options, + ca: caOptions.ca, + } + } + + // First, normalize the options.ca option. It can be a string, a Buffer, an array of strings, or an array of Buffers + const caArray = _.castArray(options.ca).map((caOption) => caOption.toString()) + + return { + ...options, + ca: [...caArray, ...caOptions.ca], + } +} + export const clientCertificateStore = new ClientCertificateStore() type WithProxyOpts = RequestOptions & { @@ -195,7 +237,7 @@ export class CombinedAgent { if (isHttps) { _.assign(options, clientCertificateStore.getClientCertificateAgentOptionsForUrl(options.uri)) - return this.httpsAgent.addRequest(req, options) + return this.httpsAgent.addRequest(req, options as https.RequestOptions) } this.httpAgent.addRequest(req, options) @@ -283,7 +325,7 @@ class HttpsAgent extends https.Agent { super(opts) } - addRequest (req: http.ClientRequest, options: http.RequestOptions) { + addRequest (req: http.ClientRequest, options: https.RequestOptions) { // Ensure we have a proper port defined otherwise node has assumed we are port 80 // (https://github.com/nodejs/node/blob/master/lib/_http_client.js#L164) since we are a combined agent // rather than an http or https agent. This will cause issues with fetch requests (@cypress/request already handles it: @@ -293,7 +335,13 @@ class HttpsAgent extends https.Agent { options.port = 443 } - super.addRequest(req, options) + if (baseCaOptions) { + super.addRequest(req, mergeCAOptions(options, baseCaOptions)) + } else { + baseCaOptionsPromise.then((caOptions) => { + super.addRequest(req, mergeCAOptions(options, caOptions)) + }) + } } createConnection (options: HttpsRequestOptions, cb: http.SocketCallback) { diff --git a/packages/network/lib/ca.ts b/packages/network/lib/ca.ts new file mode 100644 index 0000000000..136ffe4ea6 --- /dev/null +++ b/packages/network/lib/ca.ts @@ -0,0 +1,54 @@ +import { promises as fs } from 'fs' +import tls from 'tls' + +export type CaOptions = { ca?: string[] } + +const getNpmConfigCAFileValue = (): Promise => { + if (process.env.npm_config_cafile) { + return fs.readFile(process.env.npm_config_cafile, 'utf8').then((ca) => { + return ca + }).catch(() => { + return undefined + }) + } + + return Promise.resolve(undefined) +} +const getNodeExtraCACertsFileValue = (): Promise => { + if (process.env.NODE_EXTRA_CA_CERTS) { + return fs.readFile(process.env.NODE_EXTRA_CA_CERTS, 'utf8').then((ca) => { + return ca + }).catch(() => { + return undefined + }) + } + + return Promise.resolve(undefined) +} + +const getCaOptions = (): Promise => { + // Load the contents of process.env.npm_config_cafile and process.env.NODE_EXTRA_CA_CERTS + // They will be cached so we don't have to actually read them on every call + return Promise.all([getNpmConfigCAFileValue(), getNodeExtraCACertsFileValue()]).then(([npm_config_cafile, NODE_EXTRA_CA_CERTS]) => { + // Merge the contents of ca with the npm config options. These options are meant to be replacements, but we want to keep the tls client certificate + // config that our consumers provide + if (npm_config_cafile) { + return { ca: [npm_config_cafile] } + } + + if (process.env.npm_config_ca) { + return { ca: [process.env.npm_config_ca] } + } + + // Merge the contents of ca with the NODE_EXTRA_CA_CERTS options. This option is additive to the tls root certificates + if (NODE_EXTRA_CA_CERTS) { + return { ca: tls.rootCertificates.concat(NODE_EXTRA_CA_CERTS) } + } + + return {} + }) +} + +export { + getCaOptions, +} diff --git a/packages/network/test/support/servers.ts b/packages/network/test/support/servers.ts index 42b98742f4..b93291424c 100644 --- a/packages/network/test/support/servers.ts +++ b/packages/network/test/support/servers.ts @@ -27,9 +27,11 @@ function createExpressApp () { return app } -function getLocalhostCertKeys () { +function getCAInformation () { return CA.create() - .then((ca) => ca.generateServerCertificateKeys('localhost')) + .then((ca) => { + return Promise.all([ca.generateServerCertificateKeys('localhost'), ca.getCACertPath()]) + }) } function onWsConnection (socket) { @@ -42,20 +44,22 @@ export class Servers { httpsServer: https.Server & AsyncServer wsServer: any wssServer: any + caCertificatePath: string start (httpPort: number, httpsPort: number) { return Promise.join( createExpressApp(), - getLocalhostCertKeys(), + getCAInformation(), ) - .spread((app: Express.Application, [cert, key]: string[]) => { + .spread((app: Express.Application, [serverCertificateKeys, caCertificatePath]: [serverCertificateKeys: string[], caCertificatePath: string]) => { this.httpServer = Promise.promisifyAll( allowDestroy(http.createServer(app)), ) as http.Server & AsyncServer this.wsServer = new SocketIOServer(this.httpServer) - this.https = { cert, key } + this.caCertificatePath = caCertificatePath + this.https = { cert: serverCertificateKeys[0], key: serverCertificateKeys[1] } this.httpsServer = Promise.promisifyAll( allowDestroy(https.createServer(this.https, app)), ) as https.Server & AsyncServer diff --git a/packages/network/test/unit/agent_spec.ts b/packages/network/test/unit/agent_spec.ts index f25d78c6f1..38f4942be9 100644 --- a/packages/network/test/unit/agent_spec.ts +++ b/packages/network/test/unit/agent_spec.ts @@ -18,14 +18,16 @@ import { regenerateRequestHead, CombinedAgent, clientCertificateStore, + _resetBaseCaOptionsPromise, } from '../../lib/agent' import { allowDestroy } from '../../lib/allow-destroy' import { AsyncServer, Servers } from '../support/servers' import { UrlClientCertificates, ClientCertificates, PemKey } from '../../lib/client-certificates' -import Forge from 'node-forge' +import { pki } from 'node-forge' import fetch from 'cross-fetch' import os from 'os' -const { pki } = Forge +import path from 'path' +import fs from 'fs' const expect = chai.expect @@ -35,7 +37,14 @@ const PROXY_PORT = 31000 const HTTP_PORT = 31080 const HTTPS_PORT = 443 -function createCertAndKey (): [object, object] { +const tempDirName = 'ca-config-tests' +const tempDirPath = path.join(os.tmpdir(), tempDirName) + +if (!fs.existsSync(tempDirPath)) { + fs.mkdirSync(tempDirPath) +} + +function createCertAndKey (): [pki.Certificate, pki.rsa.PrivateKey] { let keys = pki.rsa.generateKeyPair(2048) let cert = pki.createCertificate() @@ -491,6 +500,143 @@ describe('lib/agent', function () { }) }) + context('CombinedAgent with CA overrides', function () { + const proxyUrl = `https://localhost:${PROXY_PORT}` + + before(function () { + this.servers = new Servers() + + return this.servers.start(HTTP_PORT, HTTPS_PORT) + }) + + after(function () { + return this.servers.stop() + }) + + ;[ + { + name: 'should use the npm_config_cafile override', + option: 'npm_config_cafile', + }, + { + name: 'should use the npm_config_ca override', + option: 'npm_config_ca', + }, + { + name: 'should use the NODE_EXTRA_CA_CERTS override', + option: 'NODE_EXTRA_CA_CERTS', + }, + ].slice().map((testCase) => { + context(testCase.name, function () { + beforeEach(function () { + // PROXY vars should override npm_config vars, so set them to cause failures if they are used + // @see https://github.com/cypress-io/cypress/pull/8295 + process.env.npm_config_proxy = process.env.npm_config_https_proxy = 'http://erroneously-used-npm-proxy.invalid' + process.env.npm_config_noproxy = 'just,some,nonsense' + + process.env.HTTP_PROXY = process.env.HTTPS_PROXY = proxyUrl + process.env.NO_PROXY = '' + + this.agent = new CombinedAgent() + + this.request = request.defaults({ + proxy: null, + agent: this.agent, + }) + + let options: any = { + keepRequests: true, + https: { + ...this.servers.https, + ca: this.caContents, + }, + auth: false, + } + + if (testCase.option === 'npm_config_cafile') { + process.env.npm_config_cafile = this.servers.caCertificatePath + this.caContents = [fs.readFileSync(this.servers.caCertificatePath, 'utf-8')] + + // Ensure the priority picks cafile over the next two options + process.env.npm_config_ca = 'a' + process.env.NODE_EXTRA_CA_CERTS = 'b' + } + + if (testCase.option === 'npm_config_ca') { + this.caContents = [fs.readFileSync(this.servers.caCertificatePath, 'utf-8')] + process.env.npm_config_ca = this.caContents[0] + + // Ensure the priority picks cafile over the next option + process.env.NODE_EXTRA_CA_CERTS = 'b' + } + + if (testCase.option === 'NODE_EXTRA_CA_CERTS') { + process.env.NODE_EXTRA_CA_CERTS = this.servers.caCertificatePath + this.caContents = [fs.readFileSync(this.servers.caCertificatePath, 'utf-8'), ...tls.rootCertificates] + } + + _resetBaseCaOptionsPromise() + + this.debugProxy = new DebuggingProxy(options) + + return this.debugProxy.start(PROXY_PORT) + }) + + afterEach(function () { + delete process.env.npm_config_cafile + delete process.env.npm_config_ca + delete process.env.NODE_EXTRA_CA_CERTS + this.debugProxy.stop() + }) + + it(`CA from ${testCase.option} presented for https request`, function () { + return this.request({ + url: `https://localhost:${HTTPS_PORT}/get`, + rejectUnauthorized: true, + }).then((body) => { + // Test the CA options the first time through + expect(body).to.eq('It worked!') + if (this.debugProxy) { + expect(this.debugProxy.requests[0]).to.include({ + https: true, + url: `localhost:${HTTPS_PORT}`, + }) + } + + const socketKey = Object.keys(this.agent.httpsAgent.sockets).filter((key) => key.includes(`localhost:${HTTPS_PORT}`)) + + expect(socketKey.length).to.eq(1, 'There should only be a single localhost TLS Socket') + + for (const ca of this.caContents) { + expect(socketKey[0]).to.contain(ca, `${testCase.option} should be used for the TLS Socket`) + } + + return this.request({ + url: `https://localhost:${HTTPS_PORT}/get`, + }) + }).then((body) => { + // Test that the caching of the ca options works + expect(body).to.eq('It worked!') + if (this.debugProxy) { + expect(this.debugProxy.requests[0]).to.include({ + https: true, + url: `localhost:${HTTPS_PORT}`, + }) + } + + const socketKey = Object.keys(this.agent.httpsAgent.sockets).filter((key) => key.includes(`localhost:${HTTPS_PORT}`)) + + expect(socketKey.length).to.eq(1, 'There should only be a single localhost TLS Socket') + + for (const ca of this.caContents) { + expect(socketKey[0]).to.contain(ca, `${testCase.option} should be used for the TLS Socket`) + } + }) + }) + }) + }) + }) + context('CombinedAgent with client certificates', function () { const proxyUrl = `https://localhost:${PROXY_PORT}` @@ -509,6 +655,21 @@ describe('lib/agent', function () { name: 'should present a client certificate', presentClientCertificate: true, }, + { + name: 'should present a client certificate with npm_config_cafile', + option: 'npm_config_cafile', + presentClientCertificate: true, + }, + { + name: 'should present a client certificate with npm_config_ca', + option: 'npm_config_ca', + presentClientCertificate: true, + }, + { + name: 'should present a client certificate with NODE_EXTRA_CA_CERTS', + option: 'NODE_EXTRA_CA_CERTS', + presentClientCertificate: true, + }, { name: 'should not present a client certificate', presentClientCertificate: false, @@ -546,21 +707,48 @@ describe('lib/agent', function () { const testCerts = new UrlClientCertificates(`https://localhost`) testCerts.clientCertificates = new ClientCertificates() - testCerts.clientCertificates.cert.push(pemCert) - testCerts.clientCertificates.key.push(new PemKey(pki.privateKeyToPem(certAndKey[1]), undefined)) + testCerts.clientCertificates.cert.push(Buffer.from(pemCert, 'utf-8')) + testCerts.clientCertificates.key.push(new PemKey(Buffer.from(pki.privateKeyToPem(certAndKey[1]), 'utf-8'), undefined)) clientCertificateStore.addClientCertificatesForUrl(testCerts) } + if (testCase.option === 'npm_config_cafile') { + process.env.npm_config_cafile = this.servers.caCertificatePath + this.caContents = [fs.readFileSync(this.servers.caCertificatePath, 'utf-8')] + + // Ensure the priority picks cafile over the next two options + process.env.npm_config_ca = 'a' + process.env.NODE_EXTRA_CA_CERTS = 'b' + } + + if (testCase.option === 'npm_config_ca') { + this.caContents = [fs.readFileSync(this.servers.caCertificatePath, 'utf-8')] + process.env.npm_config_ca = this.caContents[0] + + // Ensure the priority picks cafile over the next option + process.env.NODE_EXTRA_CA_CERTS = 'b' + } + + if (testCase.option === 'NODE_EXTRA_CA_CERTS') { + process.env.NODE_EXTRA_CA_CERTS = this.servers.caCertificatePath + this.caContents = [fs.readFileSync(this.servers.caCertificatePath, 'utf-8'), ...tls.rootCertificates] + } + + _resetBaseCaOptionsPromise() + this.debugProxy = new DebuggingProxy(options) return this.debugProxy.start(PROXY_PORT) }) afterEach(function () { + delete process.env.npm_config_cafile + delete process.env.npm_config_ca + delete process.env.NODE_EXTRA_CA_CERTS this.debugProxy.stop() }) - it(`Client certificate${testCase.presentClientCertificate ? ' ' : ' not '}presented for https request`, function () { + it(`Client certificate${testCase.presentClientCertificate ? ' ' : ' not '}presented for https request${testCase.option ? ` with config option ${testCase.option}` : '' }`, function () { return this.request({ url: `https://localhost:${HTTPS_PORT}/get`, }).then((body) => { @@ -583,6 +771,12 @@ describe('lib/agent', function () { } else { expect(socketKey[0]).not.to.contain(this.clientCert, 'A client cert should not be used for the TLS Socket') } + + if (this.caContents) { + for (const ca of this.caContents) { + expect(socketKey[0]).to.contain(ca, `${testCase.option} should be used for the TLS Socket`) + } + } }) }) diff --git a/packages/server/lib/cloud/api.ts b/packages/server/lib/cloud/api.ts index 9a084d82b9..93ca5d7b32 100644 --- a/packages/server/lib/cloud/api.ts +++ b/packages/server/lib/cloud/api.ts @@ -46,6 +46,7 @@ const rp = request.defaults((params, callback) => { proxy: null, gzip: true, cacheable: false, + rejectUnauthorized: true, }) const headers = params.headers != null ? params.headers : (params.headers = {}) diff --git a/packages/server/test/unit/cloud/api_spec.js b/packages/server/test/unit/cloud/api_spec.js index 3beec2c3c6..360432802e 100644 --- a/packages/server/test/unit/cloud/api_spec.js +++ b/packages/server/test/unit/cloud/api_spec.js @@ -69,6 +69,20 @@ describe('lib/cloud/api', () => { }) }) + it('sets rejectUnauthorized on the request', () => { + nock.cleanAll() + + return api.ping() + .thenThrow() + .catch(() => { + expect(agent.addRequest).to.be.calledOnce + + expect(agent.addRequest).to.be.calledWithMatch(sinon.match.any, { + rejectUnauthorized: true, + }) + }) + }) + context('with a proxy defined', () => { beforeEach(function () { nock.cleanAll() diff --git a/tooling/v8-snapshot/cache/dev-darwin/snapshot-meta.cache.json b/tooling/v8-snapshot/cache/dev-darwin/snapshot-meta.cache.json index 1545a365fd..4fd1c92a9d 100644 --- a/tooling/v8-snapshot/cache/dev-darwin/snapshot-meta.cache.json +++ b/tooling/v8-snapshot/cache/dev-darwin/snapshot-meta.cache.json @@ -1101,7 +1101,6 @@ "./node_modules/ansi_up/ansi_up.js", "./node_modules/any-base/index.js", "./node_modules/any-base/src/converter.js", - "./node_modules/anymatch/index.js", "./node_modules/archiver-utils/file.js", "./node_modules/archiver-utils/index.js", "./node_modules/archiver-utils/node_modules/glob/common.js", @@ -3285,6 +3284,7 @@ "./node_modules/yn/lenient.js", "./packages/data-context/node_modules/@babel/code-frame/lib/index.js", "./packages/data-context/node_modules/@babel/parser/lib/index.js", + "./packages/data-context/node_modules/anymatch/index.js", "./packages/data-context/node_modules/cross-spawn/index.js", "./packages/data-context/node_modules/cross-spawn/lib/enoent.js", "./packages/data-context/node_modules/cross-spawn/lib/parse.js", @@ -3528,5 +3528,5 @@ "./tooling/v8-snapshot/cache/dev-darwin/snapshot-entry.js" ], "deferredHashFile": "yarn.lock", - "deferredHash": "66156a5424fbd0d70c750ef2b16c22b0084a30220c67b632cff9212a7de7a226" + "deferredHash": "9fe6e763921f20d9fa851fee03e5b5e7c334c2bd85a88bcdf0ed49cde252e095" } \ No newline at end of file