-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add ZFS_PROP_PASSPHRASE_KDF (passphrasekdf) property and argon2id13 #15682
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
c1f56ac
to
8cdd4e7
Compare
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
3e9883f
to
d320667
Compare
checkout #14836 as well |
I read through your PR. I think more parameters for argon2 need to configurable. Argon2 provides the options for this. I think 18MB with one CPU is not enough compute resource in my opinion. But I guess for other people it's enough or even to much, for that reason it should be configurable in my opinion. But all in all it does not look that bad. You had the same Ideas like me. And I have the same points I don't like on my code and Ideas. Like what about adding a third KDF and more. BTW: I'm working on a multi key slot variant for ZFS and wanna stop the PR #14836 if I finish it. But don't know when I can finish this work. I have the multislot already finished, but the user space tools don't handle it at the current point of time. Maybe wanna contact @tcaputi for some more information about the crypto key handling. I understand the most things, but not shure about everything. |
a55cc70
to
4baa13c
Compare
This could be done quite easily by packing more parameters into pbkdf2iters. |
include/sys/fs/zfs.h
Outdated
static const uint32_t zfs_passphrase_kdf_default_iterations[ | ||
ZFS_PASSPHRASE_KDF_KDFS] = {350000, 40000}; | ||
static const uint32_t zfs_passphrase_kdf_min_iterations[ | ||
ZFS_PASSPHRASE_KDF_KDFS] = {100000, 10000}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this, @nabijaczleweli. I agree with @oromenahar that making the memory and parallelism parameters configurable is really important. 18 KiB seems like way too little memory. This minimum number of iterations seems way too high for Argon2; I think iteration counts as low as 1 are perfectly reasonable.
Here's are some examples from my laptop (from 2018, using https://packages.debian.org/bookworm/argon2).
t=40000 m=16KiB takes 0.3s
$ echo pw | time argon2 saltsalt -id -t 40000 -m 4 -p 1 -v 13
Type: Argon2id
Iterations: 40000
Memory: 16 KiB
Parallelism: 1
Hash: 908df7fe7b24164c854546dadce8a034fd0aa9ad04e6fed6bfb27c074757ca84
Encoded: $argon2id$v=19$m=16,t=40000,p=1$c2FsdHNhbHQ$kI33/nskFkyFRUba3OigNP0Kqa0E5v7Wv7J8B0dXyoQ
0.349 seconds
Verification ok
argon2 saltsalt -id -t 40000 -m 4 -p 1 -v 13 0.69s user 0.00s system 99% cpu 0.691 total
t=100 m=16MiB takes 1.2s
$ echo pw | time argon2 saltsalt -id -t 100 -m 14 -p 1 -v 13
Type: Argon2id
Iterations: 100
Memory: 16384 KiB
Parallelism: 1
Hash: 7e4127adbbe1c9ac4ec0a48f387c7c3755639d18c4f01da8b8f8c15baeee4dbc
Encoded: $argon2id$v=19$m=16384,t=100,p=1$c2FsdHNhbHQ$fkEnrbvhyaxOwKSPOHx8N1VjnRjE8B2ouPjBW67uTbw
1.185 seconds
Verification ok
argon2 saltsalt -id -t 100 -m 14 -p 1 -v 13 2.32s user 0.02s system 99% cpu 2.349 total
t=1 m=2GiB takes 2.5s
$ echo pw | time argon2 saltsalt -id -t 1 -m 21 -p 1 -v 13
Type: Argon2id
Iterations: 1
Memory: 2097152 KiB
Parallelism: 1
Hash: 015f25635ba58191504e00830b98d57917f67e5d15a35842f108ffa1397b2b64
Encoded: $argon2id$v=19$m=2097152,t=1,p=1$c2FsdHNhbHQ$AV8lY1ulgZFQTgCDC5jVeRf2fl0Vo1hC8Qj/oTl7K2Q
2.548 seconds
Verification ok
argon2 saltsalt -id -t 1 -m 21 -p 1 -v 13 3.76s user 1.29s system 99% cpu 5.047 total
This last configuration with t=1 may be best (and I should crank up parallelism if I can).
The Argon2 paper says:
Number T of passes over the memory. The running time depends linearly on this parameter. We expect
that the user chooses this number according to the time constraints on the application. Again, there is
no ”insecure value” for T .
The recommendations here currently suggest single-digit numbers of iterations: https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#argon2id
m=47104 (46 MiB), t=1, p=1
...
m=7168 (7 MiB), t=5, p=1
Cryptsetup appears to use a minimum iteration count of 4: https://gitlab.com/cryptsetup/cryptsetup/-/blob/main/lib/crypto_backend/pbkdf_check.c#L56
It's probably also a good idea to figure out the maximum usable values and make sure that parameters coming into ZFS aren't silently truncated/wrapped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me these take 0.894/2.499/4.630 (bookworm on E5645s). Updated to default to 0x000100110001 (t=1, m=17, p=1) with minimum 0x0001000C0001 (t=1, m=12, p=1).
4baa13c
to
cc820a7
Compare
Updated, now the following is possible:
|
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
Signed-off-by: Ahelenia Ziemiańska <[email protected]>
cc820a7
to
7c74565
Compare
Motivation and Context
Rowe told me to update and post the diff from #14762.
Description
id
ZFS_PROP_PASSPHRASE_KDF
name "passphrasekdf" enum of eitherZFS_PASSPHRASE_KDF_PBKDF2
("pbkdf2", PBKDF2), which is the default (and no changes there), orZFS_PASSPHRASE_KDF_ARGON2ID13
("argon2id13", Argon2id version 13, 18MB, single lane) in which case derive the key with Argon2id instead(but divide, the default is 40000 rounds and the min is 10000 rounds.pbkdf2iters
by 10 (maybe this factor can be reduced so it takes a similar amount of time, but no factor + the default 350000 took multiple seconds which is unacceptable; this does make the default be 35000 Argon2id rounds, so an Argon2 enjoyer should opine if this is "secure", but it takes >500ms in my test VM, so))Do we want to add a "kdfiters" alias for pbkdf2iters?
The initial methodology was "find everywhere where
ZFS_PROP_KEYFORMAT
is mentioned and add it there as well". Clearly a good approach.How Has This Been Tested?
(pool/datasets created on unpatched master):
What doesn't work rn:zfs change-key
will revert to pbkdf2. idk why. probably easyWhat I don't know if works (but should):
receiving encrypted datasets (every codepath in dsl_crypt that isn't trodden by change-key/load-key, really),zdb, pam_zfs_keyTypes of changes
Checklist:
Signed-off-by
.