From 694ab35f53b3367f2d06a193909a7becffa7377d Mon Sep 17 00:00:00 2001 From: Maidul Islam Date: Mon, 28 Apr 2025 07:48:31 -0400 Subject: [PATCH 1/3] Fix KMS memory leak Adds a clean up method because KMS clients like GCP use a persistent connection snd if not closed, will continue to eat up the memory. --- .../external-kms/external-kms-service.ts | 42 ++++++++++++++----- .../external-kms/providers/aws-kms.ts | 15 ++++++- .../external-kms/providers/gcp-kms.ts | 13 +++++- .../services/external-kms/providers/model.ts | 1 + .../src/services/auth/auth-login-service.ts | 2 +- backend/src/services/kms/kms-service.ts | 18 +++++--- 6 files changed, 71 insertions(+), 20 deletions(-) diff --git a/backend/src/ee/services/external-kms/external-kms-service.ts b/backend/src/ee/services/external-kms/external-kms-service.ts index 49ac293ed..4d7b1a5b5 100644 --- a/backend/src/ee/services/external-kms/external-kms-service.ts +++ b/backend/src/ee/services/external-kms/external-kms-service.ts @@ -83,18 +83,26 @@ export const externalKmsServiceFactory = ({ throw error; }); - // if missing kms key this generate a new kms key id and returns new provider input - const newProviderInput = await externalKms.generateInputKmsKey(); - sanitizedProviderInput = JSON.stringify(newProviderInput); + try { + // if missing kms key this generate a new kms key id and returns new provider input + const newProviderInput = await externalKms.generateInputKmsKey(); + sanitizedProviderInput = JSON.stringify(newProviderInput); - await externalKms.validateConnection(); + await externalKms.validateConnection(); + } finally { + await externalKms.cleanup(); + } } break; case KmsProviders.Gcp: { const externalKms = await GcpKmsProviderFactory({ inputs: provider.inputs }); - await externalKms.validateConnection(); - sanitizedProviderInput = JSON.stringify(provider.inputs); + try { + await externalKms.validateConnection(); + sanitizedProviderInput = JSON.stringify(provider.inputs); + } finally { + await externalKms.cleanup(); + } } break; default: @@ -186,8 +194,12 @@ export const externalKmsServiceFactory = ({ ); const updatedProviderInput = { ...decryptedProviderInput, ...provider.inputs }; const externalKms = await AwsKmsProviderFactory({ inputs: updatedProviderInput }); - await externalKms.validateConnection(); - sanitizedProviderInput = JSON.stringify(updatedProviderInput); + try { + await externalKms.validateConnection(); + sanitizedProviderInput = JSON.stringify(updatedProviderInput); + } finally { + await externalKms.cleanup(); + } } break; case KmsProviders.Gcp: @@ -197,8 +209,12 @@ export const externalKmsServiceFactory = ({ ); const updatedProviderInput = { ...decryptedProviderInput, ...provider.inputs }; const externalKms = await GcpKmsProviderFactory({ inputs: updatedProviderInput }); - await externalKms.validateConnection(); - sanitizedProviderInput = JSON.stringify(updatedProviderInput); + try { + await externalKms.validateConnection(); + sanitizedProviderInput = JSON.stringify(updatedProviderInput); + } finally { + await externalKms.cleanup(); + } } break; default: @@ -368,7 +384,11 @@ export const externalKmsServiceFactory = ({ const fetchGcpKeys = async ({ credential, gcpRegion }: Pick) => { const externalKms = await GcpKmsProviderFactory({ inputs: { credential, gcpRegion, keyName: "" } }); - return externalKms.getKeysList(); + try { + return await externalKms.getKeysList(); + } finally { + await externalKms.cleanup(); + } }; return { diff --git a/backend/src/ee/services/external-kms/providers/aws-kms.ts b/backend/src/ee/services/external-kms/providers/aws-kms.ts index 6d9166a3a..862239042 100644 --- a/backend/src/ee/services/external-kms/providers/aws-kms.ts +++ b/backend/src/ee/services/external-kms/providers/aws-kms.ts @@ -2,6 +2,8 @@ import { CreateKeyCommand, DecryptCommand, DescribeKeyCommand, EncryptCommand, K import { AssumeRoleCommand, STSClient } from "@aws-sdk/client-sts"; import { randomUUID } from "crypto"; +import { logger } from "@app/lib/logger"; + import { ExternalKmsAwsSchema, KmsAwsCredentialType, TExternalKmsAwsSchema, TExternalKmsProviderFns } from "./model"; const getAwsKmsClient = async (providerInputs: TExternalKmsAwsSchema) => { @@ -102,10 +104,21 @@ export const AwsKmsProviderFactory = async ({ inputs }: AwsKmsProviderArgs): Pro return { data: Buffer.from(decryptionCommand.Plaintext) }; }; + const cleanup = async () => { + try { + awsClient.destroy(); + return true; + } catch (error) { + logger.error(error, "cleanup: failed to destroy AWS KMS client"); + return false; + } + }; + return { generateInputKmsKey, validateConnection, encrypt, - decrypt + decrypt, + cleanup }; }; diff --git a/backend/src/ee/services/external-kms/providers/gcp-kms.ts b/backend/src/ee/services/external-kms/providers/gcp-kms.ts index bee1eb24b..5abe0afc7 100644 --- a/backend/src/ee/services/external-kms/providers/gcp-kms.ts +++ b/backend/src/ee/services/external-kms/providers/gcp-kms.ts @@ -45,6 +45,16 @@ export const GcpKmsProviderFactory = async ({ inputs }: GcpKmsProviderArgs): Pro } }; + const cleanup = async () => { + try { + await gcpKmsClient.close(); + return true; + } catch (error) { + logger.error(error, "cleanup: failed to close GCP KMS client"); + return false; + } + }; + // Used when adding the KMS to fetch the list of keys in specified region const getKeysList = async () => { try { @@ -108,6 +118,7 @@ export const GcpKmsProviderFactory = async ({ inputs }: GcpKmsProviderArgs): Pro validateConnection, getKeysList, encrypt, - decrypt + decrypt, + cleanup }; }; diff --git a/backend/src/ee/services/external-kms/providers/model.ts b/backend/src/ee/services/external-kms/providers/model.ts index 436b39423..71f108dbc 100644 --- a/backend/src/ee/services/external-kms/providers/model.ts +++ b/backend/src/ee/services/external-kms/providers/model.ts @@ -98,4 +98,5 @@ export type TExternalKmsProviderFns = { validateConnection: () => Promise; encrypt: (data: Buffer) => Promise<{ encryptedBlob: Buffer }>; decrypt: (encryptedBlob: Buffer) => Promise<{ data: Buffer }>; + cleanup: () => Promise; }; diff --git a/backend/src/services/auth/auth-login-service.ts b/backend/src/services/auth/auth-login-service.ts index e576d6768..bc9c4afa3 100644 --- a/backend/src/services/auth/auth-login-service.ts +++ b/backend/src/services/auth/auth-login-service.ts @@ -12,6 +12,7 @@ import { generateSrpServerKey, srpCheckClientProof } from "@app/lib/crypto"; import { infisicalSymmetricEncypt } from "@app/lib/crypto/encryption"; import { getUserPrivateKey } from "@app/lib/crypto/srp"; import { BadRequestError, DatabaseError, ForbiddenRequestError, UnauthorizedError } from "@app/lib/errors"; +import { removeTrailingSlash } from "@app/lib/fn"; import { logger } from "@app/lib/logger"; import { getUserAgentType } from "@app/server/plugins/audit-log"; import { getServerCfg } from "@app/services/super-admin/super-admin-service"; @@ -39,7 +40,6 @@ import { AuthTokenType, MfaMethod } from "./auth-type"; -import { removeTrailingSlash } from "@app/lib/fn"; type TAuthLoginServiceFactoryDep = { userDAL: TUserDALFactory; diff --git a/backend/src/services/kms/kms-service.ts b/backend/src/services/kms/kms-service.ts index 07ed90bef..8c23ce547 100644 --- a/backend/src/services/kms/kms-service.ts +++ b/backend/src/services/kms/kms-service.ts @@ -342,9 +342,12 @@ export const kmsServiceFactory = ({ } return async ({ cipherTextBlob }: Pick) => { - const { data } = await externalKms.decrypt(cipherTextBlob); - - return data; + try { + const { data } = await externalKms.decrypt(cipherTextBlob); + return data; + } finally { + await externalKms.cleanup(); + } }; } @@ -557,9 +560,12 @@ export const kmsServiceFactory = ({ } return async ({ plainText }: Pick) => { - const { encryptedBlob } = await externalKms.encrypt(plainText); - - return { cipherTextBlob: encryptedBlob }; + try { + const { encryptedBlob } = await externalKms.encrypt(plainText); + return { cipherTextBlob: encryptedBlob }; + } finally { + await externalKms.cleanup(); + } }; } From 1dfc9511c1fedb73ca6754ed289b7f4668967fdd Mon Sep 17 00:00:00 2001 From: Maidul Islam Date: Mon, 28 Apr 2025 07:55:33 -0400 Subject: [PATCH 2/3] throw only error and remove bool return --- backend/src/ee/services/external-kms/providers/aws-kms.ts | 4 +--- backend/src/ee/services/external-kms/providers/gcp-kms.ts | 4 +--- backend/src/ee/services/external-kms/providers/model.ts | 2 +- 3 files changed, 3 insertions(+), 7 deletions(-) diff --git a/backend/src/ee/services/external-kms/providers/aws-kms.ts b/backend/src/ee/services/external-kms/providers/aws-kms.ts index 862239042..fb4d14b9f 100644 --- a/backend/src/ee/services/external-kms/providers/aws-kms.ts +++ b/backend/src/ee/services/external-kms/providers/aws-kms.ts @@ -107,10 +107,8 @@ export const AwsKmsProviderFactory = async ({ inputs }: AwsKmsProviderArgs): Pro const cleanup = async () => { try { awsClient.destroy(); - return true; } catch (error) { - logger.error(error, "cleanup: failed to destroy AWS KMS client"); - return false; + throw new Error("Failed to cleanup AWS KMS client", { cause: error }); } }; diff --git a/backend/src/ee/services/external-kms/providers/gcp-kms.ts b/backend/src/ee/services/external-kms/providers/gcp-kms.ts index 5abe0afc7..ff2820fe8 100644 --- a/backend/src/ee/services/external-kms/providers/gcp-kms.ts +++ b/backend/src/ee/services/external-kms/providers/gcp-kms.ts @@ -48,10 +48,8 @@ export const GcpKmsProviderFactory = async ({ inputs }: GcpKmsProviderArgs): Pro const cleanup = async () => { try { await gcpKmsClient.close(); - return true; } catch (error) { - logger.error(error, "cleanup: failed to close GCP KMS client"); - return false; + throw new Error("Failed to cleanup GCP KMS client", { cause: error }); } }; diff --git a/backend/src/ee/services/external-kms/providers/model.ts b/backend/src/ee/services/external-kms/providers/model.ts index 71f108dbc..6cb78a34e 100644 --- a/backend/src/ee/services/external-kms/providers/model.ts +++ b/backend/src/ee/services/external-kms/providers/model.ts @@ -98,5 +98,5 @@ export type TExternalKmsProviderFns = { validateConnection: () => Promise; encrypt: (data: Buffer) => Promise<{ encryptedBlob: Buffer }>; decrypt: (encryptedBlob: Buffer) => Promise<{ data: Buffer }>; - cleanup: () => Promise; + cleanup: () => Promise; }; From ca9825c1fe1a908bc62ec1b1c179807af86826e7 Mon Sep 17 00:00:00 2001 From: Maidul Islam Date: Mon, 28 Apr 2025 07:59:00 -0400 Subject: [PATCH 3/3] remove unused logger --- backend/src/ee/services/external-kms/providers/aws-kms.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/backend/src/ee/services/external-kms/providers/aws-kms.ts b/backend/src/ee/services/external-kms/providers/aws-kms.ts index fb4d14b9f..2bda9c75e 100644 --- a/backend/src/ee/services/external-kms/providers/aws-kms.ts +++ b/backend/src/ee/services/external-kms/providers/aws-kms.ts @@ -2,8 +2,6 @@ import { CreateKeyCommand, DecryptCommand, DescribeKeyCommand, EncryptCommand, K import { AssumeRoleCommand, STSClient } from "@aws-sdk/client-sts"; import { randomUUID } from "crypto"; -import { logger } from "@app/lib/logger"; - import { ExternalKmsAwsSchema, KmsAwsCredentialType, TExternalKmsAwsSchema, TExternalKmsProviderFns } from "./model"; const getAwsKmsClient = async (providerInputs: TExternalKmsAwsSchema) => {