From 763ea094520fad96b64e35190aa18e5dcbf7fd59 Mon Sep 17 00:00:00 2001 From: Asher Date: Wed, 9 Oct 2024 15:01:32 -0800 Subject: [PATCH 1/4] Do not require token auth with mTLS --- package.json | 6 +-- src/api.ts | 24 +++++++++ src/commands.ts | 136 +++++++++++++++++++++++++++-------------------- src/extension.ts | 10 ++-- src/storage.ts | 10 ++-- 5 files changed, 117 insertions(+), 69 deletions(-) diff --git a/package.json b/package.json index bf7687cc..7b64c133 100644 --- a/package.json +++ b/package.json @@ -74,17 +74,17 @@ "default": "" }, "coder.tlsCertFile": { - "markdownDescription": "Path to file for TLS client cert", + "markdownDescription": "Path to file for TLS client cert. When specified, token authorization will be skipped.", "type": "string", "default": "" }, "coder.tlsKeyFile": { - "markdownDescription": "Path to file for TLS client key", + "markdownDescription": "Path to file for TLS client key. When specified, token authorization will be skipped.", "type": "string", "default": "" }, "coder.tlsCaFile": { - "markdownDescription": "Path to file for TLS certificate authority", + "markdownDescription": "Path to file for TLS certificate authority.", "type": "string", "default": "" }, diff --git a/src/api.ts b/src/api.ts index ebdb8563..e784ccce 100644 --- a/src/api.ts +++ b/src/api.ts @@ -10,6 +10,21 @@ import { getProxyForUrl } from "./proxy" import { Storage } from "./storage" import { expandPath } from "./util" +/** + * Return whether the API will need a token for authorization. + * If mTLS is in use (as specified by the cert or key files being set) then + * token authorization is disabled. Otherwise, it is enabled. + */ +export function needToken(): boolean { + const cfg = vscode.workspace.getConfiguration() + const certFile = expandPath(String(cfg.get("coder.tlsCertFile") ?? "").trim()) + const keyFile = expandPath(String(cfg.get("coder.tlsKeyFile") ?? "").trim()) + return !certFile && !keyFile +} + +/** + * Create a new agent based off the current settings. + */ async function createHttpAgent(): Promise { const cfg = vscode.workspace.getConfiguration() const insecure = Boolean(cfg.get("coder.insecure")) @@ -32,7 +47,16 @@ async function createHttpAgent(): Promise { }) } +// The agent is a singleton so we only have to listen to the configuration once +// (otherwise we would have to carefully dispose agents to remove their +// configuration listeners), and to share the connection pool. let agent: Promise | undefined = undefined + +/** + * Get the existing agent or create one if necessary. On settings change, + * recreate the agent. The agent on the client is not automatically updated; + * this must be called before every request to get the latest agent. + */ async function getHttpAgent(): Promise { if (!agent) { vscode.workspace.onDidChangeConfiguration((e) => { diff --git a/src/commands.ts b/src/commands.ts index f2956d29..23e30977 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -2,7 +2,7 @@ import { Api } from "coder/site/src/api/api" import { getErrorMessage } from "coder/site/src/api/errors" import { User, Workspace, WorkspaceAgent } from "coder/site/src/api/typesGenerated" import * as vscode from "vscode" -import { makeCoderSdk } from "./api" +import { makeCoderSdk, needToken } from "./api" import { extractAgents } from "./api-helper" import { CertificateError } from "./error" import { Storage } from "./storage" @@ -147,78 +147,33 @@ export class Commands { // a host label. const label = typeof args[2] === "undefined" ? toSafeHost(url) : args[2] - // Use a temporary client to avoid messing with the global one while trying - // to log in. - const restClient = await makeCoderSdk(url, undefined, this.storage) - - let user: User | undefined - let token: string | undefined = args[1] - if (!token) { - const opened = await vscode.env.openExternal(vscode.Uri.parse(`${url}/cli-auth`)) - if (!opened) { - vscode.window.showWarningMessage("You must accept the URL prompt to generate an API key.") - return - } - - token = await vscode.window.showInputBox({ - title: "Coder API Key", - password: true, - placeHolder: "Copy your API key from the opened browser page.", - value: await this.storage.getSessionToken(), - ignoreFocusOut: true, - validateInput: async (value) => { - restClient.setSessionToken(value) - try { - user = await restClient.getAuthenticatedUser() - if (!user) { - throw new Error("Failed to get authenticated user") - } - } catch (err) { - // For certificate errors show both a notification and add to the - // text under the input box, since users sometimes miss the - // notification. - if (err instanceof CertificateError) { - err.showNotification() - - return { - message: err.x509Err || err.message, - severity: vscode.InputBoxValidationSeverity.Error, - } - } - // This could be something like the header command erroring or an - // invalid session token. - const message = getErrorMessage(err, "no response from the server") - return { - message: "Failed to authenticate: " + message, - severity: vscode.InputBoxValidationSeverity.Error, - } - } - }, - }) - } - if (!token || !user) { - return + // Try to get a token from the user, if we need one, and their user. + const res = await this.maybeAskToken(url, args[1]) + if (!res) { + return // The user aborted. } - // The URL and token are good; authenticate the global client. + // The URL is good and the token is either good or not required; authorize + // the global client. this.restClient.setHost(url) - this.restClient.setSessionToken(token) + this.restClient.setSessionToken(res.token) // Store these to be used in later sessions. await this.storage.setUrl(url) - await this.storage.setSessionToken(token) + await this.storage.setSessionToken(res.token) // Store on disk to be used by the cli. - await this.storage.configureCli(label, url, token) + await this.storage.configureCli(label, url, res.token) + // These contexts control various menu items and the sidebar. await vscode.commands.executeCommand("setContext", "coder.authenticated", true) - if (user.roles.find((role) => role.name === "owner")) { + if (res.user.roles.find((role) => role.name === "owner")) { await vscode.commands.executeCommand("setContext", "coder.isOwner", true) } vscode.window .showInformationMessage( - `Welcome to Coder, ${user.username}!`, + `Welcome to Coder, ${res.user.username}!`, { detail: "You can now use the Coder extension to manage your Coder instance.", }, @@ -234,6 +189,71 @@ export class Commands { vscode.commands.executeCommand("coder.refreshWorkspaces") } + /** + * If necessary, ask for a token, and keep asking until the token has been + * validated. Return the token and user that was fetched to validate the + * token. + */ + private async maybeAskToken(url: string, token: string): Promise<{ user: User; token: string } | null> { + const restClient = await makeCoderSdk(url, token, this.storage) + if (!needToken()) { + return { + // For non-token auth, we write a blank token since the `vscodessh` + // command currently always requires a token file. + token: "", + user: await restClient.getAuthenticatedUser(), + } + } + + // This prompt is for convenience; do not error if they close it since + // they may already have a token or already have the page opened. + await vscode.env.openExternal(vscode.Uri.parse(`${url}/cli-auth`)) + + // For token auth, start with the existing token in the prompt or the last + // used token. Once submitted, if there is a failure we will keep asking + // the user for a new token until they quit. + let user: User | undefined + const validatedToken = await vscode.window.showInputBox({ + title: "Coder API Key", + password: true, + placeHolder: "Paste your API key.", + value: token || (await this.storage.getSessionToken()), + ignoreFocusOut: true, + validateInput: async (value) => { + restClient.setSessionToken(value) + try { + user = await restClient.getAuthenticatedUser() + } catch (err) { + // For certificate errors show both a notification and add to the + // text under the input box, since users sometimes miss the + // notification. + if (err instanceof CertificateError) { + err.showNotification() + + return { + message: err.x509Err || err.message, + severity: vscode.InputBoxValidationSeverity.Error, + } + } + // This could be something like the header command erroring or an + // invalid session token. + const message = getErrorMessage(err, "no response from the server") + return { + message: "Failed to authenticate: " + message, + severity: vscode.InputBoxValidationSeverity.Error, + } + } + }, + }) + + if (validatedToken && user) { + return { token: validatedToken, user } + } + + // User aborted. + return null + } + /** * View the logs for the currently connected workspace. */ diff --git a/src/extension.ts b/src/extension.ts index 2f5c23a1..97e3a865 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -3,7 +3,7 @@ import axios, { isAxiosError } from "axios" import { getErrorMessage } from "coder/site/src/api/errors" import * as module from "module" import * as vscode from "vscode" -import { makeCoderSdk } from "./api" +import { makeCoderSdk, needToken } from "./api" import { errToStr } from "./api-helper" import { Commands } from "./commands" import { CertificateError, getErrorDetail } from "./error" @@ -92,8 +92,12 @@ export async function activate(ctx: vscode.ExtensionContext): Promise { } // If the token is missing we will get a 401 later and the user will be - // prompted to sign in again, so we do not need to ensure it is set. - const token = params.get("token") + // prompted to sign in again, so we do not need to ensure it is set now. + // For non-token auth, we write a blank token since the `vscodessh` + // command currently always requires a token file. However, if there is + // a query parameter for non-token auth go ahead and use it anyway; all + // that really matters is the file is created. + const token = needToken() ? params.get("token") : (params.get("token") ?? "") if (token) { restClient.setSessionToken(token) await storage.setSessionToken(token) diff --git a/src/storage.ts b/src/storage.ts index 4d6396db..e0230a32 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -435,8 +435,8 @@ export class Storage { /** * Configure the CLI for the deployment with the provided label. * - * Falsey values are a no-op; we avoid unconfiguring the CLI to avoid breaking - * existing connections. + * Falsey URLs and null tokens are a no-op; we avoid unconfiguring the CLI to + * avoid breaking existing connections. */ public async configureCli(label: string, url: string | undefined, token: string | undefined | null) { await Promise.all([this.updateUrlForCli(label, url), this.updateTokenForCli(label, token)]) @@ -459,15 +459,15 @@ export class Storage { /** * Update the session token for a deployment with the provided label on disk * which can be used by the CLI via --session-token-file. If the token is - * falsey, do nothing. + * null, do nothing. * * If the label is empty, read the old deployment-unaware config instead. */ private async updateTokenForCli(label: string, token: string | undefined | null) { - if (token) { + if (token !== null) { const tokenPath = this.getSessionTokenPath(label) await fs.mkdir(path.dirname(tokenPath), { recursive: true }) - await fs.writeFile(tokenPath, token) + await fs.writeFile(tokenPath, token ?? "") } } From f6769a3d98a93da154eaa244286d283310bcf5c3 Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 29 Oct 2024 11:19:25 -0800 Subject: [PATCH 2/4] Name login arguments --- src/commands.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/commands.ts b/src/commands.ts index 23e30977..a5bd3eb6 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -136,8 +136,8 @@ export class Commands { * Log into the provided deployment. If the deployment URL is not specified, * ask for it first with a menu showing recent URLs and CODER_URL, if set. */ - public async login(...args: string[]): Promise { - const url = await this.maybeAskUrl(args[0]) + public async login([inputUrl, inputLabel, inputToken]: string[]): Promise { + const url = await this.maybeAskUrl(inputUrl) if (!url) { return } @@ -145,10 +145,10 @@ export class Commands { // It is possible that we are trying to log into an old-style host, in which // case we want to write with the provided blank label instead of generating // a host label. - const label = typeof args[2] === "undefined" ? toSafeHost(url) : args[2] + const label = typeof inputLabel === "undefined" ? toSafeHost(url) : inputLabel // Try to get a token from the user, if we need one, and their user. - const res = await this.maybeAskToken(url, args[1]) + const res = await this.maybeAskToken(url, inputToken) if (!res) { return // The user aborted. } From 5809801e22d2c524957a17fe9b8bb928c387468d Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 29 Oct 2024 11:24:33 -0800 Subject: [PATCH 3/4] No undefined token please --- src/storage.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/storage.ts b/src/storage.ts index e0230a32..57d0b5a4 100644 --- a/src/storage.ts +++ b/src/storage.ts @@ -438,7 +438,7 @@ export class Storage { * Falsey URLs and null tokens are a no-op; we avoid unconfiguring the CLI to * avoid breaking existing connections. */ - public async configureCli(label: string, url: string | undefined, token: string | undefined | null) { + public async configureCli(label: string, url: string | undefined, token: string | null) { await Promise.all([this.updateUrlForCli(label, url), this.updateTokenForCli(label, token)]) } From 22714483138254541b152fda8a086c486f812c4d Mon Sep 17 00:00:00 2001 From: Asher Date: Tue, 29 Oct 2024 11:29:11 -0800 Subject: [PATCH 4/4] Cannot use destructure --- src/commands.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/commands.ts b/src/commands.ts index a5bd3eb6..a88e763a 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -136,7 +136,12 @@ export class Commands { * Log into the provided deployment. If the deployment URL is not specified, * ask for it first with a menu showing recent URLs and CODER_URL, if set. */ - public async login([inputUrl, inputLabel, inputToken]: string[]): Promise { + public async login(...args: string[]): Promise { + // Destructure would be nice but VS Code can pass undefined which errors. + const inputUrl = args[0] + const inputToken = args[1] + const inputLabel = args[2] + const url = await this.maybeAskUrl(inputUrl) if (!url) { return