Standard key manager #6884

ggershinsky · 2023-02-20T06:38:25Z

No description provided.

rdblue · 2023-02-27T18:28:50Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+  public static final String ENCRYPTION_DEK_LENGTH = "encryption.data.key.length";
+  public static final int ENCRYPTION_DEK_LENGTH_DEFAULT = 16;
+
+  public static final int ENCRYPTION_AAD_LENGTH_DEFAULT = 16;


Can we move these to EncryptionProperties? We like to keep constants in separate classes like that.

rdblue · 2023-02-27T18:29:12Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+import org.apache.iceberg.util.PropertyUtil;
+
+public class DefaultEncryptionManager implements EncryptionManager {
+  public static final String ENCRYPTION_TABLE_KEY = "encryption.table.key.id";


We should also consider what we want these to be named (ping @aokolnychyi). Something like encryption.key-id?

rdblue · 2023-02-27T18:29:44Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.util.PropertyUtil;
+
+public class DefaultEncryptionManager implements EncryptionManager {


Do we have a name for this bedsides "Default"?

Envelope? Iceberg? Other suggestions are welcome.

What about "Standard"?

rdblue · 2023-02-27T18:29:52Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+  public static final int ENCRYPTION_AAD_LENGTH_DEFAULT = 16;
+
+  /** Implementation of the KMS client for envelope encryption */
+  public static final String ENCRYPTION_KMS_CLIENT_IMPL = "encryption.kms.client-impl";


Let's move this too.

rdblue · 2023-02-27T18:30:25Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+  @Override
+  public EncryptedOutputFile encrypt(OutputFile rawOutput) {
+    if (null == workerRNG) {
+      workerRNG = new SecureRandom();


We should move this to a private method.

RussellSpitzer · 2023-02-27T18:32:22Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+  }
+
+  @Override
+  public InputFile decrypt(EncryptedInputFile encrypted) {


I think we should doc that this is a Noop method, otherwise it's a bit confusing that by default this decrypt method just returns an encrypted file.

rdblue · 2023-04-02T19:04:05Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+  }
+
+  private void createSecureRandomGenerator() {
+    workerRNG = new SecureRandom();


When setting an instance field, use the this. prefix to make it clear this is affecting persistent state.

rdblue · 2023-04-02T19:10:29Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+
+    KeyMetadata fileEnvelopeMetadata = new KeyMetadata(fileDek, null, aadPrefix);
+
+    return new BaseEncryptedOutputFile(rawOutput, fileEnvelopeMetadata);


I think this actually needs to encrypt. BaseEncryptedOutputFile doesn't actually do anything, but the intent is for this to be a file that will encrypt the stream of data as it is written to the stream created by encryptingOutputFile()

Per the comment below, I've implemented the following: EncryptedOutputFile class would returns the underlying stream as well, so that encryption can be handled by Parquet.

rdblue · 2023-04-02T19:13:56Z

core/src/main/java/org/apache/iceberg/encryption/DefaultEncryptionManager.java

+    }
+
+    // Native decryption: simply return the input file. Parquet decryption will get the key from key
+    // metadata.


I don't don't think this is correct.

These interfaces are intended to give you the option to encrypt/decrypt a stream. That's why they expose key metadata and an InputFile or OutputFile. You use the InputFile as you normally would, but it is decrypted for you as you read.

That fits with the AES GCM encryption spec. This should decrypt using AES GCM decryption. Similarly, encryption should return an encrypting stream.

For native Parquet encryption, I think the EncryptedOutputFile and EncryptedInputFile classes would need to be able to return the underlying stream as well, so that encryption can be handled by Parquet.

Done for the EncryptedOutputFile. For decryption, we have everything already in the Parquet reader: the underlying stream and the key metadata, so can decrypt with native Parquet.
(no need in changes in EncryptedInputFile, and no need in calling the encryption manager).

rdblue · 2023-05-07T21:51:41Z

core/src/main/java/org/apache/iceberg/encryption/PlaintextEncryptionManager.java

@@ -25,6 +25,8 @@
 import org.slf4j.LoggerFactory;

 public class PlaintextEncryptionManager implements EncryptionManager {
+  public static final EncryptionManager INSTANCE = new PlaintextEncryptionManager();


I think we typically hide these behind static instance methods, rather than making the instance field public.

rdblue · 2023-05-07T21:52:22Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+      String tableKeyId, KeyManagementClient kmsClient, Map<String, String> encryptionProperties) {
+    Preconditions.checkNotNull(
+        tableKeyId,
+        "Cannot create EnvelopeEncryptionManager because table encryption key ID is not specified");


This doesn't need to state what was attempted. A shorter version would be more clear: "Invalid encryption key ID: null".

rdblue · 2023-05-07T21:53:21Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+        tableKeyId,
+        "Cannot create EnvelopeEncryptionManager because table encryption key ID is not specified");
+    Preconditions.checkNotNull(
+        kmsClient, "Cannot create EnvelopeEncryptionManager because kmsClient is null");


Invalid KMS client: null

We don't use variable or class names in error messages. That's friendlier for people and anyone that will go to the file and line will quickly see what the variable name was anyway.

rdblue · 2023-05-07T21:53:33Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+        kmsClient, "Cannot create EnvelopeEncryptionManager because kmsClient is null");
+    Preconditions.checkNotNull(
+        encryptionProperties,
+        "Cannot create EnvelopeEncryptionManager because encryptionProperties are not passed");


Invalid encryption properties: null.

rdblue · 2023-05-07T21:54:24Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+
+  @Override
+  public EncryptedOutputFile encrypt(OutputFile rawOutput) {
+    if (null == workerRNG) {


We usually have a method for lazy initialized variables, like lazyRNG() rather than doing the null check everywhere.

rdblue · 2023-05-07T21:57:34Z

core/src/main/java/org/apache/iceberg/encryption/EncryptionProperties.java

+
+  private EncryptionProperties() {}
+
+  public static final String ENCRYPTION_TABLE_KEY = "encryption.table.key.id";


These names don't conform to the guidelines for properties: https://iceberg.apache.org/contribute/#config-naming

rdblue · 2023-05-07T21:59:36Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+    KeyMetadata dataEncryptionMetadata = new KeyMetadata(fileDek, null, aadPrefix);
+
+    // For metadata files
+    // This is an expensive operation, RPC to KMS server


Is this comment helpful?

There is a design gap we need to discuss. This comment, and a couple more below, are a temp text that describes the problem. TBD in our next call.

rdblue · 2023-05-07T22:00:43Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+    ByteBuffer wrappedDek = kmsClient.wrapKey(fileDek, tableKeyId);
+    KeyMetadata manifestEncryptionMetadata = new KeyMetadata(wrappedDek, tableKeyId, aadPrefix);
+
+    // We don't know which key metadata to use, because we don't know what file we encrypt.


In Iceberg, we don't use personal pronouns, like "we" or "I" in the code. It makes docs or comments longer without adding value.

Sure. As above , this is a temp text, to be removed.

rdblue · 2023-05-07T22:01:45Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+    // For metadata files
+    // This is an expensive operation, RPC to KMS server
+    ByteBuffer wrappedDek = kmsClient.wrapKey(fileDek, tableKeyId);
+    KeyMetadata manifestEncryptionMetadata = new KeyMetadata(wrappedDek, tableKeyId, aadPrefix);


Why is wrapping being done here if it is an expensive operation? Doesn't the caller need the key directly? I think it would make more sense to wrap the key when it is stored.

rdblue · 2023-05-07T22:02:44Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+  public InputFile decrypt(EncryptedInputFile encrypted) {
+    if (encrypted.keyMetadata() == null || encrypted.keyMetadata().buffer() == null) {
+      throw new RuntimeException(
+          "Unencrypted file " + encrypted.encryptedInputFile().location() + " in encrypted table");


Shouldn't use RuntimeException in general. Is there a better one?

Also, is this an exception? What happens when converting a table to use encryption? It seems like this could be a valid state.

Sometimes, due to misconfiguration etc - users think they write encrypted tables / files, but the result is not encrypted. So this is a rather important security check. But we can discuss the options and trade-offs between this and a flexibility to be able to convert an existing table to use encryption.

The check I've mentioned, is not always possible to perform. Switching to your suggestion.

rdblue · 2023-05-07T22:03:44Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+
+    this.dataKeyLength =
+        PropertyUtil.propertyAsInt(
+            encryptionProperties,


Rather than passing encryption properties, why not just pass the key length?

rdblue · 2023-08-14T16:29:10Z

core/src/main/java/org/apache/iceberg/encryption/EncryptionManagerFactory.java

+import java.util.Map;
+import org.apache.iceberg.TableMetadata;
+
+public interface EncryptionManagerFactory extends Closeable {


Do you have an example of how this is used in context? I want to make sure what we're adding to catalogs or table implementations makes sense.

Sure, in https://github.com/apache/iceberg/pull/5544/files

( eg https://github.com/apache/iceberg/pull/5544/files#diff-dcb8898b53c9729e27fea26093e90a05e86cc2682a03210cf5f56fd5a260f174R237 and https://github.com/apache/iceberg/pull/5544/files#diff-b840ad8566560883268ebb23aa1de123a05599a1aeeda31bf66dfc0cb54a7e82R194 )

Basically, it's an object in TableOperations, that allows up to create an EncryptionManager from both catalog properties and table metadata.

rdblue · 2023-09-10T21:16:38Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManagerFactory.java

+
+  @Override
+  public EncryptionManager create(TableMetadata tableMetadata) {
+    if (tableMetadata == null) {


This seems like it should be an exception. If table metadata is null, then this doesn't know what to produce.

rdblue · 2023-09-10T21:16:59Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManagerFactory.java

+
+      if (tableMetadata.formatVersion() < 2) {
+        throw new UnsupportedOperationException(
+            "Iceberg encryption works only with table format 2 or higher");


the key_metadata field for manifest files was added in v2, as far as I know.

rdblue · 2023-09-10T21:17:48Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManagerFactory.java

+    final Map<String, String> encryptionProperties = Maps.newHashMap();
+    encryptionProperties.putAll(tableProperties);
+
+    // Important: put catalog properties after table properties. Former overrides the latter.


Are there properties that are not allowed in the catalog properties? For example, table key?

In certain usecases and threat models (where an attacker can remove the table key from the metadata.json file), it'd be good to set the table key in the catalog properties - so the writer won't start producing unencrypted data files.

rdblue · 2023-09-10T21:18:38Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManagerFactory.java

+
+      if (FileFormat.fromString(fileFormat) != FileFormat.PARQUET) {
+        throw new UnsupportedOperationException(
+            "Iceberg encryption currently supports only parquet format for data files");


Is it possible to also support Avro files? Or is that a future feature?

Yep, preferably a future PR.

Co-Authored-By: Jian Tang <jian_tang@apple.com>

core/src/main/java/org/apache/iceberg/encryption/PlaintextEncryptionManager.java

rdblue · 2023-12-10T22:05:25Z

api/src/main/java/org/apache/iceberg/encryption/EncryptedOutputFile.java

@@ -37,4 +37,9 @@ public interface EncryptedOutputFile {
   * #encryptingOutputFile()}.
   */
  EncryptionKeyMetadata keyMetadata();
+
+  /** Underlying output file for native encryption. */
+  default OutputFile rawOutputFile() {


I'm not sure about this name, but I've had a hard time coming up with a better one. The best I can come up with is plainOutputFile. What do you think about that? I think it's slightly clearer than "raw".

It also looks like this isn't used in this PR, in which case I think we should include it in a PR where it is used.

rdblue · 2023-12-10T22:18:43Z

core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java

+    return KeyMetadata.parse(metadataBuffer);
+  }
+
+  public static EncryptionKeyMetadata createKeyMetadata(ByteBuffer key, ByteBuffer aadPrefix) {


This isn't used and it looks like a few places use the KeyMetadata constructor directly.

rdblue · 2023-12-10T22:33:38Z

core/src/main/java/org/apache/iceberg/encryption/StandardEncryptionManager.java

+  @Override
+  public Iterable<InputFile> decrypt(Iterable<EncryptedInputFile> encrypted) {
+    // Bulk decrypt is only applied to data files. Returning source input files for parquet.
+    return Iterables.transform(encrypted, this::getSourceFile);


This is incorrect. Based on the comment, I think the intent was to skip creating an AesGcmInputFile because you know that this is only called for data files. But that's a bad assumption and there is no guarantee it will be called that way in the future.

Instead, I think this should call decrypt. That operation should be cheap, although we may need to defer parsing the key metadata until later.

rdblue · 2023-12-10T23:30:42Z

core/src/main/java/org/apache/iceberg/encryption/EncryptedFiles.java

@@ -57,5 +57,9 @@ public static EncryptedOutputFile encryptedOutput(
        encryptedOutputFile, BaseEncryptionKeyMetadata.fromByteArray(keyMetadata));
  }

+  public static EncryptedOutputFile plainAsEncryptedOutput(OutputFile encryptingOutputFile) {


This isn't used. Can we add it when the use is added?

rdblue · 2023-12-10T23:31:17Z

core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java

+
+  private EncryptionUtil() {}
+
+  public static KeyMetadata parseKeyMetadata(ByteBuffer metadataBuffer) {


This isn't used and duplicates the wrapped method. I think we should remove it.

rdblue · 2023-12-10T23:32:39Z

core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java

+    return new KeyMetadata(key, aadPrefix);
+  }
+
+  public static long gcmEncryptionLength(long plainFileLength) {


Can we remove this since it isn't used? I'd rather not have dead and untested code.

rdblue · 2023-12-10T23:36:14Z

core/src/main/java/org/apache/iceberg/encryption/EncryptionUtil.java

+          "Cannot create StandardEncryptionManagerFactory without KMS type");
+    }
+
+    if (!kmsType.equals(CatalogProperties.ENCRYPTION_KMS_CUSTOM_TYPE)) {


I think that it is much simpler if rather than requiring a type that is "custom", this instead checks that either type or impl is set.

rdblue · 2023-12-10T23:52:36Z

@ggershinsky, I opened https://github.com/ggershinsky/iceberg/pull/9/files against your branch with the last changes I think are needed to get this in. Please review and let me know so we can get to the next one. Thanks!

rdblue · 2023-12-12T18:08:40Z

Merged in #9277! Thanks, @ggershinsky!

github-actions bot added the core label Feb 20, 2023

rdblue reviewed Feb 27, 2023

View reviewed changes

RussellSpitzer reviewed Feb 27, 2023

View reviewed changes

ggershinsky changed the title ~~Core: Default key manager~~ Core: Default key manager and factory Mar 8, 2023

rdblue reviewed Apr 2, 2023

View reviewed changes

rdblue reviewed May 7, 2023

View reviewed changes

ggershinsky changed the title ~~Core: Default key manager and factory~~ Core: Standard key manager and factory Jul 23, 2023

ggershinsky mentioned this pull request Aug 2, 2023

Deliver key metadata to parquet encryption #6762

Closed

ggershinsky changed the title ~~Core: Standard key manager and factory~~ Standard key manager and factory Aug 14, 2023

rdblue reviewed Aug 14, 2023

View reviewed changes

rdblue reviewed Sep 10, 2023

View reviewed changes

ggershinsky added 8 commits September 20, 2023 09:36

initial commit

ef53c2d

Update and add factory

20fa8cd

Co-Authored-By: Jian Tang <jian_tang@apple.com>

Encryption properties class

94ab0cc

add plaintext mng instance

c2907f8

post-review changes

ee70332

address review comments

4c6fb3a

no encryption in format 1

a9e4271

null keymetadata

aa3e562

ggershinsky force-pushed the default-key-manager branch from 4823e92 to aa3e562 Compare September 20, 2023 06:37

ggershinsky added 3 commits September 20, 2023 09:45

address review comments

7e38396

cleanup

87ba3ec

enable SEMFactory only if kms type is set

925c73a

ggershinsky changed the title ~~Standard key manager and factory~~ Standard key manager Sep 21, 2023

rdblue reviewed Oct 9, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/encryption/PlaintextEncryptionManager.java Show resolved Hide resolved

ggershinsky added 2 commits October 16, 2023 10:12

address review comments

726e568

add changes in EncryptedOutputFile

06fbac6

github-actions bot added the API label Oct 16, 2023

rdblue reviewed Dec 10, 2023

View reviewed changes

rdblue mentioned this pull request Dec 10, 2023

Updates from review ggershinsky/iceberg#9

Closed

rdblue mentioned this pull request Dec 11, 2023

Core: Add StandardEncryptionManager #9277

Merged

rdblue closed this Dec 12, 2023


		KeyMetadata fileEnvelopeMetadata = new KeyMetadata(fileDek, null, aadPrefix);

		return new BaseEncryptedOutputFile(rawOutput, fileEnvelopeMetadata);


		private EncryptionProperties() {}

		public static final String ENCRYPTION_TABLE_KEY = "encryption.table.key.id";


		private EncryptionUtil() {}

		public static KeyMetadata parseKeyMetadata(ByteBuffer metadataBuffer) {

Standard key manager #6884

Standard key manager #6884

Conversation

ggershinsky commented Feb 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue Dec 10, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rdblue commented Dec 10, 2023

rdblue commented Dec 12, 2023

ggershinsky commented Feb 20, 2023 •

edited

Loading

rdblue Dec 10, 2023 •

edited

Loading

rdblue Dec 10, 2023 •

edited

Loading