[PM-22271] Switch to SDK argon2 implementation, and drop other impls #15401
Conversation
quexten
changed the title
|
Great job, no security vulnerabilities found in this Pull Request |
|
Finally, these dependencies have caused so many compile issues in the past. You should also remove the dependencies from the renovate config: clients/.github/renovate.json5 Lines 402 to 405 in 7eb7507 Edit: Lines 63 to 69 in 7eb7507 |
## 🎟️ Tracking https://bitwarden.atlassian.net/browse/PM-22271 bitwarden/clients#15401 ## 📔 Objective We want to drop our three different argon2 implementations on `clients` (desktop-native rustcrypto, patched argon2-browser WASM, native C++ module on cli). Each of these have impactful bugs (https://bitwarden.atlassian.net/browse/PM-17450, https://bitwarden.atlassian.net/browse/PM-22217), and tech debt (dependency updates, and the patching scripts for node modules) associated. In this case, there is a small amount of tech debt introduced in the SDK because we introduce a deprecated function on PureCrypto. This small amount of tech debt introduced is far outweight by the dependencies that can be dropped on ts. (The clients PR: <img width="128" alt="image" src="https://github.com/user-attachments/assets/642c6ee8-2df3-406c-aba1-167a1647b65d" />) Specifically, since we want to fix immediate bugs, we do not yet do the work of porting all higher-level usages (masterpassword-unlock, masterkeyhash auth, pin unlock/enrollment, send password "key"(hash) deriving) to the SDK. This is follow-up feature work, and larger in scope, and involves other teams (auth,tools). Once this follow-up feature-work is complete, the tech-debt function introduced in this ticket will be removed, and a tech-debt ticket for this is linked. The tech debt function is clearly marked as `dangerous_` and `deprecated` with a note. ## ⏰ Reminders before review - Contributor guidelines followed - All formatters and local linters executed and passed - Written new unit and / or integration tests where applicable - Protected functional changes with optionality (feature flags) - Used internationalization (i18n) for all UI strings - CI builds passed - Communicated to DevOps any deployment requirements - Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team ## 🦮 Reviewer guidelines <!-- Suggested interactions but feel free to use (or not) as you desire! --> - 👍 (`:+1:`) or similar for great changes - 📝 (`:memo:`) or ℹ️ (`:information_source:`) for notes or general info - ❓ (`:question:`) for questions - 🤔 (`:thinking:`) or 💭 (`:thought_balloon:`) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion - 🎨 (`:art:`) for suggestions / improvements - ❌ (`:x:`) or⚠️ (`:warning:`) for more significant problems or concerns needing attention - 🌱 (`:seedling:`) or ♻️ (`:recycle:`) for future improvements or indications of technical debt - ⛏ (`:pick:`) for minor or nitpick changes
|
Blocked by #15408 since the client init had a breaking change in the SDK. |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #15401 +/- ##
==========================================
- Coverage 37.56% 37.55% -0.02%
==========================================
Files 3301 3299 -2
Lines 93914 93817 -97
Branches 14139 14130 -9
==========================================
- Hits 35279 35232 -47
+ Misses 57175 57125 -50
Partials 1460 1460 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There's still a reference to the argon2 npm package in the cli assets section: Lines 60 to 63 in 31156c4 Also the web webpack config: clients/apps/web/webpack.config.js Lines 401 to 402 in 31156c4 And while the NAPI code was deleted, the generated TS file wasn't updated: |
|
Fixed the remaining references. |
| @@ -35,7 +35,7 @@ describe("MainBiometricsService", function () { | |||
| const windowMain = mock<WindowMain>(); | |||
| const logService = mock<LogService>(); | |||
| const biometricStateService = mock<BiometricStateService>(); | |||
| const cryptoFunctionService = mock<MainCryptoFunctionService>(); | |||
| const cryptoFunctionService = mock<NodeCryptoFunctionService>(); | |||
There was a problem hiding this comment.
❓ why not just use CryptoFunctionService ?
There was a problem hiding this comment.
I think I just grep replaced main-crypto-function-service with node-crypto-function-service after deleting it here. I'll simplify to CryptoFunctionService.
|
…15401) * Switch to SDK argon2 implementation * Cleanup and update to the latest sdk * Update package lock * Remove copy patch * Fix builds * Fix test build * Remove error * Fix tests * Fix build * Run prettier * Remove argon2 references * Regenerate index.d.ts for desktop_native napi * Replace mocked crypto function service type (cherry picked from commit 8250e40)
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22271
bitwarden/sdk-internal#339
📔 Objective
We want to drop our three different argon2 implementations on clients (desktop-native rustcrypto, patched argon2-browser WASM, native C++ module on cli). Each of these have impactful bugs (https://bitwarden.atlassian.net/browse/PM-17450, https://bitwarden.atlassian.net/browse/PM-22217), and tech debt (dependency updates, and the patching scripts for node modules) associated.
For this we switch out the implementation of derive kdf key, and drop the argon2 dependencies and patch scripts / webpack configs.
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes