Core: Key metadata in Avro format #6450

ggershinsky · 2022-12-19T07:02:23Z

Co-authored-by: Jack Ye yezhaoqin@gmail.com

ggershinsky · 2023-02-08T09:52:40Z

core/src/main/java/org/apache/iceberg/encryption/envelope/EnvelopeKeyMetadata.java

+public class EnvelopeKeyMetadata implements EncryptionKeyMetadata {
+
+  public static final byte magic = 'I';
+  public static final byte version = 0;


actually, we don't need this header, since we go directly to data+manifest encryption. The version can be added as a field in the Avro struct. TBD.

rdblue · 2023-02-12T23:47:15Z

core/src/main/java/org/apache/iceberg/encryption/envelope/AvroKeyRecord.java

+import org.apache.avro.specific.SpecificRecordBuilderBase;
+
+public class AvroKeyRecord extends SpecificRecordBase {
+  public static final org.apache.avro.Schema SCHEMA$ =


Iceberg doesn't define Avro schemas directly. Instead, this should create an Iceberg schema and convert it to Avro if needed.

rdblue · 2023-02-12T23:47:50Z

core/src/main/java/org/apache/iceberg/encryption/envelope/AvroKeyRecord.java

+import org.apache.avro.specific.SpecificRecordBase;
+import org.apache.avro.specific.SpecificRecordBuilderBase;
+
+public class AvroKeyRecord extends SpecificRecordBase {


Avro-serializable classes should use IndexedRecord rather than specific classes.

For a simple example, see GenericPartitionFieldSummary

Ok, will handle.

rdblue · 2023-02-12T23:48:44Z

core/src/main/java/org/apache/iceberg/encryption/envelope/AvroKeyRecord.java

+    }
+  }
+
+  public ByteBuffer getEncryptionKey() {


Iceberg classes do not use get in method names. Get is not helpful or specific enough, so we prefer to either omit it for getters (like this) or replace it with a more helpful verb, like find or load.

auto-generated code (here, and in the 2 comments above..). I'll change this.

rdblue · 2023-02-12T23:49:34Z

core/src/main/java/org/apache/iceberg/encryption/envelope/AvroKeyRecord.java

+      return this;
+    }
+
+    public Builder setWrappingKeyId(CharSequence value) {


Why do we use the term "wrapping" key? Is it to avoid terms like key-encryption-key?

Yep; seems to be a standard / popular term, per google search (I looked for "encryption key wrapping")

rdblue · 2023-02-12T23:49:53Z

core/src/main/java/org/apache/iceberg/encryption/envelope/AvroKeyRecord.java

+    return new Builder();
+  }
+
+  public static class Builder extends SpecificRecordBuilderBase<AvroKeyRecord> {


No need to expose any Avro classes here.

rdblue · 2023-02-12T23:51:55Z

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

@@ -20,12 +20,13 @@

 import org.apache.iceberg.io.OutputFile;

-class BaseEncryptedOutputFile implements EncryptedOutputFile {
+public class BaseEncryptedOutputFile implements EncryptedOutputFile {


Why was this made public?

to be available in the envelope subpackage; but since we move classes to the encryption package, no need in this change anymore, I'll remove.

rdblue · 2023-02-12T23:52:26Z

core/src/main/java/org/apache/iceberg/encryption/envelope/EnvelopeEncryptionManager.java

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


Let's keep all of this package-private until we need to expose it.

rdblue · 2023-02-12T23:53:21Z

core/src/main/java/org/apache/iceberg/encryption/envelope/AvroKeyRecord.java

+  public org.apache.avro.Schema getSchema() {
+    return SCHEMA$;
+  }
+  // Used by DatumWriter.  Applications should not call.


Missing whitespace.

rdblue · 2023-02-12T23:53:38Z

core/src/main/java/org/apache/iceberg/encryption/envelope/AvroKeyRecord.java

+        throw new AvroRuntimeException("Bad index");
+    }
+  }
+  // Used by DatumReader.  Applications should not call.


Missing whitespace.

rdblue · 2023-02-12T23:55:09Z

core/src/main/java/org/apache/iceberg/encryption/envelope/AvroKeyRecord.java

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.iceberg.encryption.envelope;


Let's just put all this in the existing package. No need for a different one.

rdblue · 2023-02-12T23:56:17Z

core/src/main/java/org/apache/iceberg/encryption/envelope/EnvelopeEncryptionManager.java

+   * @param encryptionProperties encryption properties
+   */
+  public EnvelopeEncryptionManager(
+      String tableKeyId, KmsClient kmsClient, Map<String, String> encryptionProperties) {


Looks like this makes this PR dependent on KMS changes.

Instead, can you just define the Avro format and classes for serialization/deserialization? I think we need to focus on the format and keep PRs separate.

Sure, will move this to a separate PR

rdblue · 2023-02-13T00:03:42Z

core/src/main/java/org/apache/iceberg/encryption/envelope/EnvelopeKeyMetadata.java

+import org.apache.avro.specific.SpecificDatumWriter;
+import org.apache.iceberg.encryption.EncryptionKeyMetadata;
+
+public class EnvelopeKeyMetadata implements EncryptionKeyMetadata {


This class duplicates quite a bit that's already done in IcebergDecoder and IcebergEncoder.

Those classes support Avro's single-message encoding, which we don't really need. But there's a RawDecoder there that would be useful. I'd prefer to adapt classes in the data.avro package to do the raw encoding and decoding, then delegate to those. That way this is more of a refactor than a re-implementation.

Ok, will handle this.

ggershinsky · 2023-02-22T08:03:11Z

I've leveraged the IcebergDecoder and IcebergEncoder as they are, including the 8-byte header (schema hash) - this enables updating the key metadata schema in the future, while making minimal / no changes in the code. But if it's better to use an explicit 1-2 byte version header, instead of the 8-byte hash (to save 6 bytes? to make versions more formal?) - and to call RawDecoder (plus add RawEncoder), lets discuss.

RussellSpitzer · 2023-02-27T18:09:57Z

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

+
+  @Override
+  public EncryptionKeyMetadata copy() {
+    KeyMetadata metadata = new KeyMetadata(encryptionKey(), wrappingKeyId(), aadPrefix());


Not actually copying buffers here, is that an issue?

Looks like the copy method of the EncryptionKeyMetadata interface is not called anywhere. Maybe we should deprecate it.

rdblue · 2023-05-07T21:25:46Z

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

+
+  @Override
+  public org.apache.avro.Schema getSchema() {
+    return AvroSchemaUtil.convert(SCHEMA_V1, this.getClass().getCanonicalName());


Rather than doing this every time the schema is requested, can you make it a static final variable?

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

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

+  private static final ThreadLocal<KeyMetadataDecoder<IndexedRecord>> DECODER =
+      ThreadLocal.withInitial(() -> new KeyMetadataDecoder<>(V1));
+
+  static final Map<Byte, Schema> supportedSchemaVersions = ImmutableMap.of(V1, SCHEMA_V1);


This should be private. Rather than accessing the map directly, have a package-private method that exposes these.

rdblue · 2023-05-07T21:26:58Z

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

+import org.apache.iceberg.data.avro.RawDecoder;
+import org.apache.iceberg.relocated.com.google.common.collect.MapMaker;
+
+public class KeyMetadataDecoder<D> extends MessageDecoder.BaseDecoder<D> {


Why is this parameterized? Doesn't it always produce KeyMetadata?

rdblue · 2023-05-07T21:27:56Z

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

+   * <p>The {@code readSchemaVersion} is as used the version of the expected (read) schema. Datum
+   * instances created by this class will are described by the expected schema.
+   */
+  public KeyMetadataDecoder(byte readSchemaVersion) {


There's no need for this class to be public right?

rdblue · 2023-05-07T21:28:56Z

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

+    try {
+      writeSchemaVersion = (byte) stream.read();
+    } catch (IOException e) {
+      throw new IOException("Failed to read the version byte", e);


Why wrap an IOException with a checked exception that has to be handled elsewhere? I'd just use UncheckedIOException here and not worry about it in KeyMetadata

rdblue · 2023-05-07T21:30:26Z

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

+        KeyMetadata.supportedSchemaVersions.get(writeSchemaVersion);
+
+    if (writeSchema == null) {
+      throw new MissingSchemaException("Cannot resolve schema for version: " + writeSchemaVersion);


This should not leak an Avro exception through the Iceberg API. Instead, I think this should use UnsupportedOperationException. That's what is thrown most often in cases where we encounter something from a future version of Iceberg (which is what would trigger this).

rdblue · 2023-05-07T21:46:43Z

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

+
+    if (decoder == null) {
+      Function<Schema, DatumReader<?>> createReaderFunc =
+          schema -> {


I think you can just use GenericAvroReader::create instead of all this.

rdblue · 2023-05-07T21:47:13Z

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

+      decoders.put(writeSchemaVersion, decoder);
+    }
+
+    return decoder.decode(stream, reuse);


Catch IOException and wrap with UncheckedIOException here?

the decoder.decode method doesn't throw an IOException

rdblue · 2023-05-07T21:48:04Z

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

+import org.apache.iceberg.avro.AvroSchemaUtil;
+import org.apache.iceberg.avro.GenericAvroWriter;
+
+public class KeyMetadataEncoder<D> implements MessageEncoder<D> {


Also doesn't need to be parameterized.

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

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

+   * <p>Buffers returned by {@code encode} are copied and will not be modified by future calls to
+   * {@code encode}.
+   */
+  public KeyMetadataEncoder(byte schemaVersion) {


Doesn't need to be public right?

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

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

+class KeyMetadata implements EncryptionKeyMetadata, IndexedRecord {
+  private static final String encryptionKeyField = "encryption_key";
+  private static final String wrappingKeyIdField = "wrapping_key_id";
+  private static final String aadPrefixField = "aad_prefix";


Are these constants needed? They're only used to construct the schema, right?

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

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

core/src/main/java/org/apache/iceberg/data/avro/RawDecoder.java

rdblue · 2023-06-05T17:00:08Z

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

+  private static final Map<Byte, org.apache.avro.Schema> avroSchemaVersions =
+      ImmutableMap.of(V1, AVRO_SCHEMA_V1);
+
+  private static final KeyMetadataEncoder keyMetadataEncoder = new KeyMetadataEncoder(V1);


Shouldn't all the static final names be ALL_CAPS?

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

rdblue · 2023-06-05T17:01:56Z

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

+    return schemaVersions;
+  }
+
+  static Map<Byte, org.apache.avro.Schema> supportedAvroSchemaVersions() {


Why is this exposed? I don't think that we want to expose Avro schemas in the API.

This is for package-internal usage by KeyMetadataEncoder and KeyMetadataDecoder - they need an access to the map of supported schema versions.

At least we can remove it if we refactor.

rdblue · 2023-06-09T21:32:56Z

core/src/main/java/org/apache/iceberg/avro/GenericAvroReader.java

@@ -30,13 +30,17 @@
 import org.apache.iceberg.common.DynClasses;
 import org.apache.iceberg.data.avro.DecoderResolver;

-class GenericAvroReader<T> implements DatumReader<T>, SupportsRowPosition {
+public class GenericAvroReader<T> implements DatumReader<T>, SupportsRowPosition {


Note about this to myself for later...

This needs to use GenericAvroReader for a couple reasons. First, GenericAvroReader is the only one that supports creating records with a specific class. We could extend support so that DataReader can create StructLike instances, but that hits the second reason: using Iceberg generics will use different representations for some types, like using LocalDate instead of int for date type. The representation differences don't affect the KeyMetadata class right now, but it's still more correct to use the internal representation.

rdblue · 2023-06-09T21:42:48Z

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

+      AvroSchemaUtil.convert(SCHEMA_V1, KeyMetadata.class.getCanonicalName());
+
+  private static final Map<Byte, Schema> schemaVersions = ImmutableMap.of(V1, SCHEMA_V1);
+  private static final Map<Byte, org.apache.avro.Schema> avroSchemaVersions =


These should be ALL_CAPS as well.

github-actions bot added core API labels Dec 19, 2022

ggershinsky mentioned this pull request Dec 19, 2022

Core: Envelope encryption #3471

Closed

github-actions bot removed the API label Dec 22, 2022

ggershinsky force-pushed the envelope-encryption-with-avro-km branch from c87ef1b to 19baeb2 Compare December 22, 2022 06:40

ggershinsky commented Feb 8, 2023

View reviewed changes

rdblue reviewed Feb 12, 2023

View reviewed changes

rdblue reviewed Feb 13, 2023

View reviewed changes

ggershinsky added 6 commits February 20, 2023 07:51

envelope encryption with avro key metadata

c890c7b

use new kms interface

dc729c8

clean up

8980aa9

envelope encryption package and key metadata usage

786ea38

rm outdated comment

364dc54

address review comments

af0c163

ggershinsky force-pushed the envelope-encryption-with-avro-km branch from 62a43fa to af0c163 Compare February 20, 2023 05:58

cleanup

8fcd041

ggershinsky changed the title ~~Core: Envelope encryption with Avro key metadata~~ Core: Key metadata in Avro format Feb 20, 2023

move to correct package

14748cf

RussellSpitzer reviewed Feb 27, 2023

View reviewed changes

rdblue reviewed May 7, 2023

View reviewed changes

ggershinsky added 2 commits May 26, 2023 08:28

post-review, plus wrappingkey removal

a385a0a

spotless fix

0fe6913

rdblue reviewed Jun 5, 2023

View reviewed changes

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

rdblue reviewed Jun 5, 2023

View reviewed changes

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

rdblue reviewed Jun 5, 2023

View reviewed changes

core/src/main/java/org/apache/iceberg/data/avro/RawDecoder.java Outdated Show resolved Hide resolved

rdblue reviewed Jun 5, 2023

View reviewed changes

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

rdblue reviewed Jun 5, 2023

View reviewed changes

ggershinsky added 2 commits June 6, 2023 11:07

post-review changes 2

aa55b7f

update string check in unitest

1f2ae8a

github-actions bot added the data label Jun 6, 2023

rdblue reviewed Jun 9, 2023

View reviewed changes

rdblue approved these changes Jun 9, 2023

View reviewed changes

rdblue reviewed Jun 9, 2023

View reviewed changes

rdblue approved these changes Jun 9, 2023

View reviewed changes

rdblue merged commit 2a83d9e into apache:master Jun 9, 2023
41 checks passed

danielcweeks added this to In progress in [Priority 2] Spec v3: Encryption via automation Jul 18, 2023

danielcweeks moved this from In progress to Done in [Priority 2] Spec v3: Encryption Jul 19, 2023

rodmeneses pushed a commit to rodmeneses/iceberg that referenced this pull request Feb 19, 2024

Core: Key metadata in Avro format (apache#6450)

9c0a321

Core: Key metadata in Avro format #6450

Core: Key metadata in Avro format #6450

Conversation

ggershinsky commented Dec 19, 2022 • edited

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

ggershinsky commented Feb 22, 2023

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 Jun 5, 2023 • edited

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

ggershinsky commented Dec 19, 2022 •

edited

rdblue Jun 5, 2023 •

edited