Add full reader verification #1457
Conversation
Before this change, the SOCI snapshotter would verify files on read. After this change, SOCI will verify all files after the background fetcher is done, whether their read or not. Signed-off-by: Kern Walster <walster@amazon.com>
| @@ -246,10 +247,19 @@ func NewFilesystem(ctx context.Context, root string, cfg config.FSConfig, opts . | |||
| } | |||
| go bgFetcher.Run(context.Background()) | |||
There was a problem hiding this comment.
Haven't looked too closely and unrelated, but any chance we might want to pass ctx into here?
There was a problem hiding this comment.
Yes, we probably should.
| } | ||
|
|
||
| var readerVerifier *reader.Verifier | ||
| if !cfg.DisableVerification { |
There was a problem hiding this comment.
So currently this is using the TOML config variable for disabling tar header verification. I think there's a world where one might want tar header integrity per file read (a somewhat hefty but not super resource-intensive process, particularly for lighter workloads) but not want to verify the entire image (a pretty resource-intensive process).
TL;DR I think this should be its own config variable.
| m, _ := binary.Uvarint(b.Get(bucketKeyMode)) | ||
| if !os.FileMode(uint32(m)).IsRegular() { | ||
| return fmt.Errorf("%q is not a regular file", id) | ||
| } |
There was a problem hiding this comment.
Why did this chunk of code get removed?
There was a problem hiding this comment.
This prevented calling metadataReader.OpenFile on a non-regular file. That's probably not the right thing to do because a metadata.File is the information about a file's uncompressed offset/size and tar header offset/size within the uncompressed layer. That applied to all files, not just regular files.
In a practical sense, the big issue is that reader.Verify can't verify directories without this change.
WIP: needs test
Issue #, if available:
Description of changes:
Before this change, the SOCI snapshotter would verify files on read. After this change, SOCI will verify all files after the background fetcher is done, whether their read or not.
Testing performed:
makeBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.