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.

Sign up for GitHub

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

Jump to bottom

integrate with crypto module to decrypt password #2170

Merged
merged 1 commit into from
Aug 19, 2022
Merged

integrate with crypto module to decrypt password #2170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 10 additions & 3 deletions src/plugins/data_source/server/client/configure_client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,13 @@ import { configureClient } from './configure_client';
import { ClientOptions } from '@opensearch-project/opensearch';
// eslint-disable-next-line @osd/eslint/no-restricted-paths
import { opensearchClientMock } from '../../../../core/server/opensearch/client/mocks';
import { CryptographyClient } from '../cryptography';

const DATA_SOURCE_ID = 'a54b76ec86771ee865a0f74a305dfff8';
const CREDENETIAL_ID = 'a54dsaadasfasfwe22d23d23d2453df3';
const cryptoClient = new CryptographyClient('test', 'test', new Array(32).fill(0));

// TODO: improve UT
describe('configureClient', () => {
let logger: ReturnType<typeof loggingSystemMock.createLogger>;
let config: DataSourcePluginConfigType;
Expand All @@ -35,7 +38,6 @@ describe('configureClient', () => {
dsClient = opensearchClientMock.createInternalClient();
logger = loggingSystemMock.createLogger();
savedObjectsMock = savedObjectsClientMock.create();

config = {
enabled: true,
clientPool: {
Expand Down Expand Up @@ -93,7 +95,7 @@ describe('configureClient', () => {
afterEach(() => {
ClientMock.mockReset();
});
// TODO: mark as skip until we fix the issue of mocking "@opensearch-project/opensearch"

test('configure client with noAuth == true, will call new Client() to create client', async () => {
savedObjectsMock.get.mockReset().mockResolvedValueOnce({
id: DATA_SOURCE_ID,
Expand All @@ -107,6 +109,7 @@ describe('configureClient', () => {
const client = await configureClient(
DATA_SOURCE_ID,
savedObjectsMock,
cryptoClient,
clientPoolSetup,
config,
logger
Expand All @@ -119,17 +122,21 @@ describe('configureClient', () => {
expect(client).toBe(dsClient.child.mock.results[0].value);
});

test('configure client with noAuth == false, will first call new Client()', async () => {
test('configure client with noAuth == false, will first call decrypt()', async () => {
const spy = jest.spyOn(cryptoClient, 'decodeAndDecrypt').mockResolvedValue('password');

const client = await configureClient(
DATA_SOURCE_ID,
savedObjectsMock,
cryptoClient,
clientPoolSetup,
config,
logger
);

expect(ClientMock).toHaveBeenCalledTimes(1);
expect(savedObjectsMock.get).toHaveBeenCalledTimes(2);
expect(spy).toHaveBeenCalledTimes(1);
expect(client).toBe(dsClient.child.mock.results[0].value);
});
});
41 changes: 31 additions & 10 deletions src/plugins/data_source/server/client/configure_client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,23 +11,28 @@ import {
SavedObjectsErrorHelpers,
} from '../../../../../src/core/server';
import { DATA_SOURCE_SAVED_OBJECT_TYPE, CREDENTIAL_SAVED_OBJECT_TYPE } from '../../common';
import { CredentialSavedObjectAttributes } from '../../common/credentials/types';
import {
CredentialSavedObjectAttributes,
UsernamePasswordTypedContent,
} from '../../common/credentials/types';
Copy link
Contributor

Choose a reason for hiding this comment

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

../../common works as well

Copy link
Member Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor

Choose a reason for hiding this comment

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

i will refactor on this to expose directly

Copy link
Member

Choose a reason for hiding this comment

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

let's create an issue to track this

import { DataSourceAttributes } from '../../common/data_sources';
import { DataSourcePluginConfigType } from '../../config';
import { CryptographyClient } from '../cryptography';
import { parseClientOptions } from './client_config';
import { OpenSearchClientPoolSetup } from './client_pool';

export const configureClient = async (
dataSourceId: string,
savedObjects: SavedObjectsClientContract,
cryptographyClient: CryptographyClient,
openSearchClientPoolSetup: OpenSearchClientPoolSetup,
config: DataSourcePluginConfigType,
logger: Logger
): Promise<Client> => {
const dataSource = await getDataSource(dataSourceId, savedObjects);
const rootClient = getRootClient(dataSource.attributes, config, openSearchClientPoolSetup);

return getQueryClient(rootClient, dataSource, savedObjects);
return getQueryClient(rootClient, dataSource, savedObjects, cryptographyClient);
};

export const getDataSource = async (
Expand All @@ -48,13 +53,24 @@ export const getDataSource = async (

export const getCredential = async (
credentialId: string,
savedObjects: SavedObjectsClientContract
): Promise<SavedObject<CredentialSavedObjectAttributes>> => {
savedObjects: SavedObjectsClientContract,
cryptographyClient: CryptographyClient
): Promise<UsernamePasswordTypedContent> => {
try {
const credential = await savedObjects.get<CredentialSavedObjectAttributes>(
const credentialSavedObject = await savedObjects.get<CredentialSavedObjectAttributes>(
CREDENTIAL_SAVED_OBJECT_TYPE,
credentialId
);
const {
username,
password,
} = credentialSavedObject.attributes.credentialMaterials.credentialMaterialsContent;
const decodedPassword = await cryptographyClient.decodeAndDecrypt(password);
const credential = {
username,
password: decodedPassword,
};
Comment on lines +68 to +72
Copy link
Contributor

Choose a reason for hiding this comment

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

How about

const credential = {
      username,
      password: await cryptographyClient.decodeAndDecrypt(password),
 };


return credential;
} catch (error: any) {
// it will cause 500 error when failed to get saved objects, need to handle such error gracefully
Expand All @@ -73,13 +89,18 @@ export const getCredential = async (
const getQueryClient = async (
rootClient: Client,
dataSource: SavedObject<DataSourceAttributes>,
savedObjects: SavedObjectsClientContract
savedObjects: SavedObjectsClientContract,
cryptographyClient: CryptographyClient
): Promise<Client> => {
if (dataSource.attributes.noAuth) {
return rootClient.child();
} else {
const credential = await getCredential(dataSource.references[0].id, savedObjects);
return getBasicAuthClient(rootClient, credential.attributes);
const credential = await getCredential(
dataSource.references[0].id,
savedObjects,
cryptographyClient
);
return getBasicAuthClient(rootClient, credential);
}
};

Expand Down Expand Up @@ -112,9 +133,9 @@ const getRootClient = (

const getBasicAuthClient = (
rootClient: Client,
credentialAttr: CredentialSavedObjectAttributes
credential: UsernamePasswordTypedContent
): Client => {
const { username, password } = credentialAttr.credentialMaterials.credentialMaterialsContent;
const { username, password } = credential;
return rootClient.child({
auth: {
username,
Expand Down
8 changes: 6 additions & 2 deletions src/plugins/data_source/server/data_source_service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,13 @@
import { Logger, OpenSearchClient, SavedObjectsClientContract } from '../../../../src/core/server';
import { DataSourcePluginConfigType } from '../config';
import { OpenSearchClientPool, configureClient } from './client';
import { CryptographyClient } from './cryptography';
export interface DataSourceServiceSetup {
getDataSourceClient: (
dataSourceId: string,
// this saved objects client is used to fetch data source on behalf of users, caller should pass scoped saved objects client
savedObjects: SavedObjectsClientContract
savedObjects: SavedObjectsClientContract,
Copy link
Contributor

Choose a reason for hiding this comment

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

    // this saved objects client is used to fetch data source on behalf of users, caller should pass scoped saved objects client

Can I ask more context on this?

Copy link
Member Author

Choose a reason for hiding this comment

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

it should be scoped client, which means a client tight to a user in the request. Different user may have access to different saved object, including datasource or credentials.

Copy link
Contributor

Choose a reason for hiding this comment

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

it should be scoped client, which means a client tight to a user in the request. Different user may have access to different saved object, including datasource or credentials.

Follow up question - Is this already possible with the current scoped clienet? What is the isolation based on?

cryptographyClient: CryptographyClient
) => Promise<OpenSearchClient>;
}
export class DataSourceService {
Expand All @@ -25,11 +27,13 @@ export class DataSourceService {

const getDataSourceClient = async (
dataSourceId: string,
savedObjects: SavedObjectsClientContract
savedObjects: SavedObjectsClientContract,
cryptographyClient: CryptographyClient
): Promise<OpenSearchClient> => {
return configureClient(
dataSourceId,
savedObjects,
cryptographyClient,
openSearchClientPoolSetup,
config,
this.logger
Expand Down
20 changes: 14 additions & 6 deletions src/plugins/data_source/server/plugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
*/

import { first } from 'rxjs/operators';
import { OpenSearchClientError } from '@opensearch-project/opensearch/lib/errors';
import { dataSource, credential, CredentialSavedObjectsClientWrapper } from './saved_objects';
import { DataSourcePluginConfigType } from '../config';
import {
Expand All @@ -24,8 +25,8 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
private readonly dataSourceService: DataSourceService;

constructor(private initializerContext: PluginInitializerContext<DataSourcePluginConfigType>) {
this.logger = this.initializerContext.logger.get('dataSource');
this.dataSourceService = new DataSourceService(this.logger);
this.logger = this.initializerContext.logger.get();
this.dataSourceService = new DataSourceService(this.logger.get('data-source-service'));
}

public async setup(core: CoreSetup) {
Expand All @@ -44,8 +45,13 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
const { wrappingKeyName, wrappingKeyNamespace, wrappingKey } = config.encryption;

// Create credential saved objects client wrapper
const cryptographyClient = new CryptographyClient(
wrappingKeyName,
wrappingKeyNamespace,
wrappingKey
);
const credentialSavedObjectsClientWrapper = new CredentialSavedObjectsClientWrapper(
new CryptographyClient(wrappingKeyName, wrappingKeyNamespace, wrappingKey)
cryptographyClient
);

// Add credential saved objects client wrapper factory
Expand All @@ -60,7 +66,7 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
// Register data source plugin context to route handler context
core.http.registerRouteHandlerContext(
'dataSource',
this.createDataSourceRouteHandlerContext(dataSourceService, this.logger)
this.createDataSourceRouteHandlerContext(dataSourceService, cryptographyClient, this.logger)
);

return {};
Expand All @@ -77,6 +83,7 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc

private createDataSourceRouteHandlerContext = (
dataSourceService: DataSourceServiceSetup,
cryptographyClient: CryptographyClient,
logger: Logger
): IContextProvider<RequestHandler<unknown, unknown, unknown>, 'dataSource'> => {
return (context, req) => {
Expand All @@ -86,13 +93,14 @@ export class DataSourcePlugin implements Plugin<DataSourcePluginSetup, DataSourc
try {
return dataSourceService.getDataSourceClient(
dataSourceId,
context.core.savedObjects.client
context.core.savedObjects.client,
cryptographyClient
);
} catch (error: any) {
logger.error(
`Fail to get data source client for dataSourceId: [${dataSourceId}]. Detail: ${error.messages}`
);
throw error;
throw new OpenSearchClientError(error.message);
}
},
},
Expand Down