-
Notifications
You must be signed in to change notification settings - Fork 45
[PM-41798] 1Password access module #1371
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
base: main
Are you sure you want to change the base?
Changes from 27 commits
2899fc4
fc74c58
313da4d
5d8b6e8
02586c0
8bd94e4
57233d2
eef1376
40297ce
8e8f736
affa85b
3c0db34
1e431c7
043d5d1
dbbbc10
eeeaa17
69c395a
5a51ce1
178b7b8
bc51972
e3f32f7
13ab72c
45ae510
b00957a
ad71d1a
6d9de27
3f90a2d
0408811
7e06ca8
2bd446a
172724a
51ea1f8
7d3d0fe
62f9a44
de3c843
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| # Test vectors are derived from a throwaway account, no real secrets there. | ||
| exclude: | ||
| paths: | ||
| - crates/bitwarden-importers/src/importers/onepassword/access/fixtures |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| # 1Password access module | ||
|
detunized marked this conversation as resolved.
|
||
|
|
||
| Read access to a 1Password account. Logging in needs the username, master password and Secret Key, | ||
| plus a TOTP passcode when the account has 2FA. Once authenticated it downloads and decrypts every | ||
| accessible vault into a native 1Password model. | ||
|
|
||
| A Rust port of the OnePassword module in Bitwarden's C# `password-manager-access` library. | ||
|
|
||
| The 1P and BW name things differently. 1P has vaults that are independent, could be shared | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @detunized To make this easy to maintain for the tools team, would it be possible to get a higher-level spec for this? Right now it is unclear how the crypto works at a high level, and how it interacts. (cc @harr1424 I know we dm'd about this for the other crates / importers).
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hey @quexten ! Yes I spoke about this with @itsadrago and we agreed having a better understanding of the reverse engineering and crypto would go a long ways to improving maintainability.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
| separately, could have different access rights, encrypted with different keys. They will be imported | ||
| into Bitwarden collections. 1P doesn't have folders, only tags. | ||
|
|
||
| ## Notes | ||
|
|
||
| - Supports TOTP 2FA only ATM | ||
| - No SSO support | ||
| - No service account support (they are not so good for export/import) | ||
| - One entry point, `Client::download_all_vaults`. No vault selection, no random access | ||
| - Added `aes-gcm`, `hkdf`, `pbkdf2`, `crypto-bigint` and `icu_normalizer` to the workspace, will | ||
| increase the wasm size. | ||
| - SRP uses `crypto-bigint` rather than `num-bigint` for the constant-time `modpow` | ||
| - Uses RustCrypto directly rather than `bitwarden-crypto`, which keeps HKDF, AES-GCM and RSA-OAEP | ||
| private | ||
| - Only the two algorithms the web client implements, `PBES2g-HS256` and the legacy `PBES2-HS256`. | ||
| The `HS512` spellings the C# original accepts are rejected: the client throws `Invalid PBKDF2 alg` | ||
| on them, so its behaviour there is unknown | ||
| - The legacy path is transcribed from the client and cannot be tested end to end. Patching the web | ||
| client to mint a `PBES2-HS256` account fails: the server answers `POST /api/v1/user/auth` with a | ||
| 400, so no new account can be created on it | ||
| - Only the credentials and the keys are zeroed. The decrypted vault data is not | ||
|
|
||
| ## TODO | ||
|
|
||
| - The client fingerprint lives in `identity.rs`: app version, HTTP library and per-platform strings. | ||
| Question: do we need per-platform impersonation, or is one fixed identity enough? | ||
| - There are many tests converted from the C# repo, they became very noisy in Rust. Do we even need | ||
| them? See start_registers_an_unknown_device_then_retries for an example. | ||
| - Do we need to import password history? | ||
| - A wrong password reports as | ||
| `Internal("unexpected response from 'v2/auth/confirm-key' (HTTP 401)")` rather than | ||
| `BadCredentials`. The server only rejects at `confirm-key`, and its body there is not the | ||
| `errorCode` shape `parse_server_error` understands | ||
| - The wire DTOs derive `Debug`, so a debug log of one would print secrets | ||
| - The sign-in domain is taken as a raw string and never validated | ||
| - A vault we hold no key for is skipped silently, and one undecryptable item aborts the whole import | ||
| - The module is under a blanket `allow(dead_code, unused_imports)` until the conversion layer lands | ||
| - Now that only PBKDF2-SHA256 is supported, `bitwarden_crypto::pbkdf2` would do for that one step | ||
| - The username goes on the wire raw, `v2/auth/methods` and `v3/auth/start` do not get the normalized | ||
| one | ||
| - `SrpInfo` validates the SRP key method, then `compute_x` ignores it and always derives the modern | ||
| way | ||
| - `OnePasswordError::TwoFactorRequired` is never constructed | ||
Uh oh!
There was an error while loading. Please reload this page.