You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "Guosmilesmile (via GitHub)" <gi...@apache.org> on 2026/06/08 12:05:18 UTC
[PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Guosmilesmile opened a new pull request, #16724:
URL: https://github.com/apache/iceberg/pull/16724
This pr add TCK for Encrypt in FileFormat API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3395831081
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
My original intention was to test whether the calls to these two methods actually take effect, and from the results, that can be verified. If the naming is based on verifying AES and thus marked as a "miss" feature, it somewhat contradicts what this unit test is supposed to do.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3379988619
##########
data/src/test/java/org/apache/iceberg/data/FileFormatTestSupport.java:
##########
@@ -58,4 +59,6 @@ void writeRecordsWithoutFieldIds(OutputFile outputFile, Schema schema, List<Reco
String metadataValue(InputFile inputFile, String key) throws IOException;
String splitSizeProperty();
+
+ void assertEncryptedFileUnreadable(ThrowingCallable readEncryptedFile);
Review Comment:
It seems different formats throw different exceptions with different messages — Parquet throws a special ParquetCryptoRuntimeException, while the others throw a plain RuntimeException. That's why I had to split them apart.
Also, I want to verify that the write succeeded by reading the data back without the encryption info, which is why this is needed too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3379839289
##########
data/src/test/java/org/apache/iceberg/data/FileFormatTestSupport.java:
##########
@@ -58,4 +59,6 @@ void writeRecordsWithoutFieldIds(OutputFile outputFile, Schema schema, List<Reco
String metadataValue(InputFile inputFile, String key) throws IOException;
String splitSizeProperty();
+
+ void assertEncryptedFileUnreadable(ThrowingCallable readEncryptedFile);
Review Comment:
Why do we need file specific assertions?
I would probably just accept that any exception is a good exception in these tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3410722004
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
Review Comment:
Thanks for the idea. I have add FEATURE_AES_STREAM_ENCRYPTION to parquet. But we have to call keyMetadata since reader need this information.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3381216810
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
Review Comment:
The difference between these two methods lies in the purpose of testDataWriterEncryption: it is used to test whether keyMetadata(encryptedOutputFile.keyMetadata()) is actually effective. On the other hand, testDataWriterWithFileEncryptionKeyAndWithAADPrefix is used to test whether the options withFileEncryptionKey and withAADPrefix are effective.
Since FileFormat is an input parameter, both methods will test all possible file formats.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3394560231
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
I have some doubts about this. It looks like encryption can already be achieved by setting keyMetadata in Parquet. What exactly do you mean by "missing feature" — am I missing something here?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#issuecomment-4704064673
Here's a summary of the recent changes:
* Split the test cases into two directions: AES and NATIVE. Avro supports AES encryption, Parquet supports the NATIVE approach, and ORC supports neither.
* keyMetadata(...): saves the encryption metadata to the DataFile, mainly used by the read side for decryption — it's not an encryption switch.
* withFileEncryptionKey(...): passes in the actual file encryption key; Parquet native encryption relies on this to trigger encryption of the file content.
* withAADPrefix(...): passes in the AAD prefix, which participates in authentication / tamper protection, and must be paired with the encryption key.
If you all have time, please continue to help with further review.
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3380569093
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,97 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
Review Comment:
Yeah, not sure about a perfect way to test it, as all 3 formats work differently (native encr in Parquet, stream encr in Avro, no encryption in ORC).
I'll have a look at this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3381252587
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
Review Comment:
We need to call keyMetadata; otherwise, we will encounter this error.
```
Trying to read file with encrypted footer. No keys available
org.apache.parquet.crypto.ParquetCryptoRuntimeException: Trying to read file with encrypted footer. No keys available
at org.apache.parquet.hadoop.ParquetFileReader.readFooter(ParquetFileReader.java:655)
at org.apache.parquet.hadoop.ParquetFileReader.readFooter(ParquetFileReader.java:578)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3381822563
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
+ throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ String location = "test-file-" + UUID.randomUUID();
+ NativeEncryptionKeyMetadata keyMetadata =
+ (NativeEncryptionKeyMetadata)
+ encryptingFileIO.newEncryptingOutputFile(location).keyMetadata();
+
+ // Use a plain encrypted output file so Parquet cannot auto-inject the native encryption key
+ // and AAD prefix from the output file metadata. This ensures encryption is driven only by
+ // withFileEncryptionKey and withAADPrefix below.
+ EncryptedOutputFile encryptedOutputFile =
+ EncryptedFiles.plainAsEncryptedOutput(fileIO.newOutputFile(location));
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(keyMetadata)
+ .withFileEncryptionKey(keyMetadata.encryptionKey().duplicate())
+ .withAADPrefix(keyMetadata.aadPrefix().duplicate());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @SuppressWarnings("checkstyle:AssertThatThrownByWithMessageCheck")
+ private void writeAndAssertEncryptedDataWriter(
+ FileFormat fileFormat,
+ EncryptingFileIO encryptingFileIO,
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder)
+ throws IOException {
+ DataGenerator dataGenerator = new DataGenerators.DefaultSchema();
+ Schema schema = dataGenerator.schema();
+ List<Record> genericRecords = dataGenerator.generateRecords();
+ List<T> engineRecords = convertToEngineRecords(genericRecords, schema);
+
+ DataWriter<T> writer = writerBuilder.schema(schema).spec(PartitionSpec.unpartitioned()).build();
+
+ try (writer) {
+ engineRecords.forEach(writer::write);
+ }
+
+ DataFile dataFile = writer.toDataFile();
+ assertThat(dataFile).isNotNull();
+ assertThat(dataFile.recordCount()).isEqualTo(engineRecords.size());
+ assertThat(dataFile.format()).isEqualTo(fileFormat);
+ assertThat(dataFile.keyMetadata()).isNotNull();
+
+ assertThatThrownBy(
Review Comment:
Sort of; encryption is transparent to Avro, since it doesn't have a native version. Instead, we use AES streams for Avro files, https://github.com/apache/iceberg/pull/5432
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3395797443
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
So it seems to me that the underlying implementation still relies on `keyMetadata` rather than calling those other two methods. I want to verify whether these two methods actually take effect—is there a better way to do that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3395338065
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
In Iceberg, parquet files are encrypted only via Parquet native encryption, not via AES stream encryption.
> encryption can already be achieved by setting keyMetadata in Parquet
Technically, this setting might work, but it is meaningless since not used in Iceberg
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3381279856
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
+ throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ String location = "test-file-" + UUID.randomUUID();
+ NativeEncryptionKeyMetadata keyMetadata =
+ (NativeEncryptionKeyMetadata)
+ encryptingFileIO.newEncryptingOutputFile(location).keyMetadata();
+
+ // Use a plain encrypted output file so Parquet cannot auto-inject the native encryption key
+ // and AAD prefix from the output file metadata. This ensures encryption is driven only by
+ // withFileEncryptionKey and withAADPrefix below.
+ EncryptedOutputFile encryptedOutputFile =
+ EncryptedFiles.plainAsEncryptedOutput(fileIO.newOutputFile(location));
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(keyMetadata)
+ .withFileEncryptionKey(keyMetadata.encryptionKey().duplicate())
+ .withAADPrefix(keyMetadata.aadPrefix().duplicate());
Review Comment:
The purpose of using this UT is to test whether the two methods, withFileEncryptionKey and withAADPrefix, are actually successfully invoked. Therefore, I need to explicitly invoke these methods here. We want to see if there are better ways to test this area.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3379859420
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,97 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
Review Comment:
Maybe we should ask @ggershinsky what would be the best way to test the encryption on the file format level.
@ggershinsky: Any ideas?
Thanks,
Peter
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3380174672
##########
data/src/test/java/org/apache/iceberg/data/FileFormatTestSupport.java:
##########
@@ -58,4 +59,6 @@ void writeRecordsWithoutFieldIds(OutputFile outputFile, Schema schema, List<Reco
String metadataValue(InputFile inputFile, String key) throws IOException;
String splitSizeProperty();
+
+ void assertEncryptedFileUnreadable(ThrowingCallable readEncryptedFile);
Review Comment:
I could verify it by checking for a RuntimeException, but the CI requires that I also include hasMessage, and right now it seems different formats may have different error messages.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3381263411
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
+ throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ String location = "test-file-" + UUID.randomUUID();
+ NativeEncryptionKeyMetadata keyMetadata =
+ (NativeEncryptionKeyMetadata)
+ encryptingFileIO.newEncryptingOutputFile(location).keyMetadata();
+
+ // Use a plain encrypted output file so Parquet cannot auto-inject the native encryption key
+ // and AAD prefix from the output file metadata. This ensures encryption is driven only by
+ // withFileEncryptionKey and withAADPrefix below.
+ EncryptedOutputFile encryptedOutputFile =
+ EncryptedFiles.plainAsEncryptedOutput(fileIO.newOutputFile(location));
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(keyMetadata)
+ .withFileEncryptionKey(keyMetadata.encryptionKey().duplicate())
+ .withAADPrefix(keyMetadata.aadPrefix().duplicate());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @SuppressWarnings("checkstyle:AssertThatThrownByWithMessageCheck")
+ private void writeAndAssertEncryptedDataWriter(
+ FileFormat fileFormat,
+ EncryptingFileIO encryptingFileIO,
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder)
+ throws IOException {
+ DataGenerator dataGenerator = new DataGenerators.DefaultSchema();
+ Schema schema = dataGenerator.schema();
+ List<Record> genericRecords = dataGenerator.generateRecords();
+ List<T> engineRecords = convertToEngineRecords(genericRecords, schema);
+
+ DataWriter<T> writer = writerBuilder.schema(schema).spec(PartitionSpec.unpartitioned()).build();
+
+ try (writer) {
+ engineRecords.forEach(writer::write);
+ }
+
+ DataFile dataFile = writer.toDataFile();
+ assertThat(dataFile).isNotNull();
+ assertThat(dataFile.recordCount()).isEqualTo(engineRecords.size());
+ assertThat(dataFile.format()).isEqualTo(fileFormat);
+ assertThat(dataFile.keyMetadata()).isNotNull();
+
+ assertThatThrownBy(
Review Comment:
Both ORC and Avro will throw exceptions. We have added `assumeSupports(fileFormat, FEATURE_ENCRYPTION);` before the testing method. Since these formats do not support this feature, this test case will be skipped.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3381463995
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
+ throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ String location = "test-file-" + UUID.randomUUID();
+ NativeEncryptionKeyMetadata keyMetadata =
+ (NativeEncryptionKeyMetadata)
+ encryptingFileIO.newEncryptingOutputFile(location).keyMetadata();
+
+ // Use a plain encrypted output file so Parquet cannot auto-inject the native encryption key
+ // and AAD prefix from the output file metadata. This ensures encryption is driven only by
+ // withFileEncryptionKey and withAADPrefix below.
+ EncryptedOutputFile encryptedOutputFile =
+ EncryptedFiles.plainAsEncryptedOutput(fileIO.newOutputFile(location));
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(keyMetadata)
+ .withFileEncryptionKey(keyMetadata.encryptionKey().duplicate())
+ .withAADPrefix(keyMetadata.aadPrefix().duplicate());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @SuppressWarnings("checkstyle:AssertThatThrownByWithMessageCheck")
+ private void writeAndAssertEncryptedDataWriter(
+ FileFormat fileFormat,
+ EncryptingFileIO encryptingFileIO,
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder)
+ throws IOException {
+ DataGenerator dataGenerator = new DataGenerators.DefaultSchema();
+ Schema schema = dataGenerator.schema();
+ List<Record> genericRecords = dataGenerator.generateRecords();
+ List<T> engineRecords = convertToEngineRecords(genericRecords, schema);
+
+ DataWriter<T> writer = writerBuilder.schema(schema).spec(PartitionSpec.unpartitioned()).build();
+
+ try (writer) {
+ engineRecords.forEach(writer::write);
+ }
+
+ DataFile dataFile = writer.toDataFile();
+ assertThat(dataFile).isNotNull();
+ assertThat(dataFile.recordCount()).isEqualTo(engineRecords.size());
+ assertThat(dataFile.format()).isEqualTo(fileFormat);
+ assertThat(dataFile.keyMetadata()).isNotNull();
+
+ assertThatThrownBy(
Review Comment:
Thanks for pointing it out. I would like to ask, does Avro actually only use keyMetadata to transmit encrypted information, without requiring withFileEncryptionKey or withAADPrefix at all? I think Avro doesn’t support this because an exception was thrown during this process.
https://github.com/apache/iceberg/blob/4125f26d14c35db93480b825c020426ea9da2901/core/src/main/java/org/apache/iceberg/avro/AvroFormatModel.java#L149-L157
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3395797443
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
So it seems to me that the underlying implementation still relies on `keyMetadata` rather than calling those other two methods. I want to verify whether these two methods actually take effect—is there a better way to do that?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3394540029
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -176,6 +180,8 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
static final String FEATURE_REUSE_CONTAINERS = "reuseContainers";
static final String FEATURE_COLUMN_LEVEL_METRICS = "columnLevelMetrics";
static final String FEATURE_COLUMN_METRICS_TRUNCATE_BINARY = "columnMetricsTruncateBinary";
+ static final String FEATURE_ENCRYPTION = "encryption";
+ static final String FEATURE_ENCRYPTION_KEY_METADATA = "encryptionKeyMetadata";
Review Comment:
Good idea.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3380044922
##########
data/src/test/java/org/apache/iceberg/data/FileFormatTestSupport.java:
##########
@@ -58,4 +59,6 @@ void writeRecordsWithoutFieldIds(OutputFile outputFile, Schema schema, List<Reco
String metadataValue(InputFile inputFile, String key) throws IOException;
String splitSizeProperty();
+
+ void assertEncryptedFileUnreadable(ThrowingCallable readEncryptedFile);
Review Comment:
Sorry, I left something out. What I'm doing now is checking that the write is encrypted by reading the encrypted file with a normal reader that doesn't have the encryption information, and verifying it through the thrown exception — the exception indicates that the read is missing the key. My understanding is that different file types would throw different exceptions, which is why I wanted more format-specific methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3380163928
##########
data/src/test/java/org/apache/iceberg/data/FileFormatTestSupport.java:
##########
@@ -58,4 +59,6 @@ void writeRecordsWithoutFieldIds(OutputFile outputFile, Schema schema, List<Reco
String metadataValue(InputFile inputFile, String key) throws IOException;
String splitSizeProperty();
+
+ void assertEncryptedFileUnreadable(ThrowingCallable readEncryptedFile);
Review Comment:
Maybe we just check that any exception has thrown in this case?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3381366988
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
Review Comment:
Yeah, I guess it's more of a question about the `FormatModelRegistry` API
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3394241132
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1717,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryptionKeyMetaData(FileFormat fileFormat) throws IOException {
Review Comment:
rename to something like "testDataWriterAesStreamEncryption"?
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
Parquet's missing feature is "aesStreamEncryption"
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -176,6 +180,8 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
static final String FEATURE_REUSE_CONTAINERS = "reuseContainers";
static final String FEATURE_COLUMN_LEVEL_METRICS = "columnLevelMetrics";
static final String FEATURE_COLUMN_METRICS_TRUNCATE_BINARY = "columnMetricsTruncateBinary";
+ static final String FEATURE_ENCRYPTION = "encryption";
+ static final String FEATURE_ENCRYPTION_KEY_METADATA = "encryptionKeyMetadata";
Review Comment:
This naming is not clear to me. Maybe "nativeEncryption" and "aesStreamEncryption"?
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1717,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryptionKeyMetaData(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION_KEY_METADATA);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
Review Comment:
rename to something like "testDataWriterNativeEncryption"?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3394560231
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
I have some doubts about this situation. It seems that encryption can be achieved by setting keymetadata in parquet. What do you mean by “missing feature”? Do I miss something ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3395398530
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
When I only call keyMetadata in Parquet, reading the file fails if I don't go through EncryptingFileIO.This appears to be encrypted. Is there something wrong with this?
https://github.com/Guosmilesmile/iceberg/blob/5c5989477de88a252840f8b7275132e258047240/data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java#L1722-L1735
`
Trying to read file with encrypted footer. No keys available
org.apache.parquet.crypto.ParquetCryptoRuntimeException: Trying to read file with encrypted footer. No keys available
at org.apache.parquet.hadoop.ParquetFileReader.readFooter(ParquetFileReader.java:655)
at org.apache.parquet.hadoop.ParquetFileReader.readFooter(ParquetFileReader.java:578)
at org.apache.parquet.hadoop.ParquetFileReader.<init>(ParquetFileReader.java:971)
at org.apache.parquet.hadoop.ParquetFileReader.<init>(ParquetFileReader.java:961)
at org.apache.parquet.hadoop.ParquetFileReader.open(ParquetFileReader.java:730)
at org.apache.iceberg.parquet.ReadConf.newReader(ReadConf.java:194)
at org.apache.iceberg.parquet.ReadConf.<init>(ReadConf.java:76)
at org.apache.iceberg.parquet.ParquetReader.init(ParquetReader.java:74)
`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary merged PR #16724:
URL: https://github.com/apache/iceberg/pull/16724
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3380014992
##########
data/src/test/java/org/apache/iceberg/data/FileFormatTestSupport.java:
##########
@@ -58,4 +59,6 @@ void writeRecordsWithoutFieldIds(OutputFile outputFile, Schema schema, List<Reco
String metadataValue(InputFile inputFile, String key) throws IOException;
String splitSizeProperty();
+
+ void assertEncryptedFileUnreadable(ThrowingCallable readEncryptedFile);
Review Comment:
> Also, I want to verify that the write succeeded by reading the data back without the encryption info, which is why this is needed too.
Why would we need this method for this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3380212752
##########
data/src/test/java/org/apache/iceberg/data/FileFormatTestSupport.java:
##########
@@ -58,4 +59,6 @@ void writeRecordsWithoutFieldIds(OutputFile outputFile, Schema schema, List<Reco
String metadataValue(InputFile inputFile, String key) throws IOException;
String splitSizeProperty();
+
+ void assertEncryptedFileUnreadable(ThrowingCallable readEncryptedFile);
Review Comment:
Could we supress that warning somehow?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3409820552
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
Review Comment:
> Trying to read file with encrypted footer. No keys available
org.apache.parquet.crypto.ParquetCryptoRuntimeException: Trying to read file with encrypted footer. No keys available
This is another reason not to test Parquet in this method. Can you add FEATURE_AES_STREAM_ENCRYPTION as missing for Parquet, and try to remove the `.keyMetadata(encryptedOutputFile.keyMetadata());` call here
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3395338065
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
In Iceberg, parquet files are encrypted only via Parquet native encryption, not via AES stream encryption.
> encryption can already be achieved by setting keyMetadata in Parquet
Technically, this setting might work, but it is meaningless since not used in Iceberg
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3395723805
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -185,11 +191,16 @@ private static List<Record> project(List<Record> records, Schema targetSchema) {
FEATURE_CASE_SENSITIVE,
FEATURE_SPLIT,
FEATURE_COLUMN_LEVEL_METRICS,
- FEATURE_COLUMN_METRICS_TRUNCATE_BINARY
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_ENCRYPTION
},
FileFormat.ORC,
new String[] {
- FEATURE_REUSE_CONTAINERS, FEATURE_COLUMN_METRICS_TRUNCATE_BINARY, FEATURE_READER_DEFAULT
+ FEATURE_REUSE_CONTAINERS,
+ FEATURE_COLUMN_METRICS_TRUNCATE_BINARY,
+ FEATURE_READER_DEFAULT,
+ FEATURE_ENCRYPTION_KEY_METADATA,
+ FEATURE_ENCRYPTION
});
Review Comment:
This is due to the code in the link I've sent earlier. But again, it does not matter - aesStreamEncryption should be listed as a missing feature for Parquet, because it is not leveraged by Iceberg. We should not test this combination.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#issuecomment-4702209037
@ggershinsky @pvary I think the current two UTs can cover the testing of the encryption part of the file format API. If you have time, please help take a look — maybe there are just some discrepancies in the test case names or suggestions. Thanks a lot!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3380782477
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
Review Comment:
the `encryptedOutputFile` has the keyMetadata. why do we need to pass it again?
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
Review Comment:
All types of Iceberg encryption use a key and an AAD Prefix. Why do we need two different functions, `testDataWriterEncryption` and `testDataWriterWithFileEncryptionKeyAndWithAADPrefix`?
(maybe one tests Avro encryption, and the other Parquet encryption?)
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
+ throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ String location = "test-file-" + UUID.randomUUID();
+ NativeEncryptionKeyMetadata keyMetadata =
+ (NativeEncryptionKeyMetadata)
+ encryptingFileIO.newEncryptingOutputFile(location).keyMetadata();
+
+ // Use a plain encrypted output file so Parquet cannot auto-inject the native encryption key
+ // and AAD prefix from the output file metadata. This ensures encryption is driven only by
+ // withFileEncryptionKey and withAADPrefix below.
+ EncryptedOutputFile encryptedOutputFile =
+ EncryptedFiles.plainAsEncryptedOutput(fileIO.newOutputFile(location));
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(keyMetadata)
+ .withFileEncryptionKey(keyMetadata.encryptionKey().duplicate())
+ .withAADPrefix(keyMetadata.aadPrefix().duplicate());
Review Comment:
can we utilize [this](https://github.com/apache/iceberg/blob/main/parquet/src/main/java/org/apache/iceberg/parquet/Parquet.java#L153) instead?
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
+ throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ String location = "test-file-" + UUID.randomUUID();
+ NativeEncryptionKeyMetadata keyMetadata =
+ (NativeEncryptionKeyMetadata)
+ encryptingFileIO.newEncryptingOutputFile(location).keyMetadata();
+
+ // Use a plain encrypted output file so Parquet cannot auto-inject the native encryption key
+ // and AAD prefix from the output file metadata. This ensures encryption is driven only by
+ // withFileEncryptionKey and withAADPrefix below.
+ EncryptedOutputFile encryptedOutputFile =
+ EncryptedFiles.plainAsEncryptedOutput(fileIO.newOutputFile(location));
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(keyMetadata)
+ .withFileEncryptionKey(keyMetadata.encryptionKey().duplicate())
+ .withAADPrefix(keyMetadata.aadPrefix().duplicate());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @SuppressWarnings("checkstyle:AssertThatThrownByWithMessageCheck")
+ private void writeAndAssertEncryptedDataWriter(
+ FileFormat fileFormat,
+ EncryptingFileIO encryptingFileIO,
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder)
+ throws IOException {
+ DataGenerator dataGenerator = new DataGenerators.DefaultSchema();
+ Schema schema = dataGenerator.schema();
+ List<Record> genericRecords = dataGenerator.generateRecords();
+ List<T> engineRecords = convertToEngineRecords(genericRecords, schema);
+
+ DataWriter<T> writer = writerBuilder.schema(schema).spec(PartitionSpec.unpartitioned()).build();
+
+ try (writer) {
+ engineRecords.forEach(writer::write);
+ }
+
+ DataFile dataFile = writer.toDataFile();
+ assertThat(dataFile).isNotNull();
+ assertThat(dataFile.recordCount()).isEqualTo(engineRecords.size());
+ assertThat(dataFile.format()).isEqualTo(fileFormat);
+ assertThat(dataFile.keyMetadata()).isNotNull();
+
+ assertThatThrownBy(
Review Comment:
how does this work for ORC files? They can't be encrypted
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "ggershinsky (via GitHub)" <gi...@apache.org>.
ggershinsky commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3381351684
##########
data/src/test/java/org/apache/iceberg/data/BaseFormatModelTests.java:
##########
@@ -1706,6 +1715,98 @@ void testDataWriterMetaMap(FileFormat fileFormat) throws IOException {
});
}
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterEncryption(FileFormat fileFormat) throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ EncryptedOutputFile encryptedOutputFile =
+ encryptingFileIO.newEncryptingOutputFile("test-file-" + UUID.randomUUID());
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(encryptedOutputFile.keyMetadata());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @ParameterizedTest
+ @FieldSource("FILE_FORMATS")
+ void testDataWriterWithFileEncryptionKeyAndWithAADPrefix(FileFormat fileFormat)
+ throws IOException {
+ assumeSupports(fileFormat, FEATURE_ENCRYPTION);
+
+ EncryptionManager encryptionManager = EncryptionTestHelpers.createEncryptionManager();
+ EncryptingFileIO encryptingFileIO = EncryptingFileIO.combine(fileIO, encryptionManager);
+ String location = "test-file-" + UUID.randomUUID();
+ NativeEncryptionKeyMetadata keyMetadata =
+ (NativeEncryptionKeyMetadata)
+ encryptingFileIO.newEncryptingOutputFile(location).keyMetadata();
+
+ // Use a plain encrypted output file so Parquet cannot auto-inject the native encryption key
+ // and AAD prefix from the output file metadata. This ensures encryption is driven only by
+ // withFileEncryptionKey and withAADPrefix below.
+ EncryptedOutputFile encryptedOutputFile =
+ EncryptedFiles.plainAsEncryptedOutput(fileIO.newOutputFile(location));
+
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder =
+ FormatModelRegistry.dataWriteBuilder(fileFormat, engineType(), encryptedOutputFile)
+ .keyMetadata(keyMetadata)
+ .withFileEncryptionKey(keyMetadata.encryptionKey().duplicate())
+ .withAADPrefix(keyMetadata.aadPrefix().duplicate());
+
+ writeAndAssertEncryptedDataWriter(fileFormat, encryptingFileIO, writerBuilder);
+ }
+
+ @SuppressWarnings("checkstyle:AssertThatThrownByWithMessageCheck")
+ private void writeAndAssertEncryptedDataWriter(
+ FileFormat fileFormat,
+ EncryptingFileIO encryptingFileIO,
+ FileWriterBuilder<DataWriter<T>, ?> writerBuilder)
+ throws IOException {
+ DataGenerator dataGenerator = new DataGenerators.DefaultSchema();
+ Schema schema = dataGenerator.schema();
+ List<Record> genericRecords = dataGenerator.generateRecords();
+ List<T> engineRecords = convertToEngineRecords(genericRecords, schema);
+
+ DataWriter<T> writer = writerBuilder.schema(schema).spec(PartitionSpec.unpartitioned()).build();
+
+ try (writer) {
+ engineRecords.forEach(writer::write);
+ }
+
+ DataFile dataFile = writer.toDataFile();
+ assertThat(dataFile).isNotNull();
+ assertThat(dataFile.recordCount()).isEqualTo(engineRecords.size());
+ assertThat(dataFile.format()).isEqualTo(fileFormat);
+ assertThat(dataFile.keyMetadata()).isNotNull();
+
+ assertThatThrownBy(
Review Comment:
SGTM for ORC.
Encryption of tables with Avro data format is supported, https://github.com/apache/iceberg/pull/9436
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on code in PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#discussion_r3380296937
##########
data/src/test/java/org/apache/iceberg/data/FileFormatTestSupport.java:
##########
@@ -58,4 +59,6 @@ void writeRecordsWithoutFieldIds(OutputFile outputFile, Schema schema, List<Reco
String metadataValue(InputFile inputFile, String key) throws IOException;
String splitSizeProperty();
+
+ void assertEncryptedFileUnreadable(ThrowingCallable readEncryptedFile);
Review Comment:
Great idea. Add @SuppressWarnings("checkstyle:AssertThatThrownByWithMessageCheck") to ensure that this functionality works properly.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "pvary (via GitHub)" <gi...@apache.org>.
pvary commented on PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#issuecomment-4781145523
Merged to main.
Thanks @Guosmilesmile for the new tests and @ggershinsky for the review!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] Data: Add TCK for Encrypt in FileFormat API [iceberg]
Posted by "Guosmilesmile (via GitHub)" <gi...@apache.org>.
Guosmilesmile commented on PR #16724:
URL: https://github.com/apache/iceberg/pull/16724#issuecomment-4666289799
I made two adjustments:
1. Added validation of KeyMetadata encryption for the Avro format.
2. Regarding `testDataWriterWithFileEncryptionKeyAndWithAADPrefix`: I found that if `withFileEncryptionKey` and `withAADPrefix` are not explicitly called, reading the file afterwards throws Applying decryptor on plaintext file. This confirms that these two methods do take effect.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org