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

util: Limit cryptsetup PBKDF memory usage #3781

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

BenoitKnecht
Copy link
Contributor

Describe what this PR does

By default, cryptsetup luksFormat uses Argon2i as Password-Based Key Derivation Function (PBKDF), which not only has a CPU cost, but also a memory cost (to make brute-force attacks harder).

The memory cost is based on the available system memory by default, which in the context of Ceph CSI can be a problem for two reasons:

  1. Pods can have a memory limit (much lower that the memory available on the node, usually) which isn't taken into account by cryptsetup, so it can get OOM-killed when formating a new volume;
  2. The amount of memory that was used during cryptsetup luksFormat will then be needed for cryptsetup luksOpen, so if the volume was formated on a node with a lot of memory, but then needs to be opened on a different node with less memory, cryptsetup will get OOM-killed.

This commit sets the PBKDF memory limit to a fixed value to ensure consistent memory usage regardless of the specifications of the nodes where the volume happens to be formatted in the first place.

The limit is set to a relatively low value (32 MiB) so that the csi-rbdplugin container in the nodeplugin pod doesn't require an extravagantly high memory limit in order to format/open volumes (particularly with operations happening in parallel), while at the same time not being so low as to render it completely pointless.

Context

Prior to this change, cryptsetup would get OOM-killed unless we set a memory limit larger than 1GiB, which was way too much to dedicate to a container that otherwise barely needs any.


Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)
  • /retest all: run this in case the CentOS CI failed to start/report any test
    progress or results

@nixpanic nixpanic added component/rbd Issues related to RBD component/util Utility functions shared between CephFS and RBD labels Apr 26, 2023
@nixpanic
Copy link
Member

Just wondering what the downside of this change is. Will the generated key be less secure, or does it take more time to generate it?

Please update the subject of the commit message to have the prefix util: instead of the filename, that should make commitlint happy.

@BenoitKnecht BenoitKnecht changed the title internal/util/cryptsetup.go: Limit PBKDF memory usage util: Limit cryptsetup PBKDF memory usage Apr 26, 2023
@BenoitKnecht
Copy link
Contributor Author

Just wondering what the downside of this change is. Will the generated key be less secure, or does it take more time to generate it?

As far as I know, the main reason for making the PBKDF algorithm memory-intensive too is to make GPU-base brute-force attacks harder (or more expensive). GPUs can run hundreds of threads in parallel, but if each of them need a substantial amount of memory, that becomes your limiting factor.

So the generated key itself is not less secure if you reduce the memory limit, it's just "easier" to compute the derived key based on the passphrase. But since in Ceph CSI the passphrase itself already has 160 bits of entropy, a brute-force attack is already quite difficult to pull off, so in practice I don't think it reduces the level of security in a meaningful way.

@nixpanic nixpanic requested a review from a team April 26, 2023 15:46
@nixpanic nixpanic added the backport-to-release-v3.8 backport to release 3.8 branch label Apr 26, 2023
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Apr 27, 2023

@Mergifyio rebase

By default, `cryptsetup luksFormat` uses Argon2i as Password-Based Key
Derivation Function (PBKDF), which not only has a CPU cost, but also a memory
cost (to make brute-force attacks harder).

The memory cost is based on the available system memory by default, which in
the context of Ceph CSI can be a problem for two reasons:

1. Pods can have a memory limit (much lower that the memory available on the
   node, usually) which isn't taken into account by `cryptsetup`, so it can get
   OOM-killed when formating a new volume;
2. The amount of memory that was used during `cryptsetup luksFormat` will then
   be needed for `cryptsetup luksOpen`, so if the volume was formated on a node
   with a lot of memory, but then needs to be opened on a different node with
   less memory, `cryptsetup` will get OOM-killed.

This commit sets the PBKDF memory limit to a fixed value to ensure consistent
memory usage regardless of the specifications of the nodes where the volume
happens to be formatted in the first place.

The limit is set to a relatively low value (32 MiB) so that the `csi-rbdplugin`
container in the `nodeplugin` pod doesn't require an extravagantly high memory
limit in order to format/open volumes (particularly with operations happening
in parallel), while at the same time not being so low as to render it
completely pointless.

Signed-off-by: Benoît Knecht <[email protected]>
@mergify
Copy link
Contributor

mergify bot commented Apr 27, 2023

rebase

✅ Branch has been successfully rebased

@Madhu-1 Madhu-1 added the ok-to-test Label to trigger E2E tests label Apr 27, 2023
@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.24

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.25

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.26

@github-actions
Copy link

/test ci/centos/k8s-e2e-external-storage/1.27

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e-helm/k8s-1.27

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.24

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.25

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.26

@github-actions
Copy link

/test ci/centos/mini-e2e/k8s-1.27

@github-actions
Copy link

/test ci/centos/upgrade-tests-cephfs

@github-actions
Copy link

/test ci/centos/upgrade-tests-rbd

@github-actions github-actions bot removed the ok-to-test Label to trigger E2E tests label Apr 27, 2023
@mergify mergify bot merged commit 1852e97 into ceph:devel Apr 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-to-release-v3.8 backport to release 3.8 branch component/rbd Issues related to RBD component/util Utility functions shared between CephFS and RBD
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants