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

Moved encryption-sdk from lib to modules to resolve dependency issues #9812

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions distribution/tools/plugin-cli/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,6 @@ dependencies {
implementation 'org.apache.commons:commons-compress:1.23.0'
}

configurations.implementation {
exclude group: 'org.bouncycastle', module: 'bcprov-jdk15to18'
}

tasks.named("dependencyLicenses").configure {
mapping from: /bc.*/, to: 'bouncycastle'
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.opensearch.common.annotation.ExperimentalApi;
import org.opensearch.common.io.InputStreamContainer;

import java.io.Closeable;
import java.io.IOException;
import java.io.InputStream;

Expand All @@ -22,7 +23,7 @@
* U - Parsed Encryption Metadata / CryptoContext
*/
@ExperimentalApi
public interface CryptoHandler<T, U> {
public interface CryptoHandler<T, U> extends Closeable {

/**
* To initialise or create a new crypto metadata to be used in encryption. This is needed to set the context before
Expand Down

This file was deleted.

This file was deleted.

This file was deleted.

Copy link
Member

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?

Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,17 @@ thirdPartyAudit.enabled = false
forbiddenApisTest.ignoreFailures = true
testingConventions.enabled = false

opensearchplugin {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

@vikasvb90 vikasvb90 Sep 6, 2023

Choose a reason for hiding this comment

The 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 .

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @reta . Users need to provide CryptoMetadata while registering a repository. Hence this feature wouldn't get used by default. The default installation would only populate the CryptoHandlerRegistry with one option of doing enc-dec . If we install it as a module , it makes usage easier by just supplying/installing the KeyProvider and not bothering about installing/configuring CryptoModulePlugin.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @gbbafna

Hence this feature wouldn't get used by default.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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 .

Copy link
Collaborator

@reta reta Sep 6, 2023

Choose a reason for hiding this comment

The 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 plugins/example-plugins instead, definitely (to me) not in a state to be promoted to a core module.

Copy link
Collaborator

@reta reta Sep 6, 2023

Choose a reason for hiding this comment

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

@gbbafna thank you

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 .

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?

Copy link
Collaborator

@gbbafna gbbafna Sep 6, 2023

Choose a reason for hiding this comment

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

@reta , would it be fine to push this as a module for 2.11 then ? Meanwhile, we are removing encryption-sdk from core : #9833

Copy link
Collaborator

Choose a reason for hiding this comment

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

@reta , would it be fine to push this as a module for 2.11 then ? Meanwhile, we are removing encryption-sdk from core : #9833

Thanks @gbbafna , we'll definitely get back to this once the 2.10 rush is over

description 'Crypto module plugin for providing encryption and decryption support.'
Copy link
Member

Choose a reason for hiding this comment

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

[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].

Copy link
Collaborator

@Bukhtawar Bukhtawar Sep 6, 2023

Choose a reason for hiding this comment

The 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

#9422 (comment)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator

@reta reta Sep 6, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

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"
Copy link
Collaborator

Choose a reason for hiding this comment

The 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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please check this:

image

There are CVEs reported

Copy link
Contributor Author

@vikasvb90 vikasvb90 Sep 6, 2023

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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.

Copy link
Collaborator

Choose a reason for hiding this comment

The 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
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
51704a672e65456d37f444c5992c079feff31218
1 change: 1 addition & 0 deletions modules/crypto/licenses/bcprov-jdk15to18-1.75.jar.sha1
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
df22e1b6a9f6b218913f5b68dd16641344397fe0
22 changes: 22 additions & 0 deletions modules/crypto/licenses/bcprov-jdk15to18-LICENSE.txt
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.
Empty file.
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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -125,4 +125,8 @@
return new DecryptedRangedStreamProvider(range, (encryptedStream) -> encryptedStream);
}

@Override
public void close() {
// Nothing to close.
}

Check warning on line 131 in modules/crypto/src/main/java/org/opensearch/encryption/NoOpCryptoHandler.java

View check run for this annotation

Codecov / codecov/patch

modules/crypto/src/main/java/org/opensearch/encryption/NoOpCryptoHandler.java#L131

Added line #L131 was not covered by tests
}
Loading
Loading