dani-garcia / vaultwarden

Unofficial Bitwarden compatible server written in Rust, formerly known as bitwarden_rs
GNU Affero General Public License v3.0
34.71k stars 1.69k forks source link

Add support for MFA with Duo's Universal Prompt #4637

Open 0x0fbc opened 3 weeks ago

0x0fbc commented 3 weeks ago

Overview

This adds support for MFA using Duo's "Universal Prompt".

On March 30th, 2024, the Duo MFA integration that Vaultwarden uses, the 'Traditional Prompt' was end-of-lifed. While Duo's API continues to allow the traditional prompt to be used in some cases, it is entirely unsupported, and it will eventually stop working. Some Vaultwarden users have already begun reporting that their Duo API keys are no longer functional in Vaultwarden. Bitwarden introduced Universal Prompt support in release 2024.2.3 on March 5th.

Duo's Universal Prompt uses the OIDC Authorization Code flow. This flow requires us to pack information about the authentication we want to be prompted for MFA into an authorization request (a JWT signed with a secret key provided by Duo) and send the user to their service, where MFA is performed. The user is then returned to our service with a code that we use to call Duo's API and obtain the result of the MFA. Once we validate the information passed back by the user's client and returned by Duo's service, we can log the user in.

Detailed documentation is located at https://duo.com/docs/oauthapi

The web vault handles receiving the redirection from Duo. It has a 'connector' page responsible for communicating the authorization code back to the user's true client so it can be provided in a call to Vaultwarden's API.

Major additions/changes

Specific Review Items

Beyond what you'd normally check, I'd like to highlight a few decisions I made that you should probably consider in your review.

Testing

Tested on the following clients; everything is working well:

The only major client issue I found was with the unpackaged AppImage Linux desktop client. When authenticating users on desktop clients, the connector in the web vault opens a bitwarden:// link, which my system refused to use the AppImage to open. It looks like the AppImage client either doesn't or can't register itself as the x-scheme-handler for bitwarden, preventing the redirect connector from handing off to the client. It worked fine with the Flatpak packaged Bitwarden client, so I'm chalking it up to my not configuring the AppImage client correctly.

There is also a known issue upstream in the web vault where the redirect connector shows 'successful authentication', does not automatically close, and the JS 'close' button does not work. See https://github.com/bitwarden/clients/issues/8554

I also tested this while running PostgreSQL and MariaDB using the versions packaged in Debian 12; no issues.

BlackDex commented 3 weeks ago

Thanks @0x0fbc.

On first glance it looks like you need to rebase already, but besides that it looks ok. No deep checking done.

The only thing that worries me is the collation for the state. That is going to give issues i think. Especially when the collation was wrong before and people need to update it. That will change it for that column too.

Isn't there a way to not have this as a key? Or less larger in size. (I have not yet looked into this in a functional way, that is why i ask)

0x0fbc commented 3 weeks ago

The column size can be scaled back, absolutely.

I set the state and nonce column sizes in postgres and maria so that someone could change the constant dictating the state length to the largest Duo would accept (1024 characters) without having to mess with the database schema. That constant is set to 64 right now, so the large size is only useful to support this specific future proofing case. If you've seen issues with collations/charsets in the past I'd rather not tempt fate, so I'm in favor of just cutting the size down and dropping the overridden character set/collation for the columns in MariaDB. I'll take care of that and rebase when I get a chance later today.

0x0fbc commented 3 weeks ago

@BlackDex, I reduced the column sizes and removed the collation override I had for MariaDB. I also added the changes suggested by Clippy, did another rustfmt, synced my fork, and rebased.

quexten commented 1 week ago

@0x0fbc

The only major client issue I found was with the unpackaged AppImage Linux desktop client. When authenticating users on desktop clients, the connector in the web vault opens a bitwarden:// link, which my system refused to use the AppImage to open. It looks like the AppImage client either doesn't or can't register itself as the x-scheme-handler for bitwarden, preventing the redirect connector from handing off to the client. It worked fine with the Flatpak packaged Bitwarden client, so I'm chalking it up to my not configuring the AppImage client correctly.

This is an upstream client bug and unrelated to vaultwarden. Both snap and appimage builds have (different) issues with the url handlers at the moment. If I recall correctly from debugging, on AppImage the desktop file does not get created anymore. It needs to be created manually.