smaheshwar-pltr commented on code in PR #17404:
URL: https://github.com/apache/iceberg/pull/17404#discussion_r3670162293
##########
core/src/test/java/org/apache/iceberg/TestManifestListEncryption.java:
##########
@@ -116,104 +111,71 @@ public void testEncryption() throws IOException {
assertThat((long) manifest.existingRowsCount()).isEqualTo(EXISTING_ROWS);
assertThat((int) manifest.deletedFilesCount()).isEqualTo(DELETED_FILES);
assertThat((long) manifest.deletedRowsCount()).isEqualTo(DELETED_ROWS);
- assertThat(manifest.content()).isEqualTo(ManifestContent.DATA);
}
@Test
- public void testKeyWrappingAndRotation() throws IOException {
- EncryptionManager em = EncryptionTestHelpers.createEncryptionManager();
- // This manager uses UnitestKMS.MASTER_KEY_NAME1 as the table master key
- String tableMasterKeyID = UnitestKMS.MASTER_KEY_NAME1;
-
- // Initial write/read
- writeAndReadEncryptedManifestList(em);
- Map<String, EncryptedKey> keyList = EncryptionUtil.encryptionKeys(em);
- // Two keys: manifest list key (metadata), and its key encryption key
- assertThat(keyList.size()).isEqualTo(2);
- String initialKekID = EncryptionTestHelpers.keyEncryptionKeyID(em);
- int kekCount = 0;
- int mlkmCount = 0;
-
- for (String keyID : keyList.keySet()) {
- EncryptedKey key = keyList.get(keyID);
- if (key.encryptedById().equals(tableMasterKeyID)) { // key encryption key
- kekCount++;
- assertThat(keyID).isEqualTo(initialKekID);
- } else { // manifest list key metadata
- mlkmCount++;
- assertThat(key.encryptedById()).isEqualTo(initialKekID);
- }
- }
+ public void testFirstMintCreatesKeyEncryptionKey() throws IOException {
+ StandardEncryptionManager.MintedKeys minted =
+ writeAndReadEncryptedManifestList(Lists.newArrayList(), 0).minted;
+
+ assertThat(minted.newKeyEncryptionKey()).isNotNull();
+ assertThat(minted.manifestListKey().encryptedById())
+ .isEqualTo(minted.newKeyEncryptionKey().keyId());
+
assertThat(minted.newKeyEncryptionKey().encryptedById()).isEqualTo(UnitestKMS.MASTER_KEY_NAME1);
+ }
- assertThat(kekCount).isEqualTo(1);
- assertThat(mlkmCount).isEqualTo(1);
-
- // Write/read after 30 days
- EncryptionTestHelpers.shiftEncryptionManagerTime(em,
TimeUnit.DAYS.toMillis(30));
- writeAndReadEncryptedManifestList(em);
- // below rotation time, key encryption key must be the same
-
assertThat(EncryptionTestHelpers.keyEncryptionKeyID(em)).isEqualTo(initialKekID);
- keyList = EncryptionUtil.encryptionKeys(em);
- // three keys: two manifest list keys (metadata), and their key encryption
key
- assertThat(keyList.size()).isEqualTo(3);
- Set<String> intermediateKeySet = Sets.newHashSet((keyList.keySet()));
- kekCount = 0;
- mlkmCount = 0;
-
- for (String keyID : intermediateKeySet) {
- EncryptedKey key = keyList.get(keyID);
- if (key.encryptedById().equals(tableMasterKeyID)) { // key encryption key
- kekCount++;
- assertThat(keyID).isEqualTo(initialKekID);
- } else { // manifest list key metadata
- mlkmCount++;
- assertThat(key.encryptedById()).isEqualTo(initialKekID);
- }
- }
+ @Test
+ public void testKeyEncryptionKeyReusedBeforeRotation() throws IOException {
+ List<EncryptedKey> metadataKeys = Lists.newArrayList();
+ String initialKekId =
+ writeAndReadEncryptedManifestList(metadataKeys,
0).minted.newKeyEncryptionKey().keyId();
- assertThat(kekCount).isEqualTo(1);
- assertThat(mlkmCount).isEqualTo(2);
-
- // Write/read after 800 days
- EncryptionTestHelpers.shiftEncryptionManagerTime(em,
TimeUnit.DAYS.toMillis(800));
- writeAndReadEncryptedManifestList(em);
- String newKekID = EncryptionTestHelpers.keyEncryptionKeyID(em);
- // above rotation time, key encryption key must be different
- assertThat(newKekID).isNotEqualTo(initialKekID);
- keyList = EncryptionUtil.encryptionKeys(em);
- // five keys: three manifest list keys (metadata), and two key encryption
keys (old and new)
- assertThat(keyList.size()).isEqualTo(5);
- kekCount = 0;
- mlkmCount = 0;
-
- for (String keyID : keyList.keySet()) {
- if (!intermediateKeySet.contains(keyID)) { // new keys
- EncryptedKey key = keyList.get(keyID);
- if (key.encryptedById().equals(tableMasterKeyID)) { // key encryption
key
- kekCount++;
- assertThat(keyID).isEqualTo(newKekID);
- } else { // manifest list key metadata
- mlkmCount++;
- assertThat(key.encryptedById()).isEqualTo(newKekID); // wrapped by
new kek
- }
- }
- }
+ // 30 days is well within the 2-year lifespan, so the existing KEK must be
reused.
+ StandardEncryptionManager.MintedKeys second =
+ writeAndReadEncryptedManifestList(metadataKeys,
TimeUnit.DAYS.toMillis(30)).minted;
+
+ assertThat(second.newKeyEncryptionKey()).isNull();
+
assertThat(second.manifestListKey().encryptedById()).isEqualTo(initialKekId);
+ }
- // new keys
- assertThat(kekCount).isEqualTo(1);
- assertThat(mlkmCount).isEqualTo(1);
+ @Test
+ public void testKeyEncryptionKeyRotatesAfterLifespan() throws IOException {
+ List<EncryptedKey> metadataKeys = Lists.newArrayList();
+ String initialKekId =
+ writeAndReadEncryptedManifestList(metadataKeys,
0).minted.newKeyEncryptionKey().keyId();
+
+ // 800 days exceeds the 2-year (730-day) lifespan, forcing rotation.
+ StandardEncryptionManager.MintedKeys rotated =
+ writeAndReadEncryptedManifestList(metadataKeys,
TimeUnit.DAYS.toMillis(800)).minted;
+
+ assertThat(rotated.newKeyEncryptionKey()).isNotNull();
+
assertThat(rotated.newKeyEncryptionKey().keyId()).isNotEqualTo(initialKekId);
+ assertThat(rotated.manifestListKey().encryptedById())
+ .isEqualTo(rotated.newKeyEncryptionKey().keyId());
}
- private ManifestFile writeAndReadEncryptedManifestList(EncryptionManager em)
throws IOException {
+ /**
+ * Writes an encrypted manifest list with a manager built from {@code
metadataKeys}, persists the
Review Comment:
i've not audited these tests but having seen ur other tests, i'm highly
skeptical your tests in this PR are good. please do an audit for software
quality with good-testing-patterns
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
