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

Create credential management plugin #1

Merged
merged 6 commits into from
Jun 30, 2022
Merged

Conversation

noCharger
Copy link
Collaborator

@noCharger noCharger commented Jun 25, 2022

Description

Two main features:

  • Save credential via API
    • basic_auth
    • aws_iam_credential
  • Store encrypted information on metadata index

Issues Resolved

opensearch-project#1804

Check List

  • New functionality includes manual testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

Manual testing

  1. Create crypto materials
ts-node src/plugins/credential_management/server/crypto/crypto_materials_generator.ts --keyName='aes-name' --keyNamespace='aes-namespace'
crypto materials generated successfully!
  1. create credential via API
curl --include -XPOST localhost:5603/iak/api/credential_management/create \
    -H "Content-Type: application/json" \
    -H "osd-xsrf: true" \
    -d'{"credential_name": "test-credential-1", "credential_type":"basic_auth", "basic_auth_credential_JSON": {"user_name":"louis", "password":"ABC"}}'


HTTP/1.1 200 OK
osd-name: 88665a1d4119.ant.amazon.com
content-type: application/json; charset=utf-8
cache-control: private, no-cache, no-store, must-revalidate
content-length: 142
Date: Wed, 29 Jun 2022 21:32:45 GMT
Connection: keep-alive
Keep-Alive: timeout=120

{"time":"2022-06-29T21:32:45.903Z","ID":"ee95e530-f7f2-11ec-8917-79d532add1d4","Type":"credential","UN":"test-credential-1","UT":"basic_auth"}%      

*/

/*
* Licensed to Elasticsearch B.V. under one or more contributor

Choose a reason for hiding this comment

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

no need to copy elasticsearch license head

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The license head is required for each file to pass the lint check. Will replace with the OpenSearch one

Choose a reason for hiding this comment

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

there is already Apache one above.

@seraphjiang
Copy link

fail to run yarn osd bootstrap

success Saved lockfile.

 succ yarn.lock analysis completed without any issues
 succ 22 bootstrap builds are cached
 info [opensearch-dashboards] running [osd:bootstrap] script
ERROR [bootstrap] failed:
ERROR Error: Command failed with exit code 1: /Users/huanji/.nvm/versions/node/v14.19.1/lib/node_modules/yarn/bin/yarn.js run osd:bootstrap
      error Command failed with exit code 1.
      $ node scripts/build_ts_refs && node scripts/register_git_hook
      ERROR Error: Command failed with exit code 1: /Users/huanji/src/huan-osd/node_modules/typescript/bin/tsc -b tsconfig.refs.json --pretty
            src/core/server/saved_objects/service/lib/crypto/crypto_cli.ts:42:8 - error TS6307: File '/Users/huanji/src/huan-osd/src/core/server/saved_objects/service/lib/crypto/crypto_metadata.json' is not listed within the file list of project '/Users/huanji/src/huan-osd/src/core/tsconfig.json'. Projects must list all files or use an 'include' pattern.

            42 } from './crypto_metadata.json';
                      ~~~~~~~~~~~~~~~~~~~~~~~~

            src/core/server/saved_objects/service/lib/repository.ts:267:25 - error TS2339: Property 'password' does not exist on type 'T'.

            267       typeof attributes.password !== 'undefined'
                                        ~~~~~~~~

            src/core/server/saved_objects/service/lib/repository.ts:269:18 - error TS2339: Property 'password' does not exist on type 'T'.

            269       attributes.password = await this._crypto_cli.encrypt(attributes.password);
                                 ~~~~~~~~

            src/core/server/saved_objects/service/lib/repository.ts:269:71 - error TS2339: Property 'password' does not exist on type 'T'.

            269       attributes.password = await this._crypto_cli.encrypt(attributes.password);
                                                                                      ~~~~~~~~


            Found 4 errors.

                at makeError (/Users/huanji/src/huan-osd/node_modules/execa/lib/error.js:59:11)
                at handlePromise (/Users/huanji/src/huan-osd/node_modules/execa/index.js:114:26)
                at processTicksAndRejections (internal/process/task_queues.js:95:5)
                at buildRefs (/Users/huanji/src/huan-osd/src/dev/typescript/build_refs.ts:41:5)
                at buildAllRefs (/Users/huanji/src/huan-osd/src/dev/typescript/build_refs.ts:35:3)
                at description (/Users/huanji/src/huan-osd/src/dev/typescript/build_refs.ts:51:7)
                at /Users/huanji/src/huan-osd/packages/osd-dev-utils/target/run/run.js:64:13
                at Object.withProcRunner (/Users/huanji/src/huan-osd/packages/osd-dev-utils/target/proc_runner/with_proc_runner.js:45:9)
                at run (/Users/huanji/src/huan-osd/packages/osd-dev-utils/target/run/run.js:63:9)
      info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
          at makeError (/Users/huanji/src/huan-osd/packages/osd-pm/dist/index.js:25150:11)
          at handlePromise (/Users/huanji/src/huan-osd/packages/osd-pm/dist/index.js:24085:26)
          at processTicksAndRejections (internal/process/task_queues.js:95:5)
          at async /Users/huanji/src/huan-osd/packages/osd-pm/dist/index.js:9051:9
          at async scheduleItem (/Users/huanji/src/huan-osd/packages/osd-pm/dist/index.js:10938:9)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Comment on lines 264 to 270
if (
'credential' === type &&
typeof attributes !== 'undefined' &&
typeof attributes.password !== 'undefined'
) {
attributes.password = await this._crypto_cli.encrypt(attributes.password);
}

Choose a reason for hiding this comment

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

why do we add encryption logic a save object service?
saved object service should be low level data access api without adding too much specific business logic.

credential is one of saved object type, the implementation should not touch code here

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fail to run yarn osd bootstrap

success Saved lockfile.

 succ yarn.lock analysis completed without any issues
 succ 22 bootstrap builds are cached
 info [opensearch-dashboards] running [osd:bootstrap] script
ERROR [bootstrap] failed:
ERROR Error: Command failed with exit code 1: /Users/huanji/.nvm/versions/node/v14.19.1/lib/node_modules/yarn/bin/yarn.js run osd:bootstrap
      error Command failed with exit code 1.
      $ node scripts/build_ts_refs && node scripts/register_git_hook
      ERROR Error: Command failed with exit code 1: /Users/huanji/src/huan-osd/node_modules/typescript/bin/tsc -b tsconfig.refs.json --pretty
            src/core/server/saved_objects/service/lib/crypto/crypto_cli.ts:42:8 - error TS6307: File '/Users/huanji/src/huan-osd/src/core/server/saved_objects/service/lib/crypto/crypto_metadata.json' is not listed within the file list of project '/Users/huanji/src/huan-osd/src/core/tsconfig.json'. Projects must list all files or use an 'include' pattern.

            42 } from './crypto_metadata.json';
                      ~~~~~~~~~~~~~~~~~~~~~~~~

            src/core/server/saved_objects/service/lib/repository.ts:267:25 - error TS2339: Property 'password' does not exist on type 'T'.

            267       typeof attributes.password !== 'undefined'
                                        ~~~~~~~~

            src/core/server/saved_objects/service/lib/repository.ts:269:18 - error TS2339: Property 'password' does not exist on type 'T'.

            269       attributes.password = await this._crypto_cli.encrypt(attributes.password);
                                 ~~~~~~~~

            src/core/server/saved_objects/service/lib/repository.ts:269:71 - error TS2339: Property 'password' does not exist on type 'T'.

            269       attributes.password = await this._crypto_cli.encrypt(attributes.password);
                                                                                      ~~~~~~~~


            Found 4 errors.

                at makeError (/Users/huanji/src/huan-osd/node_modules/execa/lib/error.js:59:11)
                at handlePromise (/Users/huanji/src/huan-osd/node_modules/execa/index.js:114:26)
                at processTicksAndRejections (internal/process/task_queues.js:95:5)
                at buildRefs (/Users/huanji/src/huan-osd/src/dev/typescript/build_refs.ts:41:5)
                at buildAllRefs (/Users/huanji/src/huan-osd/src/dev/typescript/build_refs.ts:35:3)
                at description (/Users/huanji/src/huan-osd/src/dev/typescript/build_refs.ts:51:7)
                at /Users/huanji/src/huan-osd/packages/osd-dev-utils/target/run/run.js:64:13
                at Object.withProcRunner (/Users/huanji/src/huan-osd/packages/osd-dev-utils/target/proc_runner/with_proc_runner.js:45:9)
                at run (/Users/huanji/src/huan-osd/packages/osd-dev-utils/target/run/run.js:63:9)
      info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.
          at makeError (/Users/huanji/src/huan-osd/packages/osd-pm/dist/index.js:25150:11)
          at handlePromise (/Users/huanji/src/huan-osd/packages/osd-pm/dist/index.js:24085:26)
          at processTicksAndRejections (internal/process/task_queues.js:95:5)
          at async /Users/huanji/src/huan-osd/packages/osd-pm/dist/index.js:9051:9
          at async scheduleItem (/Users/huanji/src/huan-osd/packages/osd-pm/dist/index.js:10938:9)
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

This was not visible when submitting the PR, but based on the error message, it is related to dynamic type casting.

why do we add encryption logic a save object service? saved object service should be low level data access api without adding too much specific business logic.

credential is one of saved object type, the implementation should not touch code here

For the time being, it is hardcoded as part of the proof of concept. Will refactor it after learning more about the codebase. Before that, do you have any suggestions or proposals?

Copy link

@seraphjiang seraphjiang Jun 26, 2022

Choose a reason for hiding this comment

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

It is not about hard code. it is about it should not adding any credential specific logic to data access tier. Credential type is only one of Saved Object type.

current poc appraoch is against Dependency inversion principle

Savedobject is low-level module, credential management is high-level module
high-level modules should independent of the low-level module implementation details

One possible approach should be.
Browser --> invoke Credential RESTFul API through http --> Credential API handle receive the request (add your code here)--> Invoke Saved Object API

Choose a reason for hiding this comment

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

let's do some abstraction of credentials, we will support other auth types in addition to basic auth in the future

@seraphjiang
Copy link

seraphjiang commented Jun 25, 2022

  • We should registry credential plugin inside stack management navigation not the top level application navigation.

actual behavior in first iteration
image

expected to add to same level as index pattern management
http://localhost:5601/app/management/opensearch-dashboards/credentials

image

@seraphjiang
Copy link

are we going to implement an end to end save credential scenario from UI as next step?
i asked because i'm not able to save credential from UI yet.

@noCharger
Copy link
Collaborator Author

noCharger commented Jun 26, 2022

  • We should registry credential plugin inside stack management navigation not the top level application navigation.

actual behavior in first iteration image

expected to add to same level as index pattern management http://localhost:5601/app/management/opensearch-dashboards/credentials

image

are we going to implement an end to end save credential scenario from UI as next step? i asked because i'm not able to save credential from UI yet.

As you noticed, this PoC includes no UI changes. The main changes are as follows: 1. Save credentials (username, password) via API; and 2. Store encrypted data on metadata index. Since it shouldn't be too difficult to incorporate it into the stack management navigation; I'll update it in the next iteration.

@seraphjiang seraphjiang requested a review from a team June 26, 2022 00:15
Comment on lines 264 to 270
if (
'credential' === type &&
typeof attributes !== 'undefined' &&
typeof attributes.password !== 'undefined'
) {
attributes.password = await this._crypto_cli.encrypt(attributes.password);
}

Choose a reason for hiding this comment

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

let's do some abstraction of credentials, we will support other auth types in addition to basic auth in the future

@@ -5,3 +5,5 @@ exports[`TagCloudVisualizationTest TagCloudVisualization - basics simple draw 1`
exports[`TagCloudVisualizationTest TagCloudVisualization - basics with param change 1`] = `"<svg width=\\"256\\" height=\\"386\\"><g width=\\"256\\" height=\\"386\\"><text style=\\"font-size: 0px; font-style: normal; font-weight: normal; font-family: Inter UI, sans-serif; fill: #00a69b;\\" text-anchor=\\"middle\\" transform=\\"\\" data-test-subj=\\"CN\\">CN</text><text style=\\"font-size: 0px; font-style: normal; font-weight: normal; font-family: Inter UI, sans-serif; fill: #57c17b;\\" text-anchor=\\"middle\\" transform=\\"\\" data-test-subj=\\"IN\\">IN</text><text style=\\"font-size: 0px; font-style: normal; font-weight: normal; font-family: Inter UI, sans-serif; fill: #6f87d8;\\" text-anchor=\\"middle\\" transform=\\"\\" data-test-subj=\\"US\\">US</text><text style=\\"font-size: 0px; font-style: normal; font-weight: normal; font-family: Inter UI, sans-serif; fill: #663db8;\\" text-anchor=\\"middle\\" transform=\\"\\" data-test-subj=\\"DE\\">DE</text><text style=\\"font-size: 0px; font-style: normal; font-weight: normal; font-family: Inter UI, sans-serif; fill: #bc52bc;\\" text-anchor=\\"middle\\" transform=\\"\\" data-test-subj=\\"BR\\">BR</text></g></svg>"`;

Choose a reason for hiding this comment

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

what's change for this file. should we discard

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Will revert back to what existing on repo

```
cd <root dir>

ts-node src/plugins/credential_management/server/crypto/crypto_materials_generator.ts --keyName='aes-name' --keyNamespace='aes-namespace'

Choose a reason for hiding this comment

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

Comment on lines +37 to +50
mappings: {
dynamic: false,
properties: {
credential_name: {
type: 'text',
},
credential_type: {
type: 'keyword',
},
credential_material: { type: 'object' },
},
},
migrations: {},
};

Choose a reason for hiding this comment

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

could you run prettier to format code?

body: schema.object({
credential_name: schema.string(),
credential_type: schema.oneOf([
schema.literal('basic_auth'),

Choose a reason for hiding this comment

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

i prefer to use username_password_credential here instead of basic_auth

basic auth is authentication type, not credential type.

In basic HTTP authentication, a request contains a header field in the form of Authorization: Basic , where credentials is the Base64 encoding of ID and password joined by a single colon :

image

password: schema.string()
})),
aws_iam_credential_JSON: schema.maybe(schema.object({
aws_iam_credential: schema.string()

Choose a reason for hiding this comment

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

if we are talking about aws credential

should it contains

aws_access_key_id = YOUR_AWS_ACCESS_KEY_ID
aws_secret_access_key = YOUR_AWS_SECRET_ACCESS_KEY

and region as optional?

// TODO: Refactor handler, add logger, etc
export async function createHandler(request: OpenSearchDashboardsRequest) {
const cryptoCli = CryptoCli.getInstance()
if (request.body.credential_type == 'basic_auth') {

Choose a reason for hiding this comment

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

a good opportunity to use factory pattern to increase extensibility and testability

const credential = credentialFactory.create(options)

not required, think about it.

https://blog.sessionstack.com/how-javascript-works-the-factory-design-pattern-4-use-cases-7b9f0d22151d

private constructor() {
const wrappingSuite = RawAesWrappingSuiteIdentifier.AES256_GCM_IV12_TAG16_NO_PADDING;

// TODO: Load config during bootstrap
Copy link

@seraphjiang seraphjiang Jun 30, 2022

Choose a reason for hiding this comment

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

i like this todo, let's divide and conquer more

Copy link

@seraphjiang seraphjiang left a comment

Choose a reason for hiding this comment

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

let's merge to main branch

@seraphjiang
Copy link

Would you also update PR description to add actual cli to test against credential api instead of saved object api

      path: '/api/credential_management/create',

Cli command and output:

curl --include -XPOST localhost:5601/api/saved_objects/credential \
    -H "Content-Type: application/json" \
    -H "osd-xsrf: true" \
    -d'{"attributes": {"credential_name": "test-credential-2", "user_name": "louis", "password": "abcd"}}'
HTTP/1.1 200 OK
osd-name: 88665a1d4119.ant.amazon.com
content-type: application/json; charset=utf-8
cache-control: private, no-cache, no-store, must-revalidate
content-length: 805
Date: Sat, 25 Jun 2022 03:47:08 GMT
Connection: keep-alive
Keep-Alive: timeout=120

{"type":"credential","id":"7c42ab60-f439-11ec-8934-e9ce406d7ec9","attributes":{"credential_name":"test-credential-2","user_name":"louis","password":"AgV4zWF85l7iqS6pZbmARHsxAcPOk6FSLwWJ7aTDW9ydnBYAXwABABVhd3MtY3J5cHRvLXB1YmxpYy1rZXkAREF6TWhWSjFVTjY3bFRZWFROVEgzZkNIRndXWjd2RityMTZjdWphd3pEMGszbEpKcm5MalVZSGpCN2FOcGNxRGFmZz09AAEADWFlcy1uYW1lc3BhY2UAHGFlcy1uYW1lAAAAgAAAAAzdNZqWGaqd8jjhxFsAMEJu8HkAnXz/EsPSw8ff3+lISA0JAiRGhnViL5jDArcAkvOA5Pxjn9AWuw5tVhRjBgIAABAA5o7ye3D/GeeXXcIKl3PZz9Ye2ZpX0cBE+gekw66xIsqghprhw/lhnJkB3Ar7JXXy/////wAAAAEAAAAAAAAAAAAAAAEAAAAE9WVy/Uq/WFDNgrew2ECI37nD3fcAZjBkAjBSnsdRlLUMrvMbdQYG2NlHRNr8KWW6t7VJiM+vCoSbll5eW/WA6M9ROb7qGyMidaICMAdp8pilRjr9s0/lh44OLQHAJb1VNXbM1etbvAKAoIcBkLBfV1iQDec3GncM4CdNHw=="},"references":[],"updated_at":"2022-06-25T03:47:07.669Z","version":"WzUsMV0="}%  

Saved object on Opensearch cluster


const args = require('yargs').argv;

CryptoCli.generateCryptoMaterials(args.keyName, args.keyNamespace);

Choose a reason for hiding this comment

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

this file should not belong to src/server, it should move to scripts folder.

@noCharger noCharger merged commit f274d23 into 2.x Jun 30, 2022
@noCharger
Copy link
Collaborator Author

Would you also update PR description to add actual cli to test against credential api instead of saved object api

      path: '/api/credential_management/create',

Cli command and output:

curl --include -XPOST localhost:5601/api/saved_objects/credential \
    -H "Content-Type: application/json" \
    -H "osd-xsrf: true" \
    -d'{"attributes": {"credential_name": "test-credential-2", "user_name": "louis", "password": "abcd"}}'
HTTP/1.1 200 OK
osd-name: 88665a1d4119.ant.amazon.com
content-type: application/json; charset=utf-8
cache-control: private, no-cache, no-store, must-revalidate
content-length: 805
Date: Sat, 25 Jun 2022 03:47:08 GMT
Connection: keep-alive
Keep-Alive: timeout=120

{"type":"credential","id":"7c42ab60-f439-11ec-8934-e9ce406d7ec9","attributes":{"credential_name":"test-credential-2","user_name":"louis","password":"AgV4zWF85l7iqS6pZbmARHsxAcPOk6FSLwWJ7aTDW9ydnBYAXwABABVhd3MtY3J5cHRvLXB1YmxpYy1rZXkAREF6TWhWSjFVTjY3bFRZWFROVEgzZkNIRndXWjd2RityMTZjdWphd3pEMGszbEpKcm5MalVZSGpCN2FOcGNxRGFmZz09AAEADWFlcy1uYW1lc3BhY2UAHGFlcy1uYW1lAAAAgAAAAAzdNZqWGaqd8jjhxFsAMEJu8HkAnXz/EsPSw8ff3+lISA0JAiRGhnViL5jDArcAkvOA5Pxjn9AWuw5tVhRjBgIAABAA5o7ye3D/GeeXXcIKl3PZz9Ye2ZpX0cBE+gekw66xIsqghprhw/lhnJkB3Ar7JXXy/////wAAAAEAAAAAAAAAAAAAAAEAAAAE9WVy/Uq/WFDNgrew2ECI37nD3fcAZjBkAjBSnsdRlLUMrvMbdQYG2NlHRNr8KWW6t7VJiM+vCoSbll5eW/WA6M9ROb7qGyMidaICMAdp8pilRjr9s0/lh44OLQHAJb1VNXbM1etbvAKAoIcBkLBfV1iQDec3GncM4CdNHw=="},"references":[],"updated_at":"2022-06-25T03:47:07.669Z","version":"WzUsMV0="}%  

Saved object on Opensearch cluster

Updated and merged PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants