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

[core] Introduce encryption interface for paimon #2903

Closed

Conversation

zhangjun0x01
Copy link
Contributor

Purpose

Linked issue: (#2113)

Tests

API and Format

Documentation

.withDescription(
"The kms client for encryption, if the user has enabled encryption, the kms client must be specified.");

public static final ConfigOption<String> HADOOP_SECURITY =
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be a table option?
I think this can be configured from catalog or evironment.

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 updated it


/** The kms client for encryption. */
public enum EncryptionKmsClient implements DescribedEnum {
MEMORY("memory", "Use memory kms for encryption, this is only for test."),
Copy link
Contributor

Choose a reason for hiding this comment

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

If it is only for test, why it need to be a public option?

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's not just for test, it is a variable of the EncryptionManager , if the user has enabled encryption, the kms client must be specified

# limitations under the License

org.apache.paimon.encryption.PlaintextEncryptionManager
org.apache.paimon.encryption.EnvelopeEncryptionManager
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove this? No implementation?

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 my issue, I forgot to submit the EnvelopeEncryptionManager


private final String keyId;
private final byte[] encryptedKey;
private final byte[] plaintextKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we need plaintextKey?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When using the file writer to write data, the plaintext data key is required for encryption. EncryptionManager#createLocalDataKey method is used to get a wrapper for the data key ,KeyMetadata will be passed as a parameter between methods

KeyMetadata will be persisted, but the plaintext data key cannot be stored.

@zhangjun0x01 zhangjun0x01 force-pushed the encryption_interface branch from ae727b7 to 657b2d9 Compare March 1, 2024 13:54
@zhangjun0x01 zhangjun0x01 force-pushed the encryption_interface branch from 657b2d9 to c3a8fa7 Compare March 13, 2024 13:37
@@ -1084,6 +1085,33 @@ public class CoreOptions implements Serializable {
"Whether to enable deletion vectors mode. In this mode, index files containing deletion"
+ " vectors are generated when data is written, which marks the data for deletion."
+ " During read operations, by applying these index files, merging can be avoided.");
public static final ConfigOption<EncryptionMechanism> ENCRYPTION_MECHANISM =
Copy link
Contributor

Choose a reason for hiding this comment

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

add a line above


/** KMS client interface, provide common operations for KMS. */
public interface KmsClient extends Serializable, Closeable {
void configure(Options options);
Copy link
Contributor

Choose a reason for hiding this comment

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

add a line above


/** Provide base operations for KMS. */
public abstract class KmsClientBase implements KmsClient {
public static final int NONCE_LENGTH = 12;
Copy link
Contributor

Choose a reason for hiding this comment

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

add a line above


@Override
public byte[] decryptDataKey(byte[] ciphertext, byte[] kek) {

Copy link
Contributor

Choose a reason for hiding this comment

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

remove empty line

fields.add(new DataField(0, "_KEY_ID", newStringType(false)));
fields.add(new DataField(1, "_ENCRYPTED_KEY", newBytesType(false)));
fields.add(new DataField(2, "_AAD_PREFIX", newBytesType(true)));
return new RowType(fields);
Copy link
Contributor

Choose a reason for hiding this comment

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

Here does not have plaintextKey?


private final String keyId;
private final byte[] encryptedKey;
private final byte[] plaintextKey;
Copy link
Contributor

Choose a reason for hiding this comment

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

I still can not get it, iceberg StandardKeyMetadata has no this?

@zhangjun0x01 zhangjun0x01 marked this pull request as draft March 26, 2024 12:57
@zhangjun0x01 zhangjun0x01 force-pushed the encryption_interface branch 3 times, most recently from 75d7221 to 3a66f2a Compare April 8, 2024 15:32
@zhangjun0x01 zhangjun0x01 force-pushed the encryption_interface branch from 3a66f2a to c885771 Compare April 8, 2024 15:46
@JingsongLi
Copy link
Contributor

encryption needs lots of design, close this first.

@JingsongLi JingsongLi closed this Aug 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants