[PM-41798] 1Password access module - #1371
Conversation
Logs in with the master password and Secret Key, drives TOTP two-factor, then downloads and decrypts every accessible vault into a native 1Password model. A Rust port of the OnePassword module in the C# password-manager-access library.
The client now returns each item as its decrypted overview and details, which is what 1Password actually sends. Turning that into typed items is the importer's job, so the parsing and the Item/Field/Otp/SshKey model move to the conversion layer rather than living behind the download.
|
|
|
Thank you for your contribution! We've added this to our internal Community PR board for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
![aikido-pr-checks[bot]](https://avatars.githubusercontent.com/in/898896?s=60&v=4){kind=small}
harr1424
left a comment
harr1424
left a comment
There was a problem hiding this comment.
I saw expect used a handful of times, this in general is discouraged, but overall the error handling appears robust.
| - 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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1371 +/- ##
==========================================
+ Coverage 86.28% 86.37% +0.08%
==========================================
Files 500 518 +18
Lines 73417 76369 +2952
==========================================
+ Hits 63349 65960 +2611
- Misses 10068 10409 +341 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
coroiu
left a comment
coroiu
left a comment
There was a problem hiding this comment.
Let me know when this has been reviewed and approved by tools and I'll give it a look
There was a problem hiding this comment.
Only skimmed this but a few notes:
- We should use a bigint library appropriate for cryptographic use. I have linked the one maintained by rustcrypto.
- (optional) It would be nice to have a documentation of how these components interact at a high level. even if it's a link to a spec by 1passsword
- a lot of dangerous interfaces are publicly exposed here. This is not desireable. Can we please lock them down?
other than that great work!
|
|
||
| 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 |
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
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.
num-bigint's modpow is not constant time. crypto-bigint raises through Montgomery form, which is, and it was already in the tree via rsa and ssh-key so it costs no extra build weight. Also drops num-bigint from the workspace and removes the crypto-bigint [patch.crates-io], since 0.7.5 has reached the registry.
Everything the CLI does not name is now pub(super). In wire only the item DTOs stay public, because model::Item exposes them through its fields; the auth and session DTOs are internal. Also drops three imports the blanket allow had been hiding.
5f51eb8 to
bc51972
Compare
quexten
left a comment
quexten
left a comment
There was a problem hiding this comment.
Thanks. I don't know the 1Password protocol, but the changes look reasonable. We could probably spend time to refactor it a bit to make it more idiomatic rust in some places, but that's not a requirement from my side at any rate.
Nice work! Love the test vectors and that it's fully in SDK.
|
@quexten thanks! |
The 1Password web client normalizes with NFKD before PBKDF2. We used NFC for the master key and the raw password for SRP x, so any password with a diacritic or a compatibility character failed to log in. Verified against a real account, including Hangul, ligatures and astral characters. Also drops the *-HS512 methods, which the client rejects as `Invalid PBKDF2 alg`, and implements the legacy PBES2- construction.
The password is trimmed before NFKD, the username trimmed and lowercased, and the SRP identity also decomposes it. The Secret Key drops everything outside its alphabet rather than only dashes, so a pasted key with a newline or spaces no longer fails on length.
The formula comes from the web client's recieveServerHash: H(A || M1 || K), with A leading-zero-stripped and K raw. Confirmed against a live account.
Replaces Region, which had no place for ent.1password.com, and validates the subdomain as a DNS label so a stray slash or @ cannot repoint the session.
8 participants
🎟️ Tracking
N/A
📔 Objective
Adds a direct 1Password client to
bitwarden-importers. It logs in with the master password andSecret Key and downloads every vault the account can open.
It is a Rust port of the 1Password module from the C#
password-manager-accesslibrary.What it does:
Tests cover the crypto with known answer vectors from the C# tests, and the HTTP flows with
wiremock. I also ran it against a real account.
Not included:
#[allow(dead_code, unused_imports)]for now
README.md has the remaining notes and open questions.
The next PR will introduce a 1P to BW conversion/importer layer.
Use https://github.com/detunized/onepassword-access-cli to drive this library to test against a live account.
🚨 Breaking Changes
None. Nothing consumes the new module yet.