Fsverity content verification #10007

Jenkins-J · 2024-03-27T14:17:54Z

The Linux Kernel fs-verity feature can be utilized to verify the integrity of blob data in the content store. Enabling fs-verity on the blob data allows fs-verity to verify the integrity of the data at the time it is read. A read error will occur if fs-verity detects any accidental corruption of the data.

Here, containerd will enable fs-verity on the content blob files if it determines that both the kernel and the filesystem support fs-verity operations.

Addresses issue #3849 and issue #953.

k8s-ci-robot · 2024-03-27T14:18:03Z

Hi @Jenkins-J. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

pkg/fsverity/fsverity_linux.go

plugins/content/local/writer.go

AkihiroSuda · 2024-03-27T23:41:22Z

plugins/content/local/writer.go

Can we have a unit test?

Just to clarify, would you like a unit test of the modified writer Commit method, the functions in the fsverity package, or both?

Ideally both

Thank you, I will work on creating those unit tests.

@AkihiroSuda I've created some tests for the new functionality, please take a look. Thank you.

AkihiroSuda · 2024-03-27T23:42:55Z

Please squash the commits

AkihiroSuda · 2024-03-27T23:44:24Z

pkg/fsverity/fsverity_linux.go

+
+	args.blockSize = uint32(blockSize)
+
+	_, _, errno := unix.Syscall(syscall.SYS_IOCTL, f.Fd(), uintptr(unix.FS_IOC_ENABLE_VERITY), uintptr(unsafe.Pointer(args)))


content.Provider needs to call FS_IOC_MEASURE_VERITY ?

The fsverity module will automatically verify the integrity of the data on every read operation of the enabled file. If fsverity detects corruption, the read operation will return an error. The case where we would need to call FS_IOC_MEASURE_VERITY manually is if we wanted to detect intentional, malicious corruption of the blob data. In that case, we would measure the data after fsverity is enabled on the blob, safely store the known-good digest, then when the blob data a is read by the provider, call FS_IOC_MEASURE_VERITY again and compare the returned digest to the known-good value.

I'd be happy to implement this if the malicious corruption of blob data is a case containerd should cover.

Yes, it would be nice to cover malicious corruption.
Can be another PR.

Implement calls to the fsverity kernel module, allowing containerd to enable fsverity on blob data in the content store. This causes fsverity to veirfy the integrity of blob data when the blob is read. Signed-off-by: James Jenkins <James.Jenkins@ibm.com>

fuweid · 2024-04-19T03:21:01Z

HI @AkihiroSuda shall we target this to v2.1 release? It's good feature but we still have pending work items in 2.0, like transfer service migration for CRI.

AkihiroSuda · 2024-04-19T03:47:12Z

HI @AkihiroSuda shall we target this to v2.1 release?

I think this is safe to have in v2.0, but can be postponed to v2.1 if you have a concern.

It's good feature but we still have pending work items in 2.0, like transfer service migration for CRI.

I'm not sure we can complete this in v2.0, as we will retain schema1 (now disabled by default).
Can we discuss this in another GitHub issue?

fuweid · 2024-04-19T05:04:10Z

I think this is safe to have in v2.0, but can be postponed to v2.1 if you have a concern.

It's in content store code path. But yeah, it looks good and safe to have in 2.0.
Just want to commit existing pending patches first.

I'm not sure we can complete this in v2.0, as we will retain schema1 (now disabled by default). Can we discuss this in another GitHub issue?

Sure.

AkihiroSuda · 2024-04-25T00:18:53Z

pkg/fsverity/fsverity_test.go

+	defer func() {
+		err := m.Close()
+		if err != nil {
+			fmt.Printf("could not close mountinfo\n")


Suggested change

fmt.Printf("could not close mountinfo\n")

t.Fatalf("could not close mountinfo: %v", err)

@AkihiroSuda This change is in a helper function, resolveDevicePath, which does not have access to the testing.T variable t. I've modified it so now the appropriate error message is returned from this function. I can pass the variable t as a parameter to these helper functions instead if you believe the code would be more readable that way.

Signed-off-by: James Jenkins <James.Jenkins@ibm.com>

fuweid · 2024-04-26T08:57:22Z

Run out of bandwidth this week. Will take a look this after travel.

fuweid · 2024-05-06T09:01:53Z

pkg/fsverity/fsverity_test.go

+
+func TestIsEnabled(t *testing.T) {
+
+	testDir := filepath.Join(t.TempDir(), "content")


I think we can use truncate and mkfs.ext4 -O verity to create testable environment for this function.
verity is not default option in most distros so it would help.
It can be handled in the follow-up.

fuweid · 2024-05-06T09:05:05Z

plugins/content/local/writer.go

+		integritySupported bool
+		supportErr         error
+	)
+	if integritySupported, supportErr = fsverity.IsSupported(w.s.root); integritySupported {


I think the IsSupported cache should be handled in content local plugin.
The fsverity.IsSupported is just handled once. If we change the input from A to B, the cache might not be correct.

I've updated the implementation to cache IsSupported value in the content plugin.

Cache the result of the IsSupported fsverity method in the content plugin instead of in the fsverity package. Signed-off-by: James Jenkins <James.Jenkins@ibm.com>

fuweid · 2024-05-07T03:09:12Z

plugins/content/local/writer.go

+	startedAt          time.Time
+	updatedAt          time.Time
+	once               sync.Once
+	integritySupported bool


what about moving this to store struct?

type store struct { root string ls LabelStore integritySupported bool }

So that we can just run fsverity.IsSupported once and all the writers from same store can share it.

Thank you, that makes more sense now. I've made the change.

fuweid · 2024-05-07T03:11:37Z

pkg/fsverity/fsverity_linux.go

+	}
+
+	digestFile.Close()
+	defer os.RemoveAll(integrityDir)


move this line to L57.

fuweid · 2024-05-07T03:14:58Z

pkg/fsverity/fsverity_linux.go

+		return s, err
+	}
+
+	integrityDir := filepath.Join(rootPath, "integrity")


I think we can rename this to .fsverity-check by os.MkdirTemp. So, the node admin will know it's tempdir to verify something.

Done. Thank you for the suggestion.

Move cached fsverity integrity supported value from the local content writer to the local content store. Signed-off-by: James Jenkins <James.Jenkins@ibm.com>

Clean up fsverity IsSupproted function, improving readability. Signed-off-by: James Jenkins <James.Jenkins@ibm.com>

fuweid

Thanks!

k8s-ci-robot added the do-not-merge/work-in-progress label Mar 27, 2024

k8s-ci-robot added needs-ok-to-test size/L labels Mar 27, 2024

Jenkins-J force-pushed the fsverity-content-verification branch 4 times, most recently from e77afe7 to aa28f9e Compare March 27, 2024 15:14