Add field-level encryption to API Keys #5046

djabarovgeorge · 2024-01-03T14:58:57Z

What change does this PR introduce?

Using the encryptProviderSecret function in application-generic, update the following use-cases to encrypt prior to write:
- Update the create-environment use-case to encrypt the generated key prior to insertion in the environment DAL
Using the decryptProviderSecret function in application-generic, update the following use-cases to decrypt after read and before sending back in response:
- get-api-keys
- get-environment
- get-my-environments
Update the authService.apiKeyAuthenticate method to:
- Remove the apiKey from the cached result in authService.getUserData
- Generate a hash from the unencrypted apiKey to act as the cache key in the @CachedEntity builder for authService.getUserData
- Update the authService.getUserData method to encrypt the provided apiKey prior to environmentRepository.findByApiKey lookup
  Remove the apiKey from return of authService.getUserData
  Remove the apiKey from JWT payload in authService.getApiSignedToken
Create a migration in the API to idempotently (i.e. only once through subsequent migrations) encrypt the API keys on all environments
Update the data migration docs to include the new migration for the relevant version

Why was this change needed?

The API key is not currently encrypted at rest, this poses a vulnerability if the raw contents of the database or the cache are available to a bad actor who may use the api key with bad intentions. We need to obfuscate the value of the api key at rest to prevent direct use of the key in the event of a database breach

We want to be able to show the API key to users from the dashboard to simplify and ease product adoption, therefore hashing of the API key in the database is not an option. We must therefore implement encryption prior to writes and decryption during reads to support the necessary obfuscation.

Definition of Done

API keys are encrypted at rest in the database
API keys are hashed at rest in the cache
API keys in dashboard are visible in decrypted form
API keys remain usable as authentication credentials for the API
All existing API keys become encrypted through a data migration script
Running the data migration script more than once does not re-encrypt the api keys
https://docs.novu.co/additional-resources/data-migrations has the new migration listed

linear · 2024-01-03T14:59:00Z

NV-3172 🔐 Add field-level encryption to API Keys

djabarovgeorge · 2024-01-03T15:07:19Z

Things I was not sure about, these and could not validate with Richard because he is on vacation 🎉

Are the notes regarding authService.apiKeyAuthenticate and authService.getUserData redundant? because i could not find those functions.

Why generate a hash from the unencrypted apiKey to act as the cache key in the @CachedEntity builder for authService.getUserData
• we can use an encrypted key, so we won't expose the key, and the key should be relatively small. so i wonder if we need to hash it?

Remove the apiKey from the JWT payload in authService.getApiSignedToken
• is it already removed? i did not find apiKey in the JWT payload.

djabarovgeorge · 2024-01-03T15:08:11Z

apps/api/src/app/environments/dtos/environment-response.dto.ts

+export interface IApiKeyDto {
+  key: string;
+  _userId: string;
+}


We can not use the DAL Entity because the interfaces are different

djabarovgeorge · 2024-01-03T15:11:04Z

apps/api/src/app/environments/dtos/environment-response.dto.ts

-  })
-  apiKeys: IApiKey[];
+  @ApiPropertyOptional()
+  apiKeys?: IApiKeyDto[];


Optional because get-environment.usecase does not return the api keys, i wonder if we need to return them at all under environment-response.dto. I would suggest removing it at altogether and providing the keys only under GET /environments/api-keys

The GET /environments does use that interface and returns the apiKeys.

True, that is why I left it as optional because GET /environments/me do not return it.

djabarovgeorge · 2024-01-03T15:20:51Z

libs/shared/src/types/shared/index.ts

+
+export const NOVU_SUB_MASK = 'nvsk.';
+
+export type EncryptedSecret = `${typeof NOVU_SUB_MASK}${string}`;


kudos to Richard for helping me to be more familiar with Template Literal Types.
This small type highlighted some potential issues we could have in compilation time while developing this feature, and will probably prevent future issues.

djabarovgeorge · 2024-01-03T15:22:34Z

packages/application-generic/src/encryption/encrypt-provider.ts

+export function encryptApiKey(apiKey: string): EncryptedSecret {
+  if (novuEncrypted(apiKey)) {
+    return apiKey;
+  }
+
+  return encryptSecret(apiKey);
+}
+
+export function decryptApiKey(apiKey: string): string {
+  if (novuEncrypted(apiKey)) {
+    return decryptSecret(apiKey);
+  }
+
+  return apiKey;
+}


