security + performance improvements to ssh fns

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(() => {});
+  }
 };

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,