Root encrypted data to kms encryption #2827

akhilmhdh · 2024-12-01T17:43:30Z

Description 📣

This PR adds migration to remove all directly root encrypted schemas with our new kms architecture. This also allows migrations to use kms features.

All the unused fields are kept nullable and not dropped. Will be doing a rolling migration for this to remove it later.

Type ✨

Tests 🛠️

# Here's some code block to paste some code snippets

I have read the contributing guide, agreed and acknowledged the code of conduct. 📝

gitguardian · 2024-12-01T17:44:08Z

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

maidul98

Left some small comments. For manual testing i tried with:

ladap config [good]
dynamic secret with pg [seems to save and all but says the password is wrong?]
secret rotation [seems to save and all but says the password is wrong?]
webhooks [good]

Also what about integrations and are there any other items we are missing on this pass? What about the server admin slack secrets?

maidul98 · 2024-12-04T02:19:00Z

backend/src/db/migrations/utils/env-config.ts

+    DB_USER: zpStr(z.string().describe("Postgres database username").optional()),
+    DB_PASSWORD: zpStr(z.string().describe("Postgres database password").optional()),
+    DB_NAME: zpStr(z.string().describe("Postgres database name").optional()),
+    // TODO(akhilmhdh): will be changed to one


Q: change what?

Comment from previous config. It means the Encryption key and Root encryption key will become one key

maidul98 · 2024-12-04T02:25:38Z

backend/src/db/migrations/utils/env-config.ts

+    ENCRYPTION_KEY: zpStr(z.string().optional()),
+    ROOT_ENCRYPTION_KEY: zpStr(z.string().optional()),
+    // HSM
+    HSM_LIB_PATH: zpStr(z.string().optional()),


let's say the hsm was set up for this instance but the user didn't pass in the hsm params, then what wil happen? I think we should check if this instance has hsm configured and tell them these are required

The HSM service handles it already.

maidul98 · 2024-12-10T00:05:31Z

backend/src/db/schemas/ldap-configs.ts

  createdAt: z.date(),
  updatedAt: z.date(),
  groupSearchBase: z.string().default(""),
  groupSearchFilter: z.string().default(""),
  searchFilter: z.string().default(""),
-  uniqueUserAttribute: z.string().default("")
+  uniqueUserAttribute: z.string().default(""),
+  encryptedLdapBindDN: zodBuffer,


nit: in the other auth methods, you don't refer to the auth method in the field but for this one you do. to keep it consistent, maybe omit the ldap for these fields?

It's added because the field is already used. There exist a field encryptedBindDN.

maidul98 · 2024-12-10T00:08:57Z

backend/src/db/schemas/webhooks.ts

