-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Spec: Add key-id and deprecate key-metadata in table and partition statistics #17533
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
gaborkaszab
wants to merge
3
commits into
apache:main
Choose a base branch
from
gaborkaszab:main_spec_key_id_in_stats
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-metadatabut add an optionalkey-idthat is used to encrypt the stats file key?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
EncryptedKeyinto theencryption-keyslist. 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 ofEncryptedKeyentries across different manifest lists, apart from the KEK used to encrypt theEncryptedKeys.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.
key-idfor statistics files is just an ID to be used for finding theEncryptedKeyin theencryption-keyslist that in turn can be used to decrypt the statistics files (after decrypting the foundEncryptedFileitself using the KEK). Whether this is a reusedEncryptedFileacross 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 :)