-
Notifications
You must be signed in to change notification settings - Fork 918
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
[MD] Add data source signing support #2510
Conversation
c448da5
to
2316cc9
Compare
2316cc9
to
249f0bd
Compare
CHANGELOG.md
Outdated
@@ -10,18 +12,20 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) | |||
|
|||
### 📈 Features/Enhancements | |||
|
|||
* [MD] Support legacy client for data source ([#2204](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2204)) | |||
* [Plugin Helpers] Facilitate version changes ([#2398](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2398)) | |||
- [MD] Support legacy client for data source ([#2204](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2204)) |
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 think the auto linter here needs a fix since this will cause conflicts with all existing PR's if merged.
@noCharger Can we keep the diff limited to just the change? I know it can be tempting (or just an accident) to push in a bunch of other linting fixes but that makes the PR harder to read and is also usually a source for other merge conflicts unrelated to your change. We can address them in a separate PR. |
@ashwin-pc Yes it's triggered by the prettier. Created a seperate issue for the lint on CHANGELOG #2519 and removed the lint changes on this PR. |
249f0bd
to
494afff
Compare
494afff
to
ea3f6c4
Compare
src/plugins/data_source/server/saved_objects/data_source_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
src/plugins/data_source/server/saved_objects/data_source_saved_objects_client_wrapper.ts
Show resolved
Hide resolved
2b1ed36
to
389c718
Compare
Codecov Report
@@ Coverage Diff @@
## main #2510 +/- ##
=======================================
Coverage 66.73% 66.73%
=======================================
Files 3203 3204 +1
Lines 60978 60988 +10
Branches 9274 9275 +1
=======================================
+ Hits 40692 40702 +10
- Misses 18067 18068 +1
+ Partials 2219 2218 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
One favor, could you mention the testing steps about how to reproduce the attack and expected error message? This way others know how to test as well. |
@kristenTian Reproduce the attack seems to be a risky thing because it's not a bug but a system level security leak. From system architecture, we cannot gaurantee the document is untouched on OpenSearch side. Added one simple (non-hacking way) example here From OSD side, there are UTs on PR |
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.
lgtm. found 1 typo
389c718
to
8489917
Compare
src/plugins/data_source/server/saved_objects/data_source_saved_objects_client_wrapper.ts
Outdated
Show resolved
Hide resolved
cec2e30
to
7a1dedf
Compare
7a1dedf
to
aed93df
Compare
Signed-off-by: Louis Chu <[email protected]>
Signed-off-by: Louis Chu <[email protected]>
Signed-off-by: Louis Chu <[email protected]>
aed93df
to
2ba789e
Compare
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.
LGTM
* Add data source signing support * Optimize error handling and logging * Update wording on error message and readme Signed-off-by: Louis Chu <[email protected]> (cherry picked from commit e3bbdef)
this.logger.error(errMsg); | ||
this.logger.error(err); |
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.
better to log the error into 1 message
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.
since you throw it on L299, so you don't need to log it here
* Add data source signing support * Optimize error handling and logging * Update wording on error message and readme Signed-off-by: Louis Chu <[email protected]> (cherry picked from commit e3bbdef)
* Add data source signing support * Optimize error handling and logging * Update wording on error message and readme Signed-off-by: Louis Chu <[email protected]> (cherry picked from commit e3bbdef) Co-authored-by: Louis Chu <[email protected]>
…earch-project#2547) * Add data source signing support * Optimize error handling and logging * Update wording on error message and readme Signed-off-by: Louis Chu <[email protected]> (cherry picked from commit e3bbdef) Co-authored-by: Louis Chu <[email protected]>
* Add data source signing support * Optimize error handling and logging * Update wording on error message and readme Signed-off-by: Louis Chu <[email protected]> Signed-off-by: Sergey V. Osipov <[email protected]>
Signed-off-by: Louis Chu [email protected]
Description
Add data source signing support
Signature algorithm: ECDSA with P-384 and SHA-384. Under multiple data source case, data source indices stored on OpenSearch can be modified / replaced by attacker, as @dblock pointed out. With ECDSA signature, ciphertext decryption will fail if it’s getting pullted. No one will be able to create another signature that verifies with the public key because the private key has been dropped.
Step to change endpoint on OpenSearch
GET /.kibana/_doc/data-source:0000000
Issues Resolved
#2520
#2207
Check List
yarn test:jest src/plugins/data_source