those are middleware for the encryption-decryption that will help us make sure that we won't encrypt/decrypt twice for no reason.

shouldn't we have these checks actually in the encryptSecret, decryptSecret functions? because I see that they are exposed and used in other places.

That is a good point but i did not want to merge the base functions encryptSecret, decryptSecret that at the moment responsible for adding and removing the prefix. as it could affect the integration credentials as well.

LetItRock · 2024-01-03T17:06:13Z

apps/api/src/app/environments/dtos/environment-response.dto.ts

-  })
-  apiKeys: IApiKey[];
+  @ApiPropertyOptional()
+  apiKeys?: IApiKeyDto[];


The GET /environments does use that interface and returns the apiKeys.

...api/src/app/environments/usecases/generate-unique-api-key/generate-unique-api-key.usecase.ts

LetItRock · 2024-01-03T17:22:06Z

packages/application-generic/src/encryption/encrypt-provider.ts

+export function encryptApiKey(apiKey: string): EncryptedSecret {
+  if (novuEncrypted(apiKey)) {
+    return apiKey;
+  }
+
+  return encryptSecret(apiKey);
+}
+
+export function decryptApiKey(apiKey: string): string {
+  if (novuEncrypted(apiKey)) {
+    return decryptSecret(apiKey);
+  }
+
+  return apiKey;
+}


shouldn't we have these checks actually in the encryptSecret, decryptSecret functions? because I see that they are exposed and used in other places.

packages/application-generic/src/services/cache/key-builders/entities.ts

LetItRock · 2024-01-03T17:35:02Z

apps/api/src/app/environments/usecases/create-environment/create-environment.usecase.ts

@@ -1,9 +1,10 @@
+import { nanoid } from 'nanoid';


could you please also cover this functionality with e2e tests for all use-cases touched?

I was not sure what would be the best way to test it as the API key is i the code of our testing environment. i wanted to revisit the tests tomorrow once again.

small note, our test env organically simulates backward compatibility because we do not store an encrypted api key by the @novu/test service on sesstion init. i could not add the encryption because we do not propose application genetic in test lib.
meaning that we created not encrypted api keys in the DB on session init and the application (e2e tests) works as expected (backward compatibility).

I see, we should have in the EnvironmentService the same encryption ideally by reusing the utils encryptApiKey, decryptApiKey.
Theoretically, both packages are built to CJS, so we might include @novu/application-generic in @novu/testing, but not suggesting that 😅 Another way would be to have another lib that is pure for reusable utils and without app-related logic, which is headache 🤕 so the only easy thing would be to duplicate the code?

…el-encryption-to-api-keys # Conflicts: # packages/node/src/lib/changes/changes.spec.ts

apps/api/migrations/encrypt-api-keys/encrypt-api-keys-migration.spec.ts

apps/api/src/app/environments/usecases/create-environment/create-environment.e2e.ts

apps/api/src/app/environments/usecases/create-environment/create-environment.usecase.ts

libs/dal/src/repositories/environment/environment.repository.ts

libs/shared/src/types/shared/index.ts

packages/application-generic/src/encryption/encrypt-provider.ts

packages/application-generic/src/services/auth/auth.service.ts

rifont · 2024-01-19T15:16:36Z

Things I was not sure about, these and could not validate with Richard because he is on vacation 🎉
Are the notes regarding authService.apiKeyAuthenticate and authService.getUserData redundant? because i could not find those functions.

Yes, they are redundant - the API Key auth flow was recently simplified and these functions were removed.

Why generate a hash from the unencrypted apiKey to act as the cache key in the @CachedEntity builder for authService.getUserData • we can use an encrypted key, so we won't expose the key, and the key should be relatively small. so i wonder if we need to hash it?

The approach you have taken is much better. Nice work.

Remove the apiKey from the JWT payload in authService.getApiSignedToken • is it already removed? i did not find apiKey in the JWT payload.

Yes, they were removed in the API Key auth flow simplification mentioned above.

netlify · 2024-01-21T13:02:08Z

✅ Deploy Preview for dev-web-novu ready!

Name	Link
🔨 Latest commit	`78b472c`
🔍 Latest deploy log	https://app.netlify.com/sites/dev-web-novu/deploys/65ad159a77d0d900082c95b6
😎 Deploy Preview	https://deploy-preview-5046--dev-web-novu.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify · 2024-01-21T13:07:08Z

✅ Deploy Preview for dev-web-novu ready!

Name	Link
🔨 Latest commit	`7fe814d`
🔍 Latest deploy log	https://app.netlify.com/sites/dev-web-novu/deploys/65b0cbcfe07ef60008c5e6ed
😎 Deploy Preview	https://deploy-preview-5046--dev-web-novu.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

…on-to-api-keys' into nv-3172-add-field-level-encryption-to-api-keys

rifont

Awesome work @djabarovgeorge . Great security posture improvement 🦾

feat: add field-level encryption to api keys

3c33834

github-actions bot added @novu/api @novu/dal @novu/shared labels Jan 3, 2024

djabarovgeorge commented Jan 3, 2024

View reviewed changes

djabarovgeorge added 2 commits January 3, 2024 17:38

fix: tests stub

7d36a57

fix: tests mock

e4ec7a2

LetItRock reviewed Jan 3, 2024

View reviewed changes

djabarovgeorge and others added 4 commits January 6, 2024 11:39

feat: update after pr comments

b229fb8

fix: store and search by hash

b481c36

Merge branch 'next' into nv-3172-add-field-level-encryption-to-api-keys

6693ec5

Merge remote-tracking branch 'origin/next' into nv-3172-add-field-lev…

cf55eae

…el-encryption-to-api-keys # Conflicts: # packages/node/src/lib/changes/changes.spec.ts

rifont self-requested a review January 19, 2024 14:42

rifont reviewed Jan 19, 2024

View reviewed changes

djabarovgeorge and others added 2 commits January 21, 2024 15:00

refactor: update after pr comments

78b472c

Merge branch 'next' into nv-3172-add-field-level-encryption-to-api-keys

620cebb

djabarovgeorge and others added 3 commits January 21, 2024 16:28

test(api): fix revert error message

3422b7e

Merge remote-tracking branch 'origin/nv-3172-add-field-level-encrypti…

3e96371

…on-to-api-keys' into nv-3172-add-field-level-encryption-to-api-keys

Merge branch 'next' into nv-3172-add-field-level-encryption-to-api-keys

7fe814d

djabarovgeorge enabled auto-merge January 25, 2024 12:45

djabarovgeorge merged commit 046c0ad into next Jan 26, 2024
31 checks passed

rifont approved these changes Jan 26, 2024

View reviewed changes

djabarovgeorge deleted the nv-3172-add-field-level-encryption-to-api-keys branch January 26, 2024 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add field-level encryption to API Keys #5046

Add field-level encryption to API Keys #5046

djabarovgeorge commented Jan 3, 2024

linear bot commented Jan 3, 2024

djabarovgeorge commented Jan 3, 2024

djabarovgeorge Jan 3, 2024

djabarovgeorge Jan 3, 2024

LetItRock Jan 3, 2024

djabarovgeorge Jan 3, 2024

djabarovgeorge Jan 3, 2024

djabarovgeorge Jan 3, 2024

LetItRock Jan 3, 2024

djabarovgeorge Jan 3, 2024

LetItRock Jan 3, 2024

LetItRock Jan 3, 2024

LetItRock Jan 3, 2024

djabarovgeorge Jan 3, 2024

LetItRock Jan 4, 2024

rifont commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 21, 2024 •

edited

Loading

netlify bot commented Jan 21, 2024 •

edited

Loading

rifont left a comment


		export const NOVU_SUB_MASK = 'nvsk.';

		export type EncryptedSecret = `${typeof NOVU_SUB_MASK}${string}`;

Add field-level encryption to API Keys #5046

Add field-level encryption to API Keys #5046

Conversation

djabarovgeorge commented Jan 3, 2024

What change does this PR introduce?

Why was this change needed?

Definition of Done

linear bot commented Jan 3, 2024

djabarovgeorge commented Jan 3, 2024

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

rifont commented Jan 19, 2024 • edited Loading

netlify bot commented Jan 21, 2024 • edited Loading

✅ Deploy Preview for dev-web-novu ready!

netlify bot commented Jan 21, 2024 • edited Loading

✅ Deploy Preview for dev-web-novu ready!

rifont left a comment

Choose a reason for hiding this comment

rifont commented Jan 19, 2024 •

edited

Loading

netlify bot commented Jan 21, 2024 •

edited

Loading

netlify bot commented Jan 21, 2024 •

edited

Loading