always return KdfMemory and KdfParallelism

[stefan0xC]

As discussed in #3390 we could simplify the login logic a bit because the client will ignore the value of theses fields in case of PBKDF2 (whether they are unset or set to any value from trying out Argon2id as KDF)

With Argon2id those fields should never be null but always in a valid state. Be aware that if they are null the client will assume the bitwarden presets (i.e. m=64 and p=4) and if the fields are set to something else (not sure how this would happen but it's technically possible with a sqlite database) the login would most likely fail.

So there should not be a reason to panic.

Disclaimer: I've only tested this change with the web-vault. Since this seems to be also the behavior of vault.bitwarden.com I'm assuming that the other clients will not behave differently in this regard.

[jjlin]

With Argon2id those fields should never be null but always in a valid state. Be aware that if they are null the client will assume the bitwarden presets (i.e. m=64 and p=4) and if the fields are set to something else (not sure how this would happen but it's technically possible with a sqlite database) the login would most likely fail.

Can you elaborate on why you're calling out SQLite specifically?

Overall, this should be fine, but then I think the validation of the KDF data should be more strict. In particular, in accounts.rs:

• post_kdf() should clear the KdfMemory and KdfParallelism fields in the PBKDF2 case so these fields are properly null if someone goes from Argon2 to PBKDF2.
• _register() should have some KDF-specific processing that ignores KdfMemory and KdfParallelism fields in the PBKDF2 case. Conversely, it should error out if KdfMemory and KdfParallelism aren't valid in the Argon2 case, similar to what's done in post_kdf(). The validation logic should probably just be extracted into a separate function.

[stefan0xC]

Can you elaborate on why you're calling out SQLite specifically?

I was thinking about setting it to a non-number (because of the Value::Number check) and how SQLite allows setting it to strings. Not sure what happens in other databases (I could be wrong but I think that both PostgreSQL and MySQL/MariaDB will not allow setting non-numbers in an integer field). But yeah, as far as I've tested it with SQLite strings will apparently returned as 0 here, so it might not have been necessary to specifically mention it.

[stefan0xC]

@jjlin thanks for the suggestion. regarding the missing validation in _register I'm not sure if this is really necessary right now (registering will use the default PBKDF2 and not even send those values) also there was a small bug, so this presumably has not been used yet.

it might make sense to validate the KDF (and possible other things) on user.save() like we already do with the email.

vaultwarden/src/db/models/user.rs

Lines 260 to 263 in 525e6bb

	pub async fn save(&mut self, conn: &mut DbConn) -> EmptyResult {
	if self.email.trim().is_empty() {
	err!("User email can't be empty")
	}

[BlackDex]

I was thinking about setting it to a non-number (because of the Value::Number check) and how SQLite allows setting it to strings.

That isn't a check really. Its to make sure that it is processed as a json integer and not going to be quoted as if it were a string.

it might make sense to validate the KDF (and possible other things) on user.save() like we already do with the email.

vaultwarden/src/db/models/user.rs

Lines 260 to 263 in 525e6bb

	pub async fn save(&mut self, conn: &mut DbConn) -> EmptyResult {
	if self.email.trim().is_empty() {
	err!("User email can't be empty")
	}

Thats maybe not a bad idea. Check which hashing type is used and set those other values to None

[jjlin]

It sounds okay to clear the KdfMemory and KdfParallelism fields for PBKDF2 when saving the user, but IMO the detailed validation logic for KDF parameters doesn't belong there.

[BlackDex]

It sounds okay to clear the KdfMemory and KdfParallelism fields for PBKDF2 when saving the user, but IMO the detailed validation logic for KDF parameters doesn't belong there.

Validation indeed should be done elsewhere. Preferably in a separate function.

[BlackDex]

I quickly checked upstream. And they always return those values, and they set the KdfMemory and KdfParallelism back to null (None) when they KdfType is 0.

So, the validation should be done there where the request is handled. But i think it is fine to set those values to None if the KdfType is set to 0.