Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for apikey based logins #187

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dezeroku
Copy link
Contributor

@dezeroku dezeroku commented Jun 22, 2024

Allows users to log in with client_id&client_secret pair that can be obtained from Bitwarden.
Apikey based login does not return refresh_token as part of the response, instead it's required to just reauthenticate with the credentials (same approach as in the official CLI).

What was changed:

  1. New refresh_token type was added to differentiate between the refresh flows required. SSO and email+password return the refresh_key, apikey stores the client_id&client_secret pair instead.
    This means that these credentials are stored on disk in plaintext, in the rbw's DB file.
    It's consistent with how it's done in Bitwarden's official CLI, they do it the same way
  2. New client_id field was added to config
  3. client_secret is read via pinentry
  4. Added some docs

I also cleaned up a bit in src/api.rs, the apikey request is sent in three different places now, it looked bad without these changes:

  1. Separated the common "login request" parameters into a separate struct, to not duplicate the values all over the src/api.rs

device_id and email are needed for the relogin with apikey (to run it exactly the same way as the initial login), I've initially kept these alongside client_id and client_secret in the DB, but eventually changed to reading them from config. Thus adding device_id function non-async variant.

Tested (login + token refresh) the changes with all three flows we'd have now.

This topic started as part of #182
According to it, apikey strategy seems to be less fragile than the password based one, so maybe it'd be helpful for debugging issues in the future

CC @pfr-dev

@dezeroku dezeroku force-pushed the apikey-login branch 2 times, most recently from fdacc67 to d7a7cfd Compare June 26, 2024 10:28
@dezeroku
Copy link
Contributor Author

I'd like to follow this one with few more changes along the road, but I feel like thse would warrant separate PRs:

  1. Only collect the password and try to unseal the vault at the end of rbw login if we're trying to log in with master password
  2. Collect kdf information from the login/ endpoint response -> this will allow us to drop the prelogin/ call for SSO/apikey auth methods
  3. Remove the register command as it'll be equivalent to rbw login (with apikey) && rbw purge combination

With these changes all the auth strategies will be handled as first-class citizens and not just piggyback on the master password auth method.

BTW I kind of defaulted to using the config file as a single source of truth defining what method we should use.
If you think that passing flags to rbw login would be cleaner I'd be happy to align

@apprehensions
Copy link

Could you fix the conflicts?

@dezeroku
Copy link
Contributor Author

Sure, I'll try to get back to it during the weekend

@dezeroku
Copy link
Contributor Author

dezeroku commented Mar 25, 2025

I forgot how big this MR is 😅

If you'd like to try this with a local build you'll have to:

  • cargo build
  • killall rbw-agent to make sure no official releases are running
  • RBW_AGENT="$PWD/target/debug/rbw-agent" ./target/debug/rbw list from the root directory to run version from this MR

Add client_id field to rbw JSON config for apikey login flow to happen. I'd recommend doing it before all of the above steps.
It can get pretty messy to make sure that you run proper rbw-agent

Note: You'll be asked via pinentry for Bitwarden password before being asked for client_secret. Bitwarden password doesn't really matter there, I just didn't get around to modifying this flow from user's perspective (yet).

I've rebased and tested the login flows, but didn't really test refresh flows after the rebase (these require BW tokens to expire and take some time). Some unexpected errors may pop up after a day or two

@apprehensions
Copy link

apprehensions commented Mar 25, 2025

Note: You'll be asked via pinentry for Bitwarden password before being asked for client_secret

For something that uses an API key, which is commonly used for automation, this is not very intuitive. Wouldn't it be preferred it be prompted if unset in the configuration file - to allow people to not store the secret? My usecase for this feature is to simulate the bitwarden extension's ability to keep a session forever. And others may choose to use rbw in automation as well, so a feature like this would be nice to see.

@dezeroku
Copy link
Contributor Author

I suppose it could first try to get value from config and fallback to pinentry. It was implemented to handle unexpected sever changes that break other auth methods.

Support for automation would be a nice addition I guess

@dezeroku
Copy link
Contributor Author

PoCd, it's pretty ugly right now, whole MR requires a cleanup honestly. It's NOT mergable as-is

new stuff:

  • try to get client_secret from config and fallback to pinentry if not found
  • don't promp for master password if apikey login is performed. This part is UGLY, doing it properly requires handling of Options in multiple places and I don't have time for that rn. Under the hood the password is just set to "dummy" in case of apikey login and results in (harmless) "rbw login: failed to unlock database: Password is incorrect. Try again." message in the CLI. Though harmless it returns exit code 1 😅 The reason is that default implementation also unlocks the vault at the end of login call and with dummy password of course it won't work. That part should be refactored too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants