From dcb303a437be8c653eb78e67f314db3cb589a79a Mon Sep 17 00:00:00 2001 From: Asher Date: Thu, 15 Oct 2020 16:17:04 -0500 Subject: [PATCH] Move argument defaults into setDefaults --- src/node/cli.ts | 135 +++++++++++++++++++++++++++++++++++----------- src/node/entry.ts | 91 +++++++++++-------------------- src/node/http.ts | 12 ++--- test/cli.test.ts | 73 ++++++++++++++++++++++++- 4 files changed, 209 insertions(+), 102 deletions(-) diff --git a/src/node/cli.ts b/src/node/cli.ts index 1403d8920..ba8f5ae88 100644 --- a/src/node/cli.ts +++ b/src/node/cli.ts @@ -5,7 +5,7 @@ import * as os from "os" import * as path from "path" import { Args as VsArgs } from "../../lib/vscode/src/vs/server/ipc" import { AuthType } from "./http" -import { canConnect, generatePassword, humanPath, paths } from "./util" +import { canConnect, generateCertificate, generatePassword, humanPath, paths } from "./util" export class Optional { public constructor(public readonly value?: T) {} @@ -22,33 +22,33 @@ export enum LogLevel { export class OptionalString extends Optional {} export interface Args extends VsArgs { - readonly config?: string - readonly auth?: AuthType - readonly password?: string - readonly cert?: OptionalString - readonly "cert-key"?: string - readonly "disable-telemetry"?: boolean - readonly help?: boolean - readonly host?: string - readonly json?: boolean + config?: string + auth?: AuthType + password?: string + cert?: OptionalString + "cert-key"?: string + "disable-telemetry"?: boolean + help?: boolean + host?: string + json?: boolean log?: LogLevel - readonly open?: boolean - readonly port?: number - readonly "bind-addr"?: string - readonly socket?: string - readonly version?: boolean - readonly force?: boolean - readonly "list-extensions"?: boolean - readonly "install-extension"?: string[] - readonly "show-versions"?: boolean - readonly "uninstall-extension"?: string[] - readonly "proxy-domain"?: string[] - readonly locale?: string - readonly _: string[] - readonly "reuse-window"?: boolean - readonly "new-window"?: boolean + open?: boolean + port?: number + "bind-addr"?: string + socket?: string + version?: boolean + force?: boolean + "list-extensions"?: boolean + "install-extension"?: string[] + "show-versions"?: boolean + "uninstall-extension"?: string[] + "proxy-domain"?: string[] + locale?: string + _: string[] + "reuse-window"?: boolean + "new-window"?: boolean - readonly link?: OptionalString + link?: OptionalString } interface Option { @@ -325,13 +325,37 @@ export const parse = ( args._.push(arg) } + // If a cert was provided a key must also be provided. + if (args.cert && args.cert.value && !args["cert-key"]) { + throw new Error("--cert-key is missing") + } + logger.debug("parsed command line", field("args", args)) return args } -export async function setDefaults(args: Args): Promise { - args = { ...args } +export interface DefaultedArgs extends ConfigArgs { + auth: AuthType + cert?: { + value: string + } + host: string + port: number + "proxy-domain": string[] + verbose: boolean + usingEnvPassword: boolean + "extensions-dir": string + "user-data-dir": string +} + +/** + * Take CLI and config arguments (optional) and return a single set of arguments + * with the defaults set. Arguments from the CLI are prioritized over config + * arguments. + */ +export async function setDefaults(cliArgs: Args, configArgs?: ConfigArgs): Promise { + const args = Object.assign({}, configArgs || {}, cliArgs) if (!args["user-data-dir"]) { await copyOldMacOSDataDir() @@ -381,7 +405,52 @@ export async function setDefaults(args: Args): Promise { break } - return args + // Default to using a password. + if (!args.auth) { + args.auth = AuthType.Password + } + + const [host, port] = bindAddrFromAllSources(args, configArgs || { _: [] }) + args.host = host + args.port = port + + // If we're being exposed to the cloud, we listen on a random address and + // disable auth. + if (args.link) { + args.host = "localhost" + args.port = 0 + args.socket = undefined + args.cert = undefined + + if (args.auth !== AuthType.None) { + args.auth = AuthType.None + } + } + + if (args.cert && !args.cert.value) { + const { cert, certKey } = await generateCertificate() + args.cert = { + value: cert, + } + args["cert-key"] = certKey + } + + const usingEnvPassword = !!process.env.PASSWORD + if (process.env.PASSWORD) { + args.password = process.env.PASSWORD + } + + // Ensure it's not readable by child processes. + delete process.env.PASSWORD + + // Filter duplicate proxy domains and remove any leading `*.`. + const proxyDomains = new Set((args["proxy-domain"] || []).map((d) => d.replace(/^\*\./, ""))) + args["proxy-domain"] = Array.from(proxyDomains) + + return { + ...args, + usingEnvPassword, + } as DefaultedArgs // TODO: Technically no guarantee this is fulfilled. } async function defaultConfigFile(): Promise { @@ -392,12 +461,16 @@ cert: false ` } +interface ConfigArgs extends Args { + config: string +} + /** * Reads the code-server yaml config file and returns it as Args. * * @param configPath Read the config from configPath instead of $CODE_SERVER_CONFIG or the default. */ -export async function readConfigFile(configPath?: string): Promise { +export async function readConfigFile(configPath?: string): Promise { if (!configPath) { configPath = process.env.CODE_SERVER_CONFIG if (!configPath) { @@ -466,7 +539,7 @@ function bindAddrFromArgs(addr: Addr, args: Args): Addr { return addr } -export function bindAddrFromAllSources(cliArgs: Args, configArgs: Args): [string, number] { +function bindAddrFromAllSources(cliArgs: Args, configArgs: Args): [string, number] { let addr: Addr = { host: "localhost", port: 8080, diff --git a/src/node/entry.ts b/src/node/entry.ts index 96db046e2..826d9a484 100644 --- a/src/node/entry.ts +++ b/src/node/entry.ts @@ -12,8 +12,7 @@ import { StaticHttpProvider } from "./app/static" import { UpdateHttpProvider } from "./app/update" import { VscodeHttpProvider } from "./app/vscode" import { - Args, - bindAddrFromAllSources, + DefaultedArgs, optionDescriptions, parse, readConfigFile, @@ -24,7 +23,7 @@ import { import { coderCloudBind } from "./coder-cloud" import { AuthType, HttpServer, HttpServerOptions } from "./http" import { loadPlugins } from "./plugin" -import { generateCertificate, hash, humanPath, open } from "./util" +import { hash, humanPath, open } from "./util" import { ipcMain, WrapperProcess } from "./wrapper" let pkg: { version?: string; commit?: string } = {} @@ -37,7 +36,7 @@ try { const version = pkg.version || "development" const commit = pkg.commit || "development" -export const runVsCodeCli = (args: Args): void => { +export const runVsCodeCli = (args: DefaultedArgs): void => { logger.debug("forking vs code cli...") const vscode = cp.fork(path.resolve(__dirname, "../../lib/vscode/out/vs/server/fork"), [], { env: { @@ -61,7 +60,7 @@ export const runVsCodeCli = (args: Args): void => { vscode.on("exit", (code) => process.exit(code || 0)) } -export const openInExistingInstance = async (args: Args, socketPath: string): Promise => { +export const openInExistingInstance = async (args: DefaultedArgs, socketPath: string): Promise => { const pipeArgs: OpenCommandPipeArgs & { fileURIs: string[] } = { type: "open", folderURIs: [], @@ -117,54 +116,26 @@ export const openInExistingInstance = async (args: Args, socketPath: string): Pr vscode.end() } -const main = async (args: Args, configArgs: Args): Promise => { - if (args.link) { - // If we're being exposed to the cloud, we listen on a random address and disable auth. - args = { - ...args, - host: "localhost", - port: 0, - auth: AuthType.None, - socket: undefined, - cert: undefined, - } - logger.info("link: disabling auth and listening on random localhost port for cloud agent") - } - - if (!args.auth) { - args = { - ...args, - auth: AuthType.Password, - } - } - +const main = async (args: DefaultedArgs): Promise => { logger.info(`Using user-data-dir ${humanPath(args["user-data-dir"])}`) - logger.trace(`Using extensions-dir ${humanPath(args["extensions-dir"])}`) - const envPassword = !!process.env.PASSWORD - const password = args.auth === AuthType.Password && (process.env.PASSWORD || args.password) - if (args.auth === AuthType.Password && !password) { + if (args.auth === AuthType.Password && !args.password) { throw new Error("Please pass in a password via the config file or $PASSWORD") } - const [host, port] = bindAddrFromAllSources(args, configArgs) // Spawn the main HTTP server. const options: HttpServerOptions = { auth: args.auth, commit, - host: host, + host: args.host, // The hash does not add any actual security but we do it for obfuscation purposes. - password: password ? hash(password) : undefined, - port: port, + password: args.password ? hash(args.password) : undefined, + port: args.port, proxyDomains: args["proxy-domain"], socket: args.socket, - ...(args.cert && !args.cert.value - ? await generateCertificate() - : { - cert: args.cert && args.cert.value, - certKey: args["cert-key"], - }), + cert: args.cert && args.cert.value, + certKey: args["cert-key"], } if (options.cert && !options.certKey) { @@ -175,7 +146,7 @@ const main = async (args: Args, configArgs: Args): Promise => { httpServer.registerHttpProvider(["/", "/vscode"], VscodeHttpProvider, args) httpServer.registerHttpProvider("/update", UpdateHttpProvider, false) httpServer.registerHttpProvider("/proxy", ProxyHttpProvider) - httpServer.registerHttpProvider("/login", LoginHttpProvider, args.config!, envPassword) + httpServer.registerHttpProvider("/login", LoginHttpProvider, args.config!, args.usingEnvPassword) httpServer.registerHttpProvider("/static", StaticHttpProvider) httpServer.registerHttpProvider("/healthz", HealthHttpProvider, httpServer.heart) @@ -191,19 +162,18 @@ const main = async (args: Args, configArgs: Args): Promise => { logger.info(`Using config file ${humanPath(args.config)}`) const serverAddress = await httpServer.listen() - logger.info(`HTTP server listening on ${serverAddress}`) + logger.info(`HTTP server listening on ${serverAddress} ${args.link ? "(randomized by --link)" : ""}`) if (args.auth === AuthType.Password) { - if (envPassword) { + if (args.usingEnvPassword) { logger.info(" - Using password from $PASSWORD") } else { logger.info(` - Using password from ${humanPath(args.config)}`) } logger.info(" - To disable use `--auth none`") } else { - logger.info(" - No authentication") + logger.info(` - No authentication ${args.link ? "(disabled by --link)" : ""}`) } - delete process.env.PASSWORD if (httpServer.protocol === "https") { logger.info( @@ -215,9 +185,19 @@ const main = async (args: Args, configArgs: Args): Promise => { logger.info(" - Not serving HTTPS") } - if (httpServer.proxyDomains.size > 0) { - logger.info(` - ${plural(httpServer.proxyDomains.size, "Proxying the following domain")}:`) - httpServer.proxyDomains.forEach((domain) => logger.info(` - *.${domain}`)) + if (args["proxy-domain"].length > 0) { + logger.info(` - ${plural(args["proxy-domain"].length, "Proxying the following domain")}:`) + args["proxy-domain"].forEach((domain) => logger.info(` - *.${domain}`)) + } + + if (args.link) { + try { + await coderCloudBind(serverAddress!, args.link.value) + logger.info(" - Connected to cloud agent") + } catch (err) { + logger.error(err.message) + ipcMain.exit(1) + } } if (serverAddress && !options.socket && args.open) { @@ -228,23 +208,12 @@ const main = async (args: Args, configArgs: Args): Promise => { }) logger.info(`Opened ${openAddress}`) } - - if (args.link) { - try { - await coderCloudBind(serverAddress!, args.link.value) - } catch (err) { - logger.error(err.message) - ipcMain.exit(1) - } - } } async function entry(): Promise { const cliArgs = parse(process.argv.slice(2)) const configArgs = await readConfigFile(cliArgs.config) - // This prioritizes the flags set in args over the ones in the config file. - let args = Object.assign(configArgs, cliArgs) - args = await setDefaults(args) + const args = await setDefaults(cliArgs, configArgs) // There's no need to check flags like --help or to spawn in an existing // instance for the child process because these would have already happened in @@ -252,7 +221,7 @@ async function entry(): Promise { if (ipcMain.isChild) { await ipcMain.handshake() ipcMain.preventExit() - return main(args, configArgs) + return main(args) } if (args.help) { diff --git a/src/node/http.ts b/src/node/http.ts index c616c8837..93d6e725c 100644 --- a/src/node/http.ts +++ b/src/node/http.ts @@ -130,7 +130,7 @@ export interface HttpServerOptions { readonly host?: string readonly password?: string readonly port?: number - readonly proxyDomains?: string[] + readonly proxyDomains: string[] readonly socket?: string } @@ -463,18 +463,12 @@ export class HttpServer { public readonly heart: Heart private readonly socketProvider = new SocketProxyProvider() - /** - * Proxy domains are stored here without the leading `*.` - */ - public readonly proxyDomains: Set - /** * Provides the actual proxying functionality. */ private readonly proxy = proxy.createProxyServer({}) public constructor(private readonly options: HttpServerOptions) { - this.proxyDomains = new Set((options.proxyDomains || []).map((d) => d.replace(/^\*\./, ""))) this.heart = new Heart(path.join(paths.data, "heartbeat"), async () => { const connections = await this.getConnections() logger.trace(plural(connections, `${connections} active connection`)) @@ -892,7 +886,7 @@ export class HttpServer { return undefined } - this.proxyDomains.forEach((domain) => { + this.options.proxyDomains.forEach((domain) => { if (host.endsWith(domain) && domain.length < host.length) { host = domain } @@ -922,7 +916,7 @@ export class HttpServer { // There must be an exact match. const port = parts.shift() const proxyDomain = parts.join(".") - if (!port || !this.proxyDomains.has(proxyDomain)) { + if (!port || !this.options.proxyDomains.includes(proxyDomain)) { return undefined } diff --git a/test/cli.test.ts b/test/cli.test.ts index ae5256142..a59aac995 100644 --- a/test/cli.test.ts +++ b/test/cli.test.ts @@ -14,17 +14,23 @@ type Mutable = { describe("parser", () => { beforeEach(() => { delete process.env.LOG_LEVEL + delete process.env.PASSWORD }) // The parser should not set any defaults so the caller can determine what // values the user actually set. These are only set after explicitly calling // `setDefaults`. const defaults = { + auth: "password", + host: "localhost", + port: 8080, + "proxy-domain": [], + usingEnvPassword: false, "extensions-dir": path.join(paths.data, "extensions"), "user-data-dir": paths.data, } - it("should set defaults", () => { + it("should parse nothing", () => { assert.deepEqual(parse([]), { _: [] }) }) @@ -232,6 +238,71 @@ describe("parser", () => { "proxy-domain": ["*.coder.com", "test.com"], }) }) + + it("should enforce cert-key with cert value or otherwise generate one", async () => { + const args = parse(["--cert"]) + assert.deepEqual(args, { + _: [], + cert: { + value: undefined, + }, + }) + assert.throws(() => parse(["--cert", "test"]), /--cert-key is missing/) + assert.deepEqual(await setDefaults(args), { + _: [], + ...defaults, + cert: { + value: path.join(tmpdir, "self-signed.cert"), + }, + "cert-key": path.join(tmpdir, "self-signed.key"), + }) + }) + + it("should override with --link", async () => { + const args = parse("--cert test --cert-key test --socket test --host 0.0.0.0 --port 8888 --link test".split(" ")) + assert.deepEqual(await setDefaults(args), { + _: [], + ...defaults, + auth: "none", + host: "localhost", + link: { + value: "test", + }, + port: 0, + cert: undefined, + "cert-key": path.resolve("test"), + socket: undefined, + }) + }) + + it("should use env var password", async () => { + process.env.PASSWORD = "test" + const args = parse([]) + assert.deepEqual(args, { + _: [], + }) + + assert.deepEqual(await setDefaults(args), { + ...defaults, + _: [], + password: "test", + usingEnvPassword: true, + }) + }) + + it("should filter proxy domains", async () => { + const args = parse(["--proxy-domain", "*.coder.com", "--proxy-domain", "coder.com", "--proxy-domain", "coder.org"]) + assert.deepEqual(args, { + _: [], + "proxy-domain": ["*.coder.com", "coder.com", "coder.org"], + }) + + assert.deepEqual(await setDefaults(args), { + ...defaults, + _: [], + "proxy-domain": ["coder.com", "coder.org"], + }) + }) }) describe("cli", () => {