API Compatibility

[Zoruk]

Hi All,

I have noticed that some endpoint like identity/accounts/prelogin doesn't follow the exact same API as the official one in terms of case the project uses PascalCase and the official API uses camelCase. Is this voluntary ?

And otherwise since we can generate some code from the OpenAPI definitions is there any reason why the project doesn't use this ?

Have a nice day all

[BlackDex]

Vaultwarden doesn't support the public API of Bitwarden, only a small subset of it to support the Directory Connector. So we are unable to generate any code from that. Also, there is no code generation for the Rocket Framework as far as i know, so that won't help either.

Regarding the PascalCase or camelCase, it actually doesn't matter what you send to Vaultwarden, we convert it internally to PascalCase. The reason for this is that in the past, the Bitwarden clients were sending a mixture of them. They should have converted all to camelCase as far as i know on what they send to the server. They do still allow receiving PascalCase for most API endpoints, and that is why we have not converted anything.

If any, we are not that strict in receiving either case formating, so that should be fine i think.

If you are only revering too what we output, then it's just that it is a big task to convert all to camelCase, but we probably need to at sometime in the future I think.

[Zoruk]

Also the sdk published on the bitwarden GitHub organization https://github.com/bitwarden/sdk/tree/main/crates/bitwarden-api-identity

What I mean by this thread it that it would be nice to follow the official Open API extracted from the official server source. We can extract the OpenAPI definition using the method explained in the SDK Repository: https://github.com/bitwarden/sdk/tree/main#api-bindings

[BlackDex]

That is used for the Secrets Manager. Not the communication between clients and server. While it does have some endpoints in common like to login etc... It isn't specifically for the client-api

[BlackDex]

Though, there is a list of course of the other api's, I'm not sure what you want to do with that for Vaultwarden?

[Zoruk]

I wanted to play around with the API and maybe do a linux daemon which provide a bitwarden backend for libsecret. And trying to be compatible just for the authentication with both the official server and the vaultwarden implementation started to seams a bit too complicated to have two implementation supposed to follow the same open api reference.

[BlackDex]

It is very easy to convert it to camelCase, as @dani-garcia also mentioned, if you really want to be compatible with the clients, you need to do this anyway. The conversion isn't that hard as i showed in the link.
I also created a lower-case-first version btw, maybe that helps,

vaultwarden/src/util.rs

Lines 708 to 749 in cbdcf8e

	pub fn convert_json_key_lcase_first(src_json: Value) -> Value {
	match src_json {
	Value::Array(elm) => {
	let mut new_array: Vec<Value> = Vec::with_capacity(elm.len());
	for obj in elm {
	new_array.push(convert_json_key_lcase_first(obj));
	}
	Value::Array(new_array)
	}
	Value::Object(obj) => {
	let mut json_map = JsonMap::new();
	for (key, value) in obj.iter() {
	match (key, value) {
	(key, Value::Object(elm)) => {
	let inner_value = convert_json_key_lcase_first(Value::Object(elm.clone()));
	json_map.insert(lcase_first(key), inner_value);
	}
	(key, Value::Array(elm)) => {
	let mut inner_array: Vec<Value> = Vec::with_capacity(elm.len());
	for inner_obj in elm {
	inner_array.push(convert_json_key_lcase_first(inner_obj.clone()));
	}
	json_map.insert(lcase_first(key), Value::Array(inner_array));
	}
	(key, value) => {
	json_map.insert(lcase_first(key), value.clone());
	}
	}
	}
	Value::Object(json_map)
	}
	value => value,
	}
	}

Else, see https://docs.rs/serde-aux/latest/serde_aux/container_attributes/fn.deserialize_struct_case_insensitive.html or https://serde.rs/attr-rename.html

[zacknewman]

Isn't #4196 related to this discussion?

That's worse since that is a WebAuthn violation. Bitwarden controls every part of their API, but they don't control WebAuthn. Violating WebAuthn requires special care and should be avoided if possible. How many Vaultwarden devs have actually read the necessary standards (e.g., WebAuthn Level 2 API) and RFCs (e.g., RFC 9053)? Likely none or very few. The crate that is used even makes several warnings of digging into the internals.

As the referenced issue shows, there is exactly one issue to be aware of by adhering to the spec and that is the web-vault violating WebAuthn by using AttestationObject and clientDataJson instead of attestationObject and clientDataJSON respectively as the spec requires, but that doesn't excuse Vaultwarden from also violating the spec.

The way that I recommend solving that problem is by patching the web-vault even though I am aware that Vaultwarden devs try to only patch the web-vault for things like CSS (as they should); however there have been patches unrelated to just CSS. The reason to simply patch the web-vault such that those JSON keys are correctly formatted is the following:

• Only the web-vault can be used to register keys, and those JSON keys are only used during registration. This means patching this for the web-vault is the same thing as patching for all clients, so there is no worry that the patch only fixes the issue for one of the clients.
• The patch to the web-vault is stupid simple. Literally sed -i 's/AttestationObject/attestationObject/g' for the two files that matter and sed -i 's/clientDataJson/clientDataJSON/g' for the handful of files that need it.
• The amount of code currently used to deserialize EnableWebauthnData (including indirectly (e.g., the Visitor util::UpCaseVisitor)) is substantially more than simply deriveing Deserialize.
  • This improves code audits which Vaultwarden is at a marked disadvantage compared to upstream.
  • webauthn-rs is the only dependency that is not updated frequently, and presumably one of the factors for this is the amount of code that has to be changed. By not relying on the internals as recommended by the project itself, updating the crate will be a lot easier even though re-serialization of the data on the database is still a problem.
    • Once passkey in lieu of a master password is added to Bitwarden, webauthn-rs will have to be updated anyway so updating the crate is inevitable.

As the above points explain, that is why I think patching the web-vault makes a lot more sense especially since there is precedent (no matter how rare) to patching the web-vault for non-CSS so long as it makes sense; and simpler, more maintainable, spec-adhering, and easier-to-audit code in conjunction with a web-vault-only problem fits the bill for an exception to be made.

If patching the web-vault is still not done, then that doesn't excuse altering the entire payload but instead only the two problematic keys should be altered. This can still be achieved by not getting into the internals of webauthn-rs but does require more complex code. Specifically, a Visitor that reads the JSON payload and only alters AttestationObject and clientDataJson similar to what UpCaseVisitor is doing now.