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

Crypto plugin abstractions, encrypted repositories and metadata mgmt #7284

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
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,14 @@ public static void registerExceptions() {
V_2_7_0
)
);
registerExceptionHandle(
new OpenSearchExceptionHandle(
org.opensearch.crypto.CryptoClientMissingException.class,
org.opensearch.crypto.CryptoClientMissingException::new,
171,
V_3_0_0
)
);
registerExceptionHandle(
new OpenSearchExceptionHandle(
org.opensearch.cluster.block.IndexCreateBlockException.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,182 @@
/*
* 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.action.admin.cluster.crypto;

import org.opensearch.action.ActionRequestValidationException;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
import org.opensearch.common.io.stream.Writeable;
import org.opensearch.common.settings.Settings;
import org.opensearch.common.xcontent.XContentType;
import org.opensearch.core.xcontent.ToXContentObject;
import org.opensearch.core.xcontent.XContentBuilder;

import java.io.IOException;
import java.util.Map;

import static org.opensearch.action.ValidateActions.addValidationError;
import static org.opensearch.common.settings.Settings.Builder.EMPTY_SETTINGS;
import static org.opensearch.common.settings.Settings.readSettingsFromStream;
import static org.opensearch.common.settings.Settings.writeSettingsToStream;

/**
* Crypto settings supplied during a put repository request
*
* @opensearch.internal
*/
public class CryptoSettings implements Writeable, ToXContentObject {

private String keyProviderName;
private String keyProviderType;
private Settings settings = EMPTY_SETTINGS;

public CryptoSettings(StreamInput in) throws IOException {
keyProviderName = in.readString();
keyProviderType = in.readString();
settings = readSettingsFromStream(in);
Copy link
Member

Choose a reason for hiding this comment

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

are the settings specific to key provider or generic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These are specific to provider.

}

public CryptoSettings(String keyProviderName) {
this.keyProviderName = keyProviderName;
}

/**
* Validate settings supplied in put repository request.
* @return Exception in case validation fails.
*/
public ActionRequestValidationException validate() {
Copy link
Member

Choose a reason for hiding this comment

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

where do we run the validation for specific settings

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Over here

ActionRequestValidationException validationException = null;
if (keyProviderName == null) {
validationException = addValidationError("key_provider_name is missing", validationException);
}
if (keyProviderType == null) {
validationException = addValidationError("key_provider_type is missing", validationException);
}
return validationException;
}

/**
* Returns key provider name
* @return keyProviderName
*/
public String getKeyProviderName() {
return keyProviderName;
}

/**
* Returns key provider type
* @return keyProviderType
*/
public String getKeyProviderType() {
return keyProviderType;
}

/**
* Returns crypto settings
* @return settings
*/
public Settings getSettings() {
return settings;
}

/**
* Constructs a new crypto settings with provided key provider name.
* @param keyProviderName Name of the key provider
*/
public CryptoSettings keyProviderName(String keyProviderName) {
this.keyProviderName = keyProviderName;
return this;
}

/**
* Constructs a new crypto settings with provided key provider type.
* @param keyProviderType Type of key provider to be used in encryption.
*/
public CryptoSettings keyProviderType(String keyProviderType) {
this.keyProviderType = keyProviderType;
return this;
}

/**
* Sets the encryption settings
*
* @param settings for encryption
* @return this request
*/
public CryptoSettings settings(Settings.Builder settings) {
this.settings = settings.build();
return this;
}

/**
* Sets the encryption settings.
*
* @param source encryption settings in json or yaml format
* @param xContentType the content type of the source
* @return this request
*/
public CryptoSettings settings(String source, XContentType xContentType) {
this.settings = Settings.builder().loadFromSource(source, xContentType).build();
return this;
}

/**
* Sets the encryption settings.
*
* @param source encryption settings
* @return this request
*/
public CryptoSettings settings(Map<String, Object> source) {
this.settings = Settings.builder().loadFromMap(source).build();
return this;
}

/**
* Parses crypto settings definition.
*
* @param cryptoDefinition crypto settings definition
*/
public CryptoSettings(Map<String, Object> cryptoDefinition) {
for (Map.Entry<String, Object> entry : cryptoDefinition.entrySet()) {
if (entry.getKey().equals("key_provider_name")) {
keyProviderName(entry.getValue().toString());
} else if (entry.getKey().equals("key_provider_type")) {
keyProviderType(entry.getValue().toString());
} else if (entry.getKey().equals("settings")) {
if (!(entry.getValue() instanceof Map)) {
throw new IllegalArgumentException("Malformed settings section in crypto settings, should include an inner object");
}
@SuppressWarnings("unchecked")
Map<String, Object> sub = (Map<String, Object>) entry.getValue();
settings(sub);
}
}
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(keyProviderName);
out.writeString(keyProviderType);
writeSettingsToStream(settings, out);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.field("key_provider_name", keyProviderName);
builder.field("key_provider_type", keyProviderType);

builder.startObject("settings");
settings.toXContent(builder, params);
builder.endObject();

builder.endObject();
return builder;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* 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.
*/

/**
* Crypto client request and settings handlers.
*/
package org.opensearch.action.admin.cluster.crypto;
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@

package org.opensearch.action.admin.cluster.repositories.put;

import org.opensearch.Version;
import org.opensearch.action.ActionRequestValidationException;
import org.opensearch.action.admin.cluster.crypto.CryptoSettings;
import org.opensearch.action.support.master.AcknowledgedRequest;
import org.opensearch.common.io.stream.StreamInput;
import org.opensearch.common.io.stream.StreamOutput;
Expand All @@ -48,6 +50,7 @@
import static org.opensearch.common.settings.Settings.readSettingsFromStream;
import static org.opensearch.common.settings.Settings.writeSettingsToStream;
import static org.opensearch.common.settings.Settings.Builder.EMPTY_SETTINGS;
import static org.opensearch.common.xcontent.support.XContentMapValues.nodeBooleanValue;

/**
* Register repository request.
Expand All @@ -67,12 +70,22 @@ public class PutRepositoryRequest extends AcknowledgedRequest<PutRepositoryReque

private Settings settings = EMPTY_SETTINGS;

private Boolean encrypted;
private CryptoSettings cryptoSettings;

public PutRepositoryRequest(StreamInput in) throws IOException {
super(in);
name = in.readString();
type = in.readString();
settings = readSettingsFromStream(in);
verify = in.readBoolean();

if (in.getVersion().onOrAfter(Version.V_3_0_0)) {
encrypted = in.readOptionalBoolean();
Copy link
Member

Choose a reason for hiding this comment

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

Why is this an optional boolean? An optional boolean has three states, so what does null mean?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is marked as optional to ensure backward compatibility with existing integrations. We don't want existing repository registrations to start failing from this version. We can treat a null encrypted field as a non encrypted repo. If we move it to readBoolean then it will become a breaking change. To add more info, stream based constructors are typically used in node to node communication.

Copy link
Member

Choose a reason for hiding this comment

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

Optional doesn't give you backwards compatibility; when invoking writeOptionalBoolean you're still unconditionally writing a byte to the stream and the reader must unconditionally read this byte as well. The onOrAfter version check gives you backwards compatibility. I think you can just write a regular boolean and default the field to false if not explicitly set when constructing an instance. I think you can also use a primitive boolean type in the class and avoid the null case all together.

Copy link
Contributor Author

@vikasvb90 vikasvb90 Jun 30, 2023

Choose a reason for hiding this comment

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

I wasn't referring to version backwards compatibility but the ability for an existing api to work without any change with the new field introduction during let's say upgrading the cluster. Let me check this further if I can move it to non optional field without breaking anything and without enforcing the user to pass encrypted field.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into this and following are the two cases :

  1. We keep encrypted field Boolean and use writeBoolean or readBoolean methods instead of their optional counterparts. In this case lets say you have 2 nodes cluster. You trigger a put repository request on non active cluster manager node (say node2) without encrypted field. Node2 will try to form the request for cluster manager node using writeBoolean and it will fail with NPE as it expects a non-null boolean value. If we had used an optional Boolean then it would have done a null check first, written a byte value of 2 and sent request to cluster manager node. On cluster manager node, again it would require optional method to read since it needs to handle the possibility of null value. Basically, to keep a non-primitive field and to use non-optional methods, users will have to change their contracts.
  2. We keep primitive version this field. If you do this then you don't have a null case anymore which means that in all the get calls users will start seeing encrypted field. Again, if there are strict contracts on user side to read these responses, then they will start failing as well because of the unknown field. By keeping non-primitive fields users can selectively change contracts for encryption and other users continue to stay unchanged.

Correct me if I am missing something here which you are trying to point!

Copy link
Member

Choose a reason for hiding this comment

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

Got it, you're right that in order to preserve the "not set" state in the repository settings then we'll need the optional boolean to communicate that third possible state.

if (Boolean.TRUE.equals(encrypted)) {
cryptoSettings = new CryptoSettings(in);
}
}
}

public PutRepositoryRequest() {}
Expand All @@ -93,6 +106,13 @@ public ActionRequestValidationException validate() {
if (type == null) {
validationException = addValidationError("type is missing", validationException);
}
if (Boolean.TRUE.equals(encrypted)) {
if (cryptoSettings == null) {
validationException = addValidationError("crypto_settings is missing", validationException);
} else {
validationException = cryptoSettings.validate();
}
}
return validationException;
}

Expand Down Expand Up @@ -207,6 +227,41 @@ public boolean verify() {
return this.verify;
}

/**
* Sets whether repository data should be encrypted and stored.
*/
public PutRepositoryRequest encrypted(Boolean encrypted) {
this.encrypted = encrypted;
return this;
}

/**
* Returns true if repository should be encrypted
*/
public Boolean encrypted() {
return encrypted;
}

/**
* Sets the repository crypto settings
*
* @param cryptoSettings repository crypto settings
* @return this request
*/
public PutRepositoryRequest cryptoSettings(CryptoSettings cryptoSettings) {
this.cryptoSettings = cryptoSettings;
return this;
}

/**
* Returns repository encryption settings
*
* @return repository encryption settings
*/
public CryptoSettings cryptoSettings() {
return cryptoSettings;
}

/**
* Parses repository definition.
*
Expand All @@ -224,6 +279,16 @@ public PutRepositoryRequest source(Map<String, Object> repositoryDefinition) {
@SuppressWarnings("unchecked")
Map<String, Object> sub = (Map<String, Object>) entry.getValue();
settings(sub);
} else if (name.equals("encrypted")) {
encrypted(nodeBooleanValue(entry.getValue(), "encrypted"));
} else if (name.equals("crypto_settings")) {
if (!(entry.getValue() instanceof Map)) {
throw new IllegalArgumentException("Malformed encryption_settings section, should include an inner object");
}
@SuppressWarnings("unchecked")
Map<String, Object> sub = (Map<String, Object>) entry.getValue();
CryptoSettings cryptoSettings = new CryptoSettings(sub);
cryptoSettings(cryptoSettings);
}
}
return this;
Expand All @@ -236,6 +301,12 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(type);
writeSettingsToStream(settings, out);
out.writeBoolean(verify);
if (out.getVersion().onOrAfter(Version.V_3_0_0)) {
out.writeOptionalBoolean(encrypted);
if (Boolean.TRUE.equals(encrypted)) {
cryptoSettings.writeTo(out);
}
}
}

@Override
Expand All @@ -249,6 +320,16 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
builder.endObject();

builder.field("verify", verify);

if (null != encrypted) {
builder.field("encrypted", encrypted);
if (encrypted == true) {
builder.startObject("crypto_settings");
cryptoSettings.toXContent(builder, params);
builder.endObject();
}
}

builder.endObject();
return builder;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@

package org.opensearch.action.admin.cluster.repositories.put;

import org.opensearch.action.admin.cluster.crypto.CryptoSettings;
import org.opensearch.action.support.master.AcknowledgedRequestBuilder;
import org.opensearch.action.support.master.AcknowledgedResponse;
import org.opensearch.client.OpenSearchClient;
Expand Down Expand Up @@ -141,4 +142,26 @@ public PutRepositoryRequestBuilder setVerify(boolean verify) {
request.verify(verify);
return this;
}

/**
* Sets whether repository data should be encrypted and stored.
*
* @param encrypted true if repository data should be encrypted and stored, false otherwise
* @return this builder
*/
public PutRepositoryRequestBuilder setEncrypted(Boolean encrypted) {
request.encrypted(encrypted);
return this;
}

/**
* Sets the repository encryption settings
*
* @param cryptoSettings repository crypto settings builder
* @return this builder
*/
public PutRepositoryRequestBuilder setEncryptionSettings(CryptoSettings cryptoSettings) {
request.cryptoSettings(cryptoSettings);
return this;
}
}
Loading