@@ -25,7 +27,9 @@ export const WebhooksSchema = z.object({
  urlCipherText: z.string().nullable().optional(),
  urlIV: z.string().nullable().optional(),
  urlTag: z.string().nullable().optional(),
-  type: z.string().default("general").nullable().optional()
+  type: z.string().default("general").nullable().optional(),
+  encryptedPassKey: zodBuffer.nullable().optional(),


nit: encryptedWebhookKey

maidul98 · 2024-12-10T00:20:52Z

backend/src/ee/routes/v1/ldap-router.ts

@@ -187,7 +188,7 @@ export const registerLdapRouter = async (server: FastifyZodProvider) => {
        caCert: z.string().trim().default("")
      }),
      response: {
-        200: LdapConfigsSchema
+        200: SanitizedLdapConfigSchema


nit: I assume this is to filter out and define ony what we want the response fields to be like? Why don't we start a pattern where we call the schema of a API response V1APINameResponseSchema? This would be more clear

maidul98 · 2024-12-10T22:08:49Z

backend/src/services/webhook/webhook-fns.ts

@@ -16,28 +15,14 @@ import { WebhookType } from "./webhook-types";

 const WEBHOOK_TRIGGER_TIMEOUT = 15 * 1000;

-export const decryptWebhookDetails = (webhook: TWebhooks) => {
-  const { keyEncoding, iv, encryptedSecretKey, tag, urlCipherText, urlIV, urlTag, url } = webhook;
+export const decryptWebhookDetails = (webhook: TWebhooks, decryptor: (value: Buffer) => string) => {


why not make a instance of the decrypted here from kms service? i see you are passing on the decryptor function it self. could we pass on the decrypted data in here so we keep those things a little more isolated?

maidul98 · 2024-12-10T23:02:46Z

backend/src/db/migrations/utils/ring-buffer.ts

@@ -0,0 +1,19 @@
+export const newRingBuffer = <T>(bufferSize = 10) => {


Nit: This isn't a real ring buffer and more of a key-value store. Maybe call it createCircularCache

maidul98 · 2024-12-10T23:51:06Z

backend/src/db/migrations/20241127091918_webhook-to-kms.ts

+    await knex.schema.alterTable(TableName.Webhook, (t) => {
+      if (!hasEncryptedKey) t.binary("encryptedPassKey");
+      if (!hasEncryptedUrl) t.binary("encryptedUrl");
+      if (hasUrl) t.string("url").nullable().alter();


did you mean does not have url? because url doesn't exist currently

URL does exist in webhook

maidul98 · 2024-12-11T00:04:39Z

backend/src/db/migrations/20241127091918_webhook-to-kms.ts

+        updatedWebhooks.slice(i, i + BATCH_SIZE).map((el) => ({
+          id: el.id,
+          envId: el.envId,
+          url: "",


why is url being set to empty?

maidul98 · 2024-12-11T00:24:00Z

backend/src/db/migrations/20241128090536_secret-rotation-to-kms.ts

+          type: KmsDataKey.SecretManager,
+          projectId
+        });
+        projectEncryptionRingBuffer.push(projectId, projectKmsService);


if the cache loops over then don't you lose the id to projectKmsService and that would mean you may be encrypting the same project with one or more kms key?

Yes we loose the entire thing. It would also means we cannot find it anymore with project id, so we load it to the cache again

…c migration

…ctions

akhilmhdh requested a review from maidul98 December 1, 2024 17:43

akhilmhdh self-assigned this Dec 1, 2024

maidul98 reviewed Dec 11, 2024

View reviewed changes

akhilmhdh force-pushed the feat/enc-migration branch from eb705eb to 9554630 Compare January 9, 2025 14:40

akhilmhdh added 6 commits January 21, 2025 15:42

feat: added project level migrations for kms convernsion

1181bce

feat: updated codebase for new field changes made on project level en…

f8edf0e

…c migration

feat: made the non used columns nullable

2c3d909

feat: completed org migration in kms and updated to remove orgDAL fun…

d89b94b

…ctions

feat: resolved migration failing due to json

e6f692b

feat: updated migration to latest

b2ce14e

akhilmhdh force-pushed the feat/enc-migration branch from 9554630 to b2ce14e Compare January 21, 2025 10:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Root encrypted data to kms encryption #2827

Root encrypted data to kms encryption #2827

akhilmhdh commented Dec 1, 2024

gitguardian bot commented Dec 1, 2024 •

edited

Loading

maidul98 left a comment

maidul98 Dec 4, 2024

akhilmhdh Dec 11, 2024

maidul98 Dec 4, 2024

akhilmhdh Dec 11, 2024

maidul98 Dec 10, 2024

akhilmhdh Dec 11, 2024

maidul98 Dec 10, 2024

maidul98 Dec 10, 2024

maidul98 Dec 10, 2024

maidul98 Dec 10, 2024

maidul98 Dec 10, 2024

akhilmhdh Dec 11, 2024

maidul98 Dec 11, 2024

maidul98 Dec 11, 2024

akhilmhdh Dec 11, 2024

		@@ -0,0 +1,19 @@
		export const newRingBuffer = <T>(bufferSize = 10) => {

Root encrypted data to kms encryption #2827

Are you sure you want to change the base?

Root encrypted data to kms encryption #2827

Conversation

akhilmhdh commented Dec 1, 2024

Description 📣

Type ✨

Tests 🛠️

gitguardian bot commented Dec 1, 2024 • edited Loading

️✅ There are no secrets present in this pull request anymore.

maidul98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gitguardian bot commented Dec 1, 2024 •

edited

Loading