[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 |
There was a problem hiding this comment.
There was a problem hiding this comment.
Note we have both aes-gcm and pbkdf2 already approved in the clients repo and so I think we'd need approval for...
hkdf (Pure Rust implementation of the HMAC-based Extract-and-Expand Key Derivation Function (HKDF) generic over hash function.)
168 KiB
icu_normalizer (Normalizing text into Unicode Normalization Forms.)
76.3 KiB
num-bigint (Big integer types for Rust, BigInt and BigUint.)
109 KiB
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1371 +/- ##
==========================================
+ Coverage 86.28% 86.32% +0.03%
==========================================
Files 500 518 +18
Lines 73417 75571 +2154
==========================================
+ Hits 63349 65234 +1885
- Misses 10068 10337 +269 ☔ 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
@harr1424 I went through the |
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! |
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.