Spec: Add key-id and deprecate key-metadata in table and partition statistics

[gaborkaszab]

Storing key-metadata in TableStatistics and PartitionStatistics is unsecured, because that is persisted into table metadata, that itself is not encrypted. As an existing design for manifest list, we encrypt the key-metadata, and save the key-id of the encryption key that we used to encrypt key-metadata.

[singhpk234]

key-metadata in TableStatistics and PartitionStatistics is unsecured, because that is persisted into table metadata

i understand the rational of using key-id but what i don't understand is why storing key-metadata unsecure ? because we contain wrapped key as if encrypted value instead of raw ?

are there more details to this ....

cc @ggershinsky

[gaborkaszab]

Hey @singhpk234 ,

The problem with key-metadata is that it is the raw, unencrypted information on how to decrypt stuff. While it's safe to keep key-metadata on e.g. the data file / manifest entry level, because those files are encrypted themselves in turn encrypting key-metadata together with the file it contains this field, it's not safe to keep it on the table metadata level because that is not encrypted. It would expose the raw key-metadata. The same goes for manifest list encryption, and I believe we should follow the same approach for statistics file linked from table metadata.

See @ggershinsky 's and @RussellSpitzer 's comments here.

[singhpk234]

Ah i see, this is unwrapped key metadata which was used to encrypt this file, and not the encrypted keymedata, then it makes sense to have have this wrapped and then have key-id from the table.metadata_keys .... we will wrap this KEK then like we do with MLK then ?

[gaborkaszab]

@singhpk234 yes, this would work exactly the same way as manifest list encryption keys.

[RussellSpitzer]

This looks good to me, we should bring it up on the dev list before we merge the spec change

[gaborkaszab]

This looks good to me, we should bring it up on the dev list before we merge the spec change

Thanks for the reviews, @ggershinsky @singhpk234 @RussellSpitzer !
I opened the vote thread on the dev list now: https://lists.apache.org/thread/rx0tcnqkq0nzj1phwo64ng79pp51hzf9

[singhpk234]

+1

[stevenzwu]

We changed v2 and v3 to v1 and v2. is this intentional?

[gaborkaszab]

That is intentional. These columns are the same between "v1" and "v2 and v3". Initially I figured we could merge all these together, but I figured we might want to deprecate key-metadata earlier in V3 so kept that version separately. I can revert if this is distracting.

[gaborkaszab]

I did revert the regrouping to remove noise. I might open a follow-up refactor to merge v1-v3 cols in the table

[gaborkaszab]

@singhpk234 @RussellSpitzer @stevenzwu @ggershinsky @rdblue
I made some changes to the PR. Would you mind taking another look?

1. I did revert the regrouping of version cols. (I think a follow-up refactor could help to merge the identical cols to make the table more readable)
2. I kept the support of existing key-metadata in V4, but left it deprecated with a hint to use key-id instead.

[ggershinsky]

Thanks, LGTM

[rdblue]

I think this diff is much too large. We don't need to add a v4 column that is a copy of the other two existing columns and mix those changes into this diff. If I understand the diff correctly, the only thing that should change is that key-metadata should be deprecated and key-id should be added.

[gaborkaszab]

That's right. key-metadata is deprecated now, and there is a new field for key-id. Note, for key-metadata there was no type, I added string.
Reverted the V4 column, should be more readable now.

[rdblue]

Same here. Please revert the rest of the changes so this diff doesn't include the entire table.

[rdblue]

@gaborkaszab, I think the changes look okay, but this isn't very reviewable because I have to manually diff the tables. Since there aren't substantive changes to the other rows, let's minimize the diff.

[gaborkaszab]

Thanks for taking a look, @rdblue !
I reverted the V4 column now. Hope it's more readable.

[gaborkaszab]

Hey @rdblue ,
Do you think you can take a look?

[gaborkaszab]

@rdblue Is this something we can start a vote on?

[rdblue]

I think there is still a problem in that we are mixing file-level keys with table-level keys. The keys table is intended to be a small set of keys that are reused and occasionally rotated, not file-specific.

@ggershinsky, how should we handle these? Do we need a way to store encrypted key metadata? What I'm thinking is that we keep key-metadata but add an optional key-id that is used to encrypt the stats file key?

[ggershinsky]

for manifest lists, we did the following: #12162 #12927

An "encrypted key metadata" is a 4-field record (key-id, encrypted-key-metadata, encrypted-by-id, properties map). All of these records are kept in the TableMetadata.encryption-keys list. Some of them are reusable (key encryption keys), but most of them are specific to manifest files. Each snapshot object keeps the key-id of the "encrypted key metadata" of its manifest list file.

The situation with stats files looks similar (if not identical) to the manifest list files. A straightforward technical approach would be to do the same: write a 4-field "encrypted key metadata" record in the TableMetadata.encryption-keys list, and add pointer to the record (key-id) to the stats file object.

[gaborkaszab]

Thanks for looking into this @rdblue and @ggershinsky !

I think the main questions here are 1) reusability of keys and 2) whether reusability requires any special care in the spec.

1. I did an experiment the other day and for me it seems that each manifest list file adds a new EncryptedKey into the encryption-keys list. There is (are?) additional KEK entry(ies) on top of that in the list. I'm not sure about custom implementations but for me it seems that there is no reuse of EncryptedKey entries across different manifest lists, apart from the KEK used to encrypt the EncryptedKeys.
   Keys for stat files fit into this pattern, they could behave the same way as keys for manifest lists. Let me know if I miss something here.
2. I think the spec changes in this PR can express both the reused key and the non-reused key scenario. key-id for statistics files is just an ID to be used for finding the EncryptedKey in the encryption-keys list that in turn can be used to decrypt the statistics files (after decrypting the found EncryptedFile itself using the KEK). Whether this is a reused EncryptedFile across files or not, I don't think matters in this context.

While I'm still learning this area of the project, let me know where my assumptions go wrong :)

[ggershinsky]

these assumptions sgtm.

one general point on re-use of file encryption keys. Due to AES GCM limits, the standard practice is to create a key per file - otherwise, it's hard to guarantee the data safety. That's why our EncryptionManager generates a random key for each output file.