Skip to content
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

RBD fscrypt support #3310

Merged
merged 35 commits into from
Oct 17, 2022
Merged

RBD fscrypt support #3310

merged 35 commits into from
Oct 17, 2022

Conversation

irq0
Copy link
Contributor

@irq0 irq0 commented Aug 17, 2022

Add fscrypt integration with the Ceph CSI KMS. Supports ext4 on RBD. Snapshots are supported as well.
Implementation is based on the design doc https://github.com/ceph/ceph-csi/blob/devel/docs/design/proposals/cephfs-fscrypt.md.

Testing requires kernel support (see https://github.com/google/fscrypt#runtime-dependencies). Minikube needs kubernetes/minikube#14783 to run the e2e test suite.

Commits are ordered as follows :

  1. utils and refactoring
  2. general fscrypt integration
  3. fscrypt into RBD integration
  4. e2e testing

@irq0
Copy link
Contributor Author

irq0 commented Aug 17, 2022

Was #3158

As this is already pretty large, I left out the Ceph FS bits for now. (They are on https://github.com/irq0/ceph-csi/tree/wip/fscrypt-go-rbd-cephfs)

@mergify
Copy link
Contributor

mergify bot commented Aug 18, 2022

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@nixpanic nixpanic requested review from nixpanic and a team August 18, 2022 12:33
@nixpanic nixpanic added enhancement New feature or request component/rbd Issues related to RBD labels Aug 18, 2022
@humblec humblec mentioned this pull request Aug 23, 2022
9 tasks
@irq0 irq0 force-pushed the wip/fscrypt-go-rbd branch 2 times, most recently from 2a1a995 to c3a2790 Compare August 24, 2022 09:05
@irq0
Copy link
Contributor Author

irq0 commented Aug 24, 2022

The last push rebased on current develop branch and addressed the linter errors. The minikube tests on CI will again fail, as minikube doesn't have ext4 fscrypt support.

The tests failing with timeouts raise a question though: Maybe not having kernel support should return a not retryable error instead of an internal error.
Maybe codes.Unimplemented (spec: https://github.com/container-storage-interface/spec/blob/master/spec.md#error-scheme)? Thaughts?

@mergify
Copy link
Contributor

mergify bot commented Aug 25, 2022

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@irq0 irq0 force-pushed the wip/fscrypt-go-rbd branch 4 times, most recently from 7588f66 to 395ca31 Compare August 31, 2022 08:08
@irq0
Copy link
Contributor Author

irq0 commented Aug 31, 2022

🥳 the e2e suites passed. The linter finds a line that is a bit too long. I already fixed it in my local branch. I suggest I push the fix with the first round of review changes.

From my side this is ready to review.

Copy link
Member

@nixpanic nixpanic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks really good! Sorry for needing a long time to review. I have come across only a few minor things. Thanks for continuing with this huge contribution!

internal/util/crypto.go Show resolved Hide resolved
internal/util/crypto.go Show resolved Hide resolved
internal/kms/dummy.go Show resolved Hide resolved
internal/util/getsecret_test.go Show resolved Hide resolved
internal/util/fscrypt/fscrypt.go Show resolved Hide resolved
internal/rbd/nodeserver.go Outdated Show resolved Hide resolved
internal/rbd/encryption.go Show resolved Hide resolved
examples/rbd/storageclass.yaml Show resolved Hide resolved
e2e/rbd.go Show resolved Hide resolved
build.env Outdated Show resolved Hide resolved
@irq0 irq0 force-pushed the wip/fscrypt-go-rbd branch 4 times, most recently from 80a0541 to 059f179 Compare September 7, 2022 16:36
@irq0
Copy link
Contributor Author

irq0 commented Sep 8, 2022

The last push addresses the review comments and adds a small fix to the fscrypt integration code (fscrypt: fix metadata directory permissions). It didn't cause problems on rbd+ext4, but on cephfs.

ci/centos/k8s-e2e-external-storage/1.23, ci/centos/mini-e2e-helm/k8s-1.22 failed in the 'reserve bare-metal machine' stage. Is there a way to trigger them individually?

@irq0
Copy link
Contributor Author

irq0 commented Sep 8, 2022

/retest ci/centos/k8s-e2e-external-storage/1.23

@irq0
Copy link
Contributor Author

irq0 commented Sep 8, 2022

/retest ci/centos/mini-e2e-helm/k8s-1.22

@irq0
Copy link
Contributor Author

irq0 commented Sep 8, 2022

ci/centos/k8s-e2e-external-storage/1.23, ci/centos/mini-e2e-helm/k8s-1.22 failed in the 'reserve bare-metal machine' stage. Is there a way to trigger them individually?

Found it :)

Marcel Lauhoff added 24 commits October 17, 2022 15:18
Currently fscrypt supports policies version 1 and 2. 2 is the best
choice and was the only choice prior to this commit. This adds support
for kernels < 5.4, by selecting policy version 1 there.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
NewContextFrom{Mountpoint,Path} functions use cached
`/proc/self/mountinfo` to find mounted file systems by device ID.
Since we run fscrypt as a library in a long-lived process the cached
information is likely to be stale. Stale entries may map device IDs to
mount points of already destroyed RBDs and fail context creation.
Updating the cache beforehand prevents this.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Use constant protector name 'ceph-csi' instead of constant prefix
concatenated with the volume ID. When cloning volumes the ID changes
and fscrypt protected directories become inunlockable due to the
protector name change

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Revert once our google/fscrypt dependency is upgraded to a version
that includes google/fscrypt#359 gets accepted

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Call Mount.Setup with SingleUserWritable constant instead of 0o755,
which is silently ignored and causes the /.fscrypt/{policy,protector}/
directories to have mode 000.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Integrate basic fscrypt functionality into RBD initialization. To
activate file encryption instead of block introduce the new
'encryptionType' storage class key.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Different places have different meaningful fallback. When parsing
from user we should default to block, when parsing stored config we
should default to invalid and handle that as an error.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Add fscrypt support to the journal to support operations like
snapshotting.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Support fscrypt on RBD snapshots

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Add validation functions for fscrypt on RBD volumes

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Add a `By` wrapper to parameterize encryption related test functions
and run them on both block and file encryption

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Replace `By` with `ByFileAndBlockEncryption` in all encryption related
tests to parameterize them to file and block encryption.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Add test that enables encryption with default type. Check that we set
up block encryption.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Apply formatting for previous changes separately to make the commit
diffs easier to read.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Make iso url configurable to use pre-release minikube images or
local-built (file://)

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Add type none to distinguish disabled encryption (positive result)
from invalid configuration (negative result).

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
Add test-rbd-fscrypt feature flag to e2e suite. Default disabled as
the current CI system's kernel doesn't have the required features
enabled.

Signed-off-by: Marcel Lauhoff <marcel.lauhoff@suse.com>
@mergify mergify bot merged commit 69b8fee into ceph:devel Oct 17, 2022
@irq0 irq0 deleted the wip/fscrypt-go-rbd branch October 18, 2022 06:57
@irq0 irq0 mentioned this pull request Oct 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/rbd Issues related to RBD enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants