From 48174e25002087a4e3f2c72fc41277b17f53bfbb Mon Sep 17 00:00:00 2001 From: Tuan Dang Date: Mon, 9 Dec 2024 22:22:54 -0800 Subject: [PATCH] security + performance improvements to ssh fns --- .../ssh/ssh-certificate-authority-fns.ts | 152 ++++++++++-------- .../ssh/ssh-certificate-authority-service.ts | 14 +- 2 files changed, 88 insertions(+), 78 deletions(-) diff --git a/backend/src/ee/services/ssh/ssh-certificate-authority-fns.ts b/backend/src/ee/services/ssh/ssh-certificate-authority-fns.ts index 82be81be6..cb1549bff 100644 --- a/backend/src/ee/services/ssh/ssh-certificate-authority-fns.ts +++ b/backend/src/ee/services/ssh/ssh-certificate-authority-fns.ts @@ -1,7 +1,10 @@ -import { execSync } from "child_process"; +import { execFile } from "child_process"; import crypto from "crypto"; -import fs from "fs"; +import { promises as fs } from "fs"; import ms from "ms"; +import os from "os"; +import path from "path"; +import { promisify } from "util"; import { TSshCertificateTemplates } from "@app/db/schemas"; import { BadRequestError } from "@app/lib/errors"; @@ -13,6 +16,8 @@ import { } from "../ssh-certificate-template/ssh-certificate-template-validators"; import { SshCertType, TCreateSshCertDTO } from "./ssh-certificate-authority-types"; +const execFileAsync = promisify(execFile); + /* eslint-disable no-bitwise */ export const createSshCertSerialNumber = () => { const randomBytes = crypto.randomBytes(8); // 8 bytes = 64 bits @@ -23,17 +28,17 @@ export const createSshCertSerialNumber = () => { /** * Return a pair of SSH CA keys based on the specified key algorithm [keyAlgorithm]. * We use this function because the key format generated by `ssh-keygen` is unique. + * @param keyAlgorithm - The key algorithm to use for generating the SSH key pair + * @param comment - The comment to use for the SSH key pair + * @returns The public and private keys for the SSH key pair */ -export const createSshKeyPair = (keyAlgorithm: CertKeyAlgorithm, comment: string) => { - const uniqueId = crypto.randomBytes(8).toString("hex"); // to avoid collions if high-volume key generation - const privateKeyFile = `ssh_key_${uniqueId}`; // temp key path +export const createSshKeyPair = async (keyAlgorithm: CertKeyAlgorithm, comment: string) => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "ssh-key-")); + const privateKeyFile = path.join(tempDir, "id_key"); const publicKeyFile = `${privateKeyFile}.pub`; - if (fs.existsSync(publicKeyFile)) fs.unlinkSync(publicKeyFile); - if (fs.existsSync(privateKeyFile)) fs.unlinkSync(privateKeyFile); - - let keyType = ""; - let keyBits = ""; + let keyType: string; + let keyBits: string; switch (keyAlgorithm) { case CertKeyAlgorithm.RSA_2048: @@ -58,41 +63,40 @@ export const createSshKeyPair = (keyAlgorithm: CertKeyAlgorithm, comment: string }); } - execSync(`ssh-keygen -t ${keyType} -b ${keyBits} -f ${privateKeyFile} -N '' -C "${comment}"`); + try { + // Generate the SSH key pair + // The "-N ''" sets an empty passphrase + // The keys are created in the temporary directory + await execFileAsync("ssh-keygen", ["-t", keyType, "-b", keyBits, "-f", privateKeyFile, "-N", "", "-C", comment]); - const publicKey = fs.readFileSync(publicKeyFile, "utf8"); - const privateKey = fs.readFileSync(privateKeyFile, "utf8"); + // Read the generated keys + const publicKey = await fs.readFile(publicKeyFile, "utf8"); + const privateKey = await fs.readFile(privateKeyFile, "utf8"); - fs.unlinkSync(privateKeyFile); - fs.unlinkSync(publicKeyFile); - - return { publicKey, privateKey }; + return { publicKey, privateKey }; + } finally { + // Cleanup the temporary directory and all its contents + await fs.rm(tempDir, { recursive: true, force: true }).catch(() => {}); + } }; /** * Return the SSH public key for the given SSH private key. * @param privateKey - The SSH private key to get the public key for */ -export const getSshPublicKey = (privateKey: string) => { - const uniqueId = crypto.randomBytes(8).toString("hex"); - const privateKeyFile = `ssh_key_${uniqueId}`; - const publicKeyFile = `${privateKeyFile}.pub`; +export const getSshPublicKey = async (privateKey: string) => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "ssh-key-")); + const privateKeyFile = path.join(tempDir, "id_key"); + try { + await fs.writeFile(privateKeyFile, privateKey, { mode: 0o600 }); - if (fs.existsSync(publicKeyFile)) fs.unlinkSync(publicKeyFile); - if (fs.existsSync(privateKeyFile)) fs.unlinkSync(privateKeyFile); - - fs.writeFileSync(privateKeyFile, privateKey); - fs.chmodSync(privateKeyFile, 0o600); - - const command = `ssh-keygen -y -f ${privateKeyFile} > ${publicKeyFile}`; - execSync(command); - - const publicKey = fs.readFileSync(publicKeyFile, "utf8"); - - fs.unlinkSync(privateKeyFile); - fs.unlinkSync(publicKeyFile); - - return publicKey; + // Run ssh-keygen to extract the public key + const { stdout } = await execFileAsync("ssh-keygen", ["-y", "-f", privateKeyFile], { encoding: "utf8" }); + return stdout.trim(); + } finally { + // Ensure that files and the temporary directory are cleaned up + await fs.rm(tempDir, { recursive: true, force: true }).catch(() => {}); + } }; /** @@ -220,47 +224,53 @@ export const validateSshCertificateTtl = (template: TSshCertificateTemplates, tt /** * Create an SSH certificate for a user or host. */ -export const createSshCert = ({ caPrivateKey, userPublicKey, keyId, principals, ttl, certType }: TCreateSshCertDTO) => { - const uniqueId = crypto.randomBytes(8).toString("hex"); - const publicKeyFile = `user_key_${uniqueId}.pub`; - const privateKeyFile = `ssh_ca_key_${uniqueId}`; - const signedPublicKeyFile = `user_key_${uniqueId}-cert.pub`; +export const createSshCert = async ({ + caPrivateKey, + userPublicKey, + keyId, + principals, + ttl, + certType +}: TCreateSshCertDTO) => { + const tempDir = await fs.mkdtemp(path.join(os.tmpdir(), "ssh-cert-")); - if (fs.existsSync(publicKeyFile)) fs.unlinkSync(publicKeyFile); - if (fs.existsSync(privateKeyFile)) fs.unlinkSync(privateKeyFile); - if (fs.existsSync(signedPublicKeyFile)) fs.unlinkSync(signedPublicKeyFile); - - // write public and private keys to temp files - fs.writeFileSync(publicKeyFile, userPublicKey); - fs.writeFileSync(privateKeyFile, caPrivateKey); - fs.chmodSync(privateKeyFile, 0o600); + const publicKeyFile = path.join(tempDir, "user_key.pub"); + const privateKeyFile = path.join(tempDir, "ca_key"); + const signedPublicKeyFile = path.join(tempDir, "user_key-cert.pub"); const serialNumber = createSshCertSerialNumber(); - const certOptions = [ - `-s ${privateKeyFile}`, // path to SSH CA private key - `-I "${keyId}"`, // identity for the issued certificate (key id) - `-n "${principals.join(",")}"`, // principal(s) that is user(s) or host(s) - `-V +${ttl}s`, // TTL in seconds (validity period) for the issue certificate - `-z ${serialNumber}`, // custom serial number for certificate - certType === "host" ? "-h" : "", // host certificate flag - publicKeyFile // path to signed [publicKey] - ] - .filter(Boolean) - .join(" "); + // Build `ssh-keygen` arguments for signing + // Using an array avoids shell injection issues + const sshKeygenArgs = [ + certType === "host" ? "-h" : null, // host certificate if needed + "-s", + privateKeyFile, // path to SSH CA private key + "-I", + keyId, // identity (key ID) + "-n", + principals.join(","), // principals + "-V", + `+${ttl}s`, // validity (TTL in seconds) + "-z", + serialNumber, // serial number + publicKeyFile // public key file to sign + ].filter(Boolean) as string[]; - const command = `ssh-keygen ${certOptions}`; + try { + // Write public and private keys to the temp directory + await fs.writeFile(publicKeyFile, userPublicKey, { mode: 0o600 }); + await fs.writeFile(privateKeyFile, caPrivateKey, { mode: 0o600 }); - console.log("executing command", command); + // Execute the signing process + await execFileAsync("ssh-keygen", sshKeygenArgs, { encoding: "utf8" }); - // Execute the signing process - execSync(command); + // Read the signed public key from the generated cert file + const signedPublicKey = await fs.readFile(signedPublicKeyFile, "utf8"); - const signedPublicKey = fs.readFileSync(signedPublicKeyFile, "utf8"); - - fs.unlinkSync(publicKeyFile); - fs.unlinkSync(privateKeyFile); - fs.unlinkSync(signedPublicKeyFile); - - return { serialNumber, signedPublicKey }; + return { serialNumber, signedPublicKey }; + } finally { + // Cleanup the temporary directory and all its contents + await fs.rm(tempDir, { recursive: true, force: true }).catch(() => {}); + } }; diff --git a/backend/src/ee/services/ssh/ssh-certificate-authority-service.ts b/backend/src/ee/services/ssh/ssh-certificate-authority-service.ts index 2844407b2..145c35353 100644 --- a/backend/src/ee/services/ssh/ssh-certificate-authority-service.ts +++ b/backend/src/ee/services/ssh/ssh-certificate-authority-service.ts @@ -97,7 +97,7 @@ export const sshCertificateAuthorityServiceFactory = ({ tx ); - const { publicKey, privateKey } = createSshKeyPair(keyAlgorithm, ca.friendlyName); + const { publicKey, privateKey } = await createSshKeyPair(keyAlgorithm, ca.friendlyName); // TODO: update to sshEncryptor const { encryptor: secretManagerEncryptor } = await kmsService.createCipherPairWithDataKey({ @@ -151,7 +151,7 @@ export const sshCertificateAuthorityServiceFactory = ({ cipherTextBlob: sshCaSecret.encryptedPrivateKey }); - const publicKey = getSshPublicKey(decryptedCaPrivateKey.toString("utf-8")); + const publicKey = await getSshPublicKey(decryptedCaPrivateKey.toString("utf-8")); return { ...ca, publicKey }; }; @@ -175,7 +175,7 @@ export const sshCertificateAuthorityServiceFactory = ({ cipherTextBlob: sshCaSecret.encryptedPrivateKey }); - const publicKey = getSshPublicKey(decryptedCaPrivateKey.toString("utf-8")); + const publicKey = await getSshPublicKey(decryptedCaPrivateKey.toString("utf-8")); return publicKey; }; @@ -223,7 +223,7 @@ export const sshCertificateAuthorityServiceFactory = ({ cipherTextBlob: sshCaSecret.encryptedPrivateKey }); - const publicKey = getSshPublicKey(decryptedCaPrivateKey.toString("utf-8")); + const publicKey = await getSshPublicKey(decryptedCaPrivateKey.toString("utf-8")); return { ...updatedCa, publicKey }; }; @@ -329,9 +329,9 @@ export const sshCertificateAuthorityServiceFactory = ({ }); // create user key pair - const { publicKey, privateKey } = createSshKeyPair(keyAlgorithm, "Client Key"); + const { publicKey, privateKey } = await createSshKeyPair(keyAlgorithm, "Client Key"); - const { serialNumber, signedPublicKey } = createSshCert({ + const { serialNumber, signedPublicKey } = await createSshCert({ caPrivateKey: decryptedCaPrivateKey.toString("utf8"), userPublicKey: publicKey, keyId, @@ -460,7 +460,7 @@ export const sshCertificateAuthorityServiceFactory = ({ cipherTextBlob: sshCaSecret.encryptedPrivateKey }); - const { serialNumber, signedPublicKey } = createSshCert({ + const { serialNumber, signedPublicKey } = await createSshCert({ caPrivateKey: decryptedCaPrivateKey.toString("utf8"), userPublicKey: publicKey, keyId,