Moved encryption-sdk from lib to modules to resolve dependency issues #9812
Conversation
vikasvb90
requested review from
reta,
anasalkouz,
andrross,
Bukhtawar,
CEHENKLE,
dblock,
gbbafna,
setiah,
kartg,
kotwanikunal,
mch2,
nknize,
owaiskazi19,
peternied,
Rishikesh1159,
ryanbogan,
saratvemulapalli,
shwetathareja,
dreamer-89,
tlfeng,
VachaShah,
dbwiddis,
sachinpkale,
sohami and
msfroh
as code owners
vikasvb90
force-pushed
the
move_encryption_sdk_from_lib_to_modules
branch
from
e4d8881 to
b20327d
Compare
Gradle Check (Jenkins) Run Completed with:
|
Gradle Check (Jenkins) Run Completed with:
|
Compatibility status:Checks if related components are compatible with change e4d8881 Incompatible componentsIncompatible components: [https://github.com/opensearch-project/job-scheduler.git] Skipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git] |
Gradle Check (Jenkins) Run Completed with:
|
Compatibility status:Checks if related components are compatible with change fafeecf Incompatible componentsIncompatible components: [https://github.com/opensearch-project/job-scheduler.git] Skipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git] |
vikasvb90
force-pushed
the
move_encryption_sdk_from_lib_to_modules
branch
from
fafeecf to
2705179
Compare
Gradle Check (Jenkins) Run Completed with:
|
vikasvb90
force-pushed
the
move_encryption_sdk_from_lib_to_modules
branch
from
2705179 to
9045dab
Compare
Compatibility status:Checks if related components are compatible with change 2705179 Incompatible componentsSkipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git] |
Compatibility status:Checks if related components are compatible with change 9045dab Incompatible componentsSkipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git] |
vikasvb90
force-pushed
the
move_encryption_sdk_from_lib_to_modules
branch
from
9045dab to
23ba76e
Compare
Gradle Check (Jenkins) Run Completed with:
|
vikasvb90
force-pushed
the
move_encryption_sdk_from_lib_to_modules
branch
from
23ba76e to
0549b05
Compare
Gradle Check (Jenkins) Run Completed with:
|
Compatibility status:Checks if related components are compatible with change 23ba76e Incompatible componentsSkipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git] |
Compatibility status:Checks if related components are compatible with change 0549b05 Incompatible componentsSkipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/reporting.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git] |
Gradle Check (Jenkins) Run Completed with:
|
There was a problem hiding this comment.
I'm cross referencing with the initial pull request for encryption-sdk [1], please forgive any misses between the PRs. I've also created separate threads for the different topics that are being discussed to make it easier to track resolution.
I think there are several areas that still need to be resolved before moving forward with this change, that are each captured in their own thread.
- Bouncy Castle Dependency
- Lib, Module, vs Plugin
- End to End Testing
- Security Review
There was a problem hiding this comment.
[Bouncy Castle Dependency]
I do not see usage of bouncy castle, what is the need to include it?
| @@ -15,12 +15,17 @@ thirdPartyAudit.enabled = false | |||
| forbiddenApisTest.ignoreFailures = true | |||
| testingConventions.enabled = false | |||
|
|
|||
| opensearchplugin { | |||
| description 'Crypto module plugin for providing encryption and decryption support.' | |||
There was a problem hiding this comment.
[Lib, Module, vs Plugin]
@reta Modules supposed to have the functionality that is critical for OpenSearch to function, since this feature is optional (from above), the convince to use is not an argument to bundle it in. We could reconsider the decision(s) later on if needed.
@reta In the same vein, this is experimental API and feature, why it is not behind the experimental flag?
I share this concern, this has been done with other extensions of OpenSearch, such as Identity. See its PR [1] for how boundaries were created between the core components (IdentityService & IdentityPlugin), and external implementations (identity-shiro plugin).
Using the feature flag isolates OpenSearch from any issues with an IdentityPlugin [2].
- [1] Identity Pull Request [Extensions] Introduce Identity Plugin to Core #7246
- [2] Feature Flag for Noop https://github.com/opensearch-project/OpenSearch/pull/7246/files#diff-f839350640f8bd27f602b3249215252bf07afef5d7e480b23271788af6cda331R459
There was a problem hiding this comment.
Analyse/painless plugins can be optional based on the use cases. Not fully convinced that only critical features should sit in module.
This particular use case is solving the problem of not having end user worry about installing a plugin for handling the encryption logic and then another plugin for providing the keys
There were threads on making security a first class citizen. My understanding was we want to move to a model where security is enabled by default which makes modules a better choice
There was a problem hiding this comment.
Thanks @Bukhtawar
This particular use case is solving the problem of not having end user worry about installing a plugin for handling the encryption logic and then another plugin for providing the keys
This is not by exclusion, when we get to this stage - we could reconsider the decision to serve users more conveniently, now we are making the premature, unproven and unneeded decision that would be hard to reverse.
There were threads on making security a first class citizen. My understanding was we want to move to a model where security is enabled by default which makes modules a better choice
Yes, security is super important, and we recommend users to have security plugin installed all the time (not module), and it works very well so far.
There was a problem hiding this comment.
Thanks @Bukhtawar
This particular use case is solving the problem of not having end user worry about installing a plugin for handling the encryption logic and then another plugin for providing the keys
This is not by exclusion, when we get to this stage - we could reconsider the decision to serve users more conveniently, now we are making the premature, unproven and unneeded decision that would be hard to reverse.
Would appreciate if you could take a look at the PR before assuming the same
modules/crypto/src/main/java/org/opensearch/encryption/CryptoModulePlugin.java
Show resolved
Hide resolved
modules/crypto/src/test/java/org/opensearch/encryption/CryptoModulePluginTests.java
Show resolved
Hide resolved
Signed-off-by: Vikas Bansal <[email protected]>
vikasvb90
force-pushed
the
move_encryption_sdk_from_lib_to_modules
branch
from
0549b05 to
4214a81
Compare
| dependencies { | ||
| // Common crypto classes | ||
| api project(':libs:opensearch-common') | ||
|
|
||
| // Encryption | ||
| implementation "com.amazonaws:aws-encryption-sdk-java:2.4.0" | ||
| implementation "com.amazonaws:aws-encryption-sdk-java:1.7.0" |
There was a problem hiding this comment.
Unrelated, but why we are using version that was release in 2020? https://mvnrepository.com/artifact/com.amazonaws/aws-encryption-sdk-java/1.7.0
There was a problem hiding this comment.
There was a problem hiding this comment.
Please check this:
There are CVEs reported
There was a problem hiding this comment.
In indirect vulnerabilities, first 2 are coming from bouncy castle (bcprov-ext-jdk15on) and third one is coming from junit. I haven't used that dependency. I am using bcprov-jdk15to18 v1.75 which doesn't have a vulnerability.
The direct vulnerability is actually a feature addition to strengthen the security. In order to adopt this, SDK needs to be migrated to 2.x. This can only be done if data encrypted by previous versions <1.7 have been ported to 1.7 first.
There was a problem hiding this comment.
We don't have data encrypted with previous version <1.7, do we?
There was a problem hiding this comment.
In OS we don't. If someone has customization done in BlobContainer implementations to encrypt data then they might be encrypting from previous versions.
There was a problem hiding this comment.
If someone has customization done in BlobContainer implementations to encrypt data then they might be encrypting from previous versions.
This is not the OpenSearch concern, if someone has customization to encrypt, she definitely has the customization to decrypt. We should use the latest 2.4.1 I think.
There was a problem hiding this comment.
This is not about customization in decrypt because of customization in encrypt. This is about handling encrypted data of users who didn't have a choice then because OS didn't have encryption support and had to build their own encryption layer. OpenSearch bringing encryption now shouldn't force this on such users and just leave them with choice of either reindexing their data or ignoring the encryption altogether which might mean incompatible dependency issues in which case this option will no longer be valid.
It should be OpenSearch's concern to gracefully allow these users to migrate to newer versions especially when it can do so. AWS ESDK has v1.7 rolled out for this specific purpose and is both forward and backward compatible. OS can decide to upgrade SDK to 2.x in a major version upgrade (say 4.0) which would the right step aligned with major version bump property of introducing a breaking change.
There was a problem hiding this comment.
@vikasvb90 we are talking about one specific library, it is not feasible to know, more to support, what kind of mechanisms or algorithms users could have been used for encryption / decryption of their data prior to whatever we would be introducing in OpenSearch. To your point,. we surely could help them and I have no issues supporting 1.x release line if it is maintained, so far it does not look like. People flood us with CVEs same day they are released, if this library + version is going to be flagged by security scanning tools (which it seems like it will) - this is no go.
Gradle Check (Jenkins) Run Completed with:
|
Codecov Report
@@ Coverage Diff @@
## main #9812 +/- ##
============================================
- Coverage 71.14% 71.08% -0.06%
- Complexity 57973 57979 +6
============================================
Files 4825 4827 +2
Lines 273348 273529 +181
Branches 39841 39860 +19
============================================
- Hits 194473 194447 -26
- Misses 62523 62732 +209
+ Partials 16352 16350 -2
|
Compatibility status:Checks if related components are compatible with change 4214a81 Incompatible componentsSkipped componentsCompatible componentsCompatible components: [https://github.com/opensearch-project/geospatial.git, https://github.com/opensearch-project/security.git, https://github.com/opensearch-project/notifications.git, https://github.com/opensearch-project/neural-search.git, https://github.com/opensearch-project/index-management.git, https://github.com/opensearch-project/sql.git, https://github.com/opensearch-project/security-analytics.git, https://github.com/opensearch-project/job-scheduler.git, https://github.com/opensearch-project/observability.git, https://github.com/opensearch-project/opensearch-oci-object-storage.git, https://github.com/opensearch-project/k-nn.git, https://github.com/opensearch-project/cross-cluster-replication.git, https://github.com/opensearch-project/alerting.git, https://github.com/opensearch-project/anomaly-detection.git, https://github.com/opensearch-project/performance-analyzer.git, https://github.com/opensearch-project/asynchronous-search.git, https://github.com/opensearch-project/ml-commons.git, https://github.com/opensearch-project/performance-analyzer-rca.git, https://github.com/opensearch-project/common-utils.git, https://github.com/opensearch-project/reporting.git] |
|
This PR is stalled because it has been open for 30 days with no activity. |
|
@vikasvb90 How would you like to move forward with this pull request? |
@peternied I am closing this PR for now as we are yet to find a viable path forward to support multipart transfer. |
Description
To resolve dependency issues happened due to lib:encryption-sdk dependencies being pulled in core which was affecting other plugins, this PR is purposed to move crypto codebase to modules.
Related Issues
Resolves #[Issue number to be closed when this PR is merged]
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.