Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add field-level encryption to API Keys #5046

Merged
merged 12 commits into from
Jan 26, 2024
Merged
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
import { expect } from 'chai';
import { faker } from '@faker-js/faker';

import { UserSession } from '@novu/testing';
import { ChannelTypeEnum } from '@novu/stateless';
import { EnvironmentRepository } from '@novu/dal';

import { encryptApiKeysMigration } from './encrypt-api-keys-migration';

describe('Encrypt Old api keys', function () {
let session: UserSession;
const environmentRepository = new EnvironmentRepository();

beforeEach(async () => {
session = new UserSession();
await session.initialize();
});

it('should decrypt all old api keys', async function () {
await pruneIntegration({ environmentRepository });

for (let i = 0; i < 2; i++) {
await environmentRepository.create({
identifier: 'identifier' + i,
name: faker.name.jobTitle(),
_organizationId: session.organization._id,
apiKeys: [
{
key: 'not-encrypted-secret-key',
_userId: session.user._id,
},
],
});
}

const newEnvironments = await environmentRepository.find({});

expect(newEnvironments.length).to.equal(2);

for (const environment of newEnvironments) {
expect(environment.identifier).to.contains('identifier');
expect(environment.name).to.exist;
expect(environment._organizationId).to.equal(session.organization._id);
expect(environment.apiKeys[0].key).to.equal('not-encrypted-secret-key');
expect(environment.apiKeys[0]._userId).to.equal(session.user._id);
}

await encryptApiKeysMigration();

const encryptEnvironments = await environmentRepository.find({});

for (const environment of encryptEnvironments) {
expect(environment.identifier).to.contains('identifier');
expect(environment.name).to.exist;
expect(environment._organizationId).to.equal(session.organization._id);
expect(environment.apiKeys[0].key).to.contains('nvsk.');
expect(environment.apiKeys[0]._userId).to.equal(session.user._id);
}
});

it('should validate migration idempotence', async function () {
await pruneIntegration({ environmentRepository });

const data = {
providerId: 'sendgrid',
channel: ChannelTypeEnum.EMAIL,
active: false,
};

for (let i = 0; i < 2; i++) {
await environmentRepository.create({
identifier: 'identifier' + i,
name: faker.name.jobTitle(),
_organizationId: session.organization._id,
apiKeys: [
{
key: 'not-encrypted-secret-key',
_userId: session.user._id,
},
],
});
}

await encryptApiKeysMigration();
const firstMigrationExecution = await environmentRepository.find({});

await encryptApiKeysMigration();
const secondMigrationExecution = await environmentRepository.find({});

expect(firstMigrationExecution[0].identifier).to.contains(secondMigrationExecution[0].identifier);
expect(firstMigrationExecution[0].name).to.exist;
expect(firstMigrationExecution[0]._organizationId).to.equal(secondMigrationExecution[0]._organizationId);
expect(firstMigrationExecution[0].apiKeys[0].key).to.contains(secondMigrationExecution[0].apiKeys[0].key);
expect(firstMigrationExecution[0].apiKeys[0]._userId).to.equal(secondMigrationExecution[0].apiKeys[0]._userId);

expect(firstMigrationExecution[1].identifier).to.contains(secondMigrationExecution[1].identifier);
expect(firstMigrationExecution[1].name).to.exist;
expect(firstMigrationExecution[1]._organizationId).to.equal(secondMigrationExecution[1]._organizationId);
expect(firstMigrationExecution[1].apiKeys[0].key).to.contains(secondMigrationExecution[1].apiKeys[0].key);
expect(firstMigrationExecution[1].apiKeys[0]._userId).to.equal(secondMigrationExecution[1].apiKeys[0]._userId);
});
});

async function pruneIntegration({ environmentRepository }: { environmentRepository: EnvironmentRepository }) {
djabarovgeorge marked this conversation as resolved.
Show resolved Hide resolved
const old = await environmentRepository.find({});

for (const oldKey of old) {
await environmentRepository.delete({ _id: oldKey._id });
}
}
65 changes: 65 additions & 0 deletions apps/api/migrations/encrypt-api-keys/encrypt-api-keys-migration.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { EnvironmentRepository, IApiKey } from '@novu/dal';
import { encryptSecret } from '@novu/application-generic';
import { EncryptedSecret } from '@novu/shared';

export async function encryptApiKeysMigration() {
// eslint-disable-next-line no-console
console.log('start migration - encrypt api keys');

const environmentRepository = new EnvironmentRepository();
const environments = await environmentRepository.find({});

for (const environment of environments) {
// eslint-disable-next-line no-console
console.log(`environment ${environment._id}`);

if (!environment.apiKeys) {
// eslint-disable-next-line no-console
console.log(`environment ${environment._id} - is not contains api keys, skipping..`);
continue;
}

if (
environment.apiKeys.every((key) => {
isEncrypted(key.key);
})
) {
// eslint-disable-next-line no-console
console.log(`environment ${environment._id} - api keys are already encrypted, skipping..`);
continue;
}

const updatePayload: IEncryptedApiKey[] = encryptApiKeysWithGuard(environment.apiKeys);

await environmentRepository.update(
{ _id: environment._id },
{
$set: { apiKeys: updatePayload },
}
);
// eslint-disable-next-line no-console
console.log(`environment ${environment._id} - api keys updated`);
}
// eslint-disable-next-line no-console
console.log('end migration');
}

export function encryptApiKeysWithGuard(apiKeys: IApiKey[]): IEncryptedApiKey[] {
return apiKeys.map((apiKey: IApiKey) => {
const encryptedApiKey: IEncryptedApiKey = {
key: isEncrypted(apiKey.key) ? apiKey.key : encryptSecret(apiKey.key),
_userId: apiKey._userId,
};

return encryptedApiKey;
});
}

function isEncrypted(apiKey: string): apiKey is EncryptedSecret {
return apiKey.startsWith('nvsk.');
}

export interface IEncryptedApiKey {
key: EncryptedSecret;
_userId: string;
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,13 +51,13 @@ describe('Encrypt Old Credentials', function () {
});
}

const newIntegration = await integrationRepository.find({});
const newIntegration = await integrationRepository.find({} as any);

expect(newIntegration.length).to.equal(2);

await encryptOldCredentialsMigration();

const encryptIntegration = await integrationRepository.find({});
const encryptIntegration = await integrationRepository.find({} as any);

for (const integrationKey in encryptIntegration) {
const integration = encryptIntegration[integrationKey];
Expand Down
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
import { IntegrationEntity } from '@novu/dal';
import { encryptProviderSecret } from '../../src/app/shared/services/encryption';
import { IntegrationRepository } from '@novu/dal';
import { ICredentialsDto, secureCredentials } from '@novu/shared';
import { encryptSecret } from '@novu/application-generic';

export async function encryptOldCredentialsMigration() {
// eslint-disable-next-line no-console
console.log('start migration - encrypt credentials');

const integrationRepository = new IntegrationRepository();
const integrations = await integrationRepository.find({});
const integrations = await integrationRepository.find({} as any);

for (const integration of integrations) {
// eslint-disable-next-line no-console
Expand All @@ -25,7 +25,7 @@ export async function encryptOldCredentialsMigration() {
updatePayload.credentials = encryptCredentialsWithGuard(integration);

await integrationRepository.update(
{ _id: integration._id },
{ _id: integration._id, _environmentId: integration._environmentId },
{
$set: updatePayload,
}
Expand All @@ -45,7 +45,7 @@ export function encryptCredentialsWithGuard(integration: IntegrationEntity): ICr
const credential = credentials[key];

if (needEncryption(key, credential, integration)) {
encryptedCredentials[key] = encryptProviderSecret(credential);
encryptedCredentials[key] = encryptSecret(credential);
} else {
encryptedCredentials[key] = credential;
}
Expand Down
2 changes: 1 addition & 1 deletion apps/api/src/app/auth/e2e/user.auth.guard.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ describe('UserAuthGuard', () => {
it('should return 401 when ApiKey auth scheme is provided with an invalid value', async () => {
const response = await request(`${ApiAuthSchemeEnum.API_KEY} invalid_key`);
expect(response.statusCode).to.equal(401);
expect(response.body.message).to.equal('API Key not found');
expect(response.body.message).to.equal('Environment not found');
});

it('should return 401 when ApiKey auth scheme is used for an externally inaccessible API route', async () => {
Expand Down
13 changes: 7 additions & 6 deletions apps/api/src/app/environments/dtos/environment-response.dto.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { ApiProperty, ApiPropertyOptional } from '@nestjs/swagger';
import { IApiKey } from '@novu/dal';
import { ApiKey } from '../../shared/dtos/api-key';

export class EnvironmentResponseDto {
@ApiPropertyOptional()
Expand All @@ -15,11 +13,14 @@ export class EnvironmentResponseDto {
@ApiProperty()
identifier: string;

@ApiProperty({
type: [ApiKey],
})
apiKeys: IApiKey[];
@ApiPropertyOptional()
apiKeys?: IApiKeyDto[];
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


@ApiProperty()
_parentId: string;
}

export interface IApiKeyDto {
key: string;
_userId: string;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { nanoid } from 'nanoid';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

import { Injectable } from '@nestjs/common';

import { EnvironmentRepository } from '@novu/dal';
import { nanoid } from 'nanoid';
import { encryptApiKey } from '@novu/application-generic';

import { CreateEnvironmentCommand } from './create-environment.command';

import { GenerateUniqueApiKey } from '../generate-unique-api-key/generate-unique-api-key.usecase';
// eslint-disable-next-line max-len
import { CreateNotificationGroupCommand } from '../../../notification-groups/usecases/create-notification-group/create-notification-group.command';
Expand All @@ -20,7 +21,7 @@ export class CreateEnvironment {
) {}

async execute(command: CreateEnvironmentCommand) {
const key = await this.generateUniqueApiKey.execute();
const encryptedKey = encryptApiKey(await this.generateUniqueApiKey.execute());

const environment = await this.environmentRepository.create({
_organizationId: command.organizationId,
Expand All @@ -29,7 +30,7 @@ export class CreateEnvironment {
_parentId: command.parentEnvironmentId,
apiKeys: [
{
key,
key: encryptedKey,
djabarovgeorge marked this conversation as resolved.
Show resolved Hide resolved
_userId: command.userId,
},
],
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { Injectable, InternalServerErrorException } from '@nestjs/common';
import { EnvironmentRepository } from '@novu/dal';
import * as hat from 'hat';
import { encryptApiKey } from '@novu/application-generic';

const API_KEY_GENERATION_MAX_RETRIES = 3;

Expand All @@ -14,8 +15,7 @@ export class GenerateUniqueApiKey {
let isApiKeyUsed = true;
while (isApiKeyUsed) {
apiKey = this.generateApiKey();
const environment = await this.environmentRepository.findByApiKey(apiKey);
isApiKeyUsed = environment ? true : false;
isApiKeyUsed = await this.validateIsApiKeyUsed(apiKey);
count += 1;

if (count === API_KEY_GENERATION_MAX_RETRIES) {
Expand All @@ -27,6 +27,18 @@ export class GenerateUniqueApiKey {
return apiKey as string;
}

private async validateIsApiKeyUsed(apiKey: string) {
const encryptedApiKey = encryptApiKey(apiKey);

// backward compatibility - update to `findByApiKey` after encrypt-api-keys-migration run
const environment = await this.environmentRepository.findByApiKeyBackwardCompatibility({
decryptedKey: apiKey,
encryptedKey: encryptedApiKey,
});
djabarovgeorge marked this conversation as resolved.
Show resolved Hide resolved

return !!environment;
}

/**
* Extracting the generation functionality so it can be stubbed for functional testing
*
Expand Down
Original file line number Diff line number Diff line change
@@ -1,12 +1,23 @@
import { Injectable } from '@nestjs/common';
import { IApiKey, EnvironmentRepository } from '@novu/dal';

import { EnvironmentRepository, IApiKey } from '@novu/dal';

import { GetApiKeysCommand } from './get-api-keys.command';
import { ApiKey } from '../../../shared/dtos/api-key';
import { decryptApiKey } from '@novu/application-generic';

@Injectable()
export class GetApiKeys {
constructor(private environmentRepository: EnvironmentRepository) {}

async execute(command: GetApiKeysCommand): Promise<IApiKey[]> {
return await this.environmentRepository.getApiKeys(command.environmentId);
async execute(command: GetApiKeysCommand): Promise<ApiKey[]> {
const keys = await this.environmentRepository.getApiKeys(command.environmentId);

return keys.map((apiKey: IApiKey) => {
return {
key: decryptApiKey(apiKey.key),
_userId: apiKey._userId,
};
});
}
}
Original file line number Diff line number Diff line change
@@ -1,13 +1,16 @@
import { Injectable, NotFoundException } from '@nestjs/common';
import { EnvironmentRepository } from '@novu/dal';

import { EnvironmentEntity, EnvironmentRepository } from '@novu/dal';

import { GetEnvironmentCommand } from './get-environment.command';
import { EnvironmentResponseDto } from '../../dtos/environment-response.dto';

@Injectable()
export class GetEnvironment {
constructor(private environmentRepository: EnvironmentRepository) {}

async execute(command: GetEnvironmentCommand) {
const environment = await this.environmentRepository.findOne(
async execute(command: GetEnvironmentCommand): Promise<EnvironmentResponseDto> {
const environment: Omit<EnvironmentEntity, 'apiKeys'> | null = await this.environmentRepository.findOne(
{
_id: command.environmentId,
_organizationId: command.organizationId,
Expand Down
Loading
Loading