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

csi: add cephfs encryption support #14199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NymanRobin
Copy link
Contributor

@NymanRobin NymanRobin commented May 14, 2024

Ceph-CSI support for fscrypt encryption of cephfs.To achieve this commit add capability of mounting the required rook-ceph-csi-kms-config configmap into csi-cephfsplugin-provisioner and nodeplugin pods.

Further it modifies the modifies the ClusterRoles cephfs-csi-nodeplugin and cephfs-external-provisioner-runner to grant privileges required for reading encryption configuration and fetching encryption secrets from either kubernetes secrets or Key Management System (KMS).

These privileges are essential for the proper functioning of ceph-csi-cephfs with fscrypt encryption.

The following privileges have been added:

  • secrets/get: Allows reading of secrets for encryption.
  • configmaps/get: Grants access to configuration maps, this is used to read encryption configuration.
  • serviceaccounts/get: Enables retrieval of service accounts for authentication to KMS and encryption secret retrieval from there.
  • serviceaccounts/token/create: Allows creation of service account tokens, required for authenticating request to KMS when retrieving encryption secrets.

The commit also updated the csi documentation to include cephfs in the encryption section, with examples updated accordingly.

This changes fixes: #14035

@NymanRobin NymanRobin force-pushed the update-cephfs-clusterroles branch 2 times, most recently from df2dbed to fdd5df5 Compare May 14, 2024 11:31
Copy link
Member

@parth-gr parth-gr left a comment

Choose a reason for hiding this comment

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

Should we also document how to use CephFS + fscrypt?

@NymanRobin
Copy link
Contributor Author

Yes, ideally, @parth-gr, but currently, I don't have an official ISO image that supports kernel 6.6 and works with Minikube for development purposes. However, if it's acceptable to leave the kernel-related details to the reader, I can proceed with documenting the process and create some deployment examples.

@parth-gr
Copy link
Member

Yes, ideally, @parth-gr, but currently, I don't have an official ISO image that supports kernel 6.6 and works with Minikube for development purposes. However, if it's acceptable to leave the kernel-related details to the reader, I can proceed with documenting the process and create some deployment examples.

lets wait for there ideas @travisn @iamniting

Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

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

While talking to @Rakshith-R I got to know we also need the below permissions in both the clusterroles

  - apiGroups: [""]
    resources: ["serviceaccounts"]
    verbs: ["get"]
  - apiGroups: [""]
    resources: ["serviceaccounts/token"]
    verbs: ["create"]

@NymanRobin
Copy link
Contributor Author

While talking to @Rakshith-R I got to know we also need the below permissions in both the clusterroles

  - apiGroups: [""]
    resources: ["serviceaccounts"]
    verbs: ["get"]
  - apiGroups: [""]
    resources: ["serviceaccounts/token"]
    verbs: ["create"]

Thanks for pointing it out, I had only used it with a secret directly and thus I missed this detail!
The PR is updated now! What does everyone think should this PR include some documentation for the cephfs + fscrypt feature or should we target that for another PR?

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

This has expanded to more than helm. How about the commit prefix manifest:, and also more description in the commit message for the privileges being added?

Copy link
Member

@iamniting iamniting left a comment

Choose a reason for hiding this comment

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

The changes looks good to me, I leave the other decisions to Travis.

@NymanRobin NymanRobin closed this May 15, 2024
@NymanRobin NymanRobin reopened this May 15, 2024
@NymanRobin NymanRobin changed the title helm: sync the ceph-csi-cephfs clusterroles with ceph-csi manifest: grant privileges to ceph-csi-cephfs clusterroles May 15, 2024
@NymanRobin
Copy link
Contributor Author

Thanks @travisn for pointing out that commit message was not descriptive, sorry for that!

Also managed to close the PR once while updating it 😅
Now the commit message should be more descriptive of what privileges are added and what is the reason behind them

@parth-gr
Copy link
Member

Should we also document how to use CephFS + fscrypt?

we should have the feature documented IMO, with this PR or a follow up never mind

@travisn
Copy link
Member

travisn commented May 15, 2024

LGTM, @Rakshith-R please also review

Copy link
Member

@Rakshith-R Rakshith-R left a comment

Choose a reason for hiding this comment

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

Thanks,
It looks good to me.

Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

If the target is only to update RBAC the changes looks good but for complete implementation, As per we are enabling a Key CSI_ENABLE_ENCRYPTION in the configmap https://rook.io/docs/rook/latest-release/Storage-Configuration/Ceph-CSI/ceph-csi-drivers/?h=encryption#enable-rbd-encryption-support, when this key is set we need to mount configmap to the cephfs as well like we do for rbd which is missing in this PR.

@NymanRobin
Copy link
Contributor Author

I see your point @Madhu-1, I did not initially considered it in this PR as it is outside of the scope of the issue I submitted.

However, I am not opposed to extending the scope of this PR as it probably would make most sense to finish it all at once, depending on what others who have partaken in the conversation think? I should be able to extend this PR in reasonable time
I also see from the link that the rbd case just have a note for the kernel version so I think we can simply do the same for cephfs case

I found the rbd PR #9940 for reference

@Madhu-1
Copy link
Member

Madhu-1 commented May 16, 2024

I see your point @Madhu-1, I did not initially considered it in this PR as it is outside of the scope of the issue I submitted.

However, I am not opposed to extending the scope of this PR as it probably would make most sense to finish it all at once, depending on what others who have partaken in the conversation think? I should be able to extend this PR in reasonable time I also see from the link that the rbd case just have a note for the kernel version so I think we can simply do the same for cephfs case

I found the rbd PR #9940 for reference

@NymanRobin yes you are correct, Thanks for looking for the RBD implementation as well. For me the change is not that big we can accommodate in this PR and get it done instead of sending one more PR for implementation and document.

Ceph-CSI support for fscrypt encryption of cephfs.
To achieve this commit add capability of mounting the
required `rook-ceph-csi-kms-config` configmap into
csi-cephfsplugin-provisioner and nodeplugin pods.

Further it modifies the ClusterRoles `cephfs-csi-nodeplugin` and
`cephfs-external-provisioner-runner` to grant privileges
required for reading encryption configuration and fetching
encryption secrets from either kubernetes secrets or
from a Key Management System (KMS).

These privileges are essential for the proper functioning of
ceph-csi-cephfs with fscrypt encryption.

The following privileges have been added:
- `secrets/get`: Allows reading of secrets for encryption.
- `configmaps/get`: Grants access to configuration maps,
    this is used to read encryption configuration.
- `serviceaccounts/get`: Enables retrieval of service accounts for
    authentication to KMS and for retrieving encryption secrets
    stored there.
- `serviceaccounts/token/create`: Allows creation of service account tokens,
    which are required for authenticating requests to KMS
    when retrieving encryption secrets.

The commit also updated the csi documentation to include cephfs
in the encryption section, with examples updated accordingly.

Signed-off-by: NymanRobin <[email protected]>
@NymanRobin
Copy link
Contributor Author

NymanRobin commented May 20, 2024

I have updated the pull request to include the mounting of the ConfigMap and the relevant documentation.
I extended the existing documentation since this feature utilizes the same parameter and ConfigMap. @Madhu-1, please review these changes and let me know if you have any concerns or suggestions. It was not 100% clear to me if these both types of encryption should be behind the same configuration

@NymanRobin NymanRobin changed the title manifest: grant privileges to ceph-csi-cephfs clusterroles csi: add cephfs encryption support May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for cephfs + fscrypt in dev-env
6 participants