add argon2 kdf fields

[tessus]

Changes:

• added columns to db schema with DEFAULT NULL
• added a pub enum UserKdfType
• if UserKdfType is Argon2id, server responses will include 2 more KVPs for memory and parallelism

Test plan:

• used quexten's webvault (which includes the UI changes for Argon2) to switch KDF to argon2 and back to pbkdf2

Previous text (for reference)

This is still a work in progess. The following items have to be clarified first:

• default db values or NULL?
• default iterations might have to be changed depending on the type (PBKDF2, ARGON2)
• enum for kdf type?

During compilation I get an error:

error: recursion limit reached while expanding `__static_cond!`

consider increasing the recursion limit by adding a `#![recursion_limit = "194"]` attribute to your crate (`vaultwarden`)

However in the code it says:

// The recursion_limit is mainly triggered by the json!() macro.
// The more key/value pairs there are the more recursion occurs.
// We want to keep this as low as possible, but not higher then 128.
// If you go above 128 it will cause rust-analyzer to fail,

So, what now?

[quexten]

default db values or NULL?

In the official server, kdfmemory and kdfparallelism are null if pbkdf2 is selected, and not null when argon2 is selected. If they are null, the fields can (should) be omitted from the API responses.

default iterations might have to be changed depending on the type (PBKDF2, ARGON2)
Currently, PBKDF2 is default anyways for the clients. The only way they can get argon2 is to create an account, log in, and then change to argon2.

[tessus]

In the official server, kdfmemory and kdfparallelism are null if pbkdf2 is selected, and not null when argon2 is selected.

Yep, I saw the DEFAULT NULL in the database DDL. That doesn't mean we have to do the same in our database schema. Since I am new to rust and diesel, I have no idea how NULL values are handled. But it is certainly easy to change the DDL to use DEFAULT NULL.

If they are null, the fields can (should) be omitted from the API responses.

They will only be NULL in the db, if we use DEFAULT NULL and argon2 has never been set/used. Or do you just mean the variables themselves should be set to NULL and omitted from the API response.

Currently, PBKDF2 is default anyways for the clients. The only way they can get argon2 is to create an account, log in, and then change to argon2.

Right, but when someone switches to argon2, I rather not have 600,000 iterations... I haven't seen the UI yet, thus I don't know what the UI will do. e.g. will it automatically change the iterations to another value or will the user have to set the iterations manually?

Btw, I am heading to bed, so I won't respond in the next few hours.

[BlackDex]

I'd rather not have any defaults set into the database. Also, as @quexten mentioned the NULL is probably better.
Regarding the recursions, just up it +1 at a time until that message disappears, Rust just mentions a number mostly twice as high.

[quexten]

Some checks such as :

vaultwarden/src/config.rs

Lines 676 to 679 in 9366e31

	if cfg.password_iterations < 100_000 {
	err!("PASSWORD_ITERATIONS should be at least 100000 or higher. The default is 600000!");
	}

Need adjusting to distinguish between pbkdf2 and argon2.

Right, but when someone switches to argon2, I rather not have 600,000 iterations... I haven't seen the UI yet, thus I don't know what the UI will do. e.g. will it automatically change the iterations to another value or will the user have to set the iterations manually?

The iterations when switching to argon2 are set in the client, not on the server. I think they should only matter on the server in case the client omits the values:

vaultwarden/src/api/core/accounts.rs

Lines 773 to 778 in 9366e31

	let (kdf_type, kdf_iter) = match User::find_by_mail(&data.Email, &mut conn).await {
	Some(user) => (user.client_kdf_type, user.client_kdf_iter),
	None => (User::CLIENT_KDF_TYPE_DEFAULT, User::CLIENT_KDF_ITER_DEFAULT),
	};
	Json(json!({

[quexten]

By the way, the limits should be:
0 < iterations
14 < memory < 1025
0 < parallelism < 17

The defaults on the clients are:
iterations: 3
memory: 64 (mb)
parallelism: 4
(The server has other, lower defaults)

https://github.com/bitwarden/server/blob/cb1ba50ce26ce33b2a5acf30536a2075e4fadebd/src/Core/Utilities/KdfSettingsValidator.cs#L10-L36

Edit:
Ah I see you already have some of these implemented in the PR, very nice!

vaultwarden/src/api/core/accounts.rs

Lines 367 to 378 in 3896a82

	if data.KdfIterations < 100_000 {
	err!("KDF iterations lower then 100000 are not allowed.")
	}
	if data.Kdf == 1 {
	if data.KdfMemory < 15 || data.KdfMemory > 1024 {
	err!("Argon2 memory must be between 15mb and 1024mb.")
	}
	if data.KdfParallelism < 1 || data.KdfParallelism > 16 {
	err!("Argon2 parallelism must be between 1 and 16.")
	}
	}

Probably need to change the kdfIterations check though. So something like:

   if data.Kdf == 0 {
      if data.KdfIterations < 100_000 {
          err!("PBKDF2 iterations lower than 100000 are not allowed.")
      }
   } else if data.Kdf == 1 {
        if data.KdfIterations < 1 {
          err!("Argon2 iterations lower than 1 are not allowed.")
        }
        if data.KdfMemory < 15 || data.KdfMemory > 1024 {
            err!("Argon2 memory must be between 15mb and 1024mb.")
        }
        if data.KdfParallelism < 1 || data.KdfParallelism > 16 {
            err!("Argon2 parallelism must be between 1 and 16.")
        }
    }

[quexten]

Oh, one more note: The CSP needs to be changed for the webvault. Due to webassembly not supporting hashes for the CSP yet, it requires the "wasm-unsafe-eval" policy in the CSP.

bitwarden/server@522df6e

Since the current CSP does not seem to allow it:

vaultwarden/src/util.rs

Lines 56 to 82 in 9b7e86e

	let csp = format!(
	"default-src 'self'; \
	base-uri 'self'; \
	form-action 'self'; \
	object-src 'self' blob:; \
	script-src 'self'; \
	style-src 'self' 'unsafe-inline'; \
	child-src 'self' https://*.duosecurity.com https://*.duofederal.com; \
	frame-src 'self' https://*.duosecurity.com https://*.duofederal.com; \
	frame-ancestors 'self' \
	chrome-extension://nngceckbapebfimnlniiiahkandclblb \
	chrome-extension://jbkfoedolllekgbhcbcoahefnbanhhlh \
	moz-extension://* \
	{allowed_iframe_ancestors}; \
	img-src 'self' data: \
	https://haveibeenpwned.com \
	https://www.gravatar.com \
	{icon_service_csp}; \
	connect-src 'self' \
	https://api.pwnedpasswords.com \
	https://api.2fa.directory \
	https://app.simplelogin.io/api/ \
	https://app.anonaddy.com/api/ \
	https://api.fastmail.com/ \
	;\
	",
	icon_service_csp = CONFIG._icon_service_csp(),

Hope my notes help!

[tessus]

@quexten sorry, I read your edits too late, since I read the email notifications instead.
I can change the err messages later.

I only add memory/parallelism when argon2 is used to the JSON in _password_login.

Let me know, if I should do the same for:

_refresh_login
_api_key_login
_prelogin
takeover_emergency_access

I can do the rest this evening.

[quexten]

@quexten sorry, I read your edits too late, since I read the email notifications instead. I can change the err messages later.

I only add memory/parallelism when argon2 is used to the JSON in _password_login.

Let me know, if I should do the same for:

_refresh_login
_api_key_login
_prelogin
takeover_emergency_access

I can do the rest this evening.

_refresh_login

Yep, everwhere where KdfType, KdfIterations are sent, KdfMemory and KdfParallelism should now be sent too.

[quexten]

By the way, feel free to ping me once you are done with the PR, I have the argon2 builds of the web and mobile clients anyways, so I'm happy to test.

[tessus]

@quexten

Yep, everwhere where KdfType, KdfIterations are sent, KdfMemory and KdfParallelism should now be sent too.

This is already happening. What I was asking was whether I should also exclude the argon2 values from the JSON response as you have stated here #3210 (comment) for the 4 functions I have listed.

I have already done this for one function: https://github.com/dani-garcia/vaultwarden/pull/3210/files#diff-1c5b0c87dec2154a167b89110c637a7a4bc04f59af0b83e8ddba39eb2134518cR254

I can look into this again in 5-6 hours.

[quexten]

Oh, sorry misunderstood. I don't think it would cause issues to respond values, since for the pbkdf2 case the wouldn't be used, but since the should be null in that case, it's better to just exclude them from the response (the same way as on the official server). For argon2 of course do send them.

[quexten]

I have already done this for one function: https://github.com/dani-garcia/vaultwarden/pull/3210/files#diff-1c5b0c87dec2154a167b89110c637a7a4bc04f59af0b83e8ddba39eb2134518cR254

I had a look, and this looks correct to me 👍

[BlackDex]

Looks good for your (as far as i know) first shot at Rust :).
Some small items to address.

I haven't validated/run the code it self though.

[tessus]

I am sorry, I had to take care of somehing last evening. I'll try to finish it in the morning.

[tessus]

Unfortunately I have issues with enum. It is a pub, but it's not recognized in the other files, even though it seems to be imported with

use crate::{
    db::{models::*, DbConn},
}

Sorry, I just don't know Rust very well.

But other than that the code should be fine. Can someone please help me with this enum?

[BlackDex]

Unfortunately I have issues with enum. It is a pub, but it's not recognized in the other files, even though it seems to be imported with

use crate::{
    db::{models::*, DbConn},
}

Sorry, I just don't know Rust very well.

But other than that the code should be fine. Can someone please help me with this enum?

I suggest to place that enum in the src/db/models/user.rs either before or after the enum UserStatus
You also need to update the src/db/models/mod.rs and add that enum to the list of the pub use self::user{...} list.

That should make it globally available when either db::{models::*, ..} is used or db::models::{UserKdfType, ...}

[tessus]

I suggest to place that enum in the src/db/models/user.rs either before or after the enum UserStatus

Yes, I have done that already in my last commit.

You also need to update the src/db/models/mod.rs and add that enum to the list of the pub use self::user{...} list.

This was the missing part. Thank you! I'll push another commit in a sec. At the end, after the review and when it is good to go, I'll squash my commits and force-push.

[tessus]

@quexten I think it's ready for a test spin.

[tessus]

Seriously, the pipeline failed because of the list not in alphabetical order?
This is ridiculous.

-pub use self::user::{Invitation, User, UserStampException, KdfType};
+pub use self::user::{Invitation, KdfType, User, UserStampException};

Anyway, it would make sense to have the format check in the beginning and abend BEFORE building the code....

[quexten]

@quexten I think it's ready for a test spin.

Very nice. I'll test it in a few hours.

[quexten]

@quexten I think it's ready for a test spin.

Very nice. I'll test it in a few hours.

@tessus
Initial test works correctly. For pbkdf2 the fields are omitted. I can switch to argon2 and configure different fields, and the customized fields get sent correctly when logging in.

However, switching back to pbkdf2 does not work. I get "422 Unprocessable Entity"

Did not tet database migrations (yet).

[quexten]

@quexten I think it's ready for a test spin.

Very nice. I'll test it in a few hours.

@tessus Initial test works correctly. For pbkdf2 the fields are omitted. I can switch to argon2 and configure different fields, and the customized fields get sent correctly when logging in.

However, switching back to pbkdf2 does not work. I get "422 Unprocessable Entity"

Did not tet database migrations (yet).

From the log:

[2023-02-02 14:34:31.440][_][WARN] Data guard JsonUpcase < ChangeKdfData >failed: Parse("{\"kdf\":0,\"kdfIterations\":600000,\"masterPasswordHash\":\"CrB8NCVWzgH3A9rXcV0Ks4e1g9wuu7n/aLGImbPJOkM=\",\"newMasterPasswordHash\":\"W6YUqnaEd2ns3TR42KPCrKVZ3CIVvevrTBjXUcV+wKI=\",\"key\":\"2.bK/vVVrmGEQ3UCa78xymkg==|5eZFTrmR2J8A+FoRckzr8d8JlMAB0SSve5ENGSbIJdLkAXi6692MC22fobnJmDpkwYpNaG8YgOeRaaXc0ALuRTtJ4rY/CDY2eKzUVVz8Omw=|TpAPAbtZIC078vr6rsengMlenrqiXTcKB/H19q6CRIg=\"}", Error("missing fieldKdfMemory", line: 1, column: 360)).

[tessus]

Hmm, not sure why it needs a KdfMemory field when switching back to pbkdf2. The field is in the database, but it won't be in a response when the type is not argon2.

Any chance you can send me the web-vault? http://g0to.ca/drop should do it.

[quexten]

Hmm, not sure why it needs a KdfMemory field when switching back to pbkdf2. The field is in the database, but it won't be in a response when the type is not argon2.

Any chance you can send me the web-vault? http://g0to.ca/drop should do it.

I think some structure or function in the server code might be expecting the field when parsing the json request. Since it is not sent when using pbkdf2, the server throws an error.
I have uploaded the web vault to the site you linked.

[tessus]

Thanks a bunch for the zip. I will look at this error this evening (hopefully). I think I might know what the problem could be (from thinking about the code I changed - I don't have it in front of me right now). But maybe BlackDex will find the issue during the review, who knows. ;-)

[BlackDex]

Thanks a bunch for the zip. I will look at this error this evening (hopefully). I think I might know what the problem could be (from thinking about the code I changed - I don't have it in front of me right now). But maybe BlackDex will find the issue during the review, who knows. ;-)

Yea, that was very easy :).
https://github.com/dani-garcia/vaultwarden/pull/3210/files#diff-e6ed38cdf0d7a02240d1d2a5d7ad72682905ea9bfb7e4cc2f0edef05f9de2d88R350-R351

Those probably need to be converted to an Option<>.

[tessus]

Nice, I thought along the same lines. I have a few minutes. I'll fix it now.

[quexten]

Nice, I thought along the same lines. I have a few minutes. I'll fix it now.

Switching back to Pbkdf2 works now.

[tessus]

Thanks for the reply.

enums can support more then just a specific type. Setting a type on an enum is possible for exporting as a c-repr for example.

Yep, I know, but in this case the compiler sees that only integers were used for the 2 values in the enum.

The compiler doesn't know what you want to do exactly

I also understand this, but that's why I mentioned that the compiler could use a default, if nothing is explicitly specified. We have to enclose the integer values in Some to do the comparison tests. IMO this should not be necessary.

[tessus]

Back to WIP. No idea how to fix those compile errors yet. Will have to do some research, but any hint might help.

P.S.: Hmm, so I think I have to check whether the value is not None, and if so, unwrap. But I am not sure if this is the Rust way, or if there are other patterns to use.

[tessus]

Sqashed and force-pushed again. I dislike a messy git history, although it probably wouldn't matter, since merge commits are done when PRs are merged to master.

[tessus]

Sorry, it seems that I can't re-request a review from more than one person.

[tessus]

Is there still something missing for this change?

[BlackDex]

Is there still something missing for this change?

In the sense of an approval?
I did not had time yet to check it. Also a web-vault needs to be released with supports for this of course. It could be that Bitwarden will make some changes to this part while not yet released by them of course, wouldn't be the first time.

[tessus]

In the sense of an approval?

No, I know that reviewing a PR can take time. But rather whether I have addressed all the issues that came up in previous conversations. I think I addressed everything, but I could have missed something. But the people who have left comments would most likely know right away and can say "hold on, dude, what about xyz? you missed that one."

Also a web-vault needs to be released with supports for this of course.

Well, this change does not require a new web-vault. It's the other way around. The new clients that have the argon2 code will require this code change.
I am not saying it must be merged now, I am just saying that merging this PR will not mess up the current or older clients.

[dani-garcia]

Tested with the main branch of the web vault, works correctly for me

[pjs128]

In what version should we start seeing Argon2id in the Security>Keys>KDF_algorithm dropdown?

[BlackDex]

Either currently in testing, or wait until next release.