feat(encryption) [8/N] Read encrypted manifest file#2586
Conversation
xanderbailey
changed the title
mbutrovich
left a comment
mbutrovich
left a comment
There was a problem hiding this comment.
The change is correct and minimal - decode StandardKeyMetadata from self.key_metadata and read through EncryptedInputFile when present, plaintext otherwise - and it's nice that the roundtrip test exercises the AAD prefix.
Just a test request.
| let input = file_io.new_input(&self.manifest_path)?; | ||
| let avro = match &self.key_metadata { | ||
| Some(key_metadata_bytes) => { | ||
| let key_metadata = StandardKeyMetadata::decode(key_metadata_bytes)?; |
There was a problem hiding this comment.
Same hardcoded-decode point as #2584 (so I won't re-litigate it here) - flagging only that if the FileKeyResolver lands, this path needs to be included, and there's a threading wrinkle: load_manifest(&self, file_io: &FileIO) has no parameter to receive a resolver today. Whoever lands the seam will need to thread it (or an EncryptionManager) into load_manifest.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn test_load_manifest_decrypts_when_key_metadata_present() { |
There was a problem hiding this comment.
The roundtrip test is good. For parity with the data-file tests in #2584 (which cover missing-key and wrong-key), consider a wrong-key (or wrong-AAD) case asserting load_manifest fails cleanly, so a future change that silently mis-resolves the key is caught.
There was a problem hiding this comment.
Added a couple of tests
xanderbailey
force-pushed
the
xb/encrypted_manifest_file_read
branch
from
d415799 to
4c7e2df
Compare
Add failure-case coverage for load_manifest alongside the existing roundtrip test: decrypting with the wrong DEK or the wrong AAD prefix must fail cleanly rather than silently returning garbage. Extract a shared write_encrypted_manifest helper to avoid duplicating the writer setup across the three tests.
|
Thanks for the review @mbutrovich. Have added additional tests. @blackmwk would you be able to take a look here when you have a moment? |
2 participants
Which issue does this PR close?
What changes are included in this PR?
Read decrypted manifest files. This one is pretty straightforward, if the
ManifestFilehas key metadata on it, we should use that to construct anEncryptedInputFileand read the file using that.Are these changes tested?
Roundtrip test added showing that we can write and then read encrypted manifest files.