-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Moved encryption-sdk from lib to modules to resolve dependency issues #9812
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,12 +15,17 @@ thirdPartyAudit.enabled = false | |
forbiddenApisTest.ignoreFailures = true | ||
testingConventions.enabled = false | ||
|
||
opensearchplugin { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vikasvb90 please move it to plugins (or even better, sandbox plugins), we should not install it by default, this is experimental feature There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @reta We had a discussion here and have been having discussions in the past. Based on this it was decided that security should be a first class member of the codebase. The only reason of moving it to modules is because of dependency issues which might need a bigger campaign. Adding @gbbafna @Bukhtawar @andrross . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @reta . Users need to provide There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @gbbafna
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the same vein, this is experimental API and feature, why it is not behind the experimental flag? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The vision here is this module itself would contain all types of encryption handlers - frame based , block based , CTR . Depending upon the settings, a cluster would be able to chose its own method of encryption . There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @vikasvb90 thank you, since more details on the matter are revealed, I am more on the side to not even have changes delivered in 2.10 but move it off to 2.11 when with actual CryptoHandler which does actual encryption/decryption. If we want to show case users this new feature, it is valid point - it should go to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @gbbafna thank you
Could we introduce this module / plugin / lib when vision becomes a bit more materialized? What value the current vision (== this pull request in current form) has for users? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
description 'Crypto module plugin for providing encryption and decryption support.' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Lib, Module, vs Plugin]
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].
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc: @reta @peternied There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @Bukhtawar
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.
Yes, security is super important, and we recommend users to have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Would appreciate if you could take a look at the PR before assuming the same |
||
classname 'org.opensearch.encryption.CryptoModulePlugin' | ||
} | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't have data encrypted with previous version <1.7, do we? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
implementation "org.bouncycastle:bcprov-jdk15to18:${versions.bouncycastle}" | ||
implementation "org.apache.commons:commons-lang3:${versions.commonslang}" | ||
|
||
//Tests | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
51704a672e65456d37f444c5992c079feff31218 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
df22e1b6a9f6b218913f5b68dd16641344397fe0 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
The MIT License (MIT) | ||
|
||
Copyright (c) 2000 - 2013 The Legion of the Bouncy Castle Inc. | ||
(http://www.bouncycastle.org) | ||
|
||
Permission is hereby granted, free of charge, to any person obtaining a copy | ||
of this software and associated documentation files (the "Software"), to deal | ||
in the Software without restriction, including without limitation the rights | ||
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell | ||
copies of the Software, and to permit persons to whom the Software is | ||
furnished to do so, subject to the following conditions: | ||
|
||
The above copyright notice and this permission notice shall be included in | ||
all copies or substantial portions of the Software. | ||
|
||
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR | ||
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, | ||
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE | ||
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER | ||
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, | ||
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN | ||
THE SOFTWARE. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,67 @@ | ||
/* | ||
* SPDX-License-Identifier: Apache-2.0 | ||
* | ||
* The OpenSearch Contributors require contributions made to | ||
* this file be licensed under the Apache-2.0 license or a | ||
* compatible open source license. | ||
*/ | ||
|
||
package org.opensearch.encryption; | ||
|
||
import org.opensearch.common.crypto.CryptoHandler; | ||
import org.opensearch.common.crypto.MasterKeyProvider; | ||
import org.opensearch.common.unit.TimeValue; | ||
import org.opensearch.encryption.keyprovider.CryptoMasterKey; | ||
import org.opensearch.plugins.CryptoPlugin; | ||
import org.opensearch.plugins.Plugin; | ||
|
||
import java.security.SecureRandom; | ||
import java.util.concurrent.TimeUnit; | ||
|
||
import com.amazonaws.encryptionsdk.caching.CachingCryptoMaterialsManager; | ||
import com.amazonaws.encryptionsdk.caching.LocalCryptoMaterialsCache; | ||
|
||
public class CryptoModulePlugin extends Plugin implements CryptoPlugin<Object, Object> { | ||
|
||
private final int dataKeyCacheSize = 500; | ||
private final String algorithm = "ALG_AES_256_GCM_HKDF_SHA512_COMMIT_KEY"; | ||
gbbafna marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// - Cache TTL and Jitter is used to decide the Crypto Cache TTL. | ||
// - Random number between: (TTL Jitter, TTL - Jitter) | ||
private final long dataKeyCacheTTL = TimeValue.timeValueDays(2).getMillis(); | ||
private static final long dataKeyCacheJitter = TimeUnit.MINUTES.toMillis(30); // - 30 minutes | ||
|
||
public CryptoHandler<Object, Object> getOrCreateCryptoHandler( | ||
MasterKeyProvider keyProvider, | ||
String keyProviderName, | ||
String keyProviderType, | ||
Runnable onClose | ||
) { | ||
CachingCryptoMaterialsManager materialsManager = createMaterialsManager(keyProvider, keyProviderName, algorithm); | ||
return createCryptoHandler(algorithm, materialsManager, keyProvider); | ||
} | ||
|
||
// package private for tests | ||
CryptoHandler<Object, Object> createCryptoHandler( | ||
String algorithm, | ||
CachingCryptoMaterialsManager materialsManager, | ||
MasterKeyProvider masterKeyProvider | ||
) { | ||
return new NoOpCryptoHandler(); | ||
} | ||
|
||
// Package private for tests | ||
CachingCryptoMaterialsManager createMaterialsManager(MasterKeyProvider masterKeyProvider, String keyProviderName, String algorithm) { | ||
SecureRandom r = new SecureRandom(); | ||
long low = dataKeyCacheTTL - dataKeyCacheJitter; | ||
long high = dataKeyCacheTTL + dataKeyCacheJitter; | ||
long masterKeyCacheTTL = r.nextInt((int) (high - low)) + low; | ||
|
||
CryptoMasterKey cryptoMasterKey = new CryptoMasterKey(masterKeyProvider, keyProviderName, algorithm); | ||
return CachingCryptoMaterialsManager.newBuilder() | ||
.withMasterKeyProvider(cryptoMasterKey) | ||
.withCache(new LocalCryptoMaterialsCache(dataKeyCacheSize)) | ||
.withMaxAge(masterKeyCacheTTL, TimeUnit.MILLISECONDS) | ||
.build(); | ||
} | ||
} |
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.
[Bouncy Castle Dependency]
I do not see usage of bouncy castle, what is the need to include it?