composefs: Properly add verity info to generated dump file#816
composefs: Properly add verity info to generated dump file#816alexlarsson wants to merge 1 commit into
Conversation
mtrmac
left a comment
mtrmac
left a comment
There was a problem hiding this comment.
Thanks! ACK to the core idea for regular files.
Can you, please, update the unit test to be more realistic? We shouldn’t be able to make such a change without unit tests failing.
(Perhaps even run each current test case twice, once with digests and once without? That would need manipulating the expected data…)
| digest := verityDigests[payload] | ||
| digest := "" | ||
| if entry.Type == minimal.TypeReg || entry.Type == minimal.TypeLink { | ||
| digest = verityDigests["/"+payload] |
There was a problem hiding this comment.
AFAICS the map is only updated when writing regular files, not hard links.
So, if, for a hard link the “payload” is a link to another file as seen in the composefs mount, this will never find an entry for a hard link. Am I missing anything?
There was a problem hiding this comment.
No, you're right, I just didn't completely think through the hardlink case. For hardlinks we don't need to generate any expected digest, because it will just use whatever was specified for the target that they payload points at.
So, we're fine with just if entry.Type == minimal.TypeReg here.
| digest := verityDigests[payload] | ||
| digest := "" | ||
| if entry.Type == minimal.TypeReg || entry.Type == minimal.TypeLink { | ||
| digest = verityDigests["/"+payload] |
There was a problem hiding this comment.
Non-blocking, pre-existing: Should this, really, fail if an entry isn’t found and we expect one?
I guess ideally we would differentiate between verityDigest == nil (not available) and != nil (available and expected to be fully populated), but that’s not what the chunked code currently produces.
There was a problem hiding this comment.
Honestly, probably. I mean, there are two usecases here, first, we don't care about verity at all, and secondly, we want to require verity for all regular file accesses. And in the first case you either do it right or pass a nil, and in the second we really do want to error out during image creation, not at runtime when accessing a file.
|
@giuseppe I think containers/fuse-overlayfs#457 changed the build system and broke our CI here… |
I am working on a fix here #818 |
The payloads in the dump files are relative, but the verityDigests map has absolute filenames. Also, lets only look for verity digests for regular files, not fot e.g. symlinks (where the payload is the target of the link). Signed-off-by: Alexander Larsson <alexl@redhat.com>
alexlarsson
force-pushed
the
fix-composefs-verity
branch
from
1051eb9 to
b8fa3ea
Compare
|
I rebased for the ci fix, added some tests and removed the lookup for hardlinks. |
4 participants
The payloads in the dump files are relative, but the verityDigests map has absolute filenames.
Also, lets only look for verity digests for regular files, not fot e.g. symlinks (where the payload is the target of the link).