Open
Conversation
Rebases doy#116 onto current main and upgrades the webauthn-rs stack from the PR's 0.5.0-dev git pins to crates.io 0.5.4 stable. Migrates Error::TwoFactorRequired and ConnectErrorRes to a HashMap keyed by TwoFactorProviderType so the per-provider WebAuthn challenge (PublicKeyCredentialRequestOptions) from TwoFactorProviders2 can be threaded through to the agent's 2FA flow. Origin for the assertion comes from Config::ui_url() so it works for default-cloud accounts where base_url is unset. Gated behind the off-by-default `webauthn` cargo feature; non-feature builds use a stub PublicKeyCredentialRequestOptions to keep the type signatures identical.
Bitwarden's API expects camelCase `clientDataJson` (vs. webauthn-rs-proto's W3C-spec `clientDataJSON`) and a non-nullable `appid: bool` in the extensions object. The previous approach serialized the upstream type and patched the string; this only worked when both extension fields happened to be `None` in a specific order, and would silently break on any upstream change. Introduce a dedicated `BitwardenAssertion` wire type with the exact shape the server wants and convert `PublicKeyCredential` into it before serializing. Also clean up the WebAuthn pinentry prompt strings (dedicated header / description with a touch reminder), drop a stray `eprintln!` from `request_touch` in favor of `log::debug!`, and pull in `base64urlsafedata` behind the `webauthn` feature for the new wire type's byte fields.
Three fixes on top of the initial WebAuthn support: * Restore the legacy `TwoFactorProviders` Vec deserialization and read the per-provider challenge data from a separate `TwoFactorProviders2` field (now `providers_data` on `Error::TwoFactorRequired`). The previous combined HashMap shape regressed login for any server response that only emitted the legacy array (older Vaultwarden, malformed challenges), and affected non-WebAuthn builds too. Provider dispatch is once again driven by the Vec; the HashMap is consulted only to pull the WebAuthn challenge for the WebAuthn arm. * Derive the assertion origin from the challenge's `rp_id` rather than `Config::ui_url()`. The authenticator only requires the origin host to match (or be a subdomain of) `rp_id`, and Bitwarden registers credentials against the vault host it serves; using `rp_id` directly avoids silent signature rejection on self-hosted setups where the user's configured `ui_url` doesn't line up with the registered host. * Wrap the synchronous `perform_auth` USB HID call in `tokio::task::block_in_place` so a 60s wait on the security key doesn't starve other tasks on the agent's tokio worker. `spawn_blocking` is not usable here because `CtapAuthenticator` borrows from the local `UiCallback` and is not `'static`.
The 60s perform_auth timeout was stricter than peer 2FA paths, which block on pinentry indefinitely; replace it with u32::MAX so Ctrl+C is the only escape, matching the rest. Drop the unconditional eager PIN prompt: bridge UiCallback::request_pin into async pinentry on demand via Handle::current().spawn + a sync channel under block_in_place, so users whose keys have no client_pin skip the prompt entirely. Treat webauthn() failures as fatal (propagate via ?) instead of looping into unreachable!().
Restore the WebAuthn arms on header()/message() and look them up via TwoFactorProviderType::WebAuthn so the user-facing strings live in one place, like every other provider.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Manually tested with a Yubikey 5C NFC.
See #116 for previous work on this.