-
Notifications
You must be signed in to change notification settings - Fork 44
[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 all commits
2899fc4
fc74c58
313da4d
5d8b6e8
02586c0
8bd94e4
57233d2
eef1376
40297ce
8e8f736
affa85b
3c0db34
1e431c7
043d5d1
dbbbc10
eeeaa17
69c395a
5a51ce1
178b7b8
bc51972
e3f32f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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,38 @@ | ||
| # 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. `crypto-bigint` is the exception, `rsa` and `ssh-key` already pull it in | ||
| - 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 and has no PBKDF2-SHA512 | ||
| - `icu_normalizer` only NFC-normalizes the password before PBKDF2. Heavy for one call, | ||
| `unicode-normalization` would be smaller | ||
| - 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? | ||
| - Only the credentials and the keys are zeroed. The decrypted vault data is not | ||
|
Comment on lines
+26
to
+31
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. @itsadrago more open questions to review |
||
| - The server is never authenticated, `verify_key` does not recompute `serverVerifyHash` | ||
| - The wire DTOs derive `Debug`, so a debug log of one would print secrets | ||
| - Credentials are not trimmed, a pasted Secret Key with a trailing newline fails on length | ||
| - 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 | ||
| - Only the item DTOs in `wire` are public; the auth and session ones are `pub(super)` | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note we have both
aes-gcmandpbkdf2already approved in the clients repo and so I think we'd need approval for...