-
Notifications
You must be signed in to change notification settings - Fork 0
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
Conversation
*/ | ||
|
||
/* | ||
* Licensed to Elasticsearch B.V. under one or more contributor |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
fail to run
|
if ( | ||
'credential' === type && | ||
typeof attributes !== 'undefined' && | ||
typeof attributes.password !== 'undefined' | ||
) { | ||
attributes.password = await this._crypto_cli.encrypt(attributes.password); | ||
} |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
actual behavior in first iteration expected to add to same level as index pattern management |
are we going to implement an end to end save credential scenario from UI as next step? |
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. |
src/core/server/saved_objects/service/lib/crypto/crypto_metadata.json
Outdated
Show resolved
Hide resolved
if ( | ||
'credential' === type && | ||
typeof attributes !== 'undefined' && | ||
typeof attributes.password !== 'undefined' | ||
) { | ||
attributes.password = await this._crypto_cli.encrypt(attributes.password); | ||
} |
There was a problem hiding this comment.
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
Should also add to .git_ignore
This reverts commit e4ade32.
@@ -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>"`; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if this is script/utility tool, we should move to https://github.com/elastic-analytics/OpenSearch-Dashboards/tree/main/scripts
mappings: { | ||
dynamic: false, | ||
properties: { | ||
credential_name: { | ||
type: 'text', | ||
}, | ||
credential_type: { | ||
type: 'keyword', | ||
}, | ||
credential_material: { type: 'object' }, | ||
}, | ||
}, | ||
migrations: {}, | ||
}; |
There was a problem hiding this comment.
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'), |
There was a problem hiding this comment.
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 :
password: schema.string() | ||
})), | ||
aws_iam_credential_JSON: schema.maybe(schema.object({ | ||
aws_iam_credential: schema.string() |
There was a problem hiding this comment.
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') { |
There was a problem hiding this comment.
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.
private constructor() { | ||
const wrappingSuite = RawAesWrappingSuiteIdentifier.AES256_GCM_IV12_TAG16_NO_PADDING; | ||
|
||
// TODO: Load config during bootstrap |
There was a problem hiding this comment.
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
There was a problem hiding this 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
Would you also update PR description to add actual cli to test against credential api instead of saved object api
|
|
||
const args = require('yargs').argv; | ||
|
||
CryptoCli.generateCryptoMaterials(args.keyName, args.keyNamespace); |
There was a problem hiding this comment.
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.
Updated and merged PR |
Description
Two main features:
Issues Resolved
opensearch-project#1804
Check List
yarn test:jest
yarn test:jest_integration
yarn test:ftr
Manual